Skip to content

Conversation

uncatched
Copy link
Contributor

@uncatched uncatched commented Jul 8, 2025

Ticket link

DOC-14148

PR description

  • I was unable to reproduce the crash, but it seems that the audio player crashes due to overflowing the audio buffer.

PR submission checklist

  • PR name contains Jira ticket number
  • PR have correct target branch
  • .s7substat has correct subrepos revisions
  • Common (Obj-C / Swift) and your team's coding conventions and style guidelines are followed
  • Unit tests to cover the critical parts of the code are written
  • Relevant documentation updated / added if needed
  • Self-reviewed using the self-review checklist prior to submitting a PR
  • All recommendations from the How to create Pull Request document are followed

@uncatched uncatched self-assigned this Jul 8, 2025
@uncatched uncatched requested a review from Copilot July 8, 2025 11:14
Copilot

This comment was marked as outdated.

@uncatched uncatched requested a review from lyeskin July 9, 2025 07:35
Comment on lines 1224 to 1226
if framebuffer.bufferedAudioDuration >= RDMPEGPlayerMaxAudioBufferSize {
return true
}
Copy link
Contributor

@lyeskin lyeskin Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Не могу понять что эти изменения делают, так как следом идет проверка на > minSize, который всегда меньше и всегда будет выполняться при выполнении условия на >= maxSize. То есть проверка, по идее, не несет смысла.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Передивився цей ПР. Трохи мене не туди занесло із фіксом. Виглядає так, що краш відбувається в кложурі в якому сильно захоплюється self. Раніше ще думав, що можна додати перевірку на максимальний розмір аудіо буфера, але не думаю що це відноситься до цього краша

@uncatched uncatched requested a review from Copilot July 29, 2025 10:39
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR addresses a potential audio buffer overflow crash in the RDMPEGPlayer by adding weak self references to prevent retain cycles and fixing property access in audio callback closures. The changes ensure proper memory management and thread safety in the audio processing pipeline.

  • Adds weak self references to closure parameters to prevent memory leaks
  • Replaces direct property access with explicit self references in audio callbacks
  • Improves memory safety in audio frame processing methods

@@ -875,7 +877,7 @@ public class RDMPEGPlayer: NSObject {
autoreleasepool {
var outData = outData

if videoStreamExist && correctionInfo == nil {
if self.videoStreamExist && self.correctionInfo == nil {
Copy link
Preview

Copilot AI Jul 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This property access occurs within an audio callback closure but doesn't use weak self capture like other closures in this method. This could lead to a strong reference cycle and potential crashes when the player is deallocated during audio processing.

Copilot uses AI. Check for mistakes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only closure here is the autoreleasepool. Self has been properly captured as weak below

@@ -887,7 +889,7 @@ public class RDMPEGPlayer: NSObject {
var numFramesLeft = numFrames

while numFramesLeft > 0 {
if rawAudioFrame == nil {
if self.rawAudioFrame == nil {
Copy link
Preview

Copilot AI Jul 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to line 880, this property access in the audio callback doesn't use weak self capture, which could cause retain cycles and crashes during player deallocation.

Copilot uses AI. Check for mistakes.

@uncatched uncatched requested a review from lyeskin July 29, 2025 10:44
@uncatched uncatched merged commit 8361182 into release/documents Jul 29, 2025
2 checks passed
@uncatched uncatched deleted the bugfix/DOC-14148-audio-player-crash branch July 29, 2025 11:41
@uncatched uncatched restored the bugfix/DOC-14148-audio-player-crash branch July 31, 2025 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants