Last Comment Bug 620331 - Remote nsAudioStreamRemote::GetMinWriteSamples
: Remote nsAudioStreamRemote::GetMinWriteSamples
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: x86 Windows 7
-- normal (vote)
: mozilla6
Assigned To: Paul Adenot (:padenot)
: Maire Reavy [:mreavy] Please needinfo me
Depends on: 616800
  Show dependency treegraph
Reported: 2010-12-19 21:48 PST by Matthew Gregan [:kinetik]
Modified: 2011-05-15 07:50 PDT (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Fix bug 620331 (8.41 KB, patch)
2011-05-09 10:02 PDT, Paul Adenot (:padenot)
kinetik: review-
Details | Diff | Splinter Review
Patch v2 adressing :kinetik concerns. (8.44 KB, patch)
2011-05-10 01:27 PDT, Paul Adenot (:padenot)
kinetik: review+
Details | Diff | Splinter Review
v3 of the patch (8.50 KB, patch)
2011-05-12 03:48 PDT, Paul Adenot (:padenot)
kinetik: review+
Details | Diff | Splinter Review

Description User image Matthew Gregan [:kinetik] 2010-12-19 21:48:53 PST
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.
Comment 1 User image Paul Adenot (:padenot) 2011-05-09 09:32:06 PDT
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.
Comment 2 User image Paul Adenot (:padenot) 2011-05-09 10:02:56 PDT
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.
Comment 3 User image Matthew Gregan [:kinetik] 2011-05-09 22:25:41 PDT
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.
Comment 4 User image Paul Adenot (:padenot) 2011-05-10 01:27:23 PDT
Created attachment 531265 [details] [diff] [review]
Patch v2 adressing :kinetik concerns.
Comment 5 User image Matthew Gregan [:kinetik] 2011-05-10 01:47:17 PDT
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.
Comment 6 User image Paul Adenot (:padenot) 2011-05-12 03:48:52 PDT
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.
Comment 7 User image :Ms2ger (⌚ UTC+1/+2) 2011-05-15 07:50:24 PDT
The patch did require quite a bit of merging...

Note You need to log in before you can comment on or make changes to this bug.