Closed Bug 720697 Opened 8 years ago Closed 8 years ago

Provide internal API to get canvas image data as nsIInputStream

Categories

(Core :: Canvas: 2D, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: ttaubert, Assigned: ttaubert)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 2 obsolete files)

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.
Attached patch patch v1 (obsolete) — Splinter Review
Not sure if that's the right way to do it but hey, it works.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #591111 - Flags: feedback?(khuey)
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.
Attachment #591111 - Flags: feedback?(khuey) → feedback-
(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.
(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?
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.
Attached patch patch v2 (obsolete) — Splinter Review
(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.
Attachment #591111 - Attachment is obsolete: true
Attachment #591248 - Flags: review?(khuey)
Blocks: 720838
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.
Attachment #591248 - Flags: review?(khuey) → review-
Attached patch patch v3Splinter Review
All issues addressed.
Attachment #591248 - Attachment is obsolete: true
Attachment #593062 - Flags: review?(khuey)
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.
Attachment #593062 - Flags: review?(khuey) → review+
Thanks for the review! Removed optional_argc.

https://hg.mozilla.org/integration/fx-team/rev/abb0f62c7605
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/abb0f62c7605
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
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?
Filed bug 723852.
Depends on: 723852
You need to log in before you can comment on or make changes to this bug.