720697 - Provide internal API to get canvas image data as nsIInputStream

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

The thumbnail service currently creates a hidden canvas and uses drawWindow() to capture thumbnails of tabs. After that we use the following to store this image into the file cache:


let url = canvas.toDataUrl("image/png");
let uri = Services.io.newURI(url, "UTF8", null);

NetUtil.asyncFetch(uri, function (aData, aResult) {
  // aData is now the stream we can asyncCopy() to the cache entry...
});


This seems more like a workaround. It would be great (and way more efficient) to somehow expose nsHTMLCanvasElement::ExtractData() to get an nsIInputStream filled with the desired image format.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Not sure if that's the right way to do it but hey, it works.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 591111 [details] [diff] [review]
patch v1

At the very least, this needs a security check (e.g. nsContentUtils::IsCallerChrome(), we can't pass an nsIInputStream out to random webpage code).

I need to think a bit about the implications of this.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #2)
> Comment on attachment 591111 [details] [diff] [review]
> patch v1
> 
> At the very least, this needs a security check (e.g.
> nsContentUtils::IsCallerChrome(), we can't pass an nsIInputStream out to
> random webpage code).
> 
> I need to think a bit about the implications of this.

Specifically, this will synchronously do the image encoding, which I don't think we want.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #2)
> At the very least, this needs a security check (e.g.
> nsContentUtils::IsCallerChrome(), we can't pass an nsIInputStream out to
> random webpage code).

Yes, good point.

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #3)
> Specifically, this will synchronously do the image encoding, which I don't
> think we want.

We currently use toDataURL() so that's synchronous as well but would already improve our situation a little. But I agree we should do it right and make it possible to retrieve the image data stream asynchronously. Is this way more complicated? Can you give me any hints on how to do that?

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

It's more complicated.  I think we should design the API so that it can be asynchronous (takes a callback) but we can invoke the callback synchronously for the moment.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #5)
> It's more complicated.  I think we should design the API so that it can be
> asynchronous (takes a callback) but we can invoke the callback synchronously
> for the moment.

Done. I'll file a follow-up bug to make it asynchronous in the future.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 591248 [details] [diff] [review]
patch v2

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

This looks mostly ok, but I'd like to see it again before giving an r+.

::: content/html/content/src/nsHTMLCanvasElement.cpp
@@ +222,5 @@
> +nsHTMLCanvasElement::MozFetchAsStream(nsIInputStreamCallback *aCallback,
> +                                      const nsAString& aType,
> +                                      PRUint8 optional_argc)
> +{
> +  NS_ENSURE_ARG_POINTER(aCallback);

Don't bother checking the argument, since this is only called by chrome.

@@ +229,5 @@
> +    return NS_ERROR_FAILURE;
> +
> +  // do a trust check if this is a write-only canvas
> +  if (mWriteOnly && !nsContentUtils::IsCallerTrustedForRead())
> +    return NS_ERROR_DOM_SECURITY_ERR;

There's no need to do this.  By this point the caller is chrome.

@@ +236,5 @@
> +  nsCOMPtr<nsIInputStream> inputData;
> +  nsresult rv = ExtractData(aType, EmptyString(), getter_AddRefs(inputData),
> +                            fellBackToPNG);
> +
> +  if (NS_SUCCEEDED(rv)) {

NS_ENSURE_SUCCESS(rv, rv);

@@ +238,5 @@
> +                            fellBackToPNG);
> +
> +  if (NS_SUCCEEDED(rv)) {
> +    nsCOMPtr<nsIAsyncInputStream> asyncData = do_QueryInterface(inputData, &rv);
> +    if (NS_SUCCEEDED(rv))

NS_ENSURE_SUCCESS(rv, rv);

@@ +239,5 @@
> +
> +  if (NS_SUCCEEDED(rv)) {
> +    nsCOMPtr<nsIAsyncInputStream> asyncData = do_QueryInterface(inputData, &rv);
> +    if (NS_SUCCEEDED(rv))
> +      aCallback->OnInputStreamReady(asyncData);

You should check the return value here.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v3

All issues addressed.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 593062 [details] [diff] [review]
patch v3

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

::: content/html/content/src/nsHTMLCanvasElement.cpp
@@ +220,5 @@
> +
> +NS_IMETHODIMP
> +nsHTMLCanvasElement::MozFetchAsStream(nsIInputStreamCallback *aCallback,
> +                                      const nsAString& aType,
> +                                      PRUint8 optional_argc)

And remove optional_argc from the signature.

::: dom/interfaces/html/nsIDOMHTMLCanvasElement.idl
@@ +86,5 @@
> +
> +  // A Mozilla-only extension that returns the canvas' image data as a data
> +  // stream in the desired image format.
> +  [optional_argc] void mozFetchAsStream(in nsIInputStreamCallback callback,
> +                                        [optional] in DOMString type);

Remove optional_argc from the IDL since it's not needed.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Thanks for the review! Removed optional_argc.

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

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 12

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

Comment on attachment 593062 [details] [diff] [review]
patch v3

>diff --git a/content/html/content/src/nsHTMLCanvasElement.cpp b/content/html/content/src/nsHTMLCanvasElement.cpp

>+nsHTMLCanvasElement::MozFetchAsStream(nsIInputStreamCallback *aCallback,

>+  return aCallback->OnInputStreamReady(asyncData);

It's generally a bad idea to design an API to be async-friendly, but then invoke the callback synchronously - people can start depending on that, making your life difficult when it comes time to change it in the future. Can you use a runnable here?

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Filed bug 723852.

Comment 14

[Eric Shepherd [:sheppy]]

Documented:

https://developer.mozilla.org/en/DOM/HTMLCanvasElement#Methods

Listed on Firefox 13 for developers.