787881 - Overlay a preview of the hovered position in the video while mouseover on the scrubber

Status: VERIFIED WONTFIX

(Toolkit :: Video/Audio Controls, defect)

Description

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

Sorry in advance for the terrible bug summary, but I'm failing at English right now.

Basically, I think we can show a preview frame of the video on top of the video when the user is hovering their mouse cursor over the scrubber.

I put together a little demo of what this could look like. There is a noticeable lag between moving the mouse and having the updated frame get painted. I'm not sure what we can do here.

This feature can only apply to videos with the "controls" attribute.

Demo can be found here: https://www.msu.edu/~weinjare/scrubber.html

Comment 1

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

Brainstorming some with Blair on IRC, and we might be able to reduce the lag by doing these two metrics:

Only update the preview if one of the following conditions is true:
a) The mouse has remained still for > n milliseconds
b) The mouse has moved over a threshold, min(% of video, m px of scrubbing distance)

where m may be 20px.

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Jared Wein [:jaws] from comment #1)
> b) The mouse has moved over a threshold, min(% of video, m px of scrubbing
> distance)

This would help, but it could also potentially be annoying. I find it useful to be able to move the mouse by a pixel or two and get an updated preview, so I can more accurately jump to a position in a video. No idea how common that behaviour is though, and I'm betting we have no telemetry on that.

I think just doing the first option (a) would be enough to reduce the number of seeks done.

I also wonder if it could blank the preview while its waiting for it to update (ie, while its seeking), since at the moment when the preview moves, the previous frame is displayed momentarily (would be longer for seeks requiring hitting the network), which feels a bit confusing.

Comment 3

[Matthew Gregan [:kinetik]]

This would probably work better with bug 778077 implemented and bug 754525 fixed.  I'd be interested to see *how* much better, because right now on my connection (~250ms RTT to msu.edu) it's too slow to be anything but frustrating.

The preview might also need a mini-throbber while it's loading to make it clear why it's slow to update, otherwise I foresee many bugs filed by users who don't understand why this preview is slower than YouTube's builtin one (which uses a server-side PNG strip).

Comment 4

[Nicholas Nethercote [inactive]]

This is awesome.  I love video players that show previews like this.

IME sites that do this have a fairly coarse granularity, e.g. there might be a few dozen preview frames for the whole video.  That's much better than no preview, but obviously more fine-grained previews are even better.

Comment 5

[nejmeddine.douma]

Hello Jared,

I would like to work on this bug. Could you please tell me if this feature is still interesting to have?

Thanks.

Nejm

Comment 6

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

The work to implement this bug will be a medium-sized project, more intensive than our regular good-first-bugs. I would recommend that you get some more experience with fixing bugs in the video controls first, and then come back to this bug a bit later. Would you be interested in bug 1072442 instead?

Comment 7

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

Chris, if we create a second, nested, <video> element can we guarantee that the video resource will be shared between the nested element and the parent <video> element? Can this work for DRM-based videos?

To implement this, we will need to have the following restrictions:
1) Only show the preview for buffered regions of the video and only if the video is seekable. We should not show the preview for the other areas.
2) Only update the preview when the mouse cursor has not moved for 50ms.
3) When seeking, we need to show a throbber as there can be delay during the seek. You may be able to re-use the loading throbber graphic that is already used by the video controls.
4) If the mouse cursor moves while a seek is underway, the seek should be canceled (not sure how?), and the buffering UI on the preview should show up.
5) While the mouse cursor is moving on the scrubber, we should show the throbber until the cursor has paused for 50ms, at which point the seek will begin.
6) We should not show the preview for videos that are smaller than specific dimensions. The preview height should be set to 48px. The width should be calculated to match the aspect ratio. The preview should not block more than the lower 1/3 of the video. The control bar is already 28px tall, and we may choose to show a 16px gap between the control bar and the preview. Therefore, we should not show the preview for videos that are under 276px tall. Also, we should only show the preview for videos that are able to show 4 non-overlapping widths of the preview.
7) There will be no ability to turn off the preview using the context menu or control bar. Sending events from the context menu to the control bar is quite problematic, as shown by bug 1274153. As well, the video controls run in lower-privileged content and are not able to access about:config preferences. Therefore, there will not be an about:config pref to disable this feature.
8) The preview should not cause extra network requests. As long as we are only showing the preview for buffered regions we should be fine.
9) The preview should stay open for as long as the mouse cursor is on top of the scrubber.
10) Touch-based and keyboard-based controls can be brainstormed and thought of in a follow-up bug.

