Remote nsAudioStreamRemote::GetMinWriteSamples

RESOLVED FIXED in mozilla6

Status

()

defect
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: kinetik, Assigned: padenot)

Tracking

Trunk
mozilla6
x86
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

9 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

9 years ago
Depends on: 616800
Assignee

Comment 1

8 years ago
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.
Assignee

Comment 2

8 years ago
Posted patch Fix bug 620331 (obsolete) — Splinter Review
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

8 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-
Assignee

Comment 4

8 years ago
Attachment #531072 - Attachment is obsolete: true
Attachment #531265 - Flags: review?(kinetik)
Reporter

Comment 5

8 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+
Assignee

Comment 6

8 years ago
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

8 years ago
Attachment #531884 - Flags: review?(kinetik) → review+
Assignee

Updated

8 years ago
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
Closed: 8 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.