Bug 37117 – Add platform-independent JPEG/PNG image encoders

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

[Yong Li]

1. Add platform-independent JPEG/PNG image encoders

2. Implement toDataURL for ImageBufferOpenVG with platform-independent code

[Yong Li]

Created attachment 52642 [details]
the patch

[WebKit Review Bot]

Attachment 52642 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/image-encoders/PNGImageEncoder.h:21:  #ifndef header guard has wrong style, please use: PNGImageEncoder_h  [build/header_guard] [5]
WebCore/platform/image-encoders/PNGImageEncoder.h:27:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/platform/image-encoders/PNGImageEncoder.cpp:50:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebCore/platform/image-encoders/PNGImageEncoder.cpp:129:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/image-encoders/JPEGImageEncoder.h:22:  #ifndef header guard has wrong style, please use: JPEGImageEncoder_h  [build/header_guard] [5]
WebCore/platform/image-encoders/JPEGImageEncoder.h:28:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/platform/image-encoders/JPEGImageEncoder.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/image-encoders/JPEGImageEncoder.cpp:34:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebCore/platform/image-encoders/JPEGImageEncoder.cpp:47:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebCore/platform/image-encoders/JPEGImageEncoder.cpp:83:  Declaration has space between type name and * in JPEGCompressErrorMgr *err  [whitespace/declaration] [3]
Total errors found: 10 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yong Li]

Created attachment 52646 [details]
fix style errors

[WebKit Review Bot]

Attachment 52646 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/image-encoders/JPEGImageEncoder.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yong Li]

(In reply to comment #4)
> Attachment 52646 [details] did not pass style-queue:
> 
> Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
> WebCore/platform/image-encoders/JPEGImageEncoder.cpp:28:  Alphabetical sorting
> problem.  [build/include_order] [4]
> Total errors found: 1 in 5 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

this style error is a must. we cannot avoid this

[Adam Barth]

Comment on attachment 52646 [details]
fix style errors

This code is nutty, but I think the image decoder libraries force you into some of this nuttiness.  Some nits below.  Please get pkasting to give your revised patch an informal review.

WebCore/platform/image-encoders/JPEGImageEncoder.cpp:39
 +          jpeg_destination_mgr* base = this;
Woah, crazy.  Is this what we're supposed to do here?

WebCore/platform/image-encoders/JPEGImageEncoder.cpp:50
 +          // Zero memory
I like the static cast trick you did above.  Can we do that here?  In principle, we can get in trouble with this code if we add virtual functions to this class.

WebCore/platform/image-encoders/JPEGImageEncoder.cpp:61
 +      dest->next_output_byte  = reinterpret_cast<JOCTET*>(dest->m_buffer.data());
We don't usually align the = sign.  We just have once space.

WebCore/platform/image-encoders/JPEGImageEncoder.cpp:59
 +      JPEGDestinationManager* dest = (JPEGDestinationManager*)compressData->dest;
Please use a C++ style cast like you do below.

WebCore/platform/image-encoders/JPEGImageEncoder.cpp:83
 +      longjmp(err->m_setjmpBuffer, -1);
Crazy!

WebCore/platform/image-encoders/JPEGImageEncoder.cpp:88
 +      struct jpeg_compress_struct compressData= { 0 };
Missing space before =

WebCore/platform/image-encoders/JPEGImageEncoder.cpp:109
 +      Vector<JSAMPLE, 600 * 3> rowBuffer;
No space around *

WebCore/platform/image-encoders/JPEGImageEncoder.cpp:108
 +      // rowBuffer must be defined here so that its destructor is always called even when "setjmp" catches an error.
setjmp/longjmp are insane, especially when mixed with C++ constructs.

WebCore/platform/image-encoders/JPEGImageEncoder.h:21
 +  
Extra blank line here.

WebCore/platform/image-encoders/PNGImageEncoder.cpp:51
 +      Vector<char>* m_out;
Why is this called m_out but the corresponding variable for JPEG is called m_dump?

WebCore/platform/image-encoders/PNGImageEncoder.cpp:62
 +      memcpy(&(*state->m_out)[oldSize], data, size);
&(*state->m_out)[oldSize] is too hard to understand.  Please use temporary variables to explain what this insanity does.

WebCore/platform/image-encoders/PNGImageEncoder.cpp:87
 +      png_struct* pngPtr = png_create_write_struct(PNG_LIBPNG_VER_STRING,
We usually put these sorts of things on one line.

[Yong Li]

(In reply to comment #6)
> (From update of attachment 52646 [details])
> This code is nutty, but I think the image decoder libraries force you into some of this nuttiness.  Some nits below.  Please get pkasting to give your revised patch an informal review.
> 
> WebCore/platform/image-encoders/JPEGImageEncoder.cpp:39
>  +          jpeg_destination_mgr* base = this;
> Woah, crazy.  Is this what we're supposed to do here?
> 
> WebCore/platform/image-encoders/JPEGImageEncoder.cpp:50
>  +          // Zero memory
> I like the static cast trick you did above.  Can we do that here?  In principle, we can get in trouble with this code if we add virtual functions to this class.

This doesn't need a static cast because it casts from child class to the parent class. Implicit cast is safer than static cast in most cases. No matter we add virtual functions to the class or add another parent class, "base" is always pointing to a good jpeg_destination_mgr object.
 
> WebCore/platform/image-encoders/JPEGImageEncoder.cpp:83
>  +      longjmp(err->m_setjmpBuffer, -1);
> Crazy!

:) Yes it is. I don't like it either. But it's the way to handle libjpeg error. JPEG decoder also uses this. 

> WebCore/platform/image-encoders/JPEGImageEncoder.cpp:109
>  +      Vector<JSAMPLE, 600 * 3> rowBuffer;
> No space around *

Are you sure? I don't think check-webkit-style will accept 600*3 with no spaces.

Thanks a lot for reviewing! I'll update the patch according to your suggestions.

[Yong Li]

Created attachment 59557 [details]
Updated again

[WebKit Review Bot]

Attachment 59557 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/image-encoders/JPEGImageEncoder.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yong Li]

(In reply to comment #9)
> Attachment 59557 [details] did not pass style-queue:
> Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
> WebCore/platform/image-encoders/JPEGImageEncoder.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
> Total errors found: 1 in 5 files
> If any of these errors are false positives, please file a bug against check-webkit-style.

we cannot avoid this

[George Staikos]

Comment on attachment 59557 [details]
Updated again

Will need a retest since it's so old.

[WebKit Commit Bot]

Comment on attachment 59557 [details]
Updated again

Clearing flags on attachment: 59557

Committed r67606: <http://trac.webkit.org/changeset/67606>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.