Last Comment Bug 648610 - Implement <canvas>.toBlob
: Implement <canvas>.toBlob
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: P1 normal with 3 votes (vote)
: mozilla19
Assigned To: Kyle Huey [:khuey] (khuey@mozilla.com)
:
Mentors:
http://lists.whatwg.org/htdig.cgi/wha...
Depends on: 802926 809587 817700 822190
Blocks: 802647 803684
  Show dependency treegraph
 
Reported: 2011-04-08 11:59 PDT by Kyle Huey [:khuey] (khuey@mozilla.com)
Modified: 2014-10-13 06:21 PDT (History)
15 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
fixed


Attachments
Patch (8.48 KB, patch)
2012-10-15 13:03 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
bugs: review+
Details | Diff | Review

Description Kyle Huey [:khuey] (khuey@mozilla.com) 2011-04-08 11:59:08 PDT
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 2011-04-10 08:48:58 PDT
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 Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-05-14 01:17:31 PDT
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 [:khuey] (khuey@mozilla.com) 2012-10-15 13:02:25 PDT
Gaia is going to want this to go fast.
Comment 4 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-10-15 13:03:16 PDT
Created attachment 671562 [details] [diff] [review]
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] (high review load, please consider other reviewers) 2012-10-15 13:30:41 PDT
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 [:khuey] (khuey@mozilla.com) 2012-10-15 14:17:04 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/227e87a7c628
Comment 7 Doug Turner (:dougt) 2012-10-15 14:23:54 PDT
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) 2012-10-15 14:54:54 PDT
(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) 2012-10-15 15:05:51 PDT
we can use mozGetAsFile.  leaving just as a nom.
Comment 10 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-10-15 15:06:53 PDT
Please don't use mozGetAsFile.  We want to remove that eventually.
Comment 12 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-10-16 07:37:34 PDT
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] 2012-10-16 17:51:06 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/cbfef8616b63 (I qfolded the 2 patches into 1)
Comment 14 David Flanagan [:djf] 2012-10-16 20:40:25 PDT
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] 2012-12-03 10:20:03 PST
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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 [:khuey] (khuey@mozilla.com) 2012-12-03 10:21:17 PST
No.
Comment 17 Markus Stange [:mstange] 2012-12-03 10:48:14 PST
Filed bug 817700.

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