Bug 145457 – [EFL] Jpeg image export implementation for Canvas.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[KwangHyuk]

Webkit GTK supports jpeg image export, but Webkit EFL doesn't support it yet.

[KwangHyuk]

Created attachment 253960 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 253960 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=253960&action=review

> Source/WebCore/ChangeLog:20
> +        * platform/image-encoders/JPEGImageEncoder.cpp:

You need to explain what is modified.

> Source/WebCore/platform/MIMETypeRegistry.cpp:270
> +#elif PLATFORM(EFL)

Why can't we support image/tiff, image/bmp, image/ico ?

Besides I think GTK and EFL have used cairo backend. Thus it might be good if you change this change as below,

 #elif PLATFORM(GTK)
-    supportedImageMIMETypesForEncoding->add("image/png");
-    supportedImageMIMETypesForEncoding->add("image/jpeg");
     supportedImageMIMETypesForEncoding->add("image/tiff");
     supportedImageMIMETypesForEncoding->add("image/bmp");
     supportedImageMIMETypesForEncoding->add("image/ico");
 #elif USE(CAIRO)
     supportedImageMIMETypesForEncoding->add("image/png");
+    supportedImageMIMETypesForEncoding->add("image/jpeg");
+    supportedImageMIMETypesForEncoding->add("image/png");
 #endif

> Source/WebCore/platform/graphics/efl/ImageBufferEfl.cpp:4
> + *  This library is free software; you can redistribute it and/or

Oops. LGPL.... Please use BSD !

