681550 - Add "Save as Image" to context menu for HTML5 video

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Description

[Justin Dolske [:Dolske]]

It would be nice to right-click on a video and copy the currently displayed frame. For example, if you want to grab a thumbnail for us when linking to an external video, or for using a famous scene in a presentation.

The implementation is trivial -- make an ephemeral <canvas>, and call ctx.drawImage(theVideo).

The bikesheddy part is what to name it and the exact function... Save As Image? Copy As Image? Image vs Screenshot vs Thumbnail vs ?

Comment 1

[Justin Dolske [:Dolske]]

Related idea in bug 508063, although I think that's wontfix at this point.

Comment 2

[Matthew Wein [:mattw]]

Hi I would like to work on this bug

Comment 3

[Matthew Wein [:mattw]]

Attachment: Patch for bug 681550

Comment 4

[Matthew Wein [:mattw]]

Attachment: Patch for bug 681550 v2

Removed pausing and playing and the current time from the file name.

Comment 5

[Justin Dolske [:Dolske]]

Comment on attachment 558121 [details] [diff] [review]
Patch for bug 681550 v2

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

Just a pile of small things to fix, but otherwise basically on target!

::: browser/base/content/browser-context.inc
@@ +109,5 @@
>                  oncommand="gContextMenu.fullScreenVideo();"/>
> +      <menuitem id="context-video-saveimage"
> +                accesskey="&videoSaveImage.accesskey;"
> +                label="&videoSaveImage.label;"
> +                oncommand="gContextMenu.saveVideoFrameAsImage();"/>

Please place this below the "context-saveaudio" menuitem.

You'll also need to modify browser/base/content/test/test_contextmenu.html so it knows when this context menu item is present. Mostly it's just adding it to existing tests on video elements, but maybe we should add one for invalid video elements? I suppose I might let that slide...

::: browser/base/content/nsContextMenu.js
@@ +414,5 @@
>      this.showItem("context-media-unmute", onMedia && this.target.muted);
>      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);
> +    this.showItem("context-video-saveimage",  this.onVideo);

This should move up to initSaveItems()

@@ +426,5 @@
>        this.setItemAttr("context-media-unmute", "disabled", hasError);
>        this.setItemAttr("context-media-showcontrols", "disabled", hasError);
>        this.setItemAttr("context-media-hidecontrols", "disabled", hasError);
> +      if (this.onVideo) {
> +        this.setItemAttr("context-video-saveimage",   "disabled", hasError);

Instead of this error check, I think it'll suffice to disable the menuitem when: this.target.readyState >= this.target.HAVE_CURRENT_DATA;

That should take care of both errors (eg 404), and trying to use the context menu when the video is stalled at some point waiting for data.

@@ +824,5 @@
> +    canvas.width = video.videoWidth;
> +    canvas.height = video.videoHeight;
> +    var ctxDraw = canvas.getContext("2d");
> +    ctxDraw.drawImage(video, 0, 0);
> +    saveImageURL(canvas.toDataURL("image/png", ""), "image.png", "SaveImageTitle", true, false, document.documentURIObject);

Hmm, wonder if we need a urlSecurityCheck(this.mediaURL, doc.nodePrincipal); call here. Not sure offhand, I'll have to look.

I think it should save as a JPEG, since videos are already generally continuous-tone media, and the existing compression artifacts will be worse than anything JPEG adds.

Two other nits, both of which would be fair to spin off to followup bugs:

1) It would be nice to derive the default filename from the video's filename. eg (rickroll.webm --> rickroll.png).

2) It would also be nice to handle the rather special case of the video having a poster frame, and save that instead when it's being shown (since otherwise you'd get frame 0 of the video, and that's probably not interesting).

And while I'm wishing for ponies, it would be more efficient if we could use .mozGetAsFile() instead of slinging around data: URIs, but that's definitely for another bug, and not worth doing until after E10S anyway.

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +428,5 @@
>  <!ENTITY mediaHideControls.label     "Hide Controls">
>  <!ENTITY mediaHideControls.accesskey "C">
>  <!ENTITY videoFullScreen.label       "Full Screen">
>  <!ENTITY videoFullScreen.accesskey   "F">
> +<!ENTITY videoSaveImage.label        "Save Current Image">

Not to bikeshed too much, but let's go with "Save Snapshot" for now.

"Save Frame" was another suggestion, although that gets confusing with HTML frames. "Save Video Frame" could work?

Also, since it displays a prompt instead of performing an immediate action, it should be "Save Current Image As…" or "Save Snapshot As…".

@@ +429,5 @@
>  <!ENTITY mediaHideControls.accesskey "C">
>  <!ENTITY videoFullScreen.label       "Full Screen">
>  <!ENTITY videoFullScreen.accesskey   "F">
> +<!ENTITY videoSaveImage.label        "Save Current Image">
> +<!ENTITY videoSaveImage.accesskey    "I">

The accesskey will need to be unique in its menu, the test_contextmenu.html test will check that so I won't bother. :)

Comment 6

[Matthew Wein [:mattw]]

Attachment: Patch for bug 681550 v3

Fixed the requested changes and handled the case with the poster frame. The snapshots now save as the video's default file name converted to the .JPEG file extension.

Comment 7

[Justin Dolske [:Dolske]]

Comment on attachment 561365 [details] [diff] [review]
Patch for bug 681550 v3

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

r-, but should be good after some minor fixup...

