Closed Bug 612799 Opened 9 years ago Closed 7 years ago

Remote Audio - Signal errors to the child AudioStream.

Categories

(Core :: Audio/Video, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED WONTFIX
Tracking Status
fennec + ---

People

(Reporter: dougt, Assigned: padenot)

References

Details

Attachments

(2 files, 7 obsolete files)

+++ This bug was initially created as a clone of Bug #599089 +++

We need to work out how to signal errors in the child AudioStream.  If the
parent stream sets mInError, it should send an event to the child to mark it as
in-error as well... otherwise the child has no way to know the AudioStream is
failing.  Events sent between the parent entering error state and the child
receiving the event will be ignored by the mInError early return tests in the
parent, so this should all work without too much pain.
tracking-fennec: 2.0b3+ → ?
tracking-fennec: ? → 2.0-
Whiteboard: [fennec-4.1?]
Assignee: nobody → doug.turner
Attached patch WIP (obsolete) — Splinter Review
I tried something to report error to the child class. This patch is not complete, but goes like this :

- While sending an event to perform an action , send a pointer to the AudioParent along with other data.
- Get the return value of the call to the API (performed by the nsAudioStreamLocal), since the call is synchronous.
- On error, send an IPC message to the child, which will set a mInError flag, thus, the child is aware of the underlying error.

For now, only the write error are reported, but I need feedback as it's my first non-trivial patch so far.
Attachment #530317 - Flags: feedback?(kinetik)
Looks good so far.  I'll take a better look at it in the next day or so.
Assignee: doug.turner → paul
Comment on attachment 530317 [details] [diff] [review]
WIP

Approach looks good so far.

Maybe change the signature of AudioWriteEvent's ctor to take the AudioParent pointer first, since this makes it consistent with AudioDrainEvent's ctor.

The next step is to make the nsAudioStreamRemote functions return an error when the AudioChild is in error state.  nsAudioStream mostly returns -1 or generic NS_ERROR_FAILURE, so using a bool to represent the error state is probably fine for now.
Attachment #530317 - Flags: feedback?(kinetik) → feedback+
tracking-fennec: - → 7+
Whiteboard: [fennec-4.1?]
Status: NEW → ASSIGNED
Attached patch Add error reporting in Drain() (obsolete) — Splinter Review
Since Drain() can call sa_stream_write(), and because error reporting is performed when this API call is used, I've added error reporting for Drain() both for safety and consistency.

The actual patch for this bug will follow.
Attachment #532238 - Flags: review?(kinetik)
Attached patch Patch bug 612799 v1 (obsolete) — Splinter Review
Added SetVolume error handling (since it can fail under macosx)
Added Shutdown error handling

Updated nsAudioStreamRemote methods to return immediately if the stream is on error.
Attachment #530317 - Attachment is obsolete: true
Attachment #532263 - Flags: review?(kinetik)
Comment on attachment 532263 [details] [diff] [review]
Patch bug 612799 v1

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

r+ with those fixed

::: content/media/nsAudioStream.cpp
@@ +515,5 @@
>    }
>  #else
>    mVolume = aVolume;
> +  //return NS_OK;
> +  return NS_ERROR_FAILURE;

Should return NS_OK here.

