Remote nsAudioStreamRemote::GetMinWriteSamples

RESOLVED FIXED in mozilla6

Status

()

Core
Audio/Video
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: kinetik, Assigned: padenot)

Tracking

Trunk
mozilla6
x86
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

7 years ago
Bug 616800 added GetMinWriteSamples to nsAudioStreamLocal, but did not add code to remote the correct value back to the child process.  If the remote nsAudioStream ever uses a backend where this value matters (ALSA, Win32), this needs to be remoted to avoid bugs like the one that bug 616800 fixed.
(Reporter)

Updated

7 years ago
Depends on: 616800
I've got a patch for this, works great on a Linux machine, using Alsa backend (i.e. I've got a good value, which is 22048 for my system).

However, this regresses badly. Although the UI is fully functionnal, when I click the Pause button either on default control, or calling the pause() function from javascript, the stream continues to play.

Seeking and muting doesn't work either.

The graphic controls respond to click, though.

I'm going to investigate, but I should probably attach the patch for this bug, and create a new one, I guess.
Created attachment 531072 [details] [diff] [review]
Fix bug 620331

Okay my bad, the regression was caused by something else, the patch works fine.

I'm attaching the patch for review.
Attachment #531072 - Flags: review?(kinetik)
(Reporter)

Comment 3

6 years ago
Comment on attachment 531072 [details] [diff] [review]
Fix bug 620331

Review of attachment 531072 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/nsAudioStream.cpp
@@ +691,5 @@
>  
>  PRInt32 nsAudioStreamRemote::GetMinWriteSamples()
>  {
> +  if(!mAudioChild)
> +    return NS_ERROR_FAILURE;

This doesn't return an nsresult.  Just return -1 here.

::: dom/ipc/AudioChild.cpp
@@ +87,5 @@
> +PRInt32
> +AudioChild::WaitForMinWriteSample()
> +{
> +  ReentrantMonitorAutoEnter mon(mAudioReentrantMonitor);
> +mAudioReentrantMonitor.Wait();

Missing indentation.

It's possible for Wait() to happen spuriously, so you need to call it in a loop and check that the state you're waiting on has actually been reached.  See WaitForDrain() in the same file for an example.
Attachment #531072 - Flags: review?(kinetik) → review-
Created attachment 531265 [details] [diff] [review]
Patch v2 adressing :kinetik concerns.
Attachment #531072 - Attachment is obsolete: true
Attachment #531265 - Flags: review?(kinetik)
(Reporter)

Comment 5

6 years ago
Comment on attachment 531265 [details] [diff] [review]
Patch v2 adressing :kinetik concerns.

Review of attachment 531265 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with that change.

::: dom/ipc/AudioChild.cpp
@@ +88,5 @@
> +AudioChild::WaitForMinWriteSample()
> +{
> +  ReentrantMonitorAutoEnter mon(mAudioReentrantMonitor);
> +  while(mMinWriteSample == -1 && mIPCOpen)
> +    mAudioReentrantMonitor.Wait();

GetMinWriteSamples can return -1 on error, so we could end up waiting forever here.  It would be safer to use some other negative value as the not-yet-initialized sentinel value (with an appropriate comment), and change the loop to exit when mMinWriteSample is no longer that value.

Space between the while and the (, please.
Attachment #531265 - Flags: review?(kinetik) → review+
Created attachment 531884 [details] [diff] [review]
v3 of the patch

I've added comments both in the initialization of the mMinWriteSample member and above the while() statement.
Attachment #531265 - Attachment is obsolete: true
Attachment #531884 - Flags: review?(kinetik)
(Reporter)

Updated

6 years ago
Attachment #531884 - Flags: review?(kinetik) → review+
Assignee: nobody → paul
Status: NEW → ASSIGNED
Keywords: checkin-needed
The patch did require quite a bit of merging...

http://hg.mozilla.org/mozilla-central/rev/f937f55b4a7f
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
You need to log in before you can comment on or make changes to this bug.