Comment 8

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

nejmeddine.douma, pending the response from Chris, I will assign this bug to you and the students you are working with. Note, as you can see by my above comment, this project may take all of your two weeks.

It would also be required that we have some automated tests that cover this feature.

I will be available to help mentor the work here.

Comment 9

[nejmeddine.douma]

Of course, I sent a patch for the bug 1072442.

Comment 10

[nejmeddine.douma]

Thank you Jared. 
Currently the player does not show a tooltip with the time when the cursor moves on the scrubber. I think we may start by adding the tooltip with only the time and then add the preview and add all the restrictions. What do you think?

Comment 11

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

I'm not sure if we want a separate tooltip with just the hovered time. I would prefer if this was all implemented together. We should still wait to hear what Chris says about resource sharing.

Comment 12

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

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8)
> Chris, if we create a second, nested, <video> element can we guarantee that
> the video resource will be shared between the nested element and the parent
> <video> element?

The HTML spec says that if two video elements have the same URI, we are allowed to assume they are the same resource, and share the downloaded data between them.

For non-MSE videos, yes, the video resources (the data downloaded over the network) will be shared between the video elements. So data won't be downloaded twice.

We will create a new decoder for the clone, so that takes up some GPU resources.

For MSE videos... It looks like cloneNode() fails for video elements that are MSE fed. So your cloneNode approach won't work there. At least not yet.

Here's a simple MSE example:
http://people.mozilla.org/~cpearce/simple-dash/

> Can this work for DRM-based videos?

We only support EME (DRM) in MSE video. So the MSE answer above applies. Additionally, there are other structural reason why cloning a video doing EME won't work.

Here's a simple MSE+EME example:
http://people.mozilla.org/~cpearce/mse-clearkey/

I'd recommend *not* doing this for EME or MSE content, just the plain-old-video case.

You can detect EME by checking whether there's a non-null "mediaKeys" attribute on the video element.

You can detect MSE by checking whether the "src" attribute starts with "mediasource:".

Comment 13

[nejmeddine.douma]

Attachment: bug-787881-fix.patch v1

Hi Jared, could you please give me a feedback concerning the patch?

I am stil working on many restrictions but I wanted to get a feedback on my current progress. Currently, these are the implemented restrictions:

1) Only show the preview for buffered zone of the video.

2) We should not show the preview for videos that are smaller than specific dimensions. The preview height should be set to 48px. The width should be calculated to match the aspect ratio. The preview should not block more than the lower 1/3 of the video. The control bar is already 28px tall, and we may choose to show a 16px gap between the control bar and the preview. Therefore, we should not show the preview for videos that are under 276px tall. Also, we should only show the preview for videos that are able to show 4 non-overlapping widths of the preview.

3) The preview should not cause extra network requests. As long as we are only showing the preview for buffered regions we should be fine.

4) The preview should stay open for as long as the mouse cursor is on top of the scrubber.

Suggestions and problems:

A tooltip indicating the time of the hovered position visible even when we do not show the preview

For the gap between the control bar and the preview I suggest to have a 8px gap rather than 16px.

