The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in Firefox 10

Status

()

Firefox
Menus
VERIFIED FIXED
6 years ago
3 years ago

People

(Reporter: Dolske, Assigned: mattw)

Tracking

({uiwanted, verified-beta})

unspecified
Firefox 10
uiwanted, verified-beta
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][fixed-in-fx-team][qa!][mentor=jwein])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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 ?
(Reporter)

Comment 1

6 years ago
Related idea in bug 508063, although I think that's wontfix at this point.
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 2

6 years ago
Hi I would like to work on this bug
Assignee: nobody → mwein2009
Status: NEW → ASSIGNED
(Assignee)

Comment 3

6 years ago
Created attachment 557434 [details] [diff] [review]
Patch for bug 681550
Attachment #557434 - Flags: review?(dolske)
(Assignee)

Comment 4

6 years ago
Created attachment 558121 [details] [diff] [review]
Patch for bug 681550 v2

Removed pausing and playing and the current time from the file name.
Attachment #557434 - Attachment is obsolete: true
Attachment #557434 - Flags: review?(dolske)
Attachment #558121 - Flags: review?(dolske)
(Reporter)

Comment 5

6 years ago
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. :)
Attachment #558121 - Flags: review?(dolske) → review-
(Assignee)

Comment 6

6 years ago
Created attachment 561365 [details] [diff] [review]
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.
Attachment #558121 - Attachment is obsolete: true
Attachment #561365 - Flags: review?(dolske)
(Reporter)

Comment 7

6 years ago
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.
Attachment #561365 - Flags: review?(dolske) → review-
(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.
(Assignee)

Comment 9

6 years ago
Created attachment 562661 [details] [diff] [review]
Patch for bug 681550 v4

I used your suggestion for parsing the file name, and made changes based on yours and Gavin's help.
Attachment #561365 - Attachment is obsolete: true
Attachment #562661 - Flags: review?(dolske)
(Reporter)

Comment 10

6 years ago
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.]
Attachment #562661 - Flags: review?(dolske) → review+
https://hg.mozilla.org/integration/fx-team/rev/97f1bf573fc8
Whiteboard: [good first bug] → [good first bug][fixed-in-fx-team]
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
https://hg.mozilla.org/mozilla-central/rev/97f1bf573fc8

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

https://hg.mozilla.org/mozilla-central/rev/cc34d8625f6b
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10

Comment 14

6 years ago
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

6 years ago
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?
(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
Duplicate of this bug: 499776
Blocks: 697124
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)

Updated

5 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:
- "Save Snapshot as..." option is added to the context menu
- Default file name is <<video_name>>.jpg
Status: RESOLVED → VERIFIED
Keywords: verified-beta
Whiteboard: [good first bug][fixed-in-fx-team] → [good first bug][fixed-in-fx-team][qa!]
Whiteboard: [good first bug][fixed-in-fx-team][qa!] → [good first bug][fixed-in-fx-team][qa!][mentor=jwein]
Depends on: 693099
Depends on: 734027
Depends on: 773988
You need to log in before you can comment on or make changes to this bug.