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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=49365





--- Comment #6 from Peter Kasting <pkasting at google.com>  2010-11-23 10:45:41 PST ---
(From update of attachment 74360)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.