Bug 200487 - [iOS] Position image information should respect the image orientation
Summary: [iOS] Position image information should respect the image orientation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-08-06 16:12 PDT by Said Abou-Hallawa
Modified: 2019-08-08 12:13 PDT (History)
6 users (show)

See Also:


Attachments
Patch (1.75 KB, patch)
2019-08-06 16:24 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (24.74 KB, patch)
2019-08-07 10:45 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (26.95 KB, patch)
2019-08-07 16:42 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2019-08-06 16:12:49 PDT
imagePositionInformation() should respect the image orientation when drawing the image in a shared bitmap context. This shared image will be drawn as a position information before showing the image in the preview widow.
Comment 1 Said Abou-Hallawa 2019-08-06 16:14:26 PDT
<rdar://problem/53925073>
Comment 2 Said Abou-Hallawa 2019-08-06 16:24:04 PDT
Created attachment 375663 [details]
Patch
Comment 3 Tim Horton 2019-08-06 16:30:53 PDT
Comment on attachment 375663 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2638
> -    graphicsContext->drawImage(*image, FloatRect(0, 0, bitmapSize.width(), bitmapSize.height()));
> +    graphicsContext->drawImage(*image, FloatRect(0, 0, bitmapSize.width(), bitmapSize.height()), ImageOrientationDescription(RespectImageOrientation));

Isn't bitmapSize above also going to need a fix?
Comment 4 Simon Fraser (smfr) 2019-08-06 16:36:56 PDT
Comment on attachment 375663 [details]
Patch

Can we test this?
Comment 5 Tim Horton 2019-08-07 10:06:38 PDT
Comment on attachment 375663 [details]
Patch

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

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2638
>> +    graphicsContext->drawImage(*image, FloatRect(0, 0, bitmapSize.width(), bitmapSize.height()), ImageOrientationDescription(RespectImageOrientation));
> 
> Isn't bitmapSize above also going to need a fix?

You explained that it doesn't. but you also need to follow the current state of the RenderImage's respect, not just respect willy-nilly.
Comment 6 Said Abou-Hallawa 2019-08-07 10:45:59 PDT
Created attachment 375713 [details]
Patch
Comment 7 Said Abou-Hallawa 2019-08-07 11:05:09 PDT
Comment on attachment 375663 [details]
Patch

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

>>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2638
>>> +    graphicsContext->drawImage(*image, FloatRect(0, 0, bitmapSize.width(), bitmapSize.height()), ImageOrientationDescription(RespectImageOrientation));
>> 
>> Isn't bitmapSize above also going to need a fix?
> 
> You explained that it doesn't. but you also need to follow the current state of the RenderImage's respect, not just respect willy-nilly.

I was not completely right.

As I mentioned in the ChangeLog boundsPositionInformation() returns the correct boundary because it gets it from the RenderImage which is correct. But you are right about bitmapSize. It should respect the image orientation.  I hit a problem when debugging the API test which I added to the new patch. I think it worked in the Mail case because the position image size is animated from the original size of the image. in the message to the size of the hint window regardless of the position image intrinsic size. I confirmed that we were returning the un-oriented size with this patch. See the fix in the new patch.
Comment 8 Simon Fraser (smfr) 2019-08-07 11:10:56 PDT
Comment on attachment 375713 [details]
Patch

Is there a reason the test image has blurry edges between the colors? I think a PNG would be better for testing (no compression artifacts).
Comment 9 Said Abou-Hallawa 2019-08-07 11:19:40 PDT
(In reply to Simon Fraser (smfr) from comment #8)
> Comment on attachment 375713 [details]
> Patch
> 
> Is there a reason the test image has blurry edges between the colors? I
> think a PNG would be better for testing (no compression artifacts).

I did not made this image. I copied it from LayoutTests/fast/images/resources/exif-orientation-8-llo.jpg.

And the test checks for the four pixels at the four corners. I made sure if I do not pass respectImageOrientation to drawImage() in imagePositionInformation(), the four tests at the end of RequestActivatedElementInfoForRotatedImage() fail.
Comment 10 Said Abou-Hallawa 2019-08-07 12:05:34 PDT
From July 2017, PNG officially supports storing the EXIF metadata, see https://stackoverflow.com/questions/9542359/does-png-contain-exif-data-like-jpg. Some online tools support writing EXIF data to PNG images, like this one https://www.thexifer.net/#exif-general. So I was able to create a PNG and set its EXIF data. The problem is CG supports EXIF data for JPEG, TIFF and ASTC images. Finally I tried to create a lossless JPEG image but I could not.
Comment 11 Tim Horton 2019-08-07 13:18:28 PDT
(In reply to Said Abou-Hallawa from comment #10)
> From July 2017, PNG officially supports storing the EXIF metadata, see
> https://stackoverflow.com/questions/9542359/does-png-contain-exif-data-like-
> jpg. Some online tools support writing EXIF data to PNG images, like this
> one https://www.thexifer.net/#exif-general. So I was able to create a PNG
> and set its EXIF data. The problem is CG supports EXIF data for JPEG, TIFF
> and ASTC images. Finally I tried to create a lossless JPEG image but I could
> not.

No no don't bother just use the existing one and ignore smfr :)
Comment 12 Darin Adler 2019-08-07 14:59:41 PDT
Comment on attachment 375713 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2630
> +    auto respectImageOrientation = renderImage.shouldRespectImageOrientation();
> +
> +    FloatSize imageSize;
> +    if (is<BitmapImage>(image) && respectImageOrientation == RespectImageOrientation)
> +        imageSize = downcast<BitmapImage>(*image).sizeRespectingOrientation();
> +    else
> +        imageSize = image->size();

This code should be factored into a separate function. Could have it be a helper function right here in this file that takes a RenderImage& plus an Image&, or it could even be a RenderImage member function. It’s basically "size respecting orientation policy".
Comment 13 Darin Adler 2019-08-07 15:00:21 PDT
Comment on attachment 375713 [details]
Patch

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

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2630
>> +        imageSize = image->size();
> 
> This code should be factored into a separate function. Could have it be a helper function right here in this file that takes a RenderImage& plus an Image&, or it could even be a RenderImage member function. It’s basically "size respecting orientation policy".

I’m really leaning toward a RenderImage member function if we can figure out the right name for it.
Comment 14 Said Abou-Hallawa 2019-08-07 16:42:20 PDT
Created attachment 375765 [details]
Patch
Comment 15 Said Abou-Hallawa 2019-08-07 16:45:56 PDT
Comment on attachment 375713 [details]
Patch

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

>>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2630
>>> +        imageSize = image->size();
>> 
>> This code should be factored into a separate function. Could have it be a helper function right here in this file that takes a RenderImage& plus an Image&, or it could even be a RenderImage member function. It’s basically "size respecting orientation policy".
> 
> I’m really leaning toward a RenderImage member function if we can figure out the right name for it.

I re-factored CachedImage::imageSizeForRenderer() instead of repeating the same code here and there. I used the same name to create a new function which returns FloatSize because it does not scale the imageSize. The old function imageSizeForRenderer() will call the new function and will scale it if needed. It still returns LayoutSize since it scales the image size.
Comment 16 WebKit Commit Bot 2019-08-08 12:13:18 PDT
Comment on attachment 375765 [details]
Patch

Clearing flags on attachment: 375765

Committed r248438: <https://trac.webkit.org/changeset/248438>
Comment 17 WebKit Commit Bot 2019-08-08 12:13:20 PDT
All reviewed patches have been landed.  Closing bug.