When suspending decode, the BlankDecoder shouldn't be wrapped in a H264Converter

RESOLVED FIXED in Firefox 51

Status

()

P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

(Blocks: 1 bug)

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
Issue presented in bug 1300296 comment 21.

when the video goes into the background, the current decoder is destroyed and switched for a BlankDecoder. Decoding continue where the previous decoder left off.

The H264Converter now ensures that the first frame seen is always a keyframe. But that means that when the video is suspended, all frames will be dropped until a new keyframes is seen.

It's probably harmless, but 1- it's unnecessary cycles wasted at monitoring the h264 streams we don't need and 2- it means some events could be missed while the video is in the background.
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Blocks: 1276556
Priority: -- → P3

Comment 2

2 years ago
mozreview-review
Comment on attachment 8788902 [details]
Bug 1301059: Do not use H264Converter when video decode is suspended.

https://reviewboard.mozilla.org/r/77224/#review75746

By this way, the extra data is not passed into the constructor of BlankMediaDataDecoder anymore, right?
I think we should also make the BlankMediaDataDecoder::mMaxRefFrames to be 16 for H264 cases.
http://searchfox.org/mozilla-central/rev/8eb4fd2c7be150b0aa1b05c0f3707e82dc8f2dc8/dom/media/platforms/agnostic/BlankDecoderModule.cpp#37
(Assignee)

Comment 3

2 years ago
mozreview-review-reply
Comment on attachment 8788902 [details]
Bug 1301059: Do not use H264Converter when video decode is suspended.

https://reviewboard.mozilla.org/r/77224/#review75746

good point.

Then in case we don't have a SPS, yes, using 16 instead is a safe approach....
feel free to take on this bug if you want to...
Okay, I will then upload a new patch for it; will also do it for bug 1301061.
Comment hidden (mozreview-request)

Comment 6

2 years ago
mozreview-review
Comment on attachment 8788902 [details]
Bug 1301059: Do not use H264Converter when video decode is suspended.

https://reviewboard.mozilla.org/r/77224/#review76516
Attachment #8788902 - Flags: review?(kaku)
(Assignee)

Comment 7

2 years ago
mozreview-review
Comment on attachment 8790099 [details]
Bug 1301059: Do not use H264Converter when video decode is suspended.

https://reviewboard.mozilla.org/r/78052/#review76870
Attachment #8790099 - Flags: review?(jyavenard) → review+
Thanks for the review, try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9eea166a46a
Keywords: checkin-needed

Comment 9

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/293b5a36dc0e
Do not use H264Converter when video decode is suspended. r=jya
Keywords: checkin-needed

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/293b5a36dc0e
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.