Bug 49365

Created attachment 73965 [details] patch

Attachment 73965 [details] did not build on chromium: Build output: http://queues.webkit.org/results/5963082

Created attachment 74069 [details] patch costs a style nit, but builds on linux

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.

Created attachment 74360 [details] patch chromium trys pass fast/canvas/canvas-toDataURL-case-insensitive-mimetype.html now, fix that and update ChangeLogs.

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.

Comment on attachment 74360 [details] patch r- based on Peter's comments.

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.

(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.

Created attachment 74908 [details] patch wip

Created attachment 75000 [details] patch wip static functions rewritten using static functions

*** Bug 38495 has been marked as a duplicate of this bug. ***

asked for clarification on jpeg image toDataURL, bug filed http://www.w3.org/Bugs/Public/show_bug.cgi?id=11431

No observable difference in performance for either implementation, so choosing the short code, adding r?

(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.

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.

Comment on attachment 75000 [details] patch wip static functions Clearing flags on attachment: 75000 Committed r73173: <http://trac.webkit.org/changeset/73173>

All reviewed patches have been landed. Closing bug.