1139256 - MOZ_ASSERT(position >= mLastGoodPosition, "cubeb position shouldn't go backward") can fail in dom/media/test/test_eme_playback.html

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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.

Comment 1

[rshenthar]

The steps to reproduce are to simply run the test on a setup with => Linux VM running under VMware Fusion, without PulseAudio ?

Comment 2

[Maire Reavy [:mreavy]]

Not a WebRTC bug

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(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.

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

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().

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

Attachment: MozReview Request: Bug 1139256 - remove the assertion in AudioStream::GetPositionInFramesUnlocked(). See bug 1139256 comment 4 for the detail. r=kinetik.

Bug 1139256 - remove the assertion in AudioStream::GetPositionInFramesUnlocked(). See bug 1139256 comment 4 for the detail. r=kinetik.

Comment 6

[Matthew Gregan [:kinetik]]

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

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

Thanks for the review!

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7811d9b3e7db

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7811d9b3e7db

Comment 11

[Shian-Yow Wu [:swu]]

[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.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/7811d9b3e7db

Comment 13

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion

Comment 14

[Mahendra Potharaju [:mahe]]

Shian, 

AFAIK, this has been uplifted to 2.5.

Tomcat,can you please confirm this. 

Thanks

Comment 15

[Carsten Book [:Tomcat]]

(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

Comment 16

[Mahendra Potharaju [:mahe]]

Thanks Tomcat.

Comment 17

[Martin Mokrejs]

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