Bug 23687 – ImageBuffer::toDataURL not implemented for Skia

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Scott Violet]

At a minimum Skia should support PNG conversion.

[Scott Violet]

Created attachment 27250 [details]
Fix for 23687

[Eric Seidel (no email)]

Comment on attachment 27250 [details]
Fix for 23687

The chromium license headers make little sense in the WebKit tree.  Also, the code from Mozilla is in Google style, not WebKIt style (at least in function naming), and it seems it's so litlte code we should just fix the style.

[Scott Violet]

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

[Scott Violet]

Created attachment 27254 [details]
Fix for 23687

Converts names to WebKit style and fixes license.

[Darin Fisher (:fishd, Google)]

Comment on attachment 27254 [details]
Fix for 23687

some drive-by comments...


>Index: WebCore/platform/graphics/skia/ImageBufferSkia.cpp

>+    WTF::Vector<unsigned char> pngEncodedData;

It is more common to not specify the WTF:: namespace.  There is a using
WTF::Vector at the bottom of Vector.h.


>Index: WebCore/platform/image-encoders/skia/PNGImageEncoder.cpp
...
>+/*
>+* Copyright (c) 2006-2009, Google Inc. All rights reserved.
>+* 

nit: comment block needs to be indented by one space


needs #include "config.h" at the top.


>+
>+#include "PNGImageEncoder.h"
>+#include "Vector.h"


>+namespace WebCore {
>+
>+namespace {

nit: the convention in webcore seems to be to use the 'static' keyword
instead of anonymous namespaces.


>+
>+// Converts BGRA->RGBA and RGBA->BGRA.
>+void convertBetweenBGRAandRGBA(const unsigned char* input, int pixel_width,

so this would stay in the WebCore namespace but be given a 'static' keyword

>+namespace {
>+
>+// Passed around as the io_ptr in the png structs so our callbacks know where
>+// to write data.
>+struct PngEncoderState {

and then for structs like this they just put them in the WebCore namespace.
since you have the Png prefix, you shouldn't have to worry about name conflicts.


>Index: WebCore/platform/image-encoders/skia/PNGImageEncoder.h
...
>+/*
>+* Copyright (c) 2006-2009, Google Inc. All rights reserved.
>+* 

nit: needs another space of indent

>+};  // namespace WebCore

nit: extraneous ';' at close of namespace

[Scott Violet]

Created attachment 27256 [details]
Fix for 23687

Incorporates changes from Darin.

[Eric Seidel (no email)]

Comment on attachment 27254 [details]
Fix for 23687

pixel_width should be pixelWidth

Since we seem to use PNGFoo we should use: PngEncoderState PNGEncoderState

int w, int h should be int width int height, or even better an IntSize.  New functions generally use the IntSize/IntPoint types, we've been slowly converting the old ones over.  If it doesn't make sense for this function to use IntSize, that's fine, but I figured it should at least be mentioned. (Skia doesn't spit out IntSize objects, so it's probably easiest to keep using separate ints for Skia.)

Likewise bytes_per_row bytesPerRow

We should give these good names:
99     png_struct** m_ps;
 100     png_info** m_pi;

m_pngStruct;
m_pngInfo; would be more clear, even though the original libpng type names are pretty useless to begin with.

WebKit uses 0 instead of NULL:
 127     void (*converter)(const unsigned char* in, int w, unsigned char* out) = NULL;

Seems silly to have a converter local variable, instead of just using convertBetweenBGRAandRGBA in the one place it's actually needed.

If ints are not required by libpng (which they may be), you might consider using size_t or unsigned for these width/height values (unless you need a special -1 value?)

this is in general pretty gnarly code.  But that seems to be libpng's fault rather than yours.

I'd like to see a final version of this patch, but otherwise this looks sane enough.

[Scott Violet]

Created attachment 27259 [details]
Fix for 23687

Addresses issues raised in last round. I went with IntSize.

[Eric Seidel (no email)]

Comment on attachment 27259 [details]
Fix for 23687

 46 static void convertBetweenBGRAandRGBA(const unsigned char* input, int pixelWidth,
Having read the rest of your code, I understand what pixelWidth means, but I think numberOfPixels would be clearer in the local context of that function.

Still has WTF::, which can be removed per darin's comment.

PngWriteStructDestroyer should be PNGWriteStructDestroyer to match PNGImageEncoder

likewise PngEncoderState should be PNGEncoderState

You could actually use an OwnArrayPtr here:
 162     unsigned char* row = new unsigned char[imageSize.width() * outputColorComponents];

    OwnArrayPtr<unsigned char> rowPixels(new unsigned char[width * height]);

I'm not sure it's super-useful here, but it does save you the delete[] at the end (which is more useful when there is lots of code in between the initialization and the delete).

Thanks for all your corrections.  I'm ready to land this for you after this last round of nits.

[Scott Violet]

Created attachment 27265 [details]
Fix for 23687

[Dimitri Glazkov (Google)]

Eric, if you r+, I'll land.

[Eric Seidel (no email)]

Comment on attachment 27265 [details]
Fix for 23687

Looks good.  Thanks.

[Darin Fisher (:fishd, Google)]

Looks like this patch was landed:
http://trac.webkit.org/changeset/40692