Closed Bug 648610 Opened 13 years ago Closed 12 years ago

Implement <canvas>.toBlob


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




blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed


(Reporter: khuey, Assigned: khuey)




(Keywords: dev-doc-complete)


(1 file)

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.
Assignee: nobody → khuey
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.
Summary: Replace canvas.mozGetAsFile with canvas.mozGetBlob → Implement <canvas>.toBlob
I am currently developing a patch using |mozGetAsFile| for bug 753768 but I believe that it would work just as well with |toBlob|.
Gaia is going to want this to go fast.
blocking-basecamp: --- → ?
Attached patch PatchSplinter Review
This doesn't do off-main-thread encoding, but it does dispatch the response off the event loop to mimic the async behavior.
Attachment #671562 - Flags: review?(bugs)
Comment on attachment 671562 [details] [diff] [review]

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

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

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()) {
>+  }
>+  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);
Attachment #671562 - Flags: review?(bugs) → review+
this is going to be needed to work around many of the the OOM in gallery, video, and music.  +'ing.
blocking-basecamp: ? → +
(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.
we can use mozGetAsFile.  leaving just as a nom.
blocking-basecamp: + → ?
Please don't use mozGetAsFile.  We want to remove that eventually.
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Comment on attachment 671562 [details] [diff] [review]

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.
Attachment #671562 - Flags: approval-mozilla-aurora?
blocking-basecamp: ? → +
Priority: -- → P1
Attachment #671562 - Flags: approval-mozilla-aurora?
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.
No longer blocks: 798002
Depends on: 802926
(In reply to Kyle Huey [:khuey] ( from comment #4)
> This doesn't do off-main-thread encoding

Is there a bug for implementing off-main-thread encoding?
Filed bug 817700.
Depends on: 817700
Depends on: 822190
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.