669260 - Add statistics overlay to videocontrols

Status: VERIFIED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[Justin Dolske [:Dolske]]

Attachment: 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.

Comment 1

[Justin Dolske [:Dolske]]

Attachment: screenshot

Comment 2

[Justin Dolske [:Dolske]]

Attachment: Patch v.2 (WIP)

Stats now live in overlay.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Screenshot of patch

Stephen: Can you please provide feedback on this statistics overlay?

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 669260

I have cleaned up your WIP from earlier.

Comment 5

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

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.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 7

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

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

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: 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.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Screenshot of patch

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 11

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

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

[Stephen Horlander [:shorlander]]

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?

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: 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.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 669260

Updated patch based on feedback from shorlander.

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 669260

Added code to prevent showing the statistics overlay if the video is too small to fit them.

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 669260

Updated context menu unit test and made the Statistics context menu item available even when controls are hidden.

Comment 17

[Justin Dolske [:Dolske]]

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. :/

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

(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

[Frank Yan (:fryn)]

(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?)

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 669260 v2

I've addressed the previous review comments. To work around the wrappedJSObject, I've used a trusted CustomEvent.

Comment 21

[Justin Dolske [:Dolske]]

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.

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/d46033973d0d

Comment 24

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/d46033973d0d

Comment 25

[Alfred Kayser]

Note, the patch pushed is NOT the same as the patch attached and reviewed in this bug.

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: patch for checkin

Comment 27

[Francesco Lodolo [:flod]]

Can someone explain to localizers (or point to some documentation explaining it) what painted, parsed, presented and decoded frames are?

Comment 28

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

(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

Comment 29

[Francesco Lodolo [:flod]]

(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).

Comment 30

[Simona Badau, Desktop QA]

Statistics do not appear on the videos that need a plug in to play (e.g YouTube videos). Is that intended?

Comment 31

[Jared Wein [:jaws] (please needinfo? me)]

(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).

Comment 32

[Simona Badau, Desktop QA]

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!

Comment 33

[Jared Wein [:jaws] (please needinfo? me)]

(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

Comment 34

[Jared Wein [:jaws] (please needinfo? me)]

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?

Comment 35

[Justin Dolske [:Dolske]]

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.

Comment 36

[Mihaela Velimiroviciu (:mihaelav)]

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