> Source/WebCore/platform/image-encoders/JPEGImageEncoder.cpp:126
> +        for (const unsigned char* rowEnd = pixel + offset + compressData.image_width * 4; pixel + offset < rowEnd;) {

I think this change can influence on other ports. So you need to explain why you modify this code.

[Gyuyoung Kim]

Comment on attachment 253960 [details]
Patch

r- because some trivial reasons.

[KwangHyuk]

(In reply to comment #2)
> Comment on attachment 253960 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253960&action=review
> 
> > Source/WebCore/ChangeLog:20
> > +        * platform/image-encoders/JPEGImageEncoder.cpp:
> 
> You need to explain what is modified.
> 
> > Source/WebCore/platform/MIMETypeRegistry.cpp:270
> > +#elif PLATFORM(EFL)
> 
> Why can't we support image/tiff, image/bmp, image/ico ?
> 
> Besides I think GTK and EFL have used cairo backend. Thus it might be good
> if you change this change as below,
> 
>  #elif PLATFORM(GTK)
> -    supportedImageMIMETypesForEncoding->add("image/png");
> -    supportedImageMIMETypesForEncoding->add("image/jpeg");
>      supportedImageMIMETypesForEncoding->add("image/tiff");
>      supportedImageMIMETypesForEncoding->add("image/bmp");
>      supportedImageMIMETypesForEncoding->add("image/ico");
>  #elif USE(CAIRO)
>      supportedImageMIMETypesForEncoding->add("image/png");
> +    supportedImageMIMETypesForEncoding->add("image/jpeg");
> +    supportedImageMIMETypesForEncoding->add("image/png");
>  #endif
> 

it's guarded by using #elif, therefore, it should be touched in order to re-use CAIRO definition.
This cairo definition might be for window port ?

> > Source/WebCore/platform/graphics/efl/ImageBufferEfl.cpp:4
> > + *  This library is free software; you can redistribute it and/or
> 
> Oops. LGPL.... Please use BSD !
> 
> > Source/WebCore/platform/image-encoders/JPEGImageEncoder.cpp:126
> > +        for (const unsigned char* rowEnd = pixel + offset + compressData.image_width * 4; pixel + offset < rowEnd;) {
> 
> I think this change can influence on other ports. So you need to explain why
> you modify this code.

ImageEncoder was not used by other port yet. :)


(In reply to comment #2)
> Comment on attachment 253960 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253960&action=review
> 
> > Source/WebCore/ChangeLog:20
> > +        * platform/image-encoders/JPEGImageEncoder.cpp:
> 
> You need to explain what is modified.
> 
> > Source/WebCore/platform/MIMETypeRegistry.cpp:270
> > +#elif PLATFORM(EFL)
> 
> Why can't we support image/tiff, image/bmp, image/ico ?
> 

Because this patch is just for adding jpeg export feature on Webkit EFL.
So, It's not necessary.
However, this can be considered as other new bug.

> Besides I think GTK and EFL have used cairo backend. Thus it might be good
> if you change this change as below,
> 
>  #elif PLATFORM(GTK)
> -    supportedImageMIMETypesForEncoding->add("image/png");
> -    supportedImageMIMETypesForEncoding->add("image/jpeg");
>      supportedImageMIMETypesForEncoding->add("image/tiff");
>      supportedImageMIMETypesForEncoding->add("image/bmp");
>      supportedImageMIMETypesForEncoding->add("image/ico");
>  #elif USE(CAIRO)
>      supportedImageMIMETypesForEncoding->add("image/png");
> +    supportedImageMIMETypesForEncoding->add("image/jpeg");
> +    supportedImageMIMETypesForEncoding->add("image/png");
>  #endif
> 
> > Source/WebCore/platform/graphics/efl/ImageBufferEfl.cpp:4
> > + *  This library is free software; you can redistribute it and/or
> 
> Oops. LGPL.... Please use BSD !
> 

Thx, let me think about it.

> > Source/WebCore/platform/image-encoders/JPEGImageEncoder.cpp:126
> > +        for (const unsigned char* rowEnd = pixel + offset + compressData.image_width * 4; pixel + offset < rowEnd;) {
> 
> I think this change can influence on other ports. So you need to explain why
> you modify this code.


Unfortunately, it just generates clobbered error caused by setjmp and longjmp.
So, I just modified some code related with pixel variable.
However, the logic is same.

[KwangHyuk]

Created attachment 254579 [details]
Patch

[KwangHyuk]

- License description was updated to use BSD.
- The reason why JPEGImageEncoder.cpp was modified was added to file.

[Gyuyoung Kim]

Comment on attachment 254579 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254579&action=review

LGTM except for a trivial comment.

> Source/WebCore/ChangeLog:10
> +        No new tests, LayoutTests/fast/canvas/toDataURL-supportedTypes.html can be reused.

Unnecessary "LayoutTests/" in test path.

> Source/WebCore/platform/graphics/efl/ImageBufferEfl.cpp:75
> +    return "data:" + mimeType + ";base64," + base64Data;

Isn't StringBuilder more efficient ?

https://trac.webkit.org/wiki/EfficientStrings

[KwangHyuk]

(In reply to comment #7)
> Comment on attachment 254579 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=254579&action=review
> 
> LGTM except for a trivial comment.
> 
> > Source/WebCore/ChangeLog:10
> > +        No new tests, LayoutTests/fast/canvas/toDataURL-supportedTypes.html can be reused.
> 
> Unnecessary "LayoutTests/" in test path.
> 

Ok, thanks.

> > Source/WebCore/platform/graphics/efl/ImageBufferEfl.cpp:75
> > +    return "data:" + mimeType + ";base64," + base64Data;
> 
> Isn't StringBuilder more efficient ?
> 
> https://trac.webkit.org/wiki/EfficientStrings

But, this is the general implementation for every ports like ImageBufferGtk, ImageBufferCairo as well as ImageBufferCG.
Therefore, may be, keeping of this implementation looks fine.

[KwangHyuk]

Created attachment 254651 [details]
Patch

[KwangHyuk]

Created attachment 254652 [details]
Patch

[KwangHyuk]

GyuYoung, Could you review one more time ?

review flag just disappeared. :(

[WebKit Commit Bot]

Comment on attachment 254652 [details]
Patch

Clearing flags on attachment: 254652

Committed r185417: <http://trac.webkit.org/changeset/185417>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.