Add statistics overlay to videocontrols

VERIFIED FIXED in mozilla10

Status

()

Toolkit
Video/Audio Controls
VERIFIED FIXED
7 years ago
5 years ago

People

(Reporter: Dolske, Assigned: jaws)

Tracking

({verified-beta})

unspecified
mozilla10
verified-beta
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-fx-team] [qa!])

Attachments

(3 attachments, 10 obsolete attachments)

702.97 KB, image/png
shorlander
: ui-review+
Details
29.01 KB, patch
Dolske
: review+
Details | Diff | Splinter Review
33.89 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
Created attachment 543885 [details] [diff] [review]
Patch v.1 (WIP)

I thought it might be neat to have a statistics overlay for the videocontrols. Attached is a proof-of-concept... You can right-click a video, select Show Statistics, and some text and numbers appear. I haven't actually hooked up the stats (they're just static strings), but that's the easy part.

Most of the glue to put this together is done, except for two wonky bits:

1) Need a better way to allow chrome to fiddle with the video control binding. ISTR that in the past document.getBoxObjectFor(video) was a way to peer into the native anonymous content, but doesn't seem to work now. Maybe the controls should just glue a method onto the video.

2) The CSS styling for the <table> is being a PITA. The namespace bits are fragile, and I'm getting oddness with rules not seeming to apply. Mixing XBL+XUL+HTML+NAC makes me cry a little on the inside.
(Reporter)

Comment 1

7 years ago
Created attachment 543886 [details]
screenshot
(Reporter)

Comment 2

7 years ago
Created attachment 544946 [details] [diff] [review]
Patch v.2 (WIP)

Stats now live in overlay.
Attachment #543885 - Attachment is obsolete: true
Assignee: nobody → jwein
Created attachment 555627 [details]
Screenshot of patch

Stephen: Can you please provide feedback on this statistics overlay?
Attachment #543886 - Attachment is obsolete: true
Attachment #555627 - Flags: feedback?(shorlander)
Created attachment 555628 [details] [diff] [review]
Patch for bug 669260

I have cleaned up your WIP from earlier.
Attachment #544946 - Attachment is obsolete: true
Attachment #555628 - Flags: review?(dolske)
Comment on attachment 555627 [details]
Screenshot of patch

