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

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

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

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

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

I also concur with comment 1.

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

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

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?

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

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.

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

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

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

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.

GetImageFrame() sounds better than GetStill(), imo

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.

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

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