648610 - Implement <canvas>.toBlob

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

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

We should do this and get it standardized.

Not sure whether or not we need to keep mozGetAsFile.  I think we can afford to just drop it completely.

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

So, the comment in the spec has basically

  void toBlob(FileCallback cb, optional DOMString type, any... args);

I'm not sure if it's useful to change the name of the API now, as we'll need to change it at least once more. Waiting until Hixie fleshes this out and implement the spec then seems more logical to me.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

I am currently developing a patch using |mozGetAsFile| for bug 753768 but I believe that it would work just as well with |toBlob|.

Comment 3

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

Gaia is going to want this to go fast.

Comment 4

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

Attachment: Patch

This doesn't do off-main-thread encoding, but it does dispatch the response off the event loop to mimic the async behavior.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 671562 [details] [diff] [review]
Patch

>+// XXXkhuey this should be async, but we're lazy.
You mean off-main-thread?


>+NS_IMETHODIMP
>+nsHTMLCanvasElement::ToBlob(nsIFileCallback* aCallback,
>+                            const nsAString& aType,
>+                            nsIVariant* aParams,
>+                            uint8_t optional_argc)
aParamName


Before using aType, it should be lowercased.


>+{
>+  // do a trust check if this is a write-only canvas
>+  if ((mWriteOnly) &&
No need for () around mWriteOnly

>+      !nsContentUtils::IsCallerTrustedForRead()) {
>+    return NS_ERROR_DOM_SECURITY_ERR;
>+  }
>+  void* imgData = nullptr;
>+  rv = NS_ReadInputStreamToBuffer(stream, &imgData, (uint32_t)imgSize);
I'd prefer C++ cast

>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  // The DOMFile takes ownership of the buffer
>+  nsRefPtr<nsDOMMemoryFile> blob =
>+    new nsDOMMemoryFile(imgData, (uint32_t)imgSize, type);
ditto

Comment 6

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/227e87a7c628

Comment 7

[Doug Turner (:dougt)]

this is going to be needed to work around many of the the OOM in gallery, video, and music.  +'ing.

Comment 8

[Justin Lebar (not reading bugmail)]

(In reply to Doug Turner (:dougt) from comment #7)
> this is going to be needed to work around many of the the OOM in gallery,
> video, and music.  +'ing.

Can you elaborate, or point to bugs?

base-64-encoded URIs are 2.7x larger than blobs, but I don't see how using this would actually solve an underlying problem; if we OOM with data URIs, we'd OOM with this, just after showing something like 3x more images.

Comment 9

[Doug Turner (:dougt)]

we can use mozGetAsFile.  leaving just as a nom.

Comment 10

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

Please don't use mozGetAsFile.  We want to remove that eventually.

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/227e87a7c628
https://hg.mozilla.org/mozilla-central/rev/30395df97c29

Comment 12

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

Comment on attachment 671562 [details] [diff] [review]
Patch

Gaia will want this to be able to use memory more efficiently

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: Certain Gaia apps may be written with less supported APIs or may crash after running out of memory more often.
Testing completed (on m-c, etc.): On m-c, has automated tests.
Risk to taking this patch (and alternatives if risky): Low.  This is just an async wrapper around an existing synchronous codepath.
String or UUID changes made by this patch: This changes the IID for nsIDOMHTMLCanvasElement.  I could avoid that if strictly necessary, but I don't think it matters.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cbfef8616b63 (I qfolded the 2 patches into 1)

Comment 14

[David Flanagan [:djf]]

This is great!  For the record, Gaia only uses data urls when for transferring images through activities because the actvities API doesn't support blobs yet.  Everywhere else we use mozGetAsFile.  So toBlob() probably won't help with memory at all, unless mozGetAsFile is really inefficient in some way that I don't know about.

I was hopeful that the fact that is async would improve the speed of thumbnail and album art metadata parsing for the Gallery and Music apps and also help the screenshot code in the window manager app.  But give Kyle's comment #4 above, I'm not sure this will help us with speed either.  Could you elaborate, Kyle?

If there is a speed improvement, then I think we should convert the metadata parsing code in Gallery and Music, and the screenshot code in the window manager to use toBlob(), but leave other mozGetAsFile() calls as is for v1.

For v2, we'll obviously want to convert everything to use the standard method instead of the moz-prefixed one.

cc'ing timdream because he owns the window manager screenshot stuff and should know about this.

Comment 15

[Markus Stange [:mstange]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #4)
> This doesn't do off-main-thread encoding

Is there a bug for implementing off-main-thread encoding?

Comment 16

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

No.

Comment 17

[Markus Stange [:mstange]]

Filed bug 817700.

Comment 18

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/Firefox/Releases/19
https://developer.mozilla.org/en-US/docs/Web/API/HTMLCanvasElement.toBlob