1230338 - Compositor frame drops are not recorded

Status: RESOLVED FIXED

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

Description

[Matt Woodrow (:mattwoodrow)]

Attachment: debug-frame-drops

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.

Comment 1

[Jean-Yves Avenard [:jya]]

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);

Comment 2

[Matt Woodrow (:mattwoodrow)]

(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 3

[Jean-Yves Avenard [:jya]]

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));

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/88b7e78a9de9

Comment 5

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

[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 6

[Jean-Yves Avenard [:jya]]

Attachment: Ensure type and subtype of MIME are lowercase.

As per RFC 2616 section 3.7, which states "The type, subtype, and parameter attribute names are case-insensitive". So ensure the type and subtype are always lower-case as all our comparisons assume that they are.

This version limits the change to nsContentTypeParser::GetType() which is exclusively used to request mimetype as defined by
RFC 2616 due to the caller context.

The reason I took this approach is because I found that working on the nsAString directly was far more cumbersome,  and would be greatly slower than working on the low level C-string. Not that this routine is used in critical loops as fas as I can tell.

It also helps abstract the retrieval of the MIME's type in one place.

Comment 7

[Jean-Yves Avenard [:jya]]

Comment on attachment 8695623 [details] [diff] [review]
Ensure type and subtype of MIME are lowercase.

wrong bug #

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/88b7e78a9de9

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

I think this fix is coming too late for 43, as we are heading into the RC build next Monday.

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for 44 as this will improve video quality and overall playback experience.

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Matt, is this fix safe for uplift to Beta44?

Comment 12

[Matt Woodrow (:mattwoodrow)]

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

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/0d5ea3ad3c40

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/0d5ea3ad3c40