VideoPlaybackQuality stats are bogus

RESOLVED FIXED in Firefox 37

Status

()

defect
P2
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ajones, Assigned: ajones)

Tracking

(Blocks 2 bugs)

Trunk
mozilla39
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox37 fixed, firefox38 fixed, firefox39 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
Assignee: nobody → ajones
Status: NEW → ASSIGNED
Posted patch Count dropped frames directly (obsolete) — Splinter Review
Attachment #8571137 - Flags: review?(cpearce)
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.
Attachment #8571136 - Flags: review?(cpearce) → review+
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.
Attachment #8571137 - Flags: review?(cpearce) → review+
(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.
Attachment #8571137 - Attachment is obsolete: true

Updated

4 years ago
Priority: -- → P2
Gonna need uplift for EME...
Flags: needinfo?(cpearce)
https://hg.mozilla.org/mozilla-central/rev/743bc389a2ce
https://hg.mozilla.org/mozilla-central/rev/0d806ade061f
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
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
Attachment #8571136 - Flags: approval-mozilla-beta?
Attachment #8571136 - Flags: approval-mozilla-aurora?
Flags: needinfo?(cpearce)
Flags: needinfo?(cpearce)
cpearce - Do you want to test the correctness of this patch on Aurora before uplift to Beta?
Flags: needinfo?(cpearce)
Comment on attachment 8571136 [details] [diff] [review]
Clean up AutoNotifyDecoded

Preapproved for MSE. Adding approval to the bug after the fact. Beta+ Aurora+
Attachment #8571136 - Flags: approval-mozilla-beta?
Attachment #8571136 - Flags: approval-mozilla-beta+
Attachment #8571136 - Flags: approval-mozilla-aurora?
Attachment #8571136 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.