661529 - Make <canvas>.toDataURL more efficient.

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

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

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!

Comment 1

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

Attachment: 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.

Comment 2

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

Attachment: Part 2: Add NS_ReadInputStreamToBuffer to complement NS_ReadInputStreamToString

Comment 3

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

Attachment: Part 3: Use the two previous patches to make .toDataURL more efficient.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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

Comment 5

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

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

Comment 6

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

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

Comment 7

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 8

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

Attachment: 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.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

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();

Comment 10

[Boris Zbarsky [:bzbarsky]]

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.

Comment 11

[Benjamin Smedberg]

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[] = "...";

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

Comment 13

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

http://hg.mozilla.org/projects/cedar/rev/78b64bf82622
http://hg.mozilla.org/projects/cedar/rev/edaa20bfaffe
http://hg.mozilla.org/projects/cedar/rev/dd0a133916c7

Comment 14

[(no longer active)]

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

Comment 15

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

I had to disable the test on Unix since it assumes wchar_t is two bytes.  I'll fix that soon.

Comment 17

[Simona Badau, Desktop QA]

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!