@@ +670,5 @@
>                             PRBool aBlocking)
>  {
>    if (!mAudioChild)
>      return NS_ERROR_FAILURE;
> +  if(mAudioChild->IsInError())

Merge this with the test above it, like so:

if (!mAudioChild || mAudioChild->IsInError())

Same goes for everywhere else this pattern occurs.
Attachment #532263 - Flags: review?(kinetik) → review+
Comment on attachment 532238 [details] [diff] [review]
Add error reporting in Drain()

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

r+ with those fixed

::: content/media/nsAudioStream.cpp
@@ +694,4 @@
>    nsCOMPtr<nsIRunnable> event = new AudioDrainEvent(mAudioChild);
>    NS_DispatchToMainThread(event);
> +  nsresult rv = mAudioChild->WaitForDrain();
> +  PR_LOG(gAudioStreamLog, PR_LOG_DEBUG, ("#-# Drain failure"));

This log message looks incorrect.

::: dom/ipc/AudioChild.cpp
@@ +80,4 @@
>  {
>    ReentrantMonitorAutoEnter mon(mAudioReentrantMonitor);
> +
> +  if(status == NS_OK)

Space between if and (.  Same goes for everywhere else.
Attachment #532238 - Flags: review?(kinetik) → review+
Attached patch 1 - Patch Drain v2 (obsolete) — Splinter Review
Updated the patch, it wasn't applying properly any more.
Attachment #532238 - Attachment is obsolete: true
Attachment #538015 - Flags: review?(kinetik)
Attached patch 2 - Patch Error reporting v2 (obsolete) — Splinter Review
Updated the patch with :kinetik remarks, and because it wasn't applying properly.
Attachment #532263 - Attachment is obsolete: true
Attachment #538016 - Flags: review?(kinetik)
Comment on attachment 538015 [details] [diff] [review]
1 - Patch Drain v2

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

r+ with the |#if defined(ANDROID)| thing fixed.

::: content/media/nsAudioStream.cpp
@@ +72,2 @@
>  #define REMOTE_AUDIO 1
> +//#endif

This change should not be checked in.

@@ +749,4 @@
>    nsCOMPtr<nsIRunnable> event = new AudioDrainEvent(mAudioChild);
>    NS_DispatchToMainThread(event);
> +  nsresult rv = mAudioChild->WaitForDrain();
> +  return rv;

You could just do |return mAudioChild->WaitForDrain();| here, but it's not a big deal either way.
Attachment #538015 - Flags: review?(kinetik) → review+
Comment on attachment 538016 [details] [diff] [review]
2 - Patch Error reporting v2

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

r+ with that fixed

::: content/media/nsAudioStream.cpp
@@ +567,4 @@
>    }
>  #else
>    mVolume = aVolume;
> +  return NS_OK;

The return needs to be after the #endif, otherwise the SA_PER_STREAM_VOLUME version will not return a value on success.
Attachment #538016 - Flags: review?(kinetik) → review+
Attached patch 1 - Patch Drain v3 (obsolete) — Splinter Review
Fixed aforementioned mistakes.
Attachment #538015 - Attachment is obsolete: true
Attachment #538218 - Flags: review?(kinetik)
Attached patch 2 - Patch Error reporting v3 (obsolete) — Splinter Review
Fixed aforementioned mistakes.
Attachment #538016 - Attachment is obsolete: true
Attachment #538219 - Flags: review?(kinetik)
Comment on attachment 538218 [details] [diff] [review]
1 - Patch Drain v3

Thanks!
Attachment #538218 - Flags: review?(kinetik) → review+
Attachment #538219 - Flags: review?(kinetik) → review+
Keywords: checkin-needed
Keywords: checkin-needed
Whiteboard: [inbound]
Attachment #538218 - Flags: checkin+
Attachment #538219 - Flags: checkin+
I had to backout due to a build bustage on all platforms. I will push them back as soon as you will attach refreshed patches.
Whiteboard: [inbound]
Applies & compile fine with current tip.
Attachment #538218 - Attachment is obsolete: true
Applies & compile fine with current tip.
Attachment #538219 - Attachment is obsolete: true
Attachment #538218 - Flags: checkin+ → checkin-
Attachment #538219 - Flags: checkin+ → checkin-
Attachment #538533 - Flags: checkin+
Attachment #538534 - Flags: checkin+
Whiteboard: [inbound]
backed out again due to permaorange in crashtests (http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-Inbound/1307726819.1307726983.15978.gz) and in mochitest-1 (http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-Inbound/1307726395.1307728362.20807.gz)

Please have a green try run before next push.
Whiteboard: [inbound]
Attachment #538533 - Flags: checkin-
Attachment #538534 - Flags: checkin-
tracking-fennec: 7+ → 8+
paul, what is the status here?  Do you want to take another swing?
Doug, yes, I've got a fix for that crash.
tracking-fennec: 8+ → +
paul, could you update the patch?  I'd love to try it out.
Strangely enough, I cannot reproduce the test failure on a device without my fix (although my fix was clumsy, so it's probably a good thing). I pushed to try to be sure : http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=e64bb8637b9a
This'll be obsoleted by bug 783827.
Depends on: 783827
I'm going to remove the old code for now - bug 812937.
Depends on: 812937
No longer depends on: 783827
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.