The cloneNode apparently does not work fine for me when I access a video directly (for example: http://www.sample-videos.com/video/mp4/720/big_buck_bunny_720p_50mb.mp4). However, it works fine if the video is embed in a HTML page using <video>. I also noticed that in the first case the Utils.init function is being called many times.

Thanks,
Nejmeddine

Comment 14

[nejmeddine.douma]

Attachment: bug-787881-fix.patch v2

Hi Jared,

In this patch I implemented all the restrictions you asked for in comment #8, except the point 4 because I don't see how I can implement it.

I still have the same problems I mentioned in comment #13.

Could you please have a look at this patch?

Thanks,
Nejmeddine

Comment 15

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

Comment on attachment 8760789 [details] [diff] [review]
bug-787881-fix.patch v2

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

You will need to update your local tree and rebase the patch. If you need help rebasing your patch, you can join #introduction on IRC and someone can walk you through it.

All of the feedback left on /toolkit/themes/windows/global/media/videocontrols.css applies to /toolkit/themes/osx/global/media/videocontrols.css too.

::: toolkit/content/widgets/videocontrols.xml
@@ +506,5 @@
> +
> +                updateEnableVideoPreview : function() {
> +                    // Disable videoPreview if the video is EME or MSE
> +                    this.enableVideoPreview = this.video.mediaKeys === null
> +                        && !this.video.src.startsWith("mediasource:");

this.enableVideoPreview = this.video.mediaKeys === null &&
                          !this.video.src &&
                          !this.video.src.toLower().startsWith("mediasource:");

@@ +513,5 @@
> +                    // then 1/3 of the player's height and if the player's widths
> +                    // cannot contain 4 non-overlapping widths of the preview
> +                    var videoRect = this.video.getBoundingClientRect();
> +                    this.enableVideoPreview = this.enableVideoPreview
> +                        && videoRect.height >= 276 && 1 >= 4 * 44/videoRect.height;

this.enableVideoPreview = this.enableVideoPreview &&
                          videoRect.height >= 276 &&
                          1 >= 4 * 44 / videoRect.width;

also, please move the magic numbers of 44 and 276 to constants so that they are not duplicated through the file here and we can name them so others will understand their purpose quicker.

@@ +1414,5 @@
>                      event.preventDefault(); // Prevent page scrolling
>                  },
>  
> +                onMouseOverScrubberStack : function() {
> +                    this.previewContainer.style.setProperty("display","inline-block");

Please move styles related to the behavior, like this one, to /toolkit/content/widgets/videocontrols.css

Styles related to the look and feel of the controls should be placed in /toolkit/themes/windows/global/media/videocontrols.css and /toolkit/themes/osx/global/media/videocontrols.css

@@ +1424,5 @@
> +                    this.isMouseOverScrubber = false;
> +                },
> +
> +                onMouseMoveScrubberStack : function(e) {
> +                    var mouseX = e.clientX;

We should only update the preview if mouseX is different than the previous mouseX.

@@ +1426,5 @@
> +
> +                onMouseMoveScrubberStack : function(e) {
> +                    var mouseX = e.clientX;
> +                    var backgroundBarRect = this.backgroundBar.getBoundingClientRect();
> +                    var p = Math.max(0, (mouseX - backgroundBarRect.left)/ backgroundBarRect.width);

nit, please use a better variable name than `p`

@@ +1429,5 @@
> +                    var backgroundBarRect = this.backgroundBar.getBoundingClientRect();
> +                    var p = Math.max(0, (mouseX - backgroundBarRect.left)/ backgroundBarRect.width);
> +                    var duration = this.video.duration;
> +                    if (isNaN(duration))
> +                        duration = this.maxCurrentTimeSeen;

nit, please wrap all one-line blocks with curly brackets

@@ +1444,5 @@
> +                        // Update preview if mouse do not move for 50ms
> +                        clearTimeout(this.updatePreviewTimer);
> +                        var self = this;
> +                        // Show throbber when seeking
> +                        this.updatePreviewTimer = setTimeout(function () {

Use an arrow function here so you don't need the `var self = this;` trick.

this.updatePreviewTimer = setTimeout(() => {
  ...
});

@@ +1446,5 @@
> +                        var self = this;
> +                        // Show throbber when seeking
> +                        this.updatePreviewTimer = setTimeout(function () {
> +                          if(self.isMouseOverScrubber) {
> +                            self.videoPreview.onseeked = function(){

This should use addEventListener and removeEventListener instead of assiging to the event listener.

@@ +1451,5 @@
> +                                self.previewThrobber.style.setProperty("visibility","hidden");
> +                            };
> +                            self.preview.style.setProperty("visibility","visible");
> +                            self.videoPreview.fastSeek(previewPosition);
> +                            self.videoPreview.currentTime = previewPosition;

fastSeek is only used on B2G, and shouldn't make a difference on desktop machines.

@@ +1461,5 @@
> +                        this.previewThrobber.style.setProperty("visibility","hidden");
> +                    }
> +
> +                    // update previewContainer position
> +                    this.previewContainer.style.setProperty("top", backgroundBarRect.top - 74 + "px");

This shouldn't need to change all the time.

@@ +1462,5 @@
> +                    }
> +
> +                    // update previewContainer position
> +                    this.previewContainer.style.setProperty("top", backgroundBarRect.top - 74 + "px");
> +                    this.previewContainer.style.setProperty("left", mouseX - this.previewContainer.getBoundingClientRect().width/2 + "px");

The previewContainer should not appear outside of the video rect. You should range bound the previewContainer position to not have a negative left, nor have the right side extend over the right side of video rect.

::: toolkit/themes/windows/global/media/videocontrols.css
@@ +199,5 @@
>    text-shadow: rgba(0,0,0,.3) 0 1px;
>    padding-top: 5px;
>  }
>  
> +.previewContainer{

nit, please put a space before the curly bracket

@@ +200,5 @@
>    padding-top: 5px;
>  }
>  
> +.previewContainer{
> +  position: fixed;

Please use `position: absolute;` here, and set `position: relative;` on the parent element so that the previewContainer is constrained to the box of the video element.

@@ +209,5 @@
> +  background-color: rgba(35,31,32,.74);
> +  display: inline-block;
> +  padding: 2px;
> +  max-height: 48px;
> +  border-radius: 3px;

border-radius: 1px;

@@ +233,5 @@
> +
> +.timeTooltipLabel {
> +  color: rgba(255, 255, 255, 0.75);
> +  font-size: 10px;
> +  font-family: Arial,sans-serif;

Please change this to:
font: message-box;
font-size: 10px;

@@ +236,5 @@
> +  font-size: 10px;
> +  font-family: Arial,sans-serif;
> +  text-shadow: 0px 1px rgba(0, 0, 0, 0.3);
> +  background-color: rgba(35,31,32,.74);
> +  border-radius: 3px;

border-radius: 1px;

Comment 16

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

I can reproduce the issue that you said about the preview not working when viewing the video directly. When I view the video directly, the preview just buffers forever. Chris, do you know why that might be happening?

Comment 17

[nejmeddine.douma]

Attachment: bug-787881-fix.patch v3

Hi Jared,
I updated the patch after your last review. Could you please reveiw it again?
Thank you
Nejmeddine

Comment 18

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

Comment on attachment 8763253 [details] [diff] [review]
bug-787881-fix.patch v3

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

Looks better. I'll test it out again once you make the following changes.

::: toolkit/content/widgets/videocontrols.xml
@@ +454,5 @@
> +                    // cannot contain 4 non-overlapping widths of the preview
> +                    var videoRect = this.video.getBoundingClientRect();
> +                    this.enableVideoPreview = this.enableVideoPreview &&
> +                                              videoRect.height >= this.minHeightForVideoPreview &&
> +                                              1 >= 4 * this.videoPreviewHeight/videoRect.height;

nit, one space on each side of the `/` operator.

@@ +1284,5 @@
> +                onMouseMoveScrubberStack : function(e) {
> +                    if (e.clientX != this.previousMouseX) {
> +                        var mouseX = e.clientX;
> +                        var backgroundBarRect = this.backgroundBar.getBoundingClientRect();
> +                        var relativePosition = Math.max(0, (mouseX - backgroundBarRect.left)/ backgroundBarRect.width);

nit, one space on each side of `/` operator.

@@ +1309,5 @@
> +                            this.updatePreviewTimer = setTimeout(() => {
> +                              if(this.isMouseOverScrubber) {
> +                                this.videoPreview.addEventListener("seeked", () => {
> +                                    this.previewThrobber.removeAttribute("show");
> +                                });

This event listener should be created during init, otherwise this will add a new event listener each time the updatePreviewTimer is run.

@@ +1324,5 @@
> +                        // update previewContainer position
> +                        var previewContainerRect = this.previewContainer.getBoundingClientRect();
> +                        var videoRect = this.video.getBoundingClientRect();
> +                        this.previewContainer.style.setProperty("left",
> +                            Math.max(0, Math.min(mouseX - videoRect.left - previewContainerRect.width/2,

nit, spaces around binary operators.

::: toolkit/themes/osx/global/media/videocontrols.css
@@ +211,5 @@
> +}
> +
> +.previewContainer .preview .previewThrobber {
> +    background: url(chrome://global/skin/media/throbber.png) no-repeat center;
> +    background-size: 25%;

Please don't use the background short-hand here. As it is, you're now defining background-size twice, and future changes to the CSS get harder to track down when all styles are being set in short-hand.

@@ +221,5 @@
> +}
> +
> +.timeTooltip {
> +  display: block;
> +  height: 13px;

I'm not sure we can make certain assumptions about the height of the text. I think it would be better to use bottom: 0; instead of positioning relative to the top.

Comment 19

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

(In reply to (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) from comment #16)
> I can reproduce the issue that you said about the preview not working when
> viewing the video directly. When I view the video directly, the preview just
> buffers forever. Chris, do you know why that might be happening?

This patch no longer applied cleanly. When I fixed it to apply cleanly, I didn't see preview frames displayed when the scrubber was mouseover'ed. I do see the timestamp to seek to appear, but no preview.

I'm not sure why a cloneNode would fail here. If you can get the patch working again Nejmeddine, I'll take a look.

Comment 20

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

Hi nejmeddine.douma, have you been able to continue work on this? Do you have any updates? Thanks!

Comment 21

[nejmeddine.douma]

Attachment: bug-787881-fix.patch v4

Hi Jared,

Sorry for my late reply. I updated the patch. Could you please have a look?
Thanks.

Nejmeddine

Comment 22

[nejmeddine.douma]

Hi Chris,

I updated the patch. Now the patch works fine for me when the video is not in standalone mode. Could you please take a look and keep me updated?

Thank you.
Nejmeddine

Comment 23

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

Comment on attachment 8769800 [details] [diff] [review]
bug-787881-fix.patch v4

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

The patch generally looks OK to me, but I can reproduce the issue that Nejmeddine is seeing with the standalone videos.

We could potentially land this and disable it for standalone videos but I would rather not. We should figure out why it's happening, maybe there is an underlying bug here that needs to be fixed.

Chris, can you take a look at this? The patch was rebased and should apply cleanly still.

::: toolkit/themes/windows/global/media/videocontrols.css
@@ +269,5 @@
> +  visibility: hidden;
> +}
> +
> +.previewContainer .preview .previewThrobber {
> +    background-image: url(chrome://global/skin/media/throbber.png);

nit, please be consistent with 2-space indenting.

@@ +290,5 @@
> +  bottom: 0px;
> +}
> +
> +.timeTooltipLabel {
> +  color: rgba(255, 255, 255, 0.75);

nit, please don't put spaces after commas inside of the color macros.

Comment 24

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

Over IRC cpearce said that he couldn't get the patch to work for him on http://pearce.org.nz/h or a standalone video document.

Comment 25

[Justin Dolske [:Dolske]]

I'm a bit concerned about how the UX of this is actually going to work out. Once comment 24 is addressed, can we get a try build to play with?

Here's a few of my specific concerns:

* Performance. Seeking and decoding isn't cheap, and so trying to show a snappy preview feels challenging. YouTube does this fairly well, but they do that by serving a pre-rendered sheet of thumbnails along with the video (which they can do since they control the server, but doesn't exist for arbitrary web video). The demo in comment 0 feels very laggy on a modern MBP.

* Oddness of a a feature which works intermittently. AIUI, this patch is only showing previews for buffered regions of the video (which might be the least interesting to seek into), and comment 12 indicates this approach won't work for video served via MSE, or for EME media. From a user's point of view, there's risk that the feature will seem frequently broken.

* Interaction of preview with existing <scale> thumb. Seems there are now two elements attaching to the <scale>, and could be tricky to get the interaction right so one doesn't get in the way when trying to use the other.

Comment 26

[nejmeddine.douma]

Attachment: bug-787881-fix.patch v5

I updated the patch according to the feedback in comment 23 and fixed the bug with the video on http://pearce.org.nz/h.
The patch also works fine with the video on https://www.msu.edu/~weinjare/scrubber.html
With a standalone video document I get "uncaught exception: undefined" and I do not know why.
For the seeking and decoding costs, I suggest generating thumbnails, for example one thumbnail for each 5 seconds of video, as soon as a region is buffered and storing them somewhere to be served when the mouse is over a buffered zone of the scrubber. I think this may also solve the problem with the standalone video document. I don't know if it will be possible to use this approach with EME or MSE content.

Comment 27

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

Comment on attachment 8777079 [details] [diff] [review]
bug-787881-fix.patch v5

Based on the feedback in comment #25 I think it would be best if we stopped working on this bug. Nejmeddine you've done good work here, but there are issues outside of your control that we won't be able to account for.

I will keep looking for another bug that you can work on.

If you would like to continue work on this feature, you may want to see what you can do to create it as a web extension.

Comment 28

[sworddragon2]

I'm not sure why you closed this bug. Another contributor might to find this feature useful and could address the mentioned issues. But if the issues are closed they add just another level of intricacy here.

Comment 29

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

The bug is closed because we don't have the technical platform to allow someone to implement this feature. If major changes are made to how videos are loaded (meaning we can show thumbnails for the full video instead of just the buffered timespans) then either this bug can be reopened or a new one filed. As it stands today though, this bug is not actionable.

Comment 30

[sworddragon2]

Likely all issues in comment #25 can be solved and a contributor who finds this useful might also implement the required backend changes first. But as already said, closing bugs makes this less likely to happen.

Comment 31

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

At this point in time, we do not know of a possible way for anybody to fix the "backend" changes. There is a fundamental limit here, because we would like to show thumbnails for the full video but the full video may not have buffered. Showing only thumbnails for the buffered content will be confusing and make Firefox appear slow. Only showing thumbnails once all of the content is buffered will make progressively loading videos appear to be missing functionality or broken.

Comment 32

[sworddragon2]

That's the problem here: You might not know how to solve this (which is fine so far) but somebody else might have the skill to find/implement a solution for this.

Comment 33

[Justin Dolske [:Dolske]]

I'd encourage anyone who wants to take a shot at this to first build a standalone prototype. The Firefox video controls don't really run with any special privileges, so one could do this in a demo web page.

Comment 34

[moz-nuc]

Very late to the party here, but the right place for adding thumbnail previews to HTML5 video would seem to be the spec itself, specifically a standardisation of a thumbnail strip image (or set of images) as a child/attribute of the video element. Or is something like this already underway?

Yes, it wouldn't fix existing videos/players, but it would give a standard for future HTML5 video delivery to coalesce around, obviating the current arbitrary heapings of JS and 16 different sketchy player script CDNs just to get a widely useful video widget feature.

Retrofitting preview-generating/HTML-updating utilities would also spring up as a consequence, so a lot of pre-existing pure HTML5 video content would gain scrub previews.