Bug 200487 – [iOS] Position image information should respect the image orientation

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

<rdar://problem/53925073>

[Said Abou-Hallawa]

Created attachment 375663 [details]
Patch

[Tim Horton]

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?

[Simon Fraser (smfr)]

Comment on attachment 375663 [details]
Patch

Can we test this?

[Tim Horton]

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.

[Said Abou-Hallawa]

Created attachment 375713 [details]
Patch

[Said Abou-Hallawa]

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.

[Simon Fraser (smfr)]

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).

[Said Abou-Hallawa]

(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.

[Said Abou-Hallawa]

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.

[Tim Horton]

(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 :)

[Darin Adler]

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".

[Darin Adler]

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.

[Said Abou-Hallawa]

Created attachment 375765 [details]
Patch

[Said Abou-Hallawa]

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.

[WebKit Commit Bot]

Comment on attachment 375765 [details]
Patch

Clearing flags on attachment: 375765

Committed r248438: <https://trac.webkit.org/changeset/248438>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.