MozReview Request: Bug 1139256 - remove the assertion in AudioStream::GetPositionInFramesUnlocked(). See bug 1139256 comment 4 for the detail. r=kinetik.
40 bytes, text/x-review-board-request
With about 50% reliability, I can cause the MOZ_ASSERT(position >= mLastGoodPosition, "cubeb position shouldn't go backward") in AudioStream.cpp to fail in dom/media/test/test_eme_playback.html. This is a Linux VM running under VMware Fusion, without PulseAudio, so there are a few ways that timing could be weird or audio could be behaving in ways that aren't normally encountered. I added a printf, and in a few reproductions the backstep was always from 92160 to 91136 — i.e., from 90*1024 to 89*1024. I'm not sure how common or serious this is, but I thought that it was worth at least having a bug on file about it.
The steps to reproduce are to simply run the test on a setup with => Linux VM running under VMware Fusion, without PulseAudio ?
Priority: -- → P4
Not a WebRTC bug
backlog: --- → webRTC+
Component: WebRTC: Audio/Video → Video/Audio
Priority: P4 → --
(In reply to rshenthar from comment #1) > The steps to reproduce are to simply run the test on a setup with => Linux > VM running under VMware Fusion, without PulseAudio ? Those were the STR. I've just tried to reproduce it with current m-c and haven't been able to, over 10 or so runs — but the test either fails or goes into an infinite loop of "TypeError: $(...) is null" (ellipsis in original), so things might have changed so that this can't happen anymore and/or it might not even be getting to the part that previously failed.
Assignee: nobody → jwwang
Component: Audio/Video → Audio/Video: Playback
OS: Linux → All
Hardware: x86_64 → All
AudioStream::GetPositionInFramesUnlocked() can be called from 2 threads. One is from the audio thread and the other is from the state machine thread (via DecodedAudioDataSink::HasUnplayedFrames()). https://hg.mozilla.org/mozilla-central/file/96377bdbcdf3/dom/media/AudioStream.cpp#l625 There is a race in getting the position from cubeb_stream_get_position() outside the lock and updating the position to |mLastGoodPosition| inside the lock. Here is the call flow: 1. mLastGoodPosition is 100 to begin with. 2. cubeb_stream_get_position() returns 101 on thread 1. 3. cubeb_stream_get_position() returns 102 on thread 2. 4. |mLastGoodPosition| is updated to 102 on thread 2. 5. thread 1 hits the assertion for position==101 < mLastGoodPosition==102. The fix is simple. We will remove the assertion from AudioStream::GetPositionInFramesUnlocked() and assert the position returned from AudioStream::GetPosition() in DecodedAudioDataSink::GetPosition().
Bug 1139256 - remove the assertion in AudioStream::GetPositionInFramesUnlocked(). See bug 1139256 comment 4 for the detail. r=kinetik.
Attachment #8681794 - Flags: review?(kinetik)
Attachment #8681794 - Flags: review?(kinetik) → review+
Comment on attachment 8681794 [details] MozReview Request: Bug 1139256 - remove the assertion in AudioStream::GetPositionInFramesUnlocked(). See bug 1139256 comment 4 for the detail. r=kinetik. https://reviewboard.mozilla.org/r/23905/#review21343
Thanks for the review!
[Blocking Requested - why for this release]: This caused an assertion error when browsing YouTube with Browser app(bug 1220015), which is easy to happen in debug build of FxOS.
blocking-b2g: --- → 2.5?
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
Shian, AFAIK, this has been uplifted to 2.5. Tomcat,can you please confirm this. Thanks
Flags: needinfo?(mpotharaju) → needinfo?(cbook)
(In reply to Mahendra Potharaju [:mahe] from comment #14) > Shian, > > AFAIK, this has been uplifted to 2.5. > > Tomcat,can you please confirm this. > > Thanks its now https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/758d29222243
Do not know if the patch landed seamonkey already, but I just hit this: Assertion failure: position >= mLastGoodPosition (cubeb position shouldn't go backward), at /var/tmp/portage/www-client/seamonkey-2.39/work/seamonkey-2.39/mozilla/dom/media/AudioStream.cpp:860
You need to log in before you can comment on or make changes to this bug.