Closed
Bug 612799
Opened 15 years ago
Closed 13 years ago
Remote Audio - Signal errors to the child AudioStream.
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
WONTFIX
Tracking | Status | |
---|---|---|
fennec | + | --- |
People
(Reporter: dougt, Assigned: padenot)
References
Details
Attachments
(2 files, 7 obsolete files)
9.00 KB,
patch
|
mounir
:
checkin-
|
Details | Diff | Splinter Review |
13.37 KB,
patch
|
mounir
:
checkin-
|
Details | Diff | Splinter Review |
+++ 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.
Reporter | ||
Updated•15 years ago
|
tracking-fennec: 2.0b3+ → ?
Updated•15 years ago
|
tracking-fennec: ? → 2.0-
Updated•14 years ago
|
Whiteboard: [fennec-4.1?]
Reporter | ||
Updated•14 years ago
|
Assignee: nobody → doug.turner
Assignee | ||
Comment 1•14 years ago
|
||
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)
Comment 2•14 years ago
|
||
Looks good so far. I'll take a better look at it in the next day or so.
Assignee: doug.turner → paul
Comment 3•14 years ago
|
||
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+
Updated•14 years ago
|
tracking-fennec: - → 7+
Whiteboard: [fennec-4.1?]
Assignee | ||
Updated•14 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•14 years ago
|
||
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)
Assignee | ||
Comment 5•14 years ago
|
||
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 6•14 years ago
|
||
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 7•14 years ago
|
||
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+
Assignee | ||
Comment 8•14 years ago
|
||
Updated the patch, it wasn't applying properly any more.
Attachment #532238 -
Attachment is obsolete: true
Attachment #538015 -
Flags: review?(kinetik)
Assignee | ||
Comment 9•14 years ago
|
||
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 10•14 years ago
|
||
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 11•14 years ago
|
||
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+
Assignee | ||
Comment 12•14 years ago
|
||
Fixed aforementioned mistakes.
Attachment #538015 -
Attachment is obsolete: true
Attachment #538218 -
Flags: review?(kinetik)
Assignee | ||
Comment 13•14 years ago
|
||
Fixed aforementioned mistakes.
Attachment #538016 -
Attachment is obsolete: true
Attachment #538219 -
Flags: review?(kinetik)
Comment 14•14 years ago
|
||
Comment on attachment 538218 [details] [diff] [review]
1 - Patch Drain v3
Thanks!
Attachment #538218 -
Flags: review?(kinetik) → review+
Updated•14 years ago
|
Attachment #538219 -
Flags: review?(kinetik) → review+
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Updated•14 years ago
|
Keywords: checkin-needed
Whiteboard: [inbound]
Updated•14 years ago
|
Attachment #538218 -
Flags: checkin+
Updated•14 years ago
|
Attachment #538219 -
Flags: checkin+
Comment 15•14 years ago
|
||
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]
Assignee | ||
Comment 16•14 years ago
|
||
Applies & compile fine with current tip.
Attachment #538218 -
Attachment is obsolete: true
Assignee | ||
Comment 17•14 years ago
|
||
Applies & compile fine with current tip.
Attachment #538219 -
Attachment is obsolete: true
Updated•14 years ago
|
Attachment #538218 -
Flags: checkin+ → checkin-
Updated•14 years ago
|
Attachment #538219 -
Flags: checkin+ → checkin-
Updated•14 years ago
|
Attachment #538533 -
Flags: checkin+
Updated•14 years ago
|
Attachment #538534 -
Flags: checkin+
Updated•14 years ago
|
Whiteboard: [inbound]
Comment 18•14 years ago
|
||
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]
Updated•14 years ago
|
Attachment #538533 -
Flags: checkin+
Updated•14 years ago
|
Attachment #538534 -
Flags: checkin+
Updated•14 years ago
|
Attachment #538533 -
Flags: checkin-
Updated•14 years ago
|
Attachment #538534 -
Flags: checkin-
Updated•14 years ago
|
tracking-fennec: 7+ → 8+
Reporter | ||
Comment 19•14 years ago
|
||
paul, what is the status here? Do you want to take another swing?
Assignee | ||
Comment 20•14 years ago
|
||
Doug, yes, I've got a fix for that crash.
Reporter | ||
Updated•14 years ago
|
tracking-fennec: 8+ → +
Reporter | ||
Comment 21•14 years ago
|
||
paul, could you update the patch? I'd love to try it out.
Assignee | ||
Comment 22•14 years ago
|
||
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
Comment 24•13 years ago
|
||
I'm going to remove the old code for now - bug 812937.
Updated•13 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•