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

Status: VERIFIED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

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

Comment 1

[Tzuhao Kuo [: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.

Comment 2

[Jean-Yves Avenard [:jya]]

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

Comment 3

[Tzuhao Kuo [:kaku]]

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

Comment 4

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1290371 - handle files with audio and video tracks but no samples at all;

Review commit: https://reviewboard.mozilla.org/r/68534/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68534/

Comment 5

[Tzuhao Kuo [:kaku]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=882fb3d40c20

Comment 6

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

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 7

[Jean-Yves Avenard [:jya]]

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.

Comment 8

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1290371 - handle files with audio and video tracks but no samples at all;

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/

Comment 9

[Jean-Yves Avenard [:jya]]

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 10

[Tzuhao Kuo [:kaku]]

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/

Comment 11

[Tzuhao Kuo [:kaku]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb71b9f1645b

Comment 12

[Tzuhao Kuo [:kaku]]

Try looks good, thanks for the review!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3e511763961
handle files with audio and video tracks but no samples at all; r=jya

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f3e511763961

Comment 16

[Carsten Book [:Tomcat]]

note a real live website where this assertion is http://www.fonlabeni.com/ found via bughunter

Comment 17

[Carsten Book [:Tomcat]]

(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

Comment 18

[Tzuhao Kuo [:kaku]]

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

Comment 19

[Maire Reavy [:mreavy]]

@jya - Do you or :kaku want to request uplift?

Comment 20

[Jean-Yves Avenard [:jya]]

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

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for 50 and 51 as it's a regression. Marking it verified for 51 based on comment 17.

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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+

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/10d3f6d868e5

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/65ab7976703a