If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 45, Firefox OS v2.5

Status

()

Core
Audio/Video: Playback
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jld, Assigned: jwwang)

Tracking

unspecified
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.5?, firefox45 fixed, b2g-v2.5 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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

2 years ago
The steps to reproduce are to simply run the test on a setup with => Linux VM running under VMware Fusion, without PulseAudio ?
Flags: needinfo?(jld)
Priority: -- → P4
Not a WebRTC bug
backlog: --- → webRTC+
Component: WebRTC: Audio/Video → Video/Audio
Priority: P4 → --
backlog: webRTC+ → ---
(Reporter)

Comment 3

2 years ago
(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.
Flags: needinfo?(jld)

Updated

2 years ago
Blocks: 1220015
(Assignee)

Updated

2 years ago
Assignee: nobody → jwwang
Component: Audio/Video → Audio/Video: Playback
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Comment 4

2 years ago
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().
(Assignee)

Comment 5

2 years ago
Created attachment 8681794 [details]
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.
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
(Assignee)

Comment 7

2 years ago
Thanks for the review!

Comment 8

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

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7811d9b3e7db
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45

Updated

2 years ago
Duplicate of this bug: 1220015

Comment 11

2 years ago
[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?

Comment 12

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/7811d9b3e7db
status-b2g-v2.5: --- → fixed
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
status-b2g-v2.5: fixed → ---

Updated

2 years ago
Flags: needinfo?(mpotharaju)
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
status-b2g-v2.5: --- → fixed
Flags: needinfo?(cbook)
Thanks Tomcat.

Comment 17

2 years ago
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.