Bug 225853 – HTMLCanvasElement toDataURL and toBlob do unnecessary data copies through a CFDataRef

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

HTMLCanvasElement toDataURL and toBlob do unnecessary data copies through a CFDataRef

[Sam Weinig]

Created attachment 428790 [details]
Patch

[Sam Weinig]

Created attachment 428796 [details]
Patch

[Sam Weinig]

Created attachment 428805 [details]
Patch

[Sam Weinig]

Created attachment 428806 [details]
Patch

[Sam Weinig]

Created attachment 428807 [details]
Patch

[Sam Weinig]

Created attachment 428835 [details]
Patch

[Sam Weinig]

Ok, ready for review now.

[Darin Adler]

Comment on attachment 428835 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428835&action=review

> Source/WTF/wtf/text/Base64.cpp:94
> +template<typename CharacterType> static inline void base64EncodeInternal(const unsigned char* inputDataBuffer, unsigned inputLength, CharacterType* destinationDataBuffer, unsigned destinationLength, Base64EncodePolicy policy)

Maybe we should be using uint8_t instead of unsigned char?

> Source/WTF/wtf/text/Base64.cpp:143
> +static inline void base64EncodeInternal(const unsigned char* inputDataBuffer, unsigned inputLength, Vector<char>& destinationVector, Base64EncodePolicy policy)

Can this use a return value instead of an out vector?

> Source/WTF/wtf/text/Base64.h:52
> +static constexpr unsigned maximumBase64EncoderInputBufferSize = UINT_MAX / 77 * 76 / 4 * 3 - 2;

std::numeric_limits instead of UINT_MAX? Comment should explain the nature of the conservative estimate.

> Source/WTF/wtf/text/Base64.h:153
> +WTF_EXPORT_PRIVATE void base64Encode(const void*, unsigned, Vector<char>&, Base64EncodePolicy = Base64DoNotInsertLFs);

Can we change this to product a return value instead of taking an out argument?

> Source/WTF/wtf/text/Base64.h:159
> +void base64Encode(ConstSignedOrUnsignedCharVectorAdapter, Vector<char>&, Base64EncodePolicy = Base64DoNotInsertLFs);
> +void base64Encode(const CString&, Vector<char>&, Base64EncodePolicy = Base64DoNotInsertLFs);

Can we change this to product a return value instead of taking an out argument?

> Source/WTF/wtf/text/Base64.h:252
> +        if (result > 76)
> +            return result + ((result - 1) / 76);

Can we name the 76 constant?

Why the if statement? Seems like values in the 1-76 range would still yield the correct result.

I would drop one set of parentheses here.

> Source/WebCore/ChangeLog:49
> +        Replace encodeImage(), which use a filled in a CFMutableDataRef with overloads
> +        of data and dataURL that now use a callback based CGDataConsumer and some callback
> +        functors to allow the encoded data to be consumed as it is being created.

Some grammar problems in this sentence. "which use a", and not enough commas.

> Source/WebCore/platform/graphics/cg/ImageBufferCGBackend.cpp:203
> +        auto image = nativeImage->platformImage();
> +        return createCroppedImageIfNecessary(image.get(), backendSize());

Better without the local?

> Source/WebCore/platform/graphics/cg/ImageBufferCGBackend.h:58
> -    virtual RetainPtr<CFDataRef> toCFData(const String& mimeType, Optional<double> quality, PreserveResolution) const;
> +
> +    virtual PlatformImagePtr copyPlatformImageForEncoding(CFStringRef destinationUTI, PreserveResolution) const;

Is this really an improvement? I’m not sure we need to use the typedef name when this is not platform-independent code.

[Sam Weinig]

(In reply to Darin Adler from comment #8)
> Comment on attachment 428835 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=428835&action=review
> 
> > Source/WTF/wtf/text/Base64.cpp:94
> > +template<typename CharacterType> static inline void base64EncodeInternal(const unsigned char* inputDataBuffer, unsigned inputLength, CharacterType* destinationDataBuffer, unsigned destinationLength, Base64EncodePolicy policy)
> 
> Maybe we should be using uint8_t instead of unsigned char?

Sounds good.

> 
> > Source/WTF/wtf/text/Base64.cpp:143
> > +static inline void base64EncodeInternal(const unsigned char* inputDataBuffer, unsigned inputLength, Vector<char>& destinationVector, Base64EncodePolicy policy)
> 
> Can this use a return value instead of an out vector?

There are still some places that are passing one in, but I plan to resolve those soon.

> 
> > Source/WTF/wtf/text/Base64.h:52
> > +static constexpr unsigned maximumBase64EncoderInputBufferSize = UINT_MAX / 77 * 76 / 4 * 3 - 2;
> 
> std::numeric_limits instead of UINT_MAX? Comment should explain the nature
> of the conservative estimate.

Will change to std::numeric_limits. I will do some digging, but the reason may be lost to time. I was just moving the comment along.

> 
> > Source/WTF/wtf/text/Base64.h:153
> > +WTF_EXPORT_PRIVATE void base64Encode(const void*, unsigned, Vector<char>&, Base64EncodePolicy = Base64DoNotInsertLFs);
> 
> Can we change this to product a return value instead of taking an out
> argument?

Yep, planning on it, just not in this round.

> 
> > Source/WTF/wtf/text/Base64.h:252
> > +        if (result > 76)
> > +            return result + ((result - 1) / 76);
> 
> Can we name the 76 constant?

Will do.

> 
> Why the if statement? Seems like values in the 1-76 range would still yield
> the correct result.

I think you are right. I was moving it from the encode function and didn't think it through.

> 
> I would drop one set of parentheses here.

But which one? :).

