Bug 95299 – [GTK][WPE] Missing Exif Orientation support

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Chris Dumez]

EFL port does not support EXIF orientation. This is covered by:
fast/images/exif-orientation.html
fast/images/exif-orientation-css.html

[Dominik Röttsches (drott)]

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

[Csaba Osztrogonác]

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

[Zan Dobersek]

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

[Dominik Röttsches (drott)]

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.

[Michael Catanzaro]

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

[Lauro Moura]

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.

[Adrian Perez]

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.

[Lauro Moura]

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

[Lauro Moura]

Created attachment 406152 [details]
Patch addressing comments.

[Lauro Moura]

(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

[Carlos Alberto Lopez Perez]

Comment on attachment 406152 [details]
Patch addressing comments.

This looks fine to me!

[Adrian Perez]

Patch LGTM as well 👍️

Lauro, feel free to go ahead and land the patch, or set the commit
queue flag :)

[Michael Catanzaro]

Comment on attachment 406152 [details]
Patch addressing comments.

I'm going to do a drive-by cq+ to see if it lands without conflicts.

[EWS]

Committed r268176: <https://trac.webkit.org/changeset/268176>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406152 [details].