Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Provide internal API to get canvas image data as nsIInputStream

RESOLVED FIXED in mozilla13

Status

()

Core
Canvas: 2D
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

({dev-doc-complete})

Trunk
mozilla13
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 591111 [details] [diff] [review]
patch v1

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

Comment 4

6 years ago
(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.
(Assignee)

Comment 6

6 years ago
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.
Attachment #591111 - Attachment is obsolete: true
Attachment #591248 - Flags: review?(khuey)
(Assignee)

Updated

6 years ago
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-
(Assignee)

Comment 8

6 years ago
Created attachment 593062 [details] [diff] [review]
patch v3

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+
(Assignee)

Comment 10

6 years ago
Thanks for the review! Removed optional_argc.

https://hg.mozilla.org/integration/fx-team/rev/abb0f62c7605
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla13
(Assignee)

Comment 11

6 years ago
https://hg.mozilla.org/mozilla-central/rev/abb0f62c7605
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
(Assignee)

Updated

6 years ago
Keywords: dev-doc-needed
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?
(Assignee)

Comment 13

6 years ago
Filed bug 723852.
Depends on: 723852
Documented:

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

Listed on Firefox 13 for developers.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.