[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 #9 from noel gordon <noel.gordon at gmail.com>  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.

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