1138253 - VideoPlaybackQuality stats are bogus

Status: RESOLVED FIXED

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

Description

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

The number of dropped frames includes the frames that have been demuxed but not yet presented. This therefore includes the frames being decoded as well as the ones in the queue for presentation. Corrupted frames is also incorrect in a similar way.

Comment 1

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

Attachment: Clean up AutoNotifyDecoded

Comment 2

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

Attachment: Count dropped frames directly

Comment 3

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3c7ad1f9aeb

Comment 4

[Matthew Gregan [:kinetik]]

Comment on attachment 8571136 [details] [diff] [review]
Clean up AutoNotifyDecoded

Might be worth fixing AutoNotifyDecoded to comply with https://developer.mozilla.org/en-US/docs/Using_RAII_classes_in_Mozilla while you're here, too.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8571137 [details] [diff] [review]
Count dropped frames directly

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +3137,5 @@
>      while (IsRealTime() || clock_time >= frame->mTime) {
>        mVideoFrameEndTime = frame->GetEndTime();
> +      if (currentFrame) {
> +        MediaDecoder::FrameStatistics& frameStats = mDecoder->GetFrameStatistics();
> +        frameStats.NotifyDecodedFrames(0, 0, 1);

You could just call mDecoder->NotifyDecodedFrames(0,0,1) here.

Comment 6

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3871f12a00a4

Comment 7

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

(In reply to Matthew Gregan [:kinetik] from comment #4)
> Comment on attachment 8571136 [details] [diff] [review]
> Clean up AutoNotifyDecoded
> 
> Might be worth fixing AutoNotifyDecoded to comply with
> https://developer.mozilla.org/en-US/docs/Using_RAII_classes_in_Mozilla while
> you're here, too.

The purpose of the change is to fix scoping issues. I don't think the above applies in this case.

Comment 8

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

Attachment: Count dropped frames directly

Comment 9

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/743bc389a2ce
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d806ade061f

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

Gonna need uplift for EME...

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/743bc389a2ce
https://hg.mozilla.org/mozilla-central/rev/0d806ade061f

Comment 12

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

Comment on attachment 8571136 [details] [diff] [review]
Clean up AutoNotifyDecoded

Approval Request Comment
[Feature/regressing bug #]: 962385
[User impact if declined]: Misreporting of dropped frames. MSE will compensate by dropping video quality
[Describe test coverage new/current, TreeHerder]:
Been on nightly for a day. Not well tested in automation.
[Risks and why]: Could potentially still be incorrect.
[String/UUID change made/needed]: None

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/97597242629b
https://hg.mozilla.org/releases/mozilla-aurora/rev/39379b6b90dc

Comment 14

[Lawrence Mandel [:lmandel] (use needinfo)]

cpearce - Do you want to test the correctness of this patch on Aurora before uplift to Beta?

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/releases/mozilla-beta/rev/620c8e250c76
https://hg.mozilla.org/releases/mozilla-beta/rev/487eadc4b118

Comment 16

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8571136 [details] [diff] [review]
Clean up AutoNotifyDecoded

Preapproved for MSE. Adding approval to the bug after the fact. Beta+ Aurora+