The default bug view has changed. See this FAQ.

Make <canvas>.toDataURL more efficient.

RESOLVED FIXED in mozilla7

Status

()

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

People

(Reporter: khuey, Assigned: khuey)

Tracking

unspecified
mozilla7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

toDataURL is fairly inefficient.  We do the following:

1. Read all of the data out of the image encoder into a buffer.
2. Base64 encode this buffer into another buffer
3. Convert that buffer from UTF-8 (not ASCII!!!) to UTF-16.

We perform two unnecessary copies here.  This can be optimized by having something use ReadSegments on the input stream and base64 encode piecemeal to the proper character set.  It so happens that I have patches to do that!
Created attachment 536881 [details] [diff] [review]
Part 1: Add an API in xpcom/ for encoding input streams to Base64

Not thrilled about having an xpcom interface but it's the only way to get at this from outside of libxul C++ code :-/

Some of this code is copied from NSPR (the actual base64 encoding routine).  The API NSPR exposes doesn't really lend itself to this, and I didn't want to try patching NSPR.
Attachment #536881 - Flags: review?(benjamin)
Created attachment 536882 [details] [diff] [review]
Part 2: Add NS_ReadInputStreamToBuffer to complement NS_ReadInputStreamToString
Attachment #536882 - Flags: review?(cbiesinger)
Created attachment 536883 [details] [diff] [review]
Part 3: Use the two previous patches to make .toDataURL more efficient.
Attachment #536883 - Flags: review?(vladimir)
So... the old code dealt with an input stream that has multiple chunks so it's not all available at once.  The new encoder code does not.  It probably should.

Your idl should probably document that the incoming stream needs to implement ReadSegments (some streams don't).

(Of course there's another issue here: our image encoders are technically non-blocking streams that as far as I can tell can in fact async encode the image on a separate thread from the thread you're reading the data on.  I don't know whether they do in practice, and the old code is broken in this situation too.)
(In reply to comment #4)
> So... the old code dealt with an input stream that has multiple chunks so
> it's not all available at once.  The new encoder code does not.  It probably
> should.

I don't believe it needs to.

> Your idl should probably document that the incoming stream needs to
> implement ReadSegments (some streams don't).

Agreed.

> (Of course there's another issue here: our image encoders are technically
> non-blocking streams that as far as I can tell can in fact async encode the
> image on a separate thread from the thread you're reading the data on.  I
> don't know whether they do in practice, and the old code is broken in this
> situation too.)

They can be used asynchronously, but they don't have to be.  The encoders encode synchronously when imgIEncoder::InitFromData is called.  You can use asynchronously if you create one and hand it out as an input stream and pump data into it later on a background thread.  This code, however, calls InitFromData before we start reading so the encoding is completely done before we start reading.  This is also why I don't think we need to deal with the multiple chunks case.
(In reply to comment #5)
> (In reply to comment #4)
> > So... the old code dealt with an input stream that has multiple chunks so
> > it's not all available at once.  The new encoder code does not.  It probably
> > should.
> 
> I don't believe it needs to.

Actually, it should do this anyways so it can be used with non-encoder streams.
> Actually, it should do this anyways so it can be used with non-encoder streams.

Precisely.  This is also why you need to document the readSegments restriction.
Created attachment 536919 [details] [diff] [review]
Part 1: Add an API in xpcom/ for encoding input streams to Base64

Revised version.  Here we abort if we get NS_ERROR_NOT_IMPLEMENTED or NS_BASE_STREAM_WOULD_BLOCK returned from ReadSegments, and these limitations are written in the IDL.
Attachment #536881 - Attachment is obsolete: true
Attachment #536881 - Flags: review?(benjamin)
Attachment #536919 - Flags: superreview?(bzbarsky)
Attachment #536919 - Flags: review?(benjamin)
Comment on attachment 536882 [details] [diff] [review]
Part 2: Add NS_ReadInputStreamToBuffer to complement NS_ReadInputStreamToString

+    if (!*aDest)
+        *aDest = malloc(aCount);
+    if (!*aDest)
         return NS_ERROR_OUT_OF_MEMORY;

please put the second nullcheck inside the first if

+    char * p = (char*)*aDest;

reinterpret_cast, please

+    void* dest = (void*)aDest.BeginWriting();

Just write void* dest = aDest.BeginWriting();
Attachment #536882 - Flags: review?(cbiesinger) → review+
Comment on attachment 536919 [details] [diff] [review]
Part 1: Add an API in xpcom/ for encoding input streams to Base64

s/must not be asynchronous/must not be a non-blocking stream that will return NS_BASE_STREAM_WOULD_BLOCK/, please.

sr=me with that.
Attachment #536919 - Flags: superreview?(bzbarsky) → superreview+
Comment on attachment 536919 [details] [diff] [review]
Part 1: Add an API in xpcom/ for encoding input streams to Base64

>diff --git a/xpcom/build/nsXPComInit.cpp b/xpcom/build/nsXPComInit.cpp
>diff --git a/xpcom/io/Base64.cpp b/xpcom/io/Base64.cpp

>+// BEGIN base64 encode code copied and modified from NSPR
>+static unsigned char *base = (unsigned char *)"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/";

This is already in an anonymous namespace, no need to also be declared static. And for performance, it should be:

const unsigned char base[] = "...";
Attachment #536919 - Flags: review?(benjamin) → review+
Comment on attachment 536883 [details] [diff] [review]
Part 3: Use the two previous patches to make .toDataURL more efficient.

Review of attachment 536883 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great to me
Attachment #536883 - Flags: review?(vladimir) → review+
http://hg.mozilla.org/projects/cedar/rev/78b64bf82622
http://hg.mozilla.org/projects/cedar/rev/edaa20bfaffe
http://hg.mozilla.org/projects/cedar/rev/dd0a133916c7
Whiteboard: fixed-in-cedar
http://hg.mozilla.org/mozilla-central/rev/78b64bf82622
http://hg.mozilla.org/mozilla-central/rev/edaa20bfaffe
http://hg.mozilla.org/mozilla-central/rev/dd0a133916c7
http://hg.mozilla.org/mozilla-central/rev/4e2b86b6025c
http://hg.mozilla.org/mozilla-central/rev/31ea40628d48
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
Target Milestone: --- → mozilla7
I had to disable the test on Unix since it assumes wchar_t is two bytes.  I'll fix that soon.
Duplicate of this bug: 318185
Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0

Can anyone please help me with a test case or with STR/guidelines in order to get this issue checked on QA side?
Thanks!
You need to log in before you can comment on or make changes to this bug.