Last Comment Bug 620331 - Remote nsAudioStreamRemote::GetMinWriteSamples
: Remote nsAudioStreamRemote::GetMinWriteSamples
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: x86 Windows 7
: -- normal (vote)
: mozilla6
Assigned To: Paul Adenot (:padenot)
:
Mentors:
Depends on: 616800
Blocks:
  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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


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

Description 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 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 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 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 Paul Adenot (:padenot) 2011-05-10 01:27:23 PDT
Created attachment 531265 [details] [diff] [review]
Patch v2 adressing :kinetik concerns.
Comment 5 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 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 :Ms2ger 2011-05-15 07:50:24 PDT
The patch did require quite a bit of merging...

http://hg.mozilla.org/mozilla-central/rev/f937f55b4a7f

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