Are you calculating the frames dropped fields correctly? You're reporting that almost all frames are dropped.
(In reply to Chris Pearce (:cpearce) (Mozilla Corporation) from comment #5)
> Comment on attachment 555627 [details]
> Screenshot of patch
> 
> Are you calculating the frames dropped fields correctly? You're reporting
> that almost all frames are dropped.

I believe the calculation is correct. I created that screenshot by heavy scrubbing on the timeline so the numbers would be more visible. Chris, can you provide feedback on the accompanied patch?
Attachment #555628 - Flags: feedback?(chris)
Comment on attachment 555628 [details] [diff] [review]
Patch for bug 669260


>+  let newFrames = (v.mozParsedFrames - s.framesParsedBase);
>+  s.framesParsed.setAttribute("value", newFrames);
>+  s.decoderDrops.setAttribute("value", -(v.mozDecodedFrames   - s.framesDecodedBase   - newFrames));
>+  s.pipelineDrops.setAttribute("value", -(v.mozPresentedFrames - s.framesPresentedBase - newFrames));
>+  s.paintDrops.setAttribute("value", -(v.mozPaintedFrames   - s.framesPaintedBase   - newFrames));

This is your problem. You're assuming that newFrames is the difference between each counter, but it's the number of frames parsed since reporting began.

Note that mozParsedFrames >= mozDecodedFrames >= mozPresentedFrames >= mozPaintedFrames. With perfect performance mozParsedFrames==mozPaintedFrames.

You don't need to store frames*Base, you can just calculate the difference between each value and its predecessor in the pipeline.

e.g.:

 s.framesParsed.setAttribute("value", v.mozParsedFrames);
 s.decoderDrops.setAttribute("value", v.mozParsedFrames - v.mozDecodedFrames);
 s.pipelineDrops.setAttribute("value", v.mozDecodedFrames - v.mozPresentedFrames);
 s.paintDrops.setAttribute("value", v.mozPresentedFrames - v.mozPaintedFrames);
Attachment #555628 - Flags: feedback?(chris)
Created attachment 555799 [details] [diff] [review]
Patch for bug 669260 v2

Thank you for your feedback Chris.

After some more testing, I have found that it is possible to have mozPaintedFrames > mozPresentedFrames. This is from heavy scrubbing on the timeline. Due to this, I have changed the output of the statistics to simply show the numbers reported directly by the moz*Frames instead of trying to make calculations based off of them.

Updated screenshot to follow.
Attachment #555628 - Attachment is obsolete: true
Attachment #555799 - Flags: review?(dolske)
Attachment #555628 - Flags: review?(dolske)
Created attachment 555803 [details]
Screenshot of patch
Attachment #555627 - Attachment is obsolete: true
Attachment #555803 - Flags: feedback?(shorlander)
Attachment #555627 - Flags: feedback?(shorlander)
I have also changed this patch to report statistics for the lifetime of the video, instead of relative to the opening of the statistics overlay.
Status: NEW → ASSIGNED
(In reply to Jared Wein [:jwein] from comment #8)
> After some more testing, I have found that it is possible to have
> mozPaintedFrames > mozPresentedFrames. This is from heavy scrubbing on the
> timeline.

This is a bug. I filed Bug 682736 for this.
Comment on attachment 555803 [details]
Screenshot of patch

Looks good. Although the space between the name and value make them look like separate things.

Maybe make them closer together and/or try bolding the name.

The second column also looks kind of empty. Does other stuff potentially go there?
Attachment #555803 - Flags: feedback?(shorlander) → feedback+
Created attachment 557302 [details]
Screenshot of patch

Made the labels bold and also made the columns a little narrower.

The filename and size are allowed almost the full width of the table. We might not need that much width for the file size, but the file name could be long.

There are four empty cells on the right column of the table. We could make the frame statistics be 2x2 instead of 1x4, but I think it is easier to watch the numbers change without shifting your visual focus if they are linear.
Attachment #555803 - Attachment is obsolete: true
Attachment #557302 - Flags: ui-review?(shorlander)
Created attachment 557305 [details] [diff] [review]
Patch for bug 669260

Updated patch based on feedback from shorlander.
Attachment #555799 - Attachment is obsolete: true
Attachment #557305 - Flags: review?(dolske)
Attachment #555799 - Flags: review?(dolske)
Created attachment 557343 [details] [diff] [review]
Patch for bug 669260

Added code to prevent showing the statistics overlay if the video is too small to fit them.
Attachment #557305 - Attachment is obsolete: true
Attachment #557343 - Flags: review?(dolske)
Attachment #557305 - Flags: review?(dolske)
Attachment #557302 - Flags: ui-review?(shorlander) → ui-review+
Created attachment 561322 [details] [diff] [review]
Patch for bug 669260

Updated context menu unit test and made the Statistics context menu item available even when controls are hidden.
Attachment #557343 - Attachment is obsolete: true
Attachment #561322 - Flags: review?(dolske)
Attachment #557343 - Flags: review?(dolske)
(Reporter)

Comment 17

7 years ago
Comment on attachment 561322 [details] [diff] [review]
Patch for bug 669260

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

::: toolkit/content/widgets/videocontrols.xml
@@ +200,5 @@
>              </vbox>
>  
> +            <vbox class="statsOverlay" hidden="true">
> +                <html:div>
> +                    <html:table class="statsTable" xmlns="http://www.w3.org/1999/xhtml">

I think the xmlns can move up onto the div, so it can just be <table> instead of <html:table>? Though, hmm, seems like the div isn't even needed. I don't remember why I had it there, deadwood or because table doesn't place nice as a direct child of XUL?

@@ +202,5 @@
> +            <vbox class="statsOverlay" hidden="true">
> +                <html:div>
> +                    <html:table class="statsTable" xmlns="http://www.w3.org/1999/xhtml">
> +                        <tr>
> +                            <td class="statLabel">Media</td>

Alas, I think that we actually should localize the statLabels. We've already got a videocontrols.dtd, so this should be easy.

@@ +207,5 @@
> +                            <td colspan="3" class="statValue filename"><xul:label class="statFilename"/></td>
> +                        </tr>
> +                        <tr>
> +                            <td class="statLabel">Size</td>
> +                            <td colspan="3" class="statValue size"><xul:label class="statSize"/></td>

Uhh, why did I use xul:label here? No idea. Seems like we should just make these <span>s. HTML ftw.

@@ +216,5 @@
> +                            <td class="statLabel">Activity</td> <td class="statValue activity"><xul:label class="statActivity"/></td>
> +                            <td class="statLabel">Volume</td> <td class="statValue"><xul:label class="statVolume"/></td>
> +                        </tr>
> +                        <tr>
> +                            <td class="statLabel">readyState</td> <td class="statValue"><xul:label class="statReadyState"/></td>

.readyState and .networkState should probably have localization notes saying they refer to to the HTML5 API properties, and suggest not translating them.

@@ +814,5 @@
> +                      this.statsOverlay.hidden = false;
> +                    }
> +                    if (this._getComputedPropertyValueAsInt(this.statsTable, "height") > this._getComputedPropertyValueAsInt(this.video, "height") ||
> +                        this._getComputedPropertyValueAsInt(this.statsTable, "width") > this._getComputedPropertyValueAsInt(this.video, "width"))
> +                        shouldShow = false;

What happens if we show the stats on a too-small video? Seems like we should just ignore the video size, it's not a big deal if the user opts to show the overlay but it's too small.

@@ +825,5 @@
> +                        this.statsOverlay.hidden = false;
> +                        this.statsInterval = setInterval(this.updateStats.bind(this), this.STATS_INTERVAL_MS);
> +                        this.updateStats();
> +                    } else {
> +                        this.video.mozMediaStatisticsShowing = false;

Should just use |delete| here instead, so when it's disabled there's no explicit sign of it.

@@ +861,5 @@
> +                    if (v.ended)
> +                        activity = "ended";
> +                    if (v.seeking)
> +                        activity += " (seeking)";
> +                    s.activity.setAttribute("value", activity);

We should localize these too. Can't use string bundles, nor dynamically use entities, so might need to selectively hide/unhide spans containing each state.

@@ +1019,5 @@
>  
>                  init : function (binding) {
>                      this.video = binding.parentNode;
>                      this.videocontrols = binding;
> +                    this.video.mozMediaControlsBinding = binding;

This was just a quick hack, we don't want to do it because it exposes the binding to content (and then things may start using it, thus changes to Firefox's controls become a webcompat issue).

We need to find a chrome-only way to get at the binding. Or be tricky and control it by setting an attribute on the <video> and, say, watching for it on mouseout/blur. Not sure. :/
Attachment #561322 - Flags: review?(dolske) → review-
(In reply to Justin Dolske [:Dolske] from comment #17)
> Comment on attachment 561322 [details] [diff] [review] [diff] [details] [review]
> Patch for bug 669260
> 
> Review of attachment 561322 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/content/widgets/videocontrols.xml
> @@ +1019,5 @@
> >  
> >                  init : function (binding) {
> >                      this.video = binding.parentNode;
> >                      this.videocontrols = binding;
> > +                    this.video.mozMediaControlsBinding = binding;
> 
> This was just a quick hack, we don't want to do it because it exposes the
> binding to content (and then things may start using it, thus changes to
> Firefox's controls become a webcompat issue).
> 
> We need to find a chrome-only way to get at the binding. Or be tricky and
> control it by setting an attribute on the <video> and, say, watching for it
> on mouseout/blur. Not sure. :/

I'm not sure I understand the usage of mouseout/blur here. Could we just set an attribute on the <video>, call |setInterval| within |init|, and then short-circuit the updateStats method if the attribute isn't set?

Comment 19

7 years ago
(In reply to Jared Wein [:jwein] from comment #18)
> (In reply to Justin Dolske [:Dolske] from comment #17)
> > Comment on attachment 561322 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > Patch for bug 669260
> > 
> > Review of attachment 561322 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: toolkit/content/widgets/videocontrols.xml
> > @@ +1019,5 @@
> > >  
> > >                  init : function (binding) {
> > >                      this.video = binding.parentNode;
> > >                      this.videocontrols = binding;
> > > +                    this.video.mozMediaControlsBinding = binding;
> > 
> > This was just a quick hack, we don't want to do it because it exposes the
> > binding to content (and then things may start using it, thus changes to
> > Firefox's controls become a webcompat issue).
> > 
> > We need to find a chrome-only way to get at the binding. Or be tricky and
> > control it by setting an attribute on the <video> and, say, watching for it
> > on mouseout/blur. Not sure. :/
> 
> I'm not sure I understand the usage of mouseout/blur here. Could we just set
> an attribute on the <video>, call |setInterval| within |init|, and then
> short-circuit the updateStats method if the attribute isn't set?

We definitely don't want to set an attribute unless it's part of the interface of that element. My suggestion was to fire an event that is trusted and have the listener check if the event is trusted. (My understanding of isTrusted is whether the event was fired by chrome-privileged code, but I could be wrong.)

Possibly relevant documentation:
https://developer.mozilla.org/en/DOM/element.addEventListener#Syntax
https://developer.mozilla.org/en/Creating_Custom_Events_That_Can_Pass_Data#Dispatching_your_event_in_JavaScript
(See the line |privEvt->SetTrusted(PR_TRUE); //make the event trusted| in the C++ section. Maybe JS events in chrome code have that set automatically?)
Created attachment 562626 [details] [diff] [review]
Patch for bug 669260 v2

I've addressed the previous review comments. To work around the wrappedJSObject, I've used a trusted CustomEvent.
Attachment #561322 - Attachment is obsolete: true
Attachment #562626 - Flags: review?(dolske)
(Reporter)

Comment 21

7 years ago
Comment on attachment 562626 [details] [diff] [review]
Patch for bug 669260 v2

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

A few nits, but they're fairly basic. So r+ with those changes.

::: browser/base/content/nsContextMenu.js
@@ +417,5 @@
>      this.showItem("context-media-showcontrols", onMedia && !this.target.controls);
>      this.showItem("context-media-hidecontrols", onMedia && this.target.controls);
>      this.showItem("context-video-fullscreen", this.onVideo);
> +    if (this.onVideo)
> +      var statsShowing = this.onVideo && this.target.wrappedJSObject.mozMediaStatisticsShowing;

No need for the if() here, since you're effectively checking it twice with the |this.onVideo && foo|.

@@ +420,5 @@
> +    if (this.onVideo)
> +      var statsShowing = this.onVideo && this.target.wrappedJSObject.mozMediaStatisticsShowing;
> +    this.showItem("context-video-showstats", this.onVideo && !statsShowing);
> +    this.showItem("context-video-hidestats", this.onVideo && statsShowing);
> +

Somewhere in here you should have a |this.target.controls| check -- if the video isn't using the default html5 controls, our overlay won't work.

@@ +1420,5 @@
>          media.setAttribute("controls", "true");
>          break;
> +      case "showstats":
> +        var event = document.createEvent("CustomEvent");
> +        event.initCustomEvent("showStatistics", false, true, true);

Nit: just to be paranoid about being unique, "media-showStatistics"?

::: toolkit/content/widgets/videocontrols.css
@@ +1,2 @@
>  @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
> +@namespace h url("http://www.w3.org/1999/xhtml");

s/h/html/ please. :)

@@ +71,5 @@
> +  width: 100%;
> +  background: rgba(68,68,68,.7);
> +}
> +h|td.statLabel {
> +  font-weight: bold;

Style changes above this point ought to be in the theme-specific videocontrols.css.

::: toolkit/content/widgets/videocontrols.xml
@@ +411,5 @@
>                        this.controlBar.setAttribute("hidden", "true");
>                      this.adjustControlSize();
> +
> +                    this._handleCustomEventsBound = this.handleCustomEvents.bind(this);
> +                    this.video.addEventListener("showStatistics", this._handleCustomEventsBound, false, true);

I wonder if this could just be dropped into |this.videoEvents|. You'd need to add a |if (event.type == "showStatistics" && !event.isTrusted") return| check, though... Guess it doesn't matter either way. Though it does make me wonder if any of the usual events we process can ever be untrusted. hmm.

@@ +866,5 @@
> +                    let srcParts = src.split('/');
> +                    let srcIdx = srcParts.length - 1;
> +                    if (src.lastIndexOf('/') == src.length - 1)
> +                        srcIdx--;
> +                    // XXX use overflow here?

Sure. :)

@@ +882,5 @@
> +                        activity = "playing";
> +                    if (v.ended)
> +                        activity = "ended";
> +                    if (s.activity.getAttribute("activity") != activity)
> +                        s.activity.setAttribute("activity", activity);

Any reason for this? Pretty sure it's just a good to set the attribute w/o checking.

@@ +895,5 @@
> +                        case 1: readyState = "HAVE_METADATA";     break;
> +                        case 2: readyState = "HAVE_CURRENT_DATA"; break;
> +                        case 3: readyState = "HAVE_FUTURE_DATA";  break;
> +                        case 4: readyState = "HAVE_ENOUGH_DATA";  break;
> +                    }

These probably should use "case v.HAVE_NOTHING" etc.

@@ +903,5 @@
> +                    switch (networkState) {
> +                        case 0: networkState = "NETWORK_EMPTY";     break;
> +                        case 1: networkState = "NETWORK_IDLE";      break;
> +                        case 2: networkState = "NETWORK_LOADING";   break;
> +                        case 3: networkState = "NETWORK_NO_SOURCE"; break;

Ditto.
Attachment #562626 - Flags: review?(dolske) → review+
(In reply to Justin Dolske [:Dolske] from comment #21)
> Comment on attachment 562626 [details] [diff] [review] [diff] [details] [review]
> Patch for bug 669260 v2
> 
> Review of attachment 562626 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> @@ +882,5 @@
> > +                        activity = "playing";
> > +                    if (v.ended)
> > +                        activity = "ended";
> > +                    if (s.activity.getAttribute("activity") != activity)
> > +                        s.activity.setAttribute("activity", activity);
> 
> Any reason for this? Pretty sure it's just a good to set the attribute w/o
> checking.
> 

I added this line here due to the comment in browser.js, http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#247
// Remember, guys, setting attributes on elements is expensive!  They
// get inherited into anonymous content, broadcast to other widgets, etc.!
// Don't do it if the value hasn't changed! - dwh

Even though these elements aren't being observed and it might be a *little* bit of premature optimization, it doesn't hurt to double check the attribute value before assigning it.
https://hg.mozilla.org/mozilla-central/rev/d46033973d0d
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10

Comment 25

7 years ago
Note, the patch pushed is NOT the same as the patch attached and reviewed in this bug.
Can someone explain to localizers (or point to some documentation explaining it) what painted, parsed, presented and decoded frames are?
(In reply to flod (Francesco Lodolo) from comment #27)
> Can someone explain to localizers (or point to some documentation explaining
> it) what painted, parsed, presented and decoded frames are?

http://blog.pearce.org.nz/2011/03/html5-video-painting-performance.html
(In reply to Chris Pearce (:cpearce) (Mozilla Corporation) from comment #28)
> (In reply to flod (Francesco Lodolo) from comment #27)
> > Can someone explain to localizers (or point to some documentation explaining
> > it) what painted, parsed, presented and decoded frames are?
> 
> http://blog.pearce.org.nz/2011/03/html5-video-painting-performance.html

Thanks a lot Chris, that's really helpful (I saw your reply only today, somehow I missed BugZilla's mail).
Blocks: 694434
Statistics do not appear on the videos that need a plug in to play (e.g YouTube videos). Is that intended?
(In reply to Simona B [QA] from comment #30)
> Statistics do not appear on the videos that need a plug in to play (e.g
> YouTube videos). Is that intended?

Yeah, that is intended. This feature is only for sites that use the HTML5 <video> tag with the |controls| attribute present (e.g. not YouTube videos).
Verified on the latest Nightly and Aurora and the Statistics overlay are added to the videos. 
Mozilla/5.0 (Windows NT 6.1; rv:10.0a2) Gecko/20111129 Firefox/10.0a2
Mozilla/5.0 (Windows NT 6.1; rv:11.0a1) Gecko/20111130 Firefox/11.0a1

But,are Statistics overlay supposed to be prevented if the video is to small to fit them or available if the controls are hidden? I'm asking this because of Comment 15 and 16. I tested on 
http://www.robwinters.co.uk/lab/html5/video/drag-and-resize.htm and statistics are shown even on a smaller video. Also, statistics are not available if the Hide Controls menu item is selected when right-clicking on a video.

Thanks!
(In reply to Simona B [QA] from comment #32)
> Verified on the latest Nightly and Aurora and the Statistics overlay are
> added to the videos. 
> Mozilla/5.0 (Windows NT 6.1; rv:10.0a2) Gecko/20111129 Firefox/10.0a2
> Mozilla/5.0 (Windows NT 6.1; rv:11.0a1) Gecko/20111130 Firefox/11.0a1
> 
> But,are Statistics overlay supposed to be prevented if the video is to small
> to fit them or available if the controls are hidden? I'm asking this because
> of Comment 15 and 16. I tested on 
> http://www.robwinters.co.uk/lab/html5/video/drag-and-resize.htm and
> statistics are shown even on a smaller video.

From the review comments:
> What happens if we show the stats on a too-small video? Seems like we should
> just ignore the video size, it's not a big deal if the user opts to show the
> overlay but it's too small.

> Also, statistics are not
> available if the Hide Controls menu item is selected when right-clicking on
> a video.

This bug is filed here and was closed as WONTFIX: https://bugzilla.mozilla.org/show_bug.cgi?id=692597
Flags: in-testsuite+
Serge: AFAIK this bug does not have a test in the automated testsuite, as such I have cleared the in-testsuite flag. Can you please point me to the test in the testsuite that automates this?
Flags: in-testsuite+
(Reporter)

Comment 35

7 years ago
Serge: Please stop setting flags on bugs you're not involved in. The test change here has little to do with the actual feature. All you're doing is adding noise to bugs, and it's not at all helpful.
(Reporter)

Updated

7 years ago
No longer blocks: 697124
Mozilla/5.0 (Windows NT 6.1; rv:10.0) Gecko/20100101 Firefox/10.0
Mozilla/5.0 (X11; Linux i686; rv:10.0) Gecko/20100101 Firefox/10.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:10.0) Gecko/20100101 Firefox/10.0

Verified the implementation on latest Firefox 10.0 beta 3 version:
- "Show Statistics" menu item is added to context menu for html5 videos
- statistics are displayed when "Show Statistics" option is selected
- statistics are not displayed when controls are hidden
- statistics are displayed even if the video size is to small to fit in
Status: RESOLVED → VERIFIED
Keywords: verified-beta
Whiteboard: [fixed-in-fx-team] → [fixed-in-fx-team] [qa!]

Updated

5 years ago
Depends on: 906635
You need to log in before you can comment on or make changes to this bug.