Closed Bug 1371188 Opened 6 years ago Closed 6 years ago

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

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: kaku, Assigned: kaku)

References

Details

Attachments

(4 files, 2 obsolete files)

Bug 1371188 P1 - remove MFR::ShouldSkip()'s aSkipToNextKeyframe parameter;
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1371188 P2 - remove MediaDecoderReader::RequestVideoData()'s aSkipToNextKeyframe parameter;
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1371188 P3 - remove DecodingState::NeedToSkipToNextKeyframe();
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1371188 P4 - add media.decoder.skip-to-next-key-frame.enabled pref;
59 bytes, text/x-review-board-request
jya
: review+
Details 
Spawn from bug 1360452 comment 34.
Assignee: nobody → kaku
Status: NEW → ASSIGNED
Blocks: 1360452
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 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.
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?
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?
(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.
(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)
Attachment #8876629 - Attachment is obsolete: true
Attachment #8876630 - Attachment is obsolete: true
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 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 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 on attachment 8876632 [details]
Bug 1371188 P3 - remove DecodingState::NeedToSkipToNextKeyframe();

https://reviewboard.mozilla.org/r/147972/#review154046
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+
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.
Thanks for the review!
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
Blocks: 1374138
Blocks: 1374189
Depends on: 1462728
You need to log in before you can comment on or make changes to this bug.