508063 - add method to get a still image from the video tag

Status: RESOLVED WONTFIX

(Core :: Audio/Video: Playback, enhancement)

Description

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: adds GetStill method returning a data uri

This is primarily a sub-bug from bug 451674, but I assume this could be useful in general.

Comment 1

[Justin Dolske [:Dolske]]

Why not just use a <canvas> and drawImage()? That's supposed to work with <video>...

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

getStill() doesn't have a nice ring to it. How about getImageAsDataURL() ? Kinda building on the terms used in canvas API.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

or even an exact duplication of the canvas method: toDataURL()

Comment 4

[Jeff Walden [:Waldo]]

I concur with comment 1 and would further discourage a non-standard method that is completely duplicative.

Comment 5

[cajbir (:cajbir)]

I also concur with comment 1.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #4)
> I concur with comment 1 and would further discourage a non-standard method that
> is completely duplicative.


(In reply to comment #5)
> I also concur with comment 1.

So, in order to get a still image from video we need to resort to DOM canvas hackery?

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Using canvas like that is not C++ XPCOM friendly either

Comment 8

[cajbir (:cajbir)]

I'd rather use standardised functionality that is already available - as it is in canva. I don't see that has 'hackery'.

Addressing the current implementation, how does it deal with cross origin concerns? Videos can play sources that exist on other domains but the user cannot get the video pixels via canvas unless the source domain allows cross origin requests. Does this GetStill API take this into account?

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #8)
> I'd rather use standardised functionality that is already available - as it is
> in canva. I don't see that has 'hackery'.

I suppose if you wanted to just display the image, using a canvas isn't bad. But if you wanted to "save to file" or "upload image", then adding a <canvas> object into the solution seems like overhead.

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

It may also be possible to capture a higher quality still than the frame that's currently being displayed by the video tag.  For what its worth, that is why I kept the return type as a generic nsIUri to allow for video's source to output a still to the file system and return a pointer to that.

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #8)
> Addressing the current implementation, how does it deal with cross origin
> concerns? Videos can play sources that exist on other domains but the user
> cannot get the video pixels via canvas unless the source domain allows cross
> origin requests. Does this GetStill API take this into account?

As implemented a call of this function from non-chrome code would throw an exception as the uri cannot be wrapped.

Comment 12

[cajbir (:cajbir)]

Comment on attachment 392281 [details] [diff] [review]
adds GetStill method returning a data uri

>+nsresult nsHTMLVideoElement::GetStill(nsIURI** aURI) {

>+  char* data = (char*)malloc(avail);
>...
>+  free(b64);

Use ns Array classes to manage the memory (nsAutoArrayPtr?).

>+  virtual nsresult GetStill(nsIURI** aURI);
>+           nsIURI getStill();

I'd prefer a different name. Maybe GetFrame? Or is that overloaded with some other meaning of the word Frame? Does it need a 'moz' prefix since it's non standard? Will you be mentioning this extension to the WHATWG group?

Comment 13

[Justin Dolske [:Dolske]]

[Oops, I didn't mean to move this bug to the A/V Controls component. Sorry for the bugspam.]

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

updated to work with trunk (i.e. layers)

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

this patch uses nsAutoArrayPtrs.  

I do think the meaning of frame is too overloaded, so I'm partial to GetStill() for its brevity, though GetImageFrame or GetStillImage would seem to work as well. I'm unsure for the need of a moz prefix, so I left it as is.  That's easy to add as a review nit.

This would seem to be a good thing to feed into the whatwg.

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

GetImageFrame() sounds better than GetStill(), imo

Comment 17

[cajbir (:cajbir)]

Comment on attachment 446780 [details] [diff] [review]
patch

+++ b/content/html/content/src/nsHTMLVideoElement.cpp	Mon Aug 03 14:00:22 2009 -0400
+nsresult nsHTMLVideoElement::GetStill(nsIURI** aURI) {
+  if (!mDecoder) {
+    return NS_ERROR_UNEXPECTED;
+  }

The standard for this file seems to be not to use braces in 'if'
statements where there is only a single return.

+  return mDecoder->GetStill(aURI);
+  
+}

Remove empty line.

I prefer GetImageFrame or similar for the name as per comment 16.

+++ b/content/media/nsMediaDecoder.cpp	Mon Aug 03 14:00:22 2009 -0400
@@ -52,6 +52,10 @@

+#include "yuv_convert.h"

Why is this included?
 
+nsresult nsMediaDecoder::GetStill(nsIURI** aUri)

Remove use of 'printf' in this function.

+  PRUint32 bpp = 4;
+  encoder->InitFromData(rgbBuffer.get(), surfSize.width * surfSize.height * bpp,
+                        surfSize.width, surfSize.height,
+                        surfSize.width *  bpp, 
+                        imgIEncoder::INPUT_FORMAT_HOSTARGB, 
+                        NS_LITERAL_STRING(""));
+  nsCOMPtr<nsIBinaryInputStream> stream = do_CreateInstance("@mozilla.org/binaryinputstream;1");
+  stream->SetInputStream(encoder);
+  PRUint32 avail = 0;
+  stream->Available(&avail);
+  nsAutoArrayPtr<char> data(new char[avail]);
+  PRUint32 read = 0;
+  stream->Read(data, avail, &read);

Check return values of API functions (Read, Available, InitFromData, etc)

+++ b/dom/interfaces/html/nsIDOMHTMLVideoElement.idl	Mon Aug 03 14:00:22 2009 -0400
+           nsIURI getStill();

Update uuid of nsIDOMHTMLVideoElement for the API change. I think this should have a moz prefix.

Needs tests.

Comment 18

[Biju]

(In reply to comment #16)
> GetImageFrame() sounds better than GetStill(), imo

what about GetFrame(), I assume we will get a corresponding javascript method.
May because I am old school, I prefer methods/function with less characters.

Comment 19

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Justin Dolske [:Dolske] from comment #1)
> Why not just use a <canvas> and drawImage()? That's supposed to work with
> <video>...

Use canvas.