Closed
Bug 899159
Opened 11 years ago
Closed 11 years ago
Upstream issues with OpenSLES audio code in webrtc
Categories
(Core :: WebRTC: Audio/Video, defect)
Tracking
()
People
(Reporter: jesup, Assigned: jesup)
References
Details
(Whiteboard: [FT: Media Recording, Sprint 2][qa-])
Attachments
(5 files, 1 obsolete file)
(deleted),
patch
|
mwu
:
review+
derf
:
review+
jesup
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
padenot
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
padenot
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bajaj
:
approval-mozilla-aurora+
bajaj
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
See bug 825112, especially comment 136 https://bugzilla.mozilla.org/show_bug.cgi?id=825112#c136 There are also some TSAN data-race issues flagged earlier in the bug against a copy of the JNI audio code, that are generally shared by many or all of the audio backends in the upstream. Depending on the compiler, this may or may not be an issue - though generally not in practice (though they should be fixed).
Assignee | ||
Comment 1•11 years ago
|
||
includes changes to the Playout stop code from upstream trunk, though we don't use playout
Assignee | ||
Updated•11 years ago
|
Attachment #782644 -
Flags: review?(tterribe)
Attachment #782644 -
Flags: review?(paul)
Attachment #782644 -
Flags: review?(mwu)
Assignee | ||
Updated•11 years ago
|
Depends on: webrtc_updates
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Comment 2•11 years ago
|
||
Filed Issue 2150 http://code.google.com/p/webrtc/issues/detail?id=2150
Comment 3•11 years ago
|
||
Comment on attachment 782644 [details] [diff] [review] clean up record issues in webrtc OpenSLES code Review of attachment 782644 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/trunk/webrtc/modules/audio_device/android/audio_device_opensles_android.cc @@ +1371,2 @@ > > + { Why this anonymous scope if you don't use RAII? I assume the WebRTC code is compiled with -fno-except, and they don't have some |CriticalSectionExitScoped| facility. @@ +1375,3 @@ > if (rec_voe_audio_queue_.size() <= 0) { > + crit_sect_.Leave(); > + rec_timer_.Wait(40); Comment over this magic number, or constant with a meaningful name. @@ +1377,5 @@ > + rec_timer_.Wait(40); > + crit_sect_.Enter(); > + if (rec_voe_audio_queue_.size() <= 0) { > + // if we went 40ms without a buffer, we need to see if we're still > + // recording or have been Stop()ed, and it's waiting for us to return In fact, I feel that this comment block should go above the |Wait(40)|, explain the general strategy, and give a rational behind the 40ms number, or say it is arbitrary.
Attachment #782644 -
Flags: review?(paul) → review+
Assignee | ||
Comment 4•11 years ago
|
||
(In reply to Paul Adenot (:padenot) from comment #3) > Comment on attachment 782644 [details] [diff] [review] > clean up record issues in webrtc OpenSLES code > > Review of attachment 782644 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: > media/webrtc/trunk/webrtc/modules/audio_device/android/ > audio_device_opensles_android.cc > @@ +1371,2 @@ > > > > + { > > Why this anonymous scope if you don't use RAII? I assume the WebRTC code is > compiled with -fno-except, and they don't have some > |CriticalSectionExitScoped| facility. There's no way to release the scoped/RAII version and then re-acquire it (just release), so I can't use their RAII functions. The block was already there; I can remove it. It does provide visual scoping, but no functional scoping now. > @@ +1375,3 @@ > > if (rec_voe_audio_queue_.size() <= 0) { > > + crit_sect_.Leave(); > > + rec_timer_.Wait(40); > > Comment over this magic number, or constant with a meaningful name. > > @@ +1377,5 @@ > > + rec_timer_.Wait(40); > > + crit_sect_.Enter(); > > + if (rec_voe_audio_queue_.size() <= 0) { > > + // if we went 40ms without a buffer, we need to see if we're still > > + // recording or have been Stop()ed, and it's waiting for us to return > > In fact, I feel that this comment block should go above the |Wait(40)|, > explain the general strategy, and give a rational behind the 40ms number, or > say it is arbitrary. sure, thanks
Assignee | ||
Comment 5•11 years ago
|
||
(In reply to Randell Jesup [:jesup] from comment #4) > There's no way to release the scoped/RAII version and then re-acquire it > (just release), so I can't use their RAII functions. Correction: CriticalSectionScoped::Leave() is private, so I don't even have that.
Assignee | ||
Comment 6•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #782644 -
Attachment is obsolete: true
Attachment #782644 -
Flags: review?(tterribe)
Attachment #782644 -
Flags: review?(mwu)
Assignee | ||
Updated•11 years ago
|
Attachment #783163 -
Flags: review?(tterribe)
Attachment #783163 -
Flags: review?(mwu)
Assignee | ||
Comment 7•11 years ago
|
||
Comment on attachment 783163 [details] [diff] [review] clean up record issues in webrtc OpenSLES code Carry forward r=padenot
Attachment #783163 -
Flags: review+
Comment 8•11 years ago
|
||
Comment on attachment 783163 [details] [diff] [review] clean up record issues in webrtc OpenSLES code Review of attachment 783163 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/trunk/webrtc/modules/audio_device/android/audio_device_opensles_android.cc @@ +1267,5 @@ > return (rec_warning_ > 0); > } > > bool AudioDeviceAndroidOpenSLES::RecordingError() const { > + CriticalSectionScoped lock(&crit_sect_); This api (recording/playout warning/error) is pretty crappy overall, but it seems like it doesn't need to be very good and thread safe to do what it does..
Attachment #783163 -
Flags: review?(mwu) → review+
Comment 9•11 years ago
|
||
Comment on attachment 783163 [details] [diff] [review] clean up record issues in webrtc OpenSLES code Review of attachment 783163 [details] [diff] [review]: ----------------------------------------------------------------- This is forward progress, but it does not by any means resolve all of the issues, even with just the recording pieces. ::: media/webrtc/trunk/webrtc/modules/audio_device/android/audio_device_opensles_android.cc @@ +1176,4 @@ > > + if (sles_player_itf_) { > + // Make sure player is stopped > + int32_t res = (*sles_player_itf_)->SetPlayState(sles_player_itf_, Note for upstream: If you try to fix the races in PlayerSimpleBufferQueueCallbackHandler() by adding locking, this will deadlock/leak, and you can't just release crit_sect_, as Terminate() calls this function while holding the lock as well. @@ +1370,5 @@ > + while (is_recording_) { > + if (rec_voe_audio_queue_.size() <= 0) { > + crit_sect_.Leave(); > + // Wait for max 40ms for incoming audio data before looping the > + // poll and checking for ::Stop() being called (which waits for us Why don't you just have StopRecording() frob rec_timer_ (which, BTW, is not a timer)? Waiting around 40 ms to tear down the recording thread for no reason seems dumb (which thread does destruction happen on? I hope it's not the main thread). @@ +1395,3 @@ > voe_audio_buffer_->SetRecordedBuffer(buf, num_samples); > voe_audio_buffer_->SetVQEData(playout_delay_, recording_delay_, 0); > voe_audio_buffer_->DeliverRecordedData(); If you hold crit_sect_ while calling all of these functions (which, you know, you need to, unless you can prove races with AttachAudioBuffer, etc., can't happen), you are blocking RecorderSimpleBufferQueueCallbackHandler() for at least as long as it takes them to execute. This means there is _no_ advantage to doing all of this on a separate thread, not even a theoretical "we shouldn't be doing any processing in the OpenSL ES callback because some day it might not have its own independent thread" (which I don't buy anyway). I reiterate that the whole thread should just go away. It serves no purpose except overhead and complexity. @@ +1428,5 @@ > + if (rec_voe_ready_queue_.size() <= 0) { > + // Log Error. > + rec_error_ = 1; > + WEBRTC_OPENSL_TRACE(kTraceError, kTraceAudioDevice, id_, > + " Audio Rec thread buffers underrun"); Didn't notice this in my earlier review, but we don't change audio here, yet we call Enqueue() anyway. That means that if this ever actually triggers, we'll have two copies of this buffer running through the queues, with its contents potentially getting overwritten before they can be used elsewhere, or while they're being used. The chance of this is high because AudioRecord delivers data to OpenSL ES in bursty (64 ms) chunks. We will never recover from this state. Every time the error triggers, it will get worse.
Attachment #783163 -
Flags: review?(tterribe) → review+
Assignee | ||
Comment 10•11 years ago
|
||
(In reply to Timothy B. Terriberry (:derf) from comment #9) > Comment on attachment 783163 [details] [diff] [review] > clean up record issues in webrtc OpenSLES code > > Review of attachment 783163 [details] [diff] [review]: > ----------------------------------------------------------------- > > This is forward progress, but it does not by any means resolve all of the > issues, even with just the recording pieces. Thanks! > @@ +1370,5 @@ > > + while (is_recording_) { > > + if (rec_voe_audio_queue_.size() <= 0) { > > + crit_sect_.Leave(); > > + // Wait for max 40ms for incoming audio data before looping the > > + // poll and checking for ::Stop() being called (which waits for us > > Why don't you just have StopRecording() frob rec_timer_ (which, BTW, is not > a timer)? Waiting around 40 ms to tear down the recording thread for no > reason seems dumb (which thread does destruction happen on? I hope it's not > the main thread). Frobbing rec_timer_ will be at best iffy, since it may well (pretty likely in fact) get serviced before ::Stop() is called, in which case it will still wait 40ms. ::Stop() waits for up to 10 seconds for the process to exit. We could frob it, but we still may wait, and likely will. The API doesn't give us an easy way to abort waits at ::Stop(). (We could add an "I'm going to kill you!" state var and then frob it, or even ask it to kill itself.) > @@ +1395,3 @@ > > voe_audio_buffer_->SetRecordedBuffer(buf, num_samples); > > voe_audio_buffer_->SetVQEData(playout_delay_, recording_delay_, 0); > > voe_audio_buffer_->DeliverRecordedData(); > > If you hold crit_sect_ while calling all of these functions (which, you > know, you need to, unless you can prove races with AttachAudioBuffer, etc., > can't happen), you are blocking RecorderSimpleBufferQueueCallbackHandler() > for at least as long as it takes them to execute. This means there is _no_ > advantage to doing all of this on a separate thread, not even a theoretical > "we shouldn't be doing any processing in the OpenSL ES callback because some > day it might not have its own independent thread" (which I don't buy anyway). > > I reiterate that the whole thread should just go away. It serves no purpose > except overhead and complexity. You have a point here, but more directly it points out that in the process of making this change, the crit_sect lock grew to include those functions, in the mods to handle not busy-waiting. The original unlocked around all three; all other desktop drivers unlock only around DeliverRecordedData() which seems a much better idea. > @@ +1428,5 @@ > > + if (rec_voe_ready_queue_.size() <= 0) { > > + // Log Error. > > + rec_error_ = 1; > > + WEBRTC_OPENSL_TRACE(kTraceError, kTraceAudioDevice, id_, > > + " Audio Rec thread buffers underrun"); > > Didn't notice this in my earlier review, but we don't change audio here, yet > we call Enqueue() anyway. That means that if this ever actually triggers, > we'll have two copies of this buffer running through the queues, with its > contents potentially getting overwritten before they can be used elsewhere, > or while they're being used. The chance of this is high because AudioRecord > delivers data to OpenSL ES in bursty (64 ms) chunks. > > We will never recover from this state. Every time the error triggers, it > will get worse. Clearly it shouldn't Enqueue in this case. Also, if this does happen, we'll end up with more than one buffer waiting to be enqueued in a future callback. (and there's a hole where if ALL the buffers end up being processed, then we won't get called back, and thus won't enqueue, and will be hung.) Clearly, we don't want to underrun....
Assignee | ||
Comment 11•11 years ago
|
||
Assignee | ||
Comment 12•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #785447 -
Flags: review?(tterribe)
Comment 13•11 years ago
|
||
Flag this bug to koi+ and add keyword in whiteboard for tracking in media recording agile team for FFOS 1.2. Since it is under review, I assume it will be resolved soon.
blocking-b2g: --- → koi+
Whiteboard: [FT: Media Recording, Sprint 2]
Comment 14•11 years ago
|
||
Hi Randell, May I know if there is any targeted date for this bug to be resolved? I am asking since this function is very important for getting the user media which is related to a big user story in FFOS 1.2 media recording team. Thank you very much. Ivan Tsay
Flags: needinfo?(rjesup)
Assignee | ||
Comment 15•11 years ago
|
||
Ivan - I'll be landing the initial 2 patches (or an update of them) as soon as they've both reviewed (still waiting on one). I'll likely incorporate comments on the patches from the upstream (webrtc.org) source that improve it further (getting rid of the magic 40ms value). These should land in 26. Note that the main effect here is performance (kill 1ms poll!), and removing TSAN data races, which in practice would be unlikely to cause problems (but that can't be guaranteed without examining the assembler generated by the compiler).
Flags: needinfo?(rjesup)
Comment 16•11 years ago
|
||
Setting a deadline of 9/4 to get this into our code.
Target Milestone: --- → mozilla26
Assignee | ||
Comment 17•11 years ago
|
||
also fixed sampling-rate initialization (which was getting overwritten immediately with the correct value, so it wasn't causing a bug)
Assignee | ||
Updated•11 years ago
|
Attachment #785447 -
Flags: review?(tterribe) → review?(paul)
Assignee | ||
Updated•11 years ago
|
Attachment #800256 -
Flags: review?(paul)
Updated•11 years ago
|
Attachment #785447 -
Flags: review?(paul) → review+
Updated•11 years ago
|
Attachment #800256 -
Flags: review?(paul) → review+
Assignee | ||
Comment 18•11 years ago
|
||
Landed as a merged patch from the three here: https://hg.mozilla.org/integration/mozilla-inbound/rev/a058786a8080
Comment 19•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a058786a8080
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Whiteboard: [FT: Media Recording, Sprint 2] → [FT: Media Recording, Sprint 2][qa-]
Assignee | ||
Comment 20•11 years ago
|
||
Assignee | ||
Comment 21•11 years ago
|
||
Comment on attachment 802741 [details] [diff] [review] (beta 24 version) clean up record issues in webrtc OpenSLES code + wallpaper to be done upstream and then will replace this [Approval Request Comment] Bug caused by (feature/regressing bug #): 904784 User impact if declined: some devices will fail to shut down camera and mic, and will lock it from other apps until the browser is force-quited Testing completed (on m-c, etc.): on m-c. Reviewed by padenot, derf and mwu, and the main Google engineer who handles the upstream code. Risk to taking this patch (and alternatives if risky): See email; additional risk on top of 904784 is low. String or IDL/UUID changes made by this patch: none
Attachment #802741 -
Flags: approval-mozilla-beta?
Attachment #802741 -
Flags: approval-mozilla-aurora?
Comment 22•11 years ago
|
||
Comment on attachment 802741 [details] [diff] [review] (beta 24 version) clean up record issues in webrtc OpenSLES code + wallpaper to be done upstream and then will replace this Approving as discussed via email.
Attachment #802741 -
Flags: approval-mozilla-beta?
Attachment #802741 -
Flags: approval-mozilla-beta+
Attachment #802741 -
Flags: approval-mozilla-aurora?
Attachment #802741 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 23•11 years ago
|
||
On both since beta->release merge has been done https://hg.mozilla.org/releases/mozilla-beta/rev/5e530d9b4cb5 https://hg.mozilla.org/releases/mozilla-release/rev/447a856a89bf
status-firefox24:
--- → fixed
status-firefox25:
--- → affected
Assignee | ||
Comment 24•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/3cb5810c6bce
status-firefox26:
--- → fixed
Comment 25•11 years ago
|
||
Merged to esr24 as well. https://hg.mozilla.org/releases/mozilla-esr24/rev/5e530d9b4cb5
status-firefox-esr24:
--- → fixed
Comment 27•11 years ago
|
||
Backed out from esr24 because it's mobile-only and shouldn't have been merged there. https://hg.mozilla.org/releases/mozilla-esr24/rev/c9b5ea714522
Updated•10 years ago
|
Blocks: webrtc_upstream_bugs
No longer depends on: webrtc_upstream_bugs
Updated•6 years ago
|
No longer blocks: webrtc_upstream_bugs
You need to log in
before you can comment on or make changes to this bug.
Description
•