Bug 67402 – [chromium] canvas.toDataURL("image/jpeg"): use libjpeg-turbo data swizzle

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[noel gordon]

Removes the need for temporary row data.

[noel gordon]

Created attachment 105960 [details]
Patch

[WebKit Review Bot]

Comment on attachment 105960 [details]
Patch

Attachment 105960 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9572999

[noel gordon]

Before I add Stephen and Kenneth to the review, Tony a question: does Chrome Linux still use the system jpeg?  If the answer is yes, then no need to continue here.

[noel gordon]

Created attachment 105961 [details]
Patch fix Linux build

[noel gordon]

I know that cr-linux has libjpeg-turbo.  My question is about other ports of chrome linux, Fedora and such, and if I need worry about them.

[Tony Chang]

(In reply to comment #5)
> I know that cr-linux has libjpeg-turbo.  My question is about other ports of chrome linux, Fedora and such, and if I need worry about them.

It looks like the Fedora rpm still uses system libjpeg, but they should probably try to switch to the system libjpeg-turbo.  I think this patch is fine and we should maybe (in the chromium repository) remove the DEPS entry for libjpeg and the gyp variable to use libjpeg.

I don't actually know this code; cc'ing people who do.

[Peter Kasting]

Comment on attachment 105961 [details]
Patch fix Linux build

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

> Source/WebCore/platform/image-encoders/skia/JPEGImageEncoder.cpp:44
> +#error "Failed: libjpeg-turbo bindings not found."

I'm not convinced we can reasonably enforce this yet.  We had to patch problems in libjpeg-turbo before it was safe to use and I don't know that Fedora or other consumers have a system libjpeg-turbo that contains those modifications.

I wonder if instead we should #ifdef callers to pass JCS_RGB for libjpeg, and still call the old conversion functions in that case.

> Source/WebCore/platform/image-encoders/skia/JPEGImageEncoder.cpp:-141
> -            preMultipliedBGRAtoRGB(pixels, cinfo.image_width, row.data());

I confess that I don't really understand the premultiplication stuff (I didn't add it), but it looks to me as if you're changing the behavior here by removing the "unpremultiply" step in this case.  Was the old behavior wrong?  Can we write tests that demonstrate the difference?

> Source/WebCore/platform/image-encoders/skia/JPEGImageEncoder.cpp:120
> +        jpeg_write_scanlines(&cinfo, &pixels, 1), pixels += pixelRowStride;

Please use two separate lines, and {} on the loop.

> Source/WebCore/platform/image-encoders/skia/JPEGImageEncoder.cpp:-153
> -bool JPEGImageEncoder::encode(const SkBitmap& bitmap, int quality, Vector<unsigned char>* output)

Don't rename |bitmap| here or |imageData| below.

> Source/WebCore/platform/image-encoders/skia/JPEGImageEncoder.cpp:135
> +    unsigned char* pixels = static_cast<unsigned char *>(image.getPixels());
> +    const J_COLOR_SPACE pixelFormat = JCS_EXT_BGRX;

Just collapse these into the statement below.

> Source/WebCore/platform/image-encoders/skia/JPEGImageEncoder.cpp:143
> +    unsigned char* pixels = image.data()->data()->data();
> +    const J_COLOR_SPACE pixelFormat = JCS_EXT_RGBX;

Just collapse these into the statement below.

> Source/WebCore/platform/image-encoders/skia/JPEGImageEncoder.cpp:145
> +    return encodePixels(IntSize(image.width(), image.height()), pixels, pixelFormat, quality, output);

Why not use image.size() for the first arg like the old code did?

[Adam Barth]

Comment on attachment 105961 [details]
Patch fix Linux build

pls address pkasting's comments.

[noel gordon]

