Bug 23687 - ImageBuffer::toDataURL not implemented for Skia
: ImageBuffer::toDataURL not implemented for Skia
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-02-02 11:29 PST by
Modified: 2009-02-10 00:23 PST (History)


Attachments
Fix for 23687 (10.65 KB, patch)
2009-02-02 11:34 PST, Scott Violet
eric: review-
Review Patch | Details | Formatted Diff | Diff
Fix for 23687 (11.47 KB, patch)
2009-02-02 12:35 PST, Scott Violet
no flags Review Patch | Details | Formatted Diff | Diff
Fix for 23687 (11.48 KB, text/plain)
2009-02-02 12:59 PST, Scott Violet
no flags Details
Fix for 23687 (11.51 KB, patch)
2009-02-02 14:04 PST, Scott Violet
eric: review-
Review Patch | Details | Formatted Diff | Diff
Fix for 23687 (11.52 KB, patch)
2009-02-02 16:25 PST, Scott Violet
eric: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-02-02 11:29:21 PST
At a minimum Skia should support PNG conversion.
------- Comment #1 From 2009-02-02 11:34:16 PST -------
Created an attachment (id=27250) [details]
Fix for 23687
------- Comment #2 From 2009-02-02 11:50:07 PST -------
(From update of attachment 27250 [details])
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.
------- Comment #3 From 2009-02-02 12:33:23 PST -------
*** Bug 23692 has been marked as a duplicate of this bug. ***
------- Comment #4 From 2009-02-02 12:35:41 PST -------
Created an attachment (id=27254) [details]
Fix for 23687

Converts names to WebKit style and fixes license.
------- Comment #5 From 2009-02-02 12:41:53 PST -------
(From update of attachment 27254 [details])
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
------- Comment #6 From 2009-02-02 12:59:13 PST -------
Created an attachment (id=27256) [details]
Fix for 23687

Incorporates changes from Darin.
------- Comment #7 From 2009-02-02 13:03:22 PST -------
(From update of attachment 27254 [details])
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.
------- Comment #8 From 2009-02-02 14:04:13 PST -------
Created an attachment (id=27259) [details]
Fix for 23687

Addresses issues raised in last round. I went with IntSize.
------- Comment #9 From 2009-02-02 15:30:10 PST -------
(From update of attachment 27259 [details])
 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.
------- Comment #10 From 2009-02-02 16:25:47 PST -------
Created an attachment (id=27265) [details]
Fix for 23687
------- Comment #11 From 2009-02-05 09:12:54 PST -------
Eric, if you r+, I'll land.
------- Comment #12 From 2009-02-05 13:05:32 PST -------
(From update of attachment 27265 [details])
Looks good.  Thanks.
------- Comment #13 From 2009-02-10 00:23:04 PST -------
Looks like this patch was landed:
http://trac.webkit.org/changeset/40692