Last Comment Bug 720697 - Provide internal API to get canvas image data as nsIInputStream
: Provide internal API to get canvas image data as nsIInputStream
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Tim Taubert [:ttaubert]
:
Mentors:
Depends on: 723852
Blocks: 720838
  Show dependency treegraph
 
Reported: 2012-01-24 07:12 PST by Tim Taubert [:ttaubert]
Modified: 2012-04-29 14:27 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (2.77 KB, patch)
2012-01-24 08:28 PST, Tim Taubert [:ttaubert]
khuey: feedback-
Details | Diff | Splinter Review
patch v2 (3.98 KB, patch)
2012-01-24 13:27 PST, Tim Taubert [:ttaubert]
khuey: review-
Details | Diff | Splinter Review
patch v3 (3.76 KB, patch)
2012-01-31 05:52 PST, Tim Taubert [:ttaubert]
khuey: review+
Details | Diff | Splinter Review

Description Tim Taubert [:ttaubert] 2012-01-24 07:12:09 PST
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] 2012-01-24 08:28:57 PST
Created attachment 591111 [details] [diff] [review]
patch v1

Not sure if that's the right way to do it but hey, it works.
Comment 2 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-01-24 08:41:02 PST
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-01-24 08:42:22 PST
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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] 2012-01-24 08:53:56 PST
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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] (khuey@mozilla.com) 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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-01-24 09:44:39 PST
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] 2012-01-24 13:27:44 PST
Created attachment 591248 [details] [diff] [review]
patch v2

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-01-30 08:50:28 PST
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] 2012-01-31 05:52:48 PST
Created attachment 593062 [details] [diff] [review]
patch v3

All issues addressed.
Comment 9 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-01-31 05:58:32 PST
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] 2012-02-01 02:52:39 PST
Thanks for the review! Removed optional_argc.

https://hg.mozilla.org/integration/fx-team/rev/abb0f62c7605
Comment 11 Tim Taubert [:ttaubert] 2012-02-02 01:08:35 PST
https://hg.mozilla.org/mozilla-central/rev/abb0f62c7605
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-02 09:11:09 PST
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] 2012-02-03 01:38:25 PST
Filed bug 723852.
Comment 14 Eric Shepherd [:sheppy] 2012-04-29 14:27:28 PDT
Documented:

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

Listed on Firefox 13 for developers.

Note You need to log in before you can comment on or make changes to this bug.