Let MFR decide skip-to-next-key-fremae alone.

RESOLVED FIXED in Firefox 56

Status

()

enhancement
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: kaku, Assigned: kaku)

Tracking

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(4 attachments, 2 obsolete attachments)

Assignee

Description

2 years ago
Spawn from bug 1360452 comment 34.
Assignee

Updated

2 years ago
Assignee: nobody → kaku
Status: NEW → ASSIGNED
Assignee

Updated

2 years ago
Blocks: 1360452
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 8

2 years ago
mozreview-review
Comment on attachment 8876628 [details]
Bug 1371188 P1 - remove MFR::ShouldSkip()'s aSkipToNextKeyframe parameter;

https://reviewboard.mozilla.org/r/147964/#review152290

::: dom/media/MediaFormatReader.cpp:1548
(Diff revision 1)
>  {
>    MOZ_ASSERT(HasVideo());
>    TimeUnit nextKeyframe;
>    nsresult rv = mVideo.mTrackDemuxer->GetNextRandomAccessPoint(&nextKeyframe);
>    if (NS_FAILED(rv)) {
> -    return aSkipToNextKeyframe;
> +    // Only OggTrackDemuxer with video type gets into here.

I don't think that comment is correct.

Comment 9

2 years ago
mozreview-review
Comment on attachment 8876629 [details]
Bug 1371188 P2 - remove ReRequestVideoWithSkipTask;

https://reviewboard.mozilla.org/r/147966/#review152292

::: dom/media/MediaDecoderReader.cpp
(Diff revision 1)
>    bool skip = aSkipToNextKeyframe;
>    while (VideoQueue().GetSize() == 0 &&
>           !VideoQueue().IsFinished()) {
>      if (!DecodeVideoFrame(skip, aTimeThreshold)) {
>        VideoQueue().Finish();
> -    } else if (skip) {

none of this code is in use anymore... see bug 1316211
can you make sure that none of the code touching MediaDecoderReader is published? it serves no purpose as it doesn't improvde anything (it's unused)

but it will force me to rebase all the work I've done for bug 1316211.
Assignee

Comment 11

2 years ago
mozreview-review-reply
Comment on attachment 8876628 [details]
Bug 1371188 P1 - remove MFR::ShouldSkip()'s aSkipToNextKeyframe parameter;

https://reviewboard.mozilla.org/r/147964/#review152290

> I don't think that comment is correct.

So, here is my thought.

We have 8 MediaTrackDemuxer implementations, ADTStrack, Flack, MP3, WAV, MediaSource, MP4, WebM, and Ogg.

ADTStrack, Flack, MP3 and WAV are audio-only, so we should never call it via RequestVideoData() -> ShouldSkip().

MediaSource, MP4, and WebM are for both audio and video. However, their GetNextRandomAccessPoint() method always returns NS_OK, so they have no way to pass the `if (NS_FAILED(rv)) {...}` statement.

Ogg is the only one supports audio and video type without overriding GetNextRandomAccessPoint(), and the default MediaTrackDemuxer::GetNextRandomAccessPoint() always returns NS_ERROR_NOT_IMPLEMENTED, so this is the only possibility to get into the if-statement.

Anything missed?
Assignee

Comment 12

2 years ago
mozreview-review-reply
Comment on attachment 8876629 [details]
Bug 1371188 P2 - remove ReRequestVideoWithSkipTask;

https://reviewboard.mozilla.org/r/147966/#review152292

> none of this code is in use anymore... see bug 1316211

I know it is going to be removed at bug 1316211 and bug 1316462.
But, at the moment, I think it is still used via AndroidMediaReader on Fennec with Android API < 15, no?
Assignee

Comment 13

2 years ago
(In reply to Jean-Yves Avenard [:jya] from comment #10)
> can you make sure that none of the code touching MediaDecoderReader is
> published? it serves no purpose as it doesn't improvde anything (it's unused)
> 
> but it will force me to rebase all the work I've done for bug 1316211.

At least, I need to remove MediaDecoderReader::RequestVideoData()'s "aSkipToNextKeyframe" parameter, so that I can complete the bug.

Or, I can postpone this bug until bug 1316211 is landed. Is bug 1316211 going to be landed soon?
Removing the parameter is fine. 

Thank you for your understanding.
Assignee

Comment 15

2 years ago
(In reply to Tzuhao Kuo [:kaku] from comment #11)
> Comment on attachment 8876628 [details]
> Bug 1371188 P1 - remove MFR::ShouldSkip()'s aSkipToNextKeyframe parameter;
> 
> https://reviewboard.mozilla.org/r/147964/#review152290
> 
> > I don't think that comment is correct.
> 
> So, here is my thought.
> 
> We have 8 MediaTrackDemuxer implementations, ADTStrack, Flack, MP3, WAV,
> MediaSource, MP4, WebM, and Ogg.
> 
> ADTStrack, Flack, MP3 and WAV are audio-only, so we should never call it via
> RequestVideoData() -> ShouldSkip().
> 
> MediaSource, MP4, and WebM are for both audio and video. However, their
> GetNextRandomAccessPoint() method always returns NS_OK, so they have no way
> to pass the `if (NS_FAILED(rv)) {...}` statement.
> 
> Ogg is the only one supports audio and video type without overriding
> GetNextRandomAccessPoint(), and the default
> MediaTrackDemuxer::GetNextRandomAccessPoint() always returns
> NS_ERROR_NOT_IMPLEMENTED, so this is the only possibility to get into the
> if-statement.
> 
> Anything missed?

:jya, is the analysis right? 

If only OggTrackDemuxer with video type gets into this `if (NS_FAILED(rv)) {...}` statement, I think it is acceptable to just done't supprt skip-to-next-key-frame for it.

Otherwise, we should try to hanlde them.
Flags: needinfo?(jyavenard)
sure
Flags: needinfo?(jyavenard)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8876629 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8876630 - Attachment is obsolete: true

Comment 22

2 years ago
mozreview-review
Comment on attachment 8876628 [details]
Bug 1371188 P1 - remove MFR::ShouldSkip()'s aSkipToNextKeyframe parameter;

https://reviewboard.mozilla.org/r/147964/#review154036
Attachment #8876628 - Flags: review?(jyavenard) → review+

Comment 23

2 years ago
mozreview-review
Comment on attachment 8876631 [details]
Bug 1371188 P2 - remove MediaDecoderReader::RequestVideoData()'s aSkipToNextKeyframe parameter;

https://reviewboard.mozilla.org/r/147970/#review154042

::: dom/media/MediaDecoderReaderWrapper.cpp:65
(Diff revision 2)
>               return AudioDataPromise::CreateAndReject(aError, __func__);
>             });
>  }
>  
>  RefPtr<MediaDecoderReaderWrapper::VideoDataPromise>
> -MediaDecoderReaderWrapper::RequestVideoData(bool aSkipToNextKeyframe,
> +MediaDecoderReaderWrapper::RequestVideoData(media::TimeUnit aTimeThreshold)

can you fix and use const media::TimeUnit& in a new patch ?

thanks
Attachment #8876631 - Flags: review?(jyavenard) → review+

Comment 24

2 years ago
mozreview-review
Comment on attachment 8876632 [details]
Bug 1371188 P3 - remove DecodingState::NeedToSkipToNextKeyframe();

https://reviewboard.mozilla.org/r/147972/#review154044

this breaks the legacy MediaDecoderReader (only AndroidDecoderRead left).

even more so the reason to get rid of it, but we have resistance there.
Attachment #8876632 - Flags: review?(jyavenard) → review+

Comment 25

2 years ago
mozreview-review
Comment on attachment 8876632 [details]
Bug 1371188 P3 - remove DecodingState::NeedToSkipToNextKeyframe();

https://reviewboard.mozilla.org/r/147972/#review154046

Comment 26

2 years ago
mozreview-review
Comment on attachment 8876633 [details]
Bug 1371188 P4 - add media.decoder.skip-to-next-key-frame.enabled pref;

https://reviewboard.mozilla.org/r/147974/#review154048
Attachment #8876633 - Flags: review?(jyavenard) → review+
Assignee

Comment 27

2 years ago
mozreview-review-reply
Comment on attachment 8876631 [details]
Bug 1371188 P2 - remove MediaDecoderReader::RequestVideoData()'s aSkipToNextKeyframe parameter;

https://reviewboard.mozilla.org/r/147970/#review154042

> can you fix and use const media::TimeUnit& in a new patch ?
> 
> thanks

Sure, will do it on a new bug.
Assignee

Comment 28

2 years ago
Thanks for the review!

Comment 29

2 years ago
Pushed by tkuo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/731c30f1c756
P1 - remove MFR::ShouldSkip()'s aSkipToNextKeyframe parameter; r=jya
https://hg.mozilla.org/integration/autoland/rev/9c2a9c99809b
P2 - remove MediaDecoderReader::RequestVideoData()'s aSkipToNextKeyframe parameter; r=jya
https://hg.mozilla.org/integration/autoland/rev/5ee041fcfc88
P3 - remove DecodingState::NeedToSkipToNextKeyframe(); r=jya
https://hg.mozilla.org/integration/autoland/rev/0aa40cfa600a
P4 - add media.decoder.skip-to-next-key-frame.enabled pref; r=jya
Assignee

Updated

2 years ago
Blocks: 1374138
Blocks: 1374189
Depends on: 1462728
You need to log in before you can comment on or make changes to this bug.