RESOLVED FIXED 49365
[chromium] Add canvas.toDataURL("image/jpeg", quality) support
https://bugs.webkit.org/show_bug.cgi?id=49365
Summary [chromium] Add canvas.toDataURL("image/jpeg", quality) support
noel gordon
Reported 2010-11-10 22:27:56 PST
Chromium win/linux support "image/png" in .toDataURL(), but not "image/jpeg".
Attachments
patch (15.65 KB, patch)
2010-11-15 23:10 PST, noel gordon
no flags
patch (14.84 KB, patch)
2010-11-16 17:24 PST, noel gordon
no flags
patch (17.07 KB, patch)
2010-11-18 23:52 PST, noel gordon
levin: review-
patch wip (17.09 KB, patch)
2010-11-26 01:40 PST, noel gordon
no flags
patch wip static functions (16.40 KB, patch)
2010-11-29 01:30 PST, noel gordon
no flags
noel gordon
Comment 1 2010-11-15 23:10:17 PST
WebKit Review Bot
Comment 2 2010-11-15 23:14:32 PST
noel gordon
Comment 3 2010-11-16 17:24:58 PST
Created attachment 74069 [details] patch costs a style nit, but builds on linux
WebKit Review Bot
Comment 4 2010-11-16 17:26:32 PST
Attachment 74069 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1 Total errors found: 0 in 0 files If any of these errors are false positives, please file a bug against check-webkit-style.
noel gordon
Comment 5 2010-11-18 23:52:35 PST
Created attachment 74360 [details] patch chromium trys pass fast/canvas/canvas-toDataURL-case-insensitive-mimetype.html now, fix that and update ChangeLogs.
Peter Kasting
Comment 6 2010-11-23 10:45:41 PST
Comment on attachment 74360 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=74360&action=review > WebCore/ChangeLog:17 > + Adds a libjpeg-based image encoder for Skia bitmaps. Default encoding quality > + is 92 to match Mozilla, also Safari, though the actual value used by Safari is > + undocumented, and it appears to pre-blur images prior to compression. > + > + JPGBitmapEncoder::preMultipliedBGRAtoRGB restores the un-multiplied RGB colors > + where there is non-zero alpha. Again, this matches Firefox and Safari, but no > + browser conforms to the HTML5 canvas standard here, I believe, considering the > + results of canvas/philip/tests/toDataURL.jpeg.alpha.html; the test ignores the > + alpha channel when extracting an "image/jpeg".toDataURL(). The correct answer > + needs more investigation. Nit: These comments are really helpful. For this reason, I'd like to see them in the source code itself. The first comment can be in the declaration of JPGImageEncoder, and the second in the declaration of JPGBitmapEncoder. (They don't need to be duplicated into the ChangeLog at that point.) > WebCore/platform/graphics/skia/ImageBufferSkia.cpp:322 > + Vector<unsigned char> encodedImage; > + Nit: Extra blank line here > WebCore/platform/image-encoders/skia/JPGImageEncoder.cpp:45 > +class JPGBitmapEncoder : public jpeg_destination_mgr { Nit: WebCore is pretty consistent about using "JPEG" rather than "JPG" in class and file names. Please follow suit. > WebCore/platform/image-encoders/skia/JPGImageEncoder.cpp:47 > +public: > + JPGBitmapEncoder(const SkBitmap& input, Vector<unsigned char>* output) Nit: Defining your methods inline in your declaration tells the compiler you want them inlined. Since that's not actually what you mean here, you should declare the class above its definition. This also makes it easier for a reader to quickly see the structure of the class by just glancing at its declaration, and for you to mark with a single comment theblock of functions responsible for implementing jpeg_destination_mgr. > WebCore/platform/image-encoders/skia/JPGImageEncoder.cpp:107 > + const SkPMColor* pixel = reinterpret_cast<const uint32_t*>(m_pixels); > + while (pixels-- > 0) { > + SkColor unmultiplied = SkUnPreMultiply::PMColorToColor(*pixel++); Nit: If you declare |m_pixels| as "const SkPMColor*" (and add a "/ sizeof(SkPMColor)" to the increment in encode() above), you can write this loop as: for (const SkPMColor* pixel = m_pixels; pixel < (m_pixels + pixels); ++pixel) { SkColor unmultiplied = SkUnPreMultiply::PMColorToColor(*pixel); ... ...which is not only shorter, it seems (to me at least) easier to understand and harder to screw up.
David Levin
Comment 7 2010-11-23 13:46:26 PST
Comment on attachment 74360 [details] patch r- based on Peter's comments.
Stephen White
Comment 8 2010-11-25 07:25:17 PST
Comment on attachment 74360 [details] patch Thanks for taking this on! View in context: https://bugs.webkit.org/attachment.cgi?id=74360&action=review > WebCore/ChangeLog:16 > + alpha channel when extracting an "image/jpeg".toDataURL(). The correct answer > + needs more investigation. I don't know if it helps for this particular test, but some of the canvas/philip tests require a different unpremultiply that the one implemented in SkUnPreMultiply, in order to achieve roundtrip lossless conversion on a certain set of colours. See http://trac.webkit.org/changeset/71760, function mulDiv255Ceil() in ImageBufferSkia.cpp. The test was canvas/philip/tests/2d.imageData.put.unchanged.html. If this does fix the problem, it would be ideal if you could upstream this function to Skia. > WebCore/platform/image-encoders/skia/JPGImageEncoder.cpp:107 > + SkColor unmultiplied = SkUnPreMultiply::PMColorToColor(*pixel++); See above: check if mulDiv255Ceil() fixes the JPEG+alpha case. > WebCore/platform/image-encoders/skia/JPGImageEncoder.cpp:117 > + out->m_buffer.resize(8192); This magic value should be in a constant.
noel gordon
Comment 9 2010-11-26 01:37:54 PST
(In reply to comment #6) > (From update of attachment 74360 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=74360&action=review > > > WebCore/ChangeLog:17 > > + Adds a libjpeg-based image encoder for Skia bitmaps. Default encoding quality > > + is 92 to match Mozilla, also Safari, though the actual value used by Safari is > > + undocumented, and it appears to pre-blur images prior to compression. > > + > > + JPGBitmapEncoder::preMultipliedBGRAtoRGB restores the un-multiplied RGB colors > > + where there is non-zero alpha. Again, this matches Firefox and Safari, but no > > + browser conforms to the HTML5 canvas standard here, I believe, considering the > > + results of canvas/philip/tests/toDataURL.jpeg.alpha.html; the test ignores the > > + alpha channel when extracting an "image/jpeg".toDataURL(). The correct answer > > + needs more investigation. > > Nit: These comments are really helpful. For this reason, I'd like to see them in the source code itself. The first comment can be in the declaration of JPGImageEncoder, and the second in the declaration of JPGBitmapEncoder. (They don't need to be duplicated into the ChangeLog at that point.) > I'm glad you found the comments helpful, that's why I put them in the ChangeLog and I believe that's the correct place for them. I'm hesitant to add en-block comments like this in the code. I did add a FIXME code comment. > > WebCore/platform/graphics/skia/ImageBufferSkia.cpp:322 > > + Vector<unsigned char> encodedImage; > > + > > Nit: Extra blank line here Done. > > WebCore/platform/image-encoders/skia/JPGImageEncoder.cpp:45 > > +class JPGBitmapEncoder : public jpeg_destination_mgr { > > Nit: WebCore is pretty consistent about using "JPEG" rather than "JPG" in class and file names. Please follow suit. Yeap, changed everywhere: class names, filenames, ... > > WebCore/platform/image-encoders/skia/JPGImageEncoder.cpp:47 > > +public: > > + JPGBitmapEncoder(const SkBitmap& input, Vector<unsigned char>* output) > > Nit: Defining your methods inline in your declaration tells the compiler you want them inlined. Since that's not actually what you mean here, you should declare the class above its definition. This also makes it easier for a reader to quickly see the structure of the class by just glancing at its declaration, and for you to mark with a single comment the block of functions responsible for implementing jpeg_destination_mgr. > that block of functions got in the way of your reading, so I moved them out of the way and into encode(). The compiler _may_ inline the code, it's not something I'd rely on or need here. I used a class to try get the reader to encode() (the only public function of a class that is private to this file) in a few lines of code, and to deal with the issues setjmp() creates for C++ object destruction. With a class, member destruction is assured regardless of libjpeg errors during encode(), whereas in our current PNGEncoder (not class based), a C++ object leaks memory on encoding failures. I'm trying to avoid that, but maybe I should just ditch the class approach and write it all as static functions, I'm fresh out of ideas for keeping the code short. > > WebCore/platform/image-encoders/skia/JPGImageEncoder.cpp:107 > > + const SkPMColor* pixel = reinterpret_cast<const uint32_t*>(m_pixels); > > + while (pixels-- > 0) { > > + SkColor unmultiplied = SkUnPreMultiply::PMColorToColor(*pixel++); > > Nit: If you declare |m_pixels| as "const SkPMColor*" (and add a "/ sizeof(SkPMColor)" to the increment in encode() above), you can write this loop as: > > for (const SkPMColor* pixel = m_pixels; pixel < (m_pixels + pixels); ++pixel) { > SkColor unmultiplied = SkUnPreMultiply::PMColorToColor(*pixel); > ... > > ...which is not only shorter, it seems (to me at least) easier to understand and harder to screw up. Shorter true, a little harder to read for me :). Appreciate your idea about defining |m_pixels| as const SkPMColor* -- I've used it and it simplified the loop, made me deal with m_pixels in one place (rather than two), and allowed me to ditch 8 more lines of code from encode(), much shorter. Uploaded a WIP patch.
noel gordon
Comment 10 2010-11-26 01:40:00 PST
Created attachment 74908 [details] patch wip
noel gordon
Comment 11 2010-11-29 01:30:12 PST
Created attachment 75000 [details] patch wip static functions rewritten using static functions
Jian Li
Comment 12 2010-11-29 20:33:50 PST
*** Bug 38495 has been marked as a duplicate of this bug. ***
noel gordon
Comment 13 2010-12-01 22:41:30 PST
asked for clarification on jpeg image toDataURL, bug filed http://www.w3.org/Bugs/Public/show_bug.cgi?id=11431
noel gordon
Comment 14 2010-12-01 22:45:49 PST
No observable difference in performance for either implementation, so choosing the short code, adding r?
noel gordon
Comment 15 2010-12-01 23:34:17 PST
(In reply to comment #8) > (From update of attachment 74360 [details]) > Thanks for taking this on! more too it than I expected :) > View in context: https://bugs.webkit.org/attachment.cgi?id=74360&action=review > > > WebCore/ChangeLog:16 > > + alpha channel when extracting an "image/jpeg".toDataURL(). The correct answer > > + needs more investigation. > > I don't know if it helps for this particular test, but some of the canvas/philip tests require a different unpremultiply that the one implemented in SkUnPreMultiply, in order to achieve roundtrip lossless conversion on a certain set of colours. See http://trac.webkit.org/changeset/71760, function mulDiv255Ceil() in ImageBufferSkia.cpp. The test was canvas/philip/tests/2d.imageData.put.unchanged.html. If this does fix the problem, it would be ideal if you could upstream this function to Skia. So mulDiv255Ceil() appears to premultiply, yes? In my case, I'm doing the opposite -- I must unpremultiply much like getImageData(). I note that getImageData() uses color-component * 255 / alpha to unpremultiply RGB values. I believe SkUnPreMultiply does exactly that using a lookup table to avoid integer division. Please check, because I wondered why getImageData() uses an integer divide. The test I mentioned, canvas/philip/tests/toDataURL.jpeg.alpha.html, suggests we should not unpremultiply at all, and instead ignore the alpha channel and JPEG encode the premultiplied RGB. Firefox and Safari unpremultiply first and JPEG encode the unpremultiplied RGB. Don't know who's right here, I asked ian to clarify the spec - http://www.w3.org/Bugs/Public/show_bug.cgi?id=11431 > > WebCore/platform/image-encoders/skia/JPGImageEncoder.cpp:107 > > + SkColor unmultiplied = SkUnPreMultiply::PMColorToColor(*pixel++); > > See above: check if mulDiv255Ceil() fixes the JPEG+alpha case. see above. > > WebCore/platform/image-encoders/skia/JPGImageEncoder.cpp:117 > > + out->m_buffer.resize(8192); > > This magic value should be in a constant. done.
Stephen White
Comment 16 2010-12-02 10:56:12 PST
Looks good. (Comments below are more conversational in nature :). (In reply to comment #15) > (In reply to comment #8) > > > I don't know if it helps for this particular test, but some of the canvas/philip tests require a different unpremultiply that the one implemented in SkUnPreMultiply, in order to achieve roundtrip lossless conversion on a certain set of colours. See http://trac.webkit.org/changeset/71760, function mulDiv255Ceil() in ImageBufferSkia.cpp. The test was canvas/philip/tests/2d.imageData.put.unchanged.html. If this does fix the problem, it would be > ideal if you could upstream this function to Skia. > > So mulDiv255Ceil() appears to premultiply, yes? In my case, I'm doing the opposite -- I must unpremultiply much like getImageData(). Oh, you're right. In that case, forget everything I said. :) > I note that > getImageData() uses color-component * 255 / alpha to unpremultiply RGB values. I believe SkUnPreMultiply does exactly that using a lookup table to > avoid integer division. Please check, because I wondered why getImageData() uses an integer divide. I think I'm to blame, actually: see r50408, where I ripped out SkPMColorToColor() and replaced it with integer divides (as the CG path does). If I recall correctly, SkPMColorToColor() rounds, while the CG path truncates, and the latter behaviour was required to get us to pass the layout test. (It also looks like the LUT in SkPMColorToColor is only accurate to with one lsb, but I could be wrong about that). It should be possible to implement a rounding LUT that behaves the same as the CG path, but it would probably need to be bigger to be fully accurate. > The test I mentioned, canvas/philip/tests/toDataURL.jpeg.alpha.html, suggests we should not unpremultiply at all, and instead ignore the alpha > channel and JPEG encode the premultiplied RGB. Firefox and Safari unpremultiply first and JPEG encode the unpremultiplied RGB. Don't know who's > right here, I asked ian to clarify the spec - http://www.w3.org/Bugs/Public/show_bug.cgi?id=11431 > > > > > WebCore/platform/image-encoders/skia/JPGImageEncoder.cpp:107 > > > + SkColor unmultiplied = SkUnPreMultiply::PMColorToColor(*pixel++); > > > > See above: check if mulDiv255Ceil() fixes the JPEG+alpha case. > > see above. > > > > WebCore/platform/image-encoders/skia/JPGImageEncoder.cpp:117 > > > + out->m_buffer.resize(8192); > > > > This magic value should be in a constant. > > done.
WebKit Commit Bot
Comment 17 2010-12-02 11:43:03 PST
Comment on attachment 75000 [details] patch wip static functions Clearing flags on attachment: 75000 Committed r73173: <http://trac.webkit.org/changeset/73173>
WebKit Commit Bot
Comment 18 2010-12-02 11:43:11 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.