> 
> > Source/WebCore/ChangeLog:49
> > +        Replace encodeImage(), which use a filled in a CFMutableDataRef with overloads
> > +        of data and dataURL that now use a callback based CGDataConsumer and some callback
> > +        functors to allow the encoded data to be consumed as it is being created.
> 
> Some grammar problems in this sentence. "which use a", and not enough commas.

Fixed.

> 
> > Source/WebCore/platform/graphics/cg/ImageBufferCGBackend.cpp:203
> > +        auto image = nativeImage->platformImage();
> > +        return createCroppedImageIfNecessary(image.get(), backendSize());
> 
> Better without the local?

Yeah. Fixed.

> 
> > Source/WebCore/platform/graphics/cg/ImageBufferCGBackend.h:58
> > -    virtual RetainPtr<CFDataRef> toCFData(const String& mimeType, Optional<double> quality, PreserveResolution) const;
> > +
> > +    virtual PlatformImagePtr copyPlatformImageForEncoding(CFStringRef destinationUTI, PreserveResolution) const;
> 
> Is this really an improvement? I’m not sure we need to use the typedef name
> when this is not platform-independent code.

Which part of it are you concerned isn't an improvement? I agree making the function return a RetainPtr<CGImageRef> and calling it something like createCGImageForEncoding() would be even better and will do that.

[Darin Adler]

Comment on attachment 428835 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428835&action=review

>>> Source/WTF/wtf/text/Base64.cpp:143
>>> +static inline void base64EncodeInternal(const unsigned char* inputDataBuffer, unsigned inputLength, Vector<char>& destinationVector, Base64EncodePolicy policy)
>> 
>> Can this use a return value instead of an out vector?
> 
> There are still some places that are passing one in, but I plan to resolve those soon.

The places that are passing one in can just assign it to the result, so I see no reason to wait.

[Darin Adler]

Comment on attachment 428835 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428835&action=review

>>> Source/WebCore/platform/graphics/cg/ImageBufferCGBackend.h:58
>>> +    virtual PlatformImagePtr copyPlatformImageForEncoding(CFStringRef destinationUTI, PreserveResolution) const;
>> 
>> Is this really an improvement? I’m not sure we need to use the typedef name when this is not platform-independent code.
> 
> Which part of it are you concerned isn't an improvement? I agree making the function return a RetainPtr<CGImageRef> and calling it something like createCGImageForEncoding() would be even better and will do that.

If PlatformImagePtr exists so we can write platform-independent code that manipulates an image pointer, I still suggest calling it RetainPtr<CGImageRef> in CoreGraphics-platform-only code. I don’t think the typedef is an abstraction. But your idea is even better because this virtual function is platform-specific!

[Sam Weinig]

Created attachment 428932 [details]
Patch

[Sam Weinig]

(In reply to Darin Adler from comment #10)
> Comment on attachment 428835 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=428835&action=review
> 
> >>> Source/WTF/wtf/text/Base64.cpp:143
> >>> +static inline void base64EncodeInternal(const unsigned char* inputDataBuffer, unsigned inputLength, Vector<char>& destinationVector, Base64EncodePolicy policy)
> >> 
> >> Can this use a return value instead of an out vector?
> > 
> > There are still some places that are passing one in, but I plan to resolve those soon.
> 
> The places that are passing one in can just assign it to the result, so I
> see no reason to wait.

Fair enough.

To do this, I am going to somehow differentiate the base64Encode() that returns a String and the one that returns a Vector<char>. Do you have a naming preference here?
I was thinking of just using base64EncodeToString() and base64EncodeToVector(). I could also do base64Encode<String>() and base64Encode<Vector<char>>()?

[Sam Weinig]

(In reply to Sam Weinig from comment #13)
> (In reply to Darin Adler from comment #10)
> > Comment on attachment 428835 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=428835&action=review
> > 
> > >>> Source/WTF/wtf/text/Base64.cpp:143
> > >>> +static inline void base64EncodeInternal(const unsigned char* inputDataBuffer, unsigned inputLength, Vector<char>& destinationVector, Base64EncodePolicy policy)
> > >> 
> > >> Can this use a return value instead of an out vector?
> > > 
> > > There are still some places that are passing one in, but I plan to resolve those soon.
> > 
> > The places that are passing one in can just assign it to the result, so I
> > see no reason to wait.
> 
> Fair enough.
> 
> To do this, I am going to somehow differentiate the base64Encode() that
> returns a String and the one that returns a Vector<char>. Do you have a
> naming preference here?
> I was thinking of just using base64EncodeToString() and
> base64EncodeToVector(). I could also do base64Encode<String>() and
> base64Encode<Vector<char>>()?

Or, I could make base64Encode() always return a Vector<char> and use makeString(base64Encoded()) for the string cases. That might be nicer

[Sam Weinig]

I decided that I should do the base64 changes first, and then land this part as they are quite disconnected. Doing the base64 bit in https://bugs.webkit.org/show_bug.cgi?id=225920.

[Sam Weinig]

Created attachment 429082 [details]
Patch

[EWS]

Committed r277751 (237921@main): <https://commits.webkit.org/237921@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 429082 [details].

[Radar WebKit Bug Importer]

<rdar://problem/78224439>