Bug 201123

Summary: EXIF orientation should be respected when rendering images
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: ImagesAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, noel.gordon, rego, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=201432
https://bugs.webkit.org/show_bug.cgi?id=201425
https://bugs.webkit.org/show_bug.cgi?id=201478
https://bugs.webkit.org/show_bug.cgi?id=203355
Bug Depends on: 201059    
Bug Blocks: 89052    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for review
none
Patch
none
Patch
none
Patch none

Description Said Abou-Hallawa 2019-08-25 01:33:09 PDT 
The EXIF orientation should be respected when rendering images in the following cases:

1. The image is the src of an <img> element
2. The image is the css background image of an HTML element
3. The image is the src of an SVG <image> element
4. The image is the src of an SVG <feImage> element
5. Drawing the image on a canvas
6. The image is the css content of an HTML element
7. The image is an ImageDocument
8. The image is data of an <object> element

This should be the behavior on all ports till the css image-orientation specs is finalized.
Comment 1 Said Abou-Hallawa 2019-08-25 01:58:16 PDT 
Created attachment 377221 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2019-08-26 16:50:54 PDT 
<rdar://problem/54728386>
Comment 3 Said Abou-Hallawa 2019-08-27 09:34:17 PDT 
Created attachment 377346 [details]
Patch
Comment 4 Said Abou-Hallawa 2019-08-27 12:25:04 PDT 
Created attachment 377362 [details]
Patch
Comment 5 Said Abou-Hallawa 2019-08-27 23:04:23 PDT 
Created attachment 377428 [details]
Patch
Comment 6 Said Abou-Hallawa 2019-08-28 08:38:37 PDT 
Created attachment 377444 [details]
Patch
Comment 7 Said Abou-Hallawa 2019-08-28 10:04:12 PDT 
Created attachment 377454 [details]
Patch for review
Comment 8 Said Abou-Hallawa 2019-08-28 15:23:18 PDT 
Created attachment 377493 [details]
Patch
Comment 9 Said Abou-Hallawa 2019-08-28 16:01:15 PDT 
Created attachment 377502 [details]
Patch
Comment 10 Simon Fraser (smfr) 2019-08-28 17:55:58 PDT 
Comment on attachment 377502 [details]
Patch

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

> LayoutTests/fast/images/exif-orientation-composited.html:-3
> -<html>
> -<head>
> -<script>

This is an important test to keep. It tests the directly-composited image code path (see RenderLayerBacking::isDirectlyCompositedImage()).
Comment 11 Said Abou-Hallawa 2019-08-31 14:45:25 PDT 
Created attachment 377807 [details]
Patch
Comment 12 WebKit Commit Bot 2019-08-31 17:47:46 PDT 
Comment on attachment 377807 [details]
Patch

Clearing flags on attachment: 377807

Committed r249364: <https://trac.webkit.org/changeset/249364>
Comment 13 WebKit Commit Bot 2019-08-31 17:47:48 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 14 noel gordon 2019-09-01 17:09:35 PDT 
(In reply to Simon Fraser (smfr) from comment #10)
> Comment on attachment 377502 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377502&action=review
> 
> > LayoutTests/fast/images/exif-orientation-composited.html:-3
> > -<html>
> > -<head>
> > -<script>
> 
> This is an important test to keep. It tests the directly-composited image
> code path (see RenderLayerBacking::isDirectlyCompositedImage()).
Comment 15 noel gordon 2019-09-01 17:33:04 PDT 
Comment on attachment 377807 [details]
Patch

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

Very nice set of tests, and thank you for adding them.  Couple of comments ...

> LayoutTests/fast/images/exif-orientation-background-expected.html:11
> +        width: 102;

Maybe 102px here?  Noticed px missing here and in other tests.

> LayoutTests/fast/images/exif-orientation-composited.html:20
> +        -webkit-transform: translateZ(0);
> +        border: 1px solid black;

Last I looked at webkit code in detail (a few years back), having CSS border style on a image disallowed direct compositing of the image.  Maybe border style no longer does that?
Comment 16 Simon Fraser (smfr) 2019-09-03 11:12:50 PDT 
(In reply to noel gordon from comment #15)
> Comment on attachment 377807 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377807&action=review
> 
> Very nice set of tests, and thank you for adding them.  Couple of comments
> ...
> 
> > LayoutTests/fast/images/exif-orientation-background-expected.html:11
> > +        width: 102;
> 
> Maybe 102px here?  Noticed px missing here and in other tests.
> 
> > LayoutTests/fast/images/exif-orientation-composited.html:20
> > +        -webkit-transform: translateZ(0);
> > +        border: 1px solid black;
> 
> Last I looked at webkit code in detail (a few years back), having CSS border
> style on a image disallowed direct compositing of the image.  Maybe border
> style no longer does that?

You're right; borders will kick you out of the directly composited image code path.
Comment 17 Said Abou-Hallawa 2019-09-03 12:00:46 PDT 
https://bugs.webkit.org/show_bug.cgi?id=201432 addresses the last review comments.
Comment 18 noel gordon 2019-09-04 03:57:56 PDT 
Thanks for checking, Said.
Comment 19 Manuel Rego Casasnovas 2019-11-28 01:15:25 PST 
Comment on attachment 377807 [details]
Patch

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

> Source/WebCore/rendering/RenderElement.cpp:-2063
> -    if (settings().shouldRespectImageOrientation() && is<HTMLImageElement>(element()))

If I'm not wrong this was the only usage of shouldRespectImageOrientation().
After this change do we still need "shouldRespectImageOrientation" flag defined at WebCore/page/Settings.yaml?
Comment 20 Said Abou-Hallawa 2019-12-02 10:58:22 PST 
Comment on attachment 377807 [details]
Patch

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

>> Source/WebCore/rendering/RenderElement.cpp:-2063
>> -    if (settings().shouldRespectImageOrientation() && is<HTMLImageElement>(element()))
> 
> If I'm not wrong this was the only usage of shouldRespectImageOrientation().
> After this change do we still need "shouldRespectImageOrientation" flag defined at WebCore/page/Settings.yaml?

No. We do not really need it. I kept the shouldRespectImageOrientation setting because the preference key WebKitShouldRespectImageOrientation is still using it. But surely we need to clean up this area.