(In reply to comment #7)
> (From update of attachment 105961 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105961&action=review
> 
> > Source/WebCore/platform/image-encoders/skia/JPEGImageEncoder.cpp:44
> > +#error "Failed: libjpeg-turbo bindings not found."
> 
> I'm not convinced we can reasonably enforce this yet.  We had to patch problems in libjpeg-turbo before it was safe to use and I don't know that Fedora or other consumers have a system libjpeg-turbo that contains those modifications.
> 
> I wonder if instead we should #ifdef callers to pass JCS_RGB for libjpeg, and still call the old conversion functions in that case.

Yes that seems safest way forward for now.

 
> > Source/WebCore/platform/image-encoders/skia/JPEGImageEncoder.cpp:-141
> > -            preMultipliedBGRAtoRGB(pixels, cinfo.image_width, row.data());
> 
> I confess that I don't really understand the premultiplication stuff (I didn't add it), but it looks to me as if you're changing the behavior here by removing the "unpremultiply" step in this case.  Was the old behavior wrong?  Can we write tests that demonstrate the difference?
> 

No change in behavior here, the tests that must pass were mentioned in the change log.  The premultiplication requirements were documented in bug 51237 based on our spec feedback on http://www.w3.org/Bugs/Public/show_bug.cgi?id=11431

The rest was addressed on bug 67598.

[noel gordon]

Ahem, addressed on bug 67589.

[noel gordon]

Created attachment 106574 [details]
Patch

[WebKit Review Bot]

Comment on attachment 106574 [details]
Patch

Attachment 106574 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9609231

[John Bauman]

Comment on attachment 106574 [details]
Patch

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

Do these formats work while decoding as well? I noticed we spent about as much time doing the swizzle and write into the imageframe as we do actually decoding the image, so there's plenty of room for improvement there.

> Source/WebCore/platform/image-encoders/skia/JPEGImageEncoder.cpp:153
> +    cinfo.in_color_space = premultiplied ? JCS_EXT_BGRX : JCS_EXT_RGBX;

Hmm, I somehow didn't notice the source-over black when I wrote the original code, so it was wrong. I think we need to multiply by alpha if the source isn't premultiplied, or else the color will be wrong. (This could probably be tested by doing something like fast/canvas/webgl/premultiplyalpha-test.html, except with image/jpeg).

[noel gordon]

Created attachment 106577 [details]
Patch tame Linux EWS

[noel gordon]

(In reply to comment #13)
> (From update of attachment 106574 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106574&action=review
> 
> Do these formats work while decoding as well? I noticed we spent about as much time doing the swizzle and write into the imageframe as we do actually decoding the image, so there's plenty of room for improvement there.
> 

Yes, the libjpeg-turbo formats work for decoding too.  I believe hbono-san is working on a decoder patch.  Should result in a very nice speed-up as you noted.

[noel gordon]

(In reply to comment #13)

> > Source/WebCore/platform/image-encoders/skia/JPEGImageEncoder.cpp:153
> > +    cinfo.in_color_space = premultiplied ? JCS_EXT_BGRX : JCS_EXT_RGBX;
> 
> Hmm, I somehow didn't notice the source-over black when I wrote the original code, so it was wrong. I think we need to multiply by alpha if the source isn't premultiplied, or else the color will be wrong. (This could probably be tested by doing something like fast/canvas/webgl/premultiplyalpha-test.html, except with image/jpeg).

Yes, you and kbr@ were next on my list here :)  Are you referring to the code you added in bug 56238 or the code here?

[John Bauman]

(In reply to comment #16)
> (In reply to comment #13)
> 
> > > Source/WebCore/platform/image-encoders/skia/JPEGImageEncoder.cpp:153
> > > +    cinfo.in_color_space = premultiplied ? JCS_EXT_BGRX : JCS_EXT_RGBX;
> > 
> > Hmm, I somehow didn't notice the source-over black when I wrote the original code, so it was wrong. I think we need to multiply by alpha if the source isn't premultiplied, or else the color will be wrong. (This could probably be tested by doing something like fast/canvas/webgl/premultiplyalpha-test.html, except with image/jpeg).
> 
> Yes, you and kbr@ were next on my list here :)  Are you referring to the code you added in bug 56238 or the code here?

The code I added in bug 56238 was broken, and your patch keeps it broken (and makes it somewhat harder to fix).

[Kenneth Russell]

Comment on attachment 106577 [details]
Patch tame Linux EWS

Please work with jbauman to fix and test the apparent preexisting alpha premultiplication bug so we don't inadvertently duplicate existing buggy code. Thanks.

[noel gordon]

Worked with John in bug 68366, fixed bug 40147 in the process.

[noel gordon]

Created attachment 108979 [details]
Patch

[noel gordon]

Created attachment 108980 [details]
Patch

[Kenneth Russell]

Comment on attachment 108980 [details]
Patch

jbauman, would you consider doing the unofficial review of this patch?

[John Bauman]

(In reply to comment #22)
> (From update of attachment 108980 [details])
> jbauman, would you consider doing the unofficial review of this patch?

Sure, it looks good to me.

[Kenneth Russell]

Comment on attachment 108980 [details]
Patch

Sounds good. rs=me

[WebKit Review Bot]

Comment on attachment 108980 [details]
Patch

Clearing flags on attachment: 108980

Committed r96364: <http://trac.webkit.org/changeset/96364>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.