Closed Bug 1290371 Opened 3 years ago Closed 3 years ago

Assertion failure: (!HasAudio() || mAudio.mFirstDemuxedSampleTime.isSome()) && (!HasVideo() || mVideo.mFirstDemuxedSampleTime.isSome()), at dom/media/MediaFormatReader.cpp:1641

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla51
Tracking Status
firefox48 --- unaffected
firefox49 + fixed
firefox50 + fixed
firefox51 --- verified

People

(Reporter: jya, Assigned: kaku)

References

(Blocks 1 open bug, )

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

When playing a video which no video frame (but with a video track). Attempting to seek in the video will assert.

Assertion failure: (!HasAudio() || mAudio.mFirstDemuxedSampleTime.isSome()) && (!HasVideo() || mVideo.mFirstDemuxedSampleTime.isSome()), at /Users/jyavenard/Work/Mozilla/mozilla-central/dom/media/MediaFormatReader.cpp:1641
#01: mozilla::MediaFormatReader::DemuxStartTime()[/Users/jyavenard/Work/Mozilla/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3d4ee41]
#02: mozilla::MediaFormatReader::SetSeekTarget(mozilla::SeekTarget const&)[/Users/jyavenard/Work/Mozilla/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3d4eb9b]
#03: mozilla::MediaFormatReader::Seek(mozilla::SeekTarget, long long)[/Users/jyavenard/Work/Mozilla/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3d4ea39]
#04: RefPtr<mozilla::MozPromise<mozilla::media::TimeUnit, nsresult, true> > mozilla::detail::MethodCallInvokeHelper<RefPtr<mozilla::MozPromise<mozilla::media::TimeUnit, nsresult, true> >, mozilla::MediaDecoderReader, mozilla::SeekTarget, long long, 0ul, 1ul>(R[/Users/jyavenard/Work/Mozilla/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3d1a60f]
#05: mozilla::detail::MethodCall<mozilla::MozPromise<mozilla::media::TimeUnit, nsresult, true>, mozilla::MediaDecoderReader, mozilla::SeekTarget, long long>::Invoke()[/Users/jyavenard/Work/Mozilla/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3d1a4de]
#06: mozilla::detail::ProxyRunnable<mozilla::MozPromise<mozilla::media::TimeUnit, nsresult, true>, mozilla::MediaDecoderReader, mozilla::SeekTarget, long long>::Run()[/Users/jyavenard/Work/Mozilla/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3d1a2cf]
#07: mozilla::AutoTaskDispatcher::TaskGroupRunnable::Run()[/Users/jyavenard/Work/Mozilla/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1bd385]
#08: mozilla::TaskQueue::Runner::Run()[/Users/jyavenard/Work/Mozilla/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1a5fab]
#09: nsThreadPool::Run()[/Users/jyavenard/Work/Mozilla/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1b229e]
#10: nsThread::ProcessNextEvent(bool, bool*)[/Users/jyavenard/Work/Mozilla/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1aeae9]
#11: NS_ProcessNextEvent(nsIThread*, bool)[/Users/jyavenard/Work/Mozilla/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x239d1c]
#12: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*)[/Users/jyavenard/Work/Mozilla/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0xb94613]
#13: MessageLoop::RunInternal()[/Users/jyavenard/Work/Mozilla/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0xa9ce65]
#14: MessageLoop::RunHandler()[/Users/jyavenard/Work/Mozilla/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0xa9cdc5]
#15: MessageLoop::Run()[/Users/jyavenard/Work/Mozilla/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0xa9cd6d]
#16: nsThread::ThreadFunc(void*)[/Users/jyavenard/Work/Mozilla/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1ac3d8]
#17: _pt_root[/Users/jyavenard/Work/Mozilla/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/libnss3.dylib +0x359c6d]
#18: _pthread_body[/usr/lib/system/introspection/libsystem_pthread.dylib +0x3805]
#19: _pthread_body[/usr/lib/system/introspection/libsystem_pthread.dylib +0x3782]
(lldb) 

STR:
1. go to https://people.mozilla.org/~jyavenard/tests/ogg/ogg-wrongduration4.html
2. Press play
3. When playback reach the end, press play again.
boom
Flags: needinfo?(kaku)
So, I think we should remove the assertions and modify the code here, http://searchfox.org/mozilla-central/source/dom/media/MediaFormatReader.cpp#1642, to be:

  return std::min(HasAudio() && mAudio.mFirstDemuxedSampleTime.isSome()
                  ? mAudio.mFirstDemuxedSampleTime.ref()
                  : TimeUnit::FromInfinity(),
                  HasVideo() && mVideo.mFirstDemuxedSampleTime.isSome()
                  ? mVideo.mFirstDemuxedSampleTime.ref()
                  : TimeUnit::FromInfinity());

