Closed Bug 1230338 Opened 4 years ago Closed 4 years ago

Compositor frame drops are not recorded

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox42 --- affected
firefox43 + wontfix
firefox44 + fixed
firefox45 + fixed
b2g-v2.5 --- fixed

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1143575 added the ability for the compositor to drop video frames if we fall behind.

This currently isn't hooked up to the frame drop count we expose to JS, and it should be.
Attachment #8695538 - Flags: review?(jyavenard)
Attachment #8695538 - Attachment is patch: true
Comment on attachment 8695538 [details] [diff] [review]
debug-frame-drops

Review of attachment 8695538 [details] [diff] [review]:
-----------------------------------------------------------------

r=me if change to MediaFormatReader is removed.

Still very suspicious that this is required however as I do see the frame count reported by youtube increase outside the number of frames parsed vs decoded in the reader. (this has had caught my attention as I was benchmarking ffvp9 vs libvpx)

What other places are calling MediaDecoder::NotifyDecodedFrames other that the MediaDecoderReader?

::: dom/media/MediaFormatReader.cpp
@@ +519,5 @@
> +      // output queue and be used.
> +      // This code will count that as dropped, which was the intent, but not quite true.
> +      uint32_t queue = decoder.mSizeOfQueue;
> +      mDecoder->NotifyDecodedFrames(0, 0, queue);
> +    }

This is not required. SkipVideoDemuxToNextKeyFrame on success will call OnVideoSkipCompleted
which itself will call mDecoder->NotifyDecodedFrames(aSkipped, 0, aSkipped);
Attachment #8695538 - Flags: review?(jyavenard) → review+
(In reply to Jean-Yves Avenard [:jya] from comment #1)

> This is not required. SkipVideoDemuxToNextKeyFrame on success will call
> OnVideoSkipCompleted
> which itself will call mDecoder->NotifyDecodedFrames(aSkipped, 0, aSkipped);

I'm not convinced :)

SkipVideoDemuxToNextKeyFrame will call OnVideoSkipCompleted with the number of frames that the demux skipped when moving up the next keyframe.

That number won't include all the already demuxed frames that are currently in the decoder pipeline (being decoded, or awaiting reordering).

This change is trying to account for this latter set of frames (which get dropped when we issue the Flush to the decoder), because I can't see how we would otherwise count them as dropped.
Comment on attachment 8695538 [details] [diff] [review]
debug-frame-drops

Review of attachment 8695538 [details] [diff] [review]:
-----------------------------------------------------------------

my bad, you're right. I had overlooked this...

::: dom/media/MediaFormatReader.cpp
@@ +517,5 @@
> +      // and is currently sitting in our event queue waiting to be processed. The following
> +      // flush won't clear it, and when we return to the event loop it'll be added to our
> +      // output queue and be used.
> +      // This code will count that as dropped, which was the intent, but not quite true.
> +      uint32_t queue = decoder.mSizeOfQueue;

mDecoder->NotifyDecodedFrames(0, 0, SizeOfQueue(TrackInfo::kVideoTrack));
[Tracking Requested - why for this release]:
Web sites such as Netflix use the frame drop count to maintain a video bitrate that the CPU is able to decode. Fixing this bug will help to avoid poor video playback experience.
Comment on attachment 8695623 [details] [diff] [review]
Ensure type and subtype of MIME are lowercase.

wrong bug #
Attachment #8695623 - Attachment is obsolete: true
Attachment #8695623 - Flags: review?(bzbarsky)
https://hg.mozilla.org/mozilla-central/rev/88b7e78a9de9
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
I think this fix is coming too late for 43, as we are heading into the RC build next Monday.
Tracked for 44 as this will improve video quality and overall playback experience.
Matt, is this fix safe for uplift to Beta44?
Flags: needinfo?(matt.woodrow)
Comment on attachment 8695538 [details] [diff] [review]
debug-frame-drops

Approval Request Comment
[Feature/regressing bug #]: Bug 1143575
[User impact if declined]: Incorrect frame drop reporting, so Netflix/Youtube might pick an inappropriate resolution.
[Describe test coverage new/current, TreeHerder]: None, manually tested.
[Risks and why]: Very low risk.
[String/UUID change made/needed]: None
Flags: needinfo?(matt.woodrow)
Attachment #8695538 - Flags: approval-mozilla-beta?
Attachment #8695538 - Flags: approval-mozilla-aurora?
Comment on attachment 8695538 [details] [diff] [review]
debug-frame-drops

This has been on Nightly for almost 3 weeks, safe to uplift to Beta44, Aurora45.
Attachment #8695538 - Flags: approval-mozilla-beta?
Attachment #8695538 - Flags: approval-mozilla-beta+
Attachment #8695538 - Flags: approval-mozilla-aurora?
Attachment #8695538 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.