Closed Bug 661529 Opened 9 years ago Closed 9 years ago

Make <canvas>.toDataURL more efficient.

Categories

(Core :: Canvas: 2D, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla7

People

(Reporter: khuey, Assigned: khuey)

References

Details

Attachments

(3 files, 1 obsolete file)

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!
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)
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.
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+
I had to disable the test on Unix since it assumes wchar_t is two bytes.  I'll fix that soon.
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.