Bug 95299

*** Bug 100852 has been marked as a duplicate of this bug. ***

*** Bug 101035 has been marked as a duplicate of this bug. ***

fast/images/exif-orientation-image-document.html was introduced in r132877, failing as well. Already skipped on EFL. http://trac.webkit.org/changeset/132877

The decoding side seems alright. The issue is in the graphics backend. The backend needs to rotate/flip the graphics context and draw the BitmapImage according to ImageFrame orientation. Will handle that for cairo in bug 101207.

Reassigning some EFL bugs that are likely shared with GTK/WPE to the GTK component.

Created attachment 395680 [details] WIP patch updating Cairo::drawSurface drawSurface was ignoring the destRect transformations from drawNativeImage, applying some padding and wrong scaling, presuming src and dest had the same orientation. This first version fixes a bunch of the exif tests, with some failures left for border images and image-orientation tests. Still need to check for side effects in other image tests.

Comment on attachment 395680 [details] WIP patch updating Cairo::drawSurface View in context: https://bugs.webkit.org/attachment.cgi?id=395680&action=review > Source/WebCore/platform/graphics/cairo/CairoOperations.cpp:890 > +void drawSurface(PlatformContextCairo& platformContext, cairo_surface_t* surface, const FloatRect& destRect, const FloatRect& originalSrcRect, InterpolationQuality imageInterpolationQuality, float globalAlpha, const ShadowState& shadowState, bool didUseWidthAsHeight) In general we tend to frown upon boolean flags; instead, it's better to use an “enum”, which is more descriptive: // Somewhere in the header. enum class Sizing { Normal, WidthAsHeight }; void drawSurface(/* ... */, Sizing sizing = Sizing::Normal); (Also you may want to have a default value for the parameter, so you don't need to add the usual one by hand on some callsites. Also feel free to suggest a better name for the “enum”, I just wrote down the first thing that came to mind.) > Source/WebCore/platform/graphics/cairo/CairoOperations.cpp:952 > + std::fabs(srcRect.height() / destRect.width()); The result of the calculations here are not being stored anywhere, which means the computed values are not used at all. Same below in the “else” branch. > Source/WebKit/Shared/cairo/ShareableBitmapCairo.cpp:79 > + Cairo::drawSurface(*context.platformContext(), surface.get(), destRect, srcRect, state.imageInterpolationQuality, state.alpha, Cairo::ShadowState(state), false); If you add a default value to the function prototype as suggested, you don't need to modify this callsite. > Source/WebKit/WebProcess/WebCoreSupport/gtk/WebDragClientGtk.cpp:58 > + Cairo::drawSurface(*graphicsContext->platformContext(), surface, IntRect(IntPoint(), imageSize), IntRect(IntPoint(), imageSize), state.imageInterpolationQuality, state.alpha, Cairo::ShadowState(state), false); Ditto.

*** Bug 215164 has been marked as a duplicate of this bug. ***

Created attachment 406152 [details] Patch addressing comments.

(In reply to Adrian Perez from comment #7) > Comment on attachment 395680 [details] > WIP patch updating Cairo::drawSurface > > View in context: > https://bugs.webkit.org/attachment.cgi?id=395680&action=review Thanks for the review, Adrian. > > > Source/WebCore/platform/graphics/cairo/CairoOperations.cpp:890 > > +void drawSurface(PlatformContextCairo& platformContext, cairo_surface_t* surface, const FloatRect& destRect, const FloatRect& originalSrcRect, InterpolationQuality imageInterpolationQuality, float globalAlpha, const ShadowState& shadowState, bool didUseWidthAsHeight) > > In general we tend to frown upon boolean flags; instead, it's better to use > an “enum”, which is more descriptive: > > // Somewhere in the header. > enum class Sizing { Normal, WidthAsHeight }; > > void drawSurface(/* ... */, Sizing sizing = Sizing::Normal); > > (Also you may want to have a default value for the parameter, so you don't > need > to add the usual one by hand on some callsites. Also feel free to suggest a > better > name for the “enum”, I just wrote down the first thing that came to mind.) Indeed bools for flags are not that great. > > > Source/WebCore/platform/graphics/cairo/CairoOperations.cpp:952 > > + std::fabs(srcRect.height() / destRect.width()); > > The result of the calculations here are not being stored anywhere, > which means the computed values are not used at all. Same below in > the “else” branch. > Oops. Must have missed adding it when git-add'ing the patch. The updated version is still working for all tests in fast/images except these two below, which seem to be arriving at drawSurface with some different orientation from FromImage. fast/images/exif-orientation-border-image.html fast/images/exif-orientation-background-image-repeat.html

Comment on attachment 406152 [details] Patch addressing comments. This looks fine to me!

Patch LGTM as well 👍️ Lauro, feel free to go ahead and land the patch, or set the commit queue flag :)

Comment on attachment 406152 [details] Patch addressing comments. I'm going to do a drive-by cq+ to see if it lands without conflicts.

Committed r268176: <https://trac.webkit.org/changeset/268176> All reviewed patches have been landed. Closing bug and clearing flags on attachment 406152 [details].