Last Comment Bug 661529 - Make <canvas>.toDataURL more efficient.
: Make <canvas>.toDataURL more efficient.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla7
Assigned To: Kyle Huey [:khuey] (khuey@mozilla.com)
:
Mentors:
: 318185 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-02 07:35 PDT by Kyle Huey [:khuey] (khuey@mozilla.com)
Modified: 2011-08-29 05:28 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Add an API in xpcom/ for encoding input streams to Base64 (30.20 KB, patch)
2011-06-02 07:37 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
no flags Details | Diff | Splinter Review
Part 2: Add NS_ReadInputStreamToBuffer to complement NS_ReadInputStreamToString (2.14 KB, patch)
2011-06-02 07:38 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
cbiesinger: review+
Details | Diff | Splinter Review
Part 3: Use the two previous patches to make .toDataURL more efficient. (7.41 KB, patch)
2011-06-02 07:39 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
vladimir: review+
Details | Diff | Splinter Review
Part 1: Add an API in xpcom/ for encoding input streams to Base64 (30.80 KB, patch)
2011-06-02 10:20 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
benjamin: review+
bzbarsky: superreview+
Details | Diff | Splinter Review

Description Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-02 07:35:32 PDT
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 [:khuey] (khuey@mozilla.com) 2011-06-02 07:37:29 PDT
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.
Comment 2 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-02 07:38:11 PDT
Created attachment 536882 [details] [diff] [review]
Part 2: Add NS_ReadInputStreamToBuffer to complement NS_ReadInputStreamToString
Comment 3 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-02 07:39:27 PDT
Created attachment 536883 [details] [diff] [review]
Part 3: Use the two previous patches to make .toDataURL more efficient.
Comment 4 Boris Zbarsky [:bz] 2011-06-02 08:19:15 PDT
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 [:khuey] (khuey@mozilla.com) 2011-06-02 08:22:42 PDT
(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 [:khuey] (khuey@mozilla.com) 2011-06-02 08:29:08 PDT
(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 [:bz] 2011-06-02 09:43:08 PDT
> 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 [:khuey] (khuey@mozilla.com) 2011-06-02 10:20:04 PDT
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.
Comment 9 Christian :Biesinger (don't email me, ping me on IRC) 2011-06-02 13:37:35 PDT
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 [:bz] 2011-06-02 21:29:48 PDT
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 AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2011-06-08 07:10:55 PDT
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] 2011-06-08 08:36:16 PDT
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 15 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-08 15:11:18 PDT
I had to disable the test on Unix since it assumes wchar_t is two bytes.  I'll fix that soon.
Comment 16 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-17 10:39:07 PDT
*** Bug 318185 has been marked as a duplicate of this bug. ***
Comment 17 Simona B [:simonab] 2011-08-29 05:28:42 PDT
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!

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