Closed Bug 1363668 Opened 2 years ago Closed 2 years ago

Decoding the first frame should always succeed

Categories

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

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: jya, Assigned: jya)

References

Details

Attachments

(4 files)

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.
(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?
At present, and error cause to skip to the next key frame. Including a crash if the OOP decoder.
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 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 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 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 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.
Attachment #8866683 - Flags: review?(jwwang) → review+
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 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
Attachment #8867551 - Flags: review?(jwwang) → review+
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.
Attachment #8866684 - Flags: review?(jwwang) → review+
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
Attachment #8867552 - Flags: review?(jwwang) → review+
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?
Flags: needinfo?(kikuo)
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.
(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]
Flags: needinfo?(kikuo)
(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.
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 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 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.
Depends on: 1366362
Pushed by jyavenard@mozilla.com:
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
Depends on: 1367376
No longer depends on: 1367376
You need to log in before you can comment on or make changes to this bug.