::: browser/base/content/nsContextMenu.js
@@ +424,5 @@
>        this.setItemAttr("context-media-unmute", "disabled", hasError);
>        this.setItemAttr("context-media-showcontrols", "disabled", hasError);
>        this.setItemAttr("context-media-hidecontrols", "disabled", hasError);
> +      if (this.onVideo) {
> +        let canSaveSnapshot = this.target.readyState >= this.target.HAVE_CURRENT_DATA;

This will miss the case of a poster frame showing but the video being slow to load. It could be |cansave = (v.poster || v.readyState >= HAVE_CURRENT). But that also raises some interesting questions, such as what happens if the poster image is invalid/broken, or what happens if the poster image is slow and loads _after_ the media. Or what to do when the video's in an error state.

But this all pretty edge-case stuff, so let's leave this as-is. :)

@@ +820,5 @@
> +  saveVideoFrameAsImage: function () {
> +    urlSecurityCheck(this.mediaURL, this.browser.contentPrincipal,
> +                     Ci.nsIScriptSecurityManager.DISALLOW_SCRIPT);
> +    var url = this.mediaURL;
> +    var videoFileName = url.substring(url.lastIndexOf("/") + 1, url.length);

Nit: don't need url.length here. It's an optional arg, default is end-of-string.

@@ +822,5 @@
> +                     Ci.nsIScriptSecurityManager.DISALLOW_SCRIPT);
> +    var url = this.mediaURL;
> +    var videoFileName = url.substring(url.lastIndexOf("/") + 1, url.length);
> +    if (videoFileName)
> +      videoFileName = videoFileName.replace(/\.[^\.]*$/, '.jpg');

If videoFileName doesn't have an extension to begin with (eg "foo" instead of "foo.bar") you'll end up with a filename without .jpg appended.

Easily fixed via:

  if (videoFileName) {
    videoFileName = videoFileName.replace(/\.[^\.]*$/, "");
    videoFileName += ".jpg";
  }

But, really, the better option here would be to let our nsIURI/nsIURL parse the goop... Something like (untested)...

  let name = "";
  try {
    let uri = Services.ios.newURI(this.mediaURL);
    let url = uri.QueryInterface(Ci.nsIURL);
    if (url.baseFileName)
      name = url.baseFileName + ".jpg";
  } catch (e) { }
  if (!name)
    name = "snapshot.jpg";

(The try/catch is noteworthy, because the newURI/QI can throw if it's unparsable.)

@@ +826,5 @@
> +      videoFileName = videoFileName.replace(/\.[^\.]*$/, '.jpg');
> +    else
> +      videoFileName = "snapshot.jpg";
> +    var video = this.target;
> +    if (video.poster && video.paused && (!video.hasAttribute("played") || video.played.length == 0)) {

We don't currently implement .played, so I don't think this is going to work as expected... Any paused video with a poster will prefer the poster.

Hmmmmmmmm. I'm not sure it's currently possible to figure out if the <video> is displaying the poster or not. A check like |if (.poster && .paused && .currentTime == 0)| would mostly work, except that condition is, I think, also true after a video finishes playing.

Bah. I guess we should just punt on supporting .poster for now. Sorry for leading you down this rabbithole. :(

@@ +829,5 @@
> +    var video = this.target;
> +    if (video.poster && video.paused && (!video.hasAttribute("played") || video.played.length == 0)) {
> +      urlSecurityCheck(video.poster, this.browser.contentPrincipal,
> +                       Ci.nsIScriptSecurityManager.DISALLOW_SCRIPT);
> +      saveImageURL(video.poster, null, "SaveImageTitle", false,

2nd arg should be |videoFileName| for consistency.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

(In reply to Justin Dolske [:Dolske] from comment #7)
>     let uri = Services.ios.newURI(this.mediaURL);
>     let url = uri.QueryInterface(Ci.nsIURL);
>     if (url.baseFileName)
>       name = url.baseFileName + ".jpg";

FWIW makeURI() from contentAreaUtils is in scope, and the nsIURL property you want is "fileName" AFAICT.

Comment 9

[Matthew Wein [:mattw]]

Attachment: Patch for bug 681550 v4

I used your suggestion for parsing the file name, and made changes based on yours and Gavin's help.

Comment 10

[Justin Dolske [:Dolske]]

Comment on attachment 562661 [details] [diff] [review]
Patch for bug 681550 v4

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

Untested, but looks good to me!

One catch: I think there will be a race between you and bug 669260 for which content menu entry can use the "S" accesskey. You and Jared can sort that out by whoever lands first. ;-) [Neither functionality strikes me as being more common than the other.]

Comment 11

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

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

Comment 12

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

Previous push contained a syntax error in test_contextmenu.html. I've fixed this and repushed.

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

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/97f1bf573fc8

https://hg.mozilla.org/mozilla-central/rev/0725f6818daa

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

Comment 14

[Loic]

Just a question: Is it intentional to save the snapshot with the extension .jpg even if it has image/png as MIME type?

Comment 15

[Loic]

An example why mixing .JPG extension with PNG MIME type may cause some errors for the user.
In Ubuntu 10.10, the Eye of GNOME image viewer (v2.32) is not able to view a snapshot taken by FF10 and displays this error message:
"Error interpreting JPEG image file (Not a JPEG file: starts with 0x89 0x50)"

Maybe should we fill a bug? What do you think?

Comment 16

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

(In reply to Loic from comment #15)
> Maybe should we fill a bug? What do you think?

This bug is logged here:
https://bugzilla.mozilla.org/show_bug.cgi?id=693099

Comment 18

[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 19

[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:
- "Save Snapshot as..." option is added to the context menu
- Default file name is <<video_name>>.jpg