Bug 95299

Summary: [GTK][WPE] Missing Exif Orientation support
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKitGTKAssignee: Lauro Moura <lmoura>
Status: RESOLVED FIXED    
Severity: Normal CC: aperez, bugs-noreply, clopez, lmoura, lucas.de.marchi, mcatanzaro, mcrha, ossy, zan
Priority: P2 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=201982
https://bugs.webkit.org/show_bug.cgi?id=215164
https://bugs.webkit.org/show_bug.cgi?id=223850
Bug Depends on: 101207, 101210    
Bug Blocks: 201982    
Attachments:
Description Flags
WIP patch updating Cairo::drawSurface
none
Patch addressing comments. none

Description Chris Dumez 2012-08-29 00:09:46 PDT
EFL port does not support EXIF orientation. This is covered by:
fast/images/exif-orientation.html
fast/images/exif-orientation-css.html
Comment 1 Dominik Röttsches (drott) 2012-10-31 07:05:21 PDT
*** Bug 100852 has been marked as a duplicate of this bug. ***
Comment 2 Csaba Osztrogonác 2012-11-04 04:18:10 PST
*** Bug 101035 has been marked as a duplicate of this bug. ***
Comment 3 Zan Dobersek 2012-11-04 23:15:55 PST
fast/images/exif-orientation-image-document.html was introduced in r132877, failing as well. Already skipped on EFL.
http://trac.webkit.org/changeset/132877
Comment 4 Dominik Röttsches (drott) 2012-11-05 03:58:59 PST
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.
Comment 5 Michael Catanzaro 2017-06-07 19:45:59 PDT
Reassigning some EFL bugs that are likely shared with GTK/WPE to the GTK component.
Comment 6 Lauro Moura 2020-04-07 07:51:24 PDT
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 7 Adrian Perez 2020-07-23 14:52:28 PDT
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.
Comment 8 Lauro Moura 2020-08-06 15:08:14 PDT
*** Bug 215164 has been marked as a duplicate of this bug. ***
Comment 9 Lauro Moura 2020-08-06 21:34:22 PDT
Created attachment 406152 [details]
Patch addressing comments.
Comment 10 Lauro Moura 2020-08-06 21:42:03 PDT
(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 11 Carlos Alberto Lopez Perez 2020-09-30 19:26:12 PDT
Comment on attachment 406152 [details]
Patch addressing comments.

This looks fine to me!
Comment 12 Adrian Perez 2020-10-02 14:15:54 PDT
Patch LGTM as well 👍️

Lauro, feel free to go ahead and land the patch, or set the commit
queue flag :)
Comment 13 Michael Catanzaro 2020-10-08 05:38:26 PDT
Comment on attachment 406152 [details]
Patch addressing comments.

I'm going to do a drive-by cq+ to see if it lands without conflicts.
Comment 14 EWS 2020-10-08 05:40:36 PDT
Committed r268176: <https://trac.webkit.org/changeset/268176>

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