@jya, is it possible that a file has both audio and video tracks but cannot demux any audio/video data at all? If yes, then we should also return 0 for this special case.

BTW, this issue cannot be reproduced at the codecase around three weeks ago; I think it's before this chage:https://hg.mozilla.org/mozilla-central/diff/8577afd12830/dom/media/ogg/OggDecoder.cpp
Before that changeset, it seems that some video frames could be decoded, the video element does show something on the screen.
Flags: needinfo?(kaku) → needinfo?(jyavenard)
(In reply to Tzuhao Kuo [:kaku] from comment #1)
> @jya, is it possible that a file has both audio and video tracks but cannot
> demux any audio/video data at all? If yes, then we should also return 0 for
> this special case.

yes that is possible with a file that is made of only the metadata.

There's a few files like this in the mochitest (ogg files), and I've also seen such files with mp4.


> 
> BTW, this issue cannot be reproduced at the codecase around three weeks ago;
> I think it's before this
> chage:https://hg.mozilla.org/mozilla-central/diff/8577afd12830/dom/media/ogg/
> OggDecoder.cpp

that could be because you've haven't used a file with the problem. but there are definitely some out there.

So maybe we should mark this bug as regression, and ensure the fix goes in 49 too (or 50?)
Flags: needinfo?(jyavenard)
(In reply to Jean-Yves Avenard [:jya] from comment #2)
> (In reply to Tzuhao Kuo [:kaku] from comment #1)
> > @jya, is it possible that a file has both audio and video tracks but cannot
> > demux any audio/video data at all? If yes, then we should also return 0 for
> > this special case.
> 
> yes that is possible with a file that is made of only the metadata.
Then I will return 0 for this special case which means do not modify the seek target at all.

> > BTW, this issue cannot be reproduced at the codecase around three weeks ago;
> > I think it's before this
> > chage:https://hg.mozilla.org/mozilla-central/diff/8577afd12830/dom/media/ogg/
> > OggDecoder.cpp
> 
> that could be because you've haven't used a file with the problem. but there
> are definitely some out there.
Yup, this is definitely a bug and I should fix it.

> So maybe we should mark this bug as regression, and ensure the fix goes in 49 too (or 50?)
Not sure, the regression is the original framework was able to decode video frame but not now, which is not the root cause of this bug. So, I think another bug to point out regression is better.
Version: unspecified → Trunk
Assignee: nobody → kaku
https://reviewboard.mozilla.org/r/68534/#review65628

::: dom/media/MediaFormatReader.cpp:1663
(Diff revision 1)
> -                  : TimeUnit::FromInfinity(),
> -                  HasVideo()
> -                  ? mVideo.mFirstDemuxedSampleTime.ref()
> -                  : TimeUnit::FromInfinity());
> +
> +  if (HasAudio()) {
> +    if (mAudio.mFirstDemuxedSampleTime.isSome()) {
> +      audioStartTime = mAudio.mFirstDemuxedSampleTime.ref();
> +    } else {
> +      audioStartTime = TimeUnit::FromMicroseconds(0);

We can't assume start time is 0 when there is no sample at all in the track.
Comment on attachment 8776873 [details]
Bug 1290371 - handle files with audio and video tracks but no samples at all;

https://reviewboard.mozilla.org/r/68534/#review65636

::: dom/media/MediaFormatReader.cpp:1663
(Diff revision 1)
> -                  : TimeUnit::FromInfinity(),
> -                  HasVideo()
> -                  ? mVideo.mFirstDemuxedSampleTime.ref()
> -                  : TimeUnit::FromInfinity());
> +
> +  if (HasAudio()) {
> +    if (mAudio.mFirstDemuxedSampleTime.isSome()) {
> +      audioStartTime = mAudio.mFirstDemuxedSampleTime.ref();
> +    } else {
> +      audioStartTime = TimeUnit::FromMicroseconds(0);

If you have no audio frame, then it shouldn't come into the calculation and must be ignored.

::: dom/media/MediaFormatReader.cpp:1667
(Diff revision 1)
> +    } else {
> +      audioStartTime = TimeUnit::FromMicroseconds(0);
> +    }
> +  }
> +
> +  if (HasVideo()) {

same for video.
Attachment #8776873 - Flags: review?(jyavenard) → review-
So, we have 4 boolean variables and here is the truth table.
Case 1, 2, 3, 4, 7, 8, 10 and 12 are not possible to happen.

Then, the remaining cases could be clustered into three categories:
(1) Case 5, 9 and 13: no sample is demuxed at all, return 0.
(2) Case 6, 11, 14 and 15: either audio or video is able to be demuxed, return the known value.
(3) Case 15: both audio and video are demuxed, return the minimum of the values.


For simplifying the logic, I will initialize the audioStartTime and videoStartTime
to be INFINITY if we don't have the first-demuxed sample, otherwise, initialize
them to be the real first-demuxed sample's time.

Then, the final calculation will be:
(1) Case 5, 9 and 13: the minimum of two INFINITY values is still INFINITY, return 0.
(2) Case 6, 11, 14 and 15: return the minimum of one real first-demuxed-time and the INFINITY.
(3) Case 15: return the minimum of two real first-demuxed-time values.

Case   HasAudio   HasVideo   HasAudioSample   HasVideoSample    ExpectedResult
---------------------------------------------------------------------------------------------------
1         F          F             F                 F          not possible
2         F          F             F                 T          not possible
3         F          F             T                 F          not possible
4         F          F             T                 T          not possible
---------------------------------------------------------------------------------------------------
5         F          T             F                 F          return 0
6         F          T             F                 T          return video sample
7         F          T             T                 F          not possible
8         F          T             T                 T          not possible
---------------------------------------------------------------------------------------------------
9         T          F             F                 F          return 0
10        T          F             F                 T          not possible
11        T          F             T                 F          return audio sample
12        T          F             T                 T          not possible
---------------------------------------------------------------------------------------------------
13        T          T             F                 F          return 0
14        T          T             F                 T          return videoSample
15        T          T             T                 F          return audioSample
16        T          T             T                 T          return min(auidoSample, videoSample)

Review commit: https://reviewboard.mozilla.org/r/68850/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68850/
Attachment #8777294 - Flags: review?(jyavenard)
Attachment #8776873 - Attachment is obsolete: true
Attachment #8777294 - Flags: review?(jyavenard) → review+
Comment on attachment 8777294 [details]
Bug 1290371 - handle files with audio and video tracks but no samples at all;

https://reviewboard.mozilla.org/r/68850/#review66288

::: dom/media/MediaFormatReader.cpp:1659
(Diff revision 1)
> -  MOZ_ASSERT((!HasAudio() || mAudio.mFirstDemuxedSampleTime.isSome()) &&
> +  MOZ_ASSERT(HasAudio() || HasVideo());
> -             (!HasVideo() || mVideo.mFirstDemuxedSampleTime.isSome()));
>  
> -  return std::min(HasAudio()
> +  const TimeUnit startTime =
> +    std::min(HasAudio() && mAudio.mFirstDemuxedSampleTime.isSome()
> -                  ? mAudio.mFirstDemuxedSampleTime.ref()
> +             ? mAudio.mFirstDemuxedSampleTime.ref()

you could use mFirstDemuxedSampleTime.refOr(TimeUnit::FromInfinity()) instead
Comment on attachment 8777294 [details]
Bug 1290371 - handle files with audio and video tracks but no samples at all;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68850/diff/1-2/
Try looks good, thanks for the review!
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3e511763961
handle files with audio and video tracks but no samples at all; r=jya
Keywords: checkin-needed
Duplicate of this bug: 1286776
https://hg.mozilla.org/mozilla-central/rev/f3e511763961
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
note a real live website where this assertion is http://www.fonlabeni.com/ found via bughunter
Blocks: 532972
(In reply to Carsten Book [:Tomcat] from comment #16)
> note a real live website where this assertion is http://www.fonlabeni.com/
> found via bughunter

btw also verified fixed this assertion with the latest m-c build
(In reply to Carsten Book [:Tomcat] from comment #17)
> btw also verified fixed this assertion with the latest m-c build
Thanks for the verification!
@jya - Do you or :kaku want to request uplift?
Flags: needinfo?(jyavenard)
Comment on attachment 8777294 [details]
Bug 1290371 - handle files with audio and video tracks but no samples at all;

Approval Request Comment
[Feature/regressing bug #]:1272267
[User impact if declined]:Assertions
[Describe test coverage new/current, TreeHerder]: In central, manual test. Verified
[Risks and why]: none
[String/UUID change made/needed]: none
Flags: needinfo?(jyavenard)
Attachment #8777294 - Flags: approval-mozilla-beta?
Attachment #8777294 - Flags: approval-mozilla-aurora?
Tracked for 50 and 51 as it's a regression. Marking it verified for 51 based on comment 17.
Status: RESOLVED → VERIFIED
Comment on attachment 8777294 [details]
Bug 1290371 - handle files with audio and video tracks but no samples at all;

Fixed a bug in AV playback on some sites, verified on Nightly, Beta49+, Aurora50+
Attachment #8777294 - Flags: approval-mozilla-beta?
Attachment #8777294 - Flags: approval-mozilla-beta+
Attachment #8777294 - Flags: approval-mozilla-aurora?
Attachment #8777294 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.