Bug 49365 – [chromium] Add canvas.toDataURL("image/jpeg", quality) support

• Component: Canvas
• Status: RESOLVED FIXED

[noel gordon]

Chromium win/linux support "image/png" in .toDataURL(), but not "image/jpeg".

[noel gordon]

Created attachment 73965 [details]
patch

[WebKit Review Bot]

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

[noel gordon]

Created attachment 74069 [details]
patch

costs a style nit, but builds on linux

[WebKit Review Bot]

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]

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 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 on attachment 74360 [details]
patch

r- based on Peter's comments.

[Stephen White]

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]

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

Created attachment 74908 [details]
patch wip

[noel gordon]

Created attachment 75000 [details]
patch wip static functions

rewritten using static functions

[Jian Li]

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

[noel gordon]

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

[noel gordon]

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

[noel gordon]

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

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 on attachment 75000 [details]
patch wip static functions

Clearing flags on attachment: 75000

Committed r73173: <http://trac.webkit.org/changeset/73173>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.