Bug 67402

Created attachment 105960 [details] Patch

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

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.

Created attachment 105961 [details] Patch fix Linux build

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.

(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.

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?

Comment on attachment 105961 [details] Patch fix Linux build pls address pkasting's comments.

(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.

Ahem, addressed on bug 67589.

Created attachment 106574 [details] Patch

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

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).

Created attachment 106577 [details] Patch tame Linux EWS

(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.

(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?

(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).

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.

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

Created attachment 108979 [details] Patch

Created attachment 108980 [details] Patch

Comment on attachment 108980 [details] Patch jbauman, would you consider doing the unofficial review of this patch?

(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.

Comment on attachment 108980 [details] Patch Sounds good. rs=me

Comment on attachment 108980 [details] Patch Clearing flags on attachment: 108980 Committed r96364: <http://trac.webkit.org/changeset/96364>

All reviewed patches have been landed. Closing bug.