1363668 - Decoding the first frame should always succeed

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

Right now, it is possible that decoding the first frame will not succeed and will make the MFR enter in WAITING_FOR_DATA mode.

An example of this would be:
- Attempt to decode the first frame
- The GPU process crash
- Skip to next keyframe
- Fallback to software decoding.

The presence of the next keyframe isn't always guaranteed.

Following discussion with JW, we want to always decode that first frame.
If it failed for whatever reason, we will seek back to the beginning and attempt to decode that same frame again.
If that fails again, it's a fatal error.

Comment 1

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

(In reply to Jean-Yves Avenard [:jya] from comment #0)
> An example of this would be:
> - Attempt to decode the first frame
> - The GPU process crash
> - Skip to next keyframe

When do we need to skip to next key frame while still decoding the 1st?

Comment 2

[Jean-Yves Avenard [:jya]]

At present, and error cause to skip to the next key frame. Including a crash if the OOP decoder.

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1363668: P1. Attempt to decode the first frame again if error occurred.

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

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1363668: P2. Don't attempt to continue decoding if no next keyframe.

When we have no future keyframe and an error occurred. We can't skip over anything and we shouldn't try to simply decode the next video frame. So we abort early.

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

Comment 5

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

Comment on attachment 8866684 [details]
Bug 1363668: P2. Don't attempt to continue decoding if no next keyframe.

https://reviewboard.mozilla.org/r/138298/#review141482

::: dom/media/MediaFormatReader.cpp:2351
(Diff revision 1)
>        SkipVideoDemuxToNextKeyFrame(
>          decoder.mLastDecodedSampleTime.refOr(TimeInterval()).Length());
>      } else if (aTrack == TrackType::kAudioTrack) {
>        decoder.Flush();
> +    } else {
> +      // We can't recover from this error.

We get here if (aTrack == TrackType::kVideoTrack && decoder.HasInternalSeekPending()). Do we want to try again when internal seeking is done?

Comment 6

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

Comment on attachment 8866683 [details]
Bug 1363668: P1. Attempt to decode the first frame again if error occurred.

https://reviewboard.mozilla.org/r/138296/#review141484

::: dom/media/MediaFormatReader.cpp:2339
(Diff revision 1)
> +      if (decoder.mFirstFrameTime) {
> +        TimeInterval seekInterval = TimeInterval(decoder.mFirstFrameTime.ref(),
> +                                                 decoder.mFirstFrameTime.ref());
> +        InternalSeek(aTrack, InternalSeekTarget(seekInterval, false));
> +      }
> +      return;

We should go on without returning from here if (!decoder.mFirstFrameTime && needsNewDecoder), right?

Comment 7

[Jean-Yves Avenard [:jya]]

Comment on attachment 8866684 [details]
Bug 1363668: P2. Don't attempt to continue decoding if no next keyframe.

https://reviewboard.mozilla.org/r/138298/#review141482

> We get here if (aTrack == TrackType::kVideoTrack && decoder.HasInternalSeekPending()). Do we want to try again when internal seeking is done?

good point...

Comment 8

[Jean-Yves Avenard [:jya]]

Comment on attachment 8866683 [details]
Bug 1363668: P1. Attempt to decode the first frame again if error occurred.

https://reviewboard.mozilla.org/r/138296/#review141484

> We should go on without returning from here if (!decoder.mFirstFrameTime && needsNewDecoder), right?

yes... I'll move return on the above block

Comment 9

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1363668: P3. Assert that no error can occur while an internal seek is pending.

An internal seek occurs when we've reached a data gap, EOS or a previous error got skipped.

During this time, the decoder has been either flushed or shut down.

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

Comment 10

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1363668: P4. Don't attempt to skip to next keyframe if there are none.

Demuxers don't typically return an error when they don't have a future keyframe, they return infinity.

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

Comment 11

[Jean-Yves Avenard [:jya]]

Comment on attachment 8866683 [details]
Bug 1363668: P1. Attempt to decode the first frame again if error occurred.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138296/diff/1-2/

Comment 12

[Jean-Yves Avenard [:jya]]

Comment on attachment 8866684 [details]
Bug 1363668: P2. Don't attempt to continue decoding if no next keyframe.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138298/diff/1-2/

Comment 13

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

Comment on attachment 8866683 [details]
Bug 1363668: P1. Attempt to decode the first frame again if error occurred.

https://reviewboard.mozilla.org/r/138296/#review142362

::: dom/media/MediaFormatReader.cpp:2329
(Diff revision 2)
>      decoder.mError.reset();
> +
>      LOG("%s decoded error count %d", TrackTypeToStr(aTrack),
>                                       decoder.mNumOfConsecutiveError);
> +
> +    if (decoder.mFirstFrameTime || needsNewDecoder) {

You can remove `if (decoder.mFirstFrameTime || needsNewDecoder)` and simply do:

if (needsNewDecoder) { ... }
if (decoder.mFirstFrameTime) { ... return; }

::: dom/media/MediaFormatReader.cpp:2786
(Diff revision 2)
>  {
>    MOZ_ASSERT(OnTaskQueue());
>    LOGV("Video seeked to %" PRId64, aTime.ToMicroseconds());
>    mVideo.mSeekRequest.Complete();
>  
> +  mVideo.mFirstFrameTime = Some(aTime);

We should overwrite |mFirstFrameTime| only before decoding 1st frame.

If OnVideoSeekCompleted() happens after decoding 1st frame, we don't want to change |mFirstFrameTime| from nothing to something.

Comment 14

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

Comment on attachment 8866684 [details]
Bug 1363668: P2. Don't attempt to continue decoding if no next keyframe.

https://reviewboard.mozilla.org/r/138298/#review142366

::: dom/media/MediaFormatReader.cpp:2351
(Diff revision 2)
>        SkipVideoDemuxToNextKeyFrame(
>          decoder.mLastDecodedSampleTime.refOr(TimeInterval()).Length());
>      } else if (aTrack == TrackType::kAudioTrack) {
>        decoder.Flush();
> +    } else {
> +      // We can't recover from this error.

revision 1 and 2 appear identical to me.

Comment 15

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

Comment on attachment 8867551 [details]
Bug 1363668: P3. Assert that no error can occur while an internal seek is pending.

https://reviewboard.mozilla.org/r/139094/#review142368

Comment 16

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

Comment on attachment 8866684 [details]
Bug 1363668: P2. Don't attempt to continue decoding if no next keyframe.

https://reviewboard.mozilla.org/r/138298/#review142370

::: dom/media/MediaFormatReader.cpp:2351
(Diff revision 2)
>        SkipVideoDemuxToNextKeyFrame(
>          decoder.mLastDecodedSampleTime.refOr(TimeInterval()).Length());
>      } else if (aTrack == TrackType::kAudioTrack) {
>        decoder.Flush();
> +    } else {
> +      // We can't recover from this error.

So you address the issue in P3.

Comment 17

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

Comment on attachment 8867552 [details]
Bug 1363668: P4. Don't attempt to skip to next keyframe if there are none.

https://reviewboard.mozilla.org/r/139096/#review142372

Comment 18

[Jean-Yves Avenard [:jya]]

The mochitests fail due to invalid data in some of the files being run.

All the ones failing (the first frame is invalid) were created in bug 1257116.

:kikuo, which tool did you use to create those files?

Comment 19

[Jean-Yves Avenard [:jya]]

Comment on attachment 8866683 [details]
Bug 1363668: P1. Attempt to decode the first frame again if error occurred.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138296/diff/2-3/

Comment 20

[Jean-Yves Avenard [:jya]]

Comment on attachment 8866684 [details]
Bug 1363668: P2. Don't attempt to continue decoding if no next keyframe.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138298/diff/2-3/

Comment 21

[Jean-Yves Avenard [:jya]]

Comment on attachment 8867551 [details]
Bug 1363668: P3. Assert that no error can occur while an internal seek is pending.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139094/diff/1-2/

Comment 22

[Jean-Yves Avenard [:jya]]

Comment on attachment 8867552 [details]
Bug 1363668: P4. Don't attempt to skip to next keyframe if there are none.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139096/diff/1-2/

Comment 23

[Jean-Yves Avenard [:jya]]

So it's not the one with audio that can't be decoded.
Even many of the first video frame can't be decoded:
like dom/media/test/test_seekToNextFrame.html | vp9-short.webm-0 timed out! [log…]

All those files need to be redone really.

We really should modify that default prefs when running mochitest so that we don't skip decode errors.

This is likely what caused the YouTube regression of bug 1361984 to go undetected by mochitest.

Comment 24

[Kilik Kuo [:kikuo] (inactive)]

(In reply to Jean-Yves Avenard [:jya] from comment #23)
> So it's not the one with audio that can't be decoded.
> Even many of the first video frame can't be decoded:
> like dom/media/test/test_seekToNextFrame.html | vp9-short.webm-0 timed out!
> [log…]
> 
> All those files need to be redone really.
> 
> We really should modify that default prefs when running mochitest so that we
> don't skip decode errors.
> 
> This is likely what caused the YouTube regression of bug 1361984 to go
> undetected by mochitest.

Haven't had a chance to look into decoder. I used "ffprobe -show_frames vp9-short.webm" to dump the frame information.
There are 7 frames in this file, 1 keyframe followed by 6 non-keyframes, and the information of first 7 frames are the same as vp9.webm.

Besides, VLC media player & Totem movie player & Chrome are able to play the shorten file, I don't think the timed out here is related to first frame decode error. 

I'll check other shorten files tomorrow.
Does the file have to contain audio ?

=================================================
[FRAME]
media_type=video
stream_index=0
key_frame=1
pkt_pts=0
pkt_pts_time=0.000000
pkt_dts=0
pkt_dts_time=0.000000
best_effort_timestamp=0
best_effort_timestamp_time=0.000000
pkt_duration=33
pkt_duration_time=0.033000
pkt_pos=458
pkt_size=1973
width=320
height=240
pix_fmt=yuv420p
sample_aspect_ratio=1:1
pict_type=I
coded_picture_number=0
display_picture_number=0
interlaced_frame=0
top_field_first=0
repeat_pict=0
[/FRAME]
[FRAME]
media_type=video
stream_index=0
key_frame=0
pkt_pts=33
pkt_pts_time=0.033000
pkt_dts=33
pkt_dts_time=0.033000
best_effort_timestamp=33
best_effort_timestamp_time=0.033000
pkt_duration=33
pkt_duration_time=0.033000
pkt_pos=2437
pkt_size=53
width=320
height=240
pix_fmt=yuv420p
sample_aspect_ratio=1:1
pict_type=P
coded_picture_number=0
display_picture_number=0
interlaced_frame=0
top_field_first=0
repeat_pict=0
[/FRAME]
[FRAME]
media_type=video
stream_index=0
key_frame=0
pkt_pts=67
pkt_pts_time=0.067000
pkt_dts=67
pkt_dts_time=0.067000
best_effort_timestamp=67
best_effort_timestamp_time=0.067000
pkt_duration=33
pkt_duration_time=0.033000
pkt_pos=2496
pkt_size=52
width=320
height=240
pix_fmt=yuv420p
sample_aspect_ratio=1:1
pict_type=P
coded_picture_number=0
display_picture_number=0
interlaced_frame=0
top_field_first=0
repeat_pict=0
[/FRAME]
[FRAME]
media_type=video
stream_index=0
key_frame=0
pkt_pts=100
pkt_pts_time=0.100000
pkt_dts=100
pkt_dts_time=0.100000
best_effort_timestamp=100
best_effort_timestamp_time=0.100000
pkt_duration=33
pkt_duration_time=0.033000
pkt_pos=2554
pkt_size=82
width=320
height=240
pix_fmt=yuv420p
sample_aspect_ratio=1:1
pict_type=P
coded_picture_number=0
display_picture_number=0
interlaced_frame=0
top_field_first=0
repeat_pict=0
[/FRAME]
[FRAME]
media_type=video
stream_index=0
key_frame=0
pkt_pts=133
pkt_pts_time=0.133000
pkt_dts=133
pkt_dts_time=0.133000
best_effort_timestamp=133
best_effort_timestamp_time=0.133000
pkt_duration=33
pkt_duration_time=0.033000
pkt_pos=2643
pkt_size=149
width=320
height=240
pix_fmt=yuv420p
sample_aspect_ratio=1:1
pict_type=P
coded_picture_number=0
display_picture_number=0
interlaced_frame=0
top_field_first=0
repeat_pict=0
[/FRAME]
[FRAME]
media_type=video
stream_index=0
key_frame=0
pkt_pts=167
pkt_pts_time=0.167000
pkt_dts=167
pkt_dts_time=0.167000
best_effort_timestamp=167
best_effort_timestamp_time=0.167000
pkt_duration=33
pkt_duration_time=0.033000
pkt_pos=2799
pkt_size=275
width=320
height=240
pix_fmt=yuv420p
sample_aspect_ratio=1:1
pict_type=P
coded_picture_number=0
display_picture_number=0
interlaced_frame=0
top_field_first=0
repeat_pict=0
[/FRAME]

Comment 25

[Jean-Yves Avenard [:jya]]

(In reply to Kilik Kuo [:kikuo] from comment #24)
> (In reply to Jean-Yves Avenard [:jya] from comment #23)
> > > 
> Haven't had a chance to look into decoder. I used "ffprobe -show_frames
> vp9-short.webm" to dump the frame information.
> There are 7 frames in this file, 1 keyframe followed by 6 non-keyframes, and
> the information of first 7 frames are the same as vp9.webm.
> 
> Besides, VLC media player & Totem movie player & Chrome are able to play the
> shorten file, I don't think the timed out here is related to first frame
> decode error. 
> 

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

It certainly is..

With the changes contained in this patch, we attempt to decode the first frame several times until it succeeds

That first frame being invalid, causes the entire media to fail, as decoding that first frame always fail.

You can achieve the same results with current nightly by setting the preferences media.audio-max-decode-error and media.video-max-decode-error to 0. In which case we don't ignore decode errors.
All the media mochitests will now fail.

we never noticed because our player is too lenient.

Comment 26

[Kilik Kuo [:kikuo] (inactive)]

I found un-shorten files, i.e. gizmo.mp4, won't play correctly after setting pref |media.video-max-decode-error=0| and |media.audio-max-decode-error=0| even without these patches applied ...
Because the 1st audio frame cannot be decoded

Comment 27

[Jean-Yves Avenard [:jya]]

Comment on attachment 8866683 [details]
Bug 1363668: P1. Attempt to decode the first frame again if error occurred.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138296/diff/3-4/

Comment 28

[Jean-Yves Avenard [:jya]]

Comment on attachment 8866684 [details]
Bug 1363668: P2. Don't attempt to continue decoding if no next keyframe.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138298/diff/3-4/

Comment 29

[Jean-Yves Avenard [:jya]]

Comment on attachment 8867551 [details]
Bug 1363668: P3. Assert that no error can occur while an internal seek is pending.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139094/diff/2-3/

Comment 30

[Jean-Yves Avenard [:jya]]

Comment on attachment 8867552 [details]
Bug 1363668: P4. Don't attempt to skip to next keyframe if there are none.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139096/diff/2-3/

Comment 31

[Jean-Yves Avenard [:jya]]

Comment on attachment 8866683 [details]
Bug 1363668: P1. Attempt to decode the first frame again if error occurred.

https://reviewboard.mozilla.org/r/138296/#review142362

> We should overwrite |mFirstFrameTime| only before decoding 1st frame.
> 
> If OnVideoSeekCompleted() happens after decoding 1st frame, we don't want to change |mFirstFrameTime| from nothing to something.

No. OnVideoSeekCompleted() is called when the seek completed. Not when the first decoded frame after the seek is returned.
As mentioned in the comment mFirstFrameTime is the first frame time. The first frame is either the first frame ever decoded, or the first frame decoded following a seek.

Storing the time where the demuxer actually seeked to is better as it guarantees you'll end up exactly in the same spot should a decoding error occurs.

Comment 32

[Jean-Yves Avenard [:jya]]

Comment on attachment 8866684 [details]
Bug 1363668: P2. Don't attempt to continue decoding if no next keyframe.

https://reviewboard.mozilla.org/r/138298/#review142366

> revision 1 and 2 appear identical to me.

yes, it was just a rebase so that the try run would succeed. While reviewboard only show you the 4 patches, there were a whole set of patches that aren't ready to be tested.

Comment 33

[Jean-Yves Avenard [:jya]]

Comment on attachment 8866683 [details]
Bug 1363668: P1. Attempt to decode the first frame again if error occurred.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138296/diff/4-5/

Comment 34

[Jean-Yves Avenard [:jya]]

Comment on attachment 8866684 [details]
Bug 1363668: P2. Don't attempt to continue decoding if no next keyframe.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138298/diff/4-5/

Comment 35

[Jean-Yves Avenard [:jya]]

Comment on attachment 8867551 [details]
Bug 1363668: P3. Assert that no error can occur while an internal seek is pending.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139094/diff/3-4/

Comment 36

[Jean-Yves Avenard [:jya]]

Comment on attachment 8867552 [details]
Bug 1363668: P4. Don't attempt to skip to next keyframe if there are none.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139096/diff/3-4/

Comment 37

[Jean-Yves Avenard [:jya]]

Comment on attachment 8866683 [details]
Bug 1363668: P1. Attempt to decode the first frame again if error occurred.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138296/diff/5-6/

Comment 38

[Jean-Yves Avenard [:jya]]

Comment on attachment 8866684 [details]
Bug 1363668: P2. Don't attempt to continue decoding if no next keyframe.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138298/diff/5-6/

Comment 39

[Jean-Yves Avenard [:jya]]

Comment on attachment 8867551 [details]
Bug 1363668: P3. Assert that no error can occur while an internal seek is pending.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139094/diff/4-5/

Comment 40

[Jean-Yves Avenard [:jya]]

Comment on attachment 8867552 [details]
Bug 1363668: P4. Don't attempt to skip to next keyframe if there are none.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139096/diff/4-5/

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3132da9afcb8
P1. Attempt to decode the first frame again if error occurred. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/610afba2f5b2
P2. Don't attempt to continue decoding if no next keyframe. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/156712916255
P3. Assert that no error can occur while an internal seek is pending. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/a0052d6a0eb1
P4. Don't attempt to skip to next keyframe if there are none. r=jwwang

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3132da9afcb8
https://hg.mozilla.org/mozilla-central/rev/610afba2f5b2
https://hg.mozilla.org/mozilla-central/rev/156712916255
https://hg.mozilla.org/mozilla-central/rev/a0052d6a0eb1