HTMLCanvasElement toDataURL and toBlob do unnecessary data copies through a CFDataRef
Created attachment 428790 [details] Patch
Created attachment 428796 [details] Patch
Created attachment 428805 [details] Patch
Created attachment 428806 [details] Patch
Created attachment 428807 [details] Patch
Created attachment 428835 [details] Patch
Ok, ready for review now.
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.
(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.
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.
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!
Created attachment 428932 [details] Patch
(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>>()?
(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
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.
Created attachment 429082 [details] Patch
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].
<rdar://problem/78224439>