Bug 221005

Summary:

Incorrect sizing of uploaded images when using background-size:cover

Product:

WebKit

Reporter:

Kenneth Kufluk <kenneth>

Component:

Images

Assignee:

Cameron McCormack (:heycam) <heycam>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

akeerthi, eric.carlson, heycam, sabouhallawa, simon.fraser, smoley, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

Safari 14

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Test image for upload	none
screenshot of the demo showing the erroneous white block	none
Testcase	none
Patch	none
Patch	none

Description Kenneth Kufluk 2021-01-26 13:02:46 PST

Created attachment 418467 [details]
Test image for upload

On twitter.com the website, when uploading an image, you are able to hit 'edit' and zoom/crop the image.

On iPhone when adding portrait images directly from camera, there is noticable flicker when later zooming the image.
This can be reproduced on desktop safari (14 and STP) by airdropping the image to macbook.

This seems to be a rendering error when sizing an image to certain dimensions.

To reproduce in reduced case:

Visit http://jsfiddle.net/r82facvo/7/
Run the code.
Upload the attached image (floor.jpg) which was taken with iPhone 12 and converted to jpg on macbook.
Observe that one of the divs showing the image has a large white block underneath.

Comment 1 Kenneth Kufluk 2021-01-26 13:03:38 PST

Created attachment 418468 [details]
screenshot of the demo showing the erroneous white block

Comment 2 Smoley 2021-01-27 17:38:20 PST

Thanks for filing, is this jsfiddle still live? I can't seem to get it to load.

Comment 3 Kenneth Kufluk 2021-01-27 20:04:53 PST

Yes, it should still be live. If Glitch is easier, I've posted it to:
https://legend-gusty-leptoceratops.glitch.me

With the code available via the fish menu or at https://glitch.com/edit/#!/legend-gusty-leptoceratops

Comment 4 Radar WebKit Bug Importer 2021-01-27 20:08:39 PST

<rdar://problem/73692426>

Comment 5 Simon Fraser (smfr) 2021-01-27 20:08:55 PST

This looks like a bug in a framework below webkit.

Comment 6 Kenneth Kufluk 2021-01-27 20:25:40 PST

Adding a border seems to help, eg

.img { border: 1px solid transparent }

Comment 7 Simon Fraser (smfr) 2021-01-27 20:35:35 PST

I take that back. The floor.jpg image has an EXIF rotation, so we get to GraphicsContext::drawPlatformImage() and hit this branch:

            if (options.orientation() != ImageOrientation::None) {
                if (auto transform = options.orientation().transformFromDefault(imageSize).inverse())
                    subimageRect = transform.value().mapRect(subimageRect);
            }

which is where I think things go wrong. We also hit the subsequent assertion:
    ASSERT(CGImageGetHeight(subImage.get()) == currHeight - CGRectIntegral(srcRect).origin.y);

Attaching a standalone testcase.

Comment 8 Simon Fraser (smfr) 2021-01-27 20:36:01 PST

Created attachment 418610 [details]
Testcase

Comment 9 Kenneth Kufluk 2021-01-27 20:38:58 PST

Ah, that explains why the photo needed to be from the camera rather than one that had been manipulated (and thus lost the exif rotation).

Comment 10 Cameron McCormack (:heycam) 2021-05-03 19:10:39 PDT

(In reply to Simon Fraser (smfr) from comment #7)
> I take that back. The floor.jpg image has an EXIF rotation, so we get to
> GraphicsContext::drawPlatformImage() and hit this branch:
> 
>             if (options.orientation() != ImageOrientation::None) {
>                 if (auto transform =
> options.orientation().transformFromDefault(imageSize).inverse())
>                     subimageRect = transform.value().mapRect(subimageRect);
>             }

I think this part's right.

> which is where I think things go wrong. We also hit the subsequent assertion:
>     ASSERT(CGImageGetHeight(subImage.get()) == currHeight -
> CGRectIntegral(srcRect).origin.y);

But this is wrong.  We need to be getting the oriented height of the image, since currHeight and srcRect are in "logical" space, and subImage deals with unoriented coordinates.  Same for the following line, which I think is causing the height to be wrongly adjusted.

Comment 11 Cameron McCormack (:heycam) 2021-05-03 21:41:13 PDT

Created attachment 427634 [details]
Patch

Comment 12 Cameron McCormack (:heycam) 2021-05-03 21:42:36 PDT

Created attachment 427635 [details]
Patch

Comment 13 EWS 2021-05-04 14:40:37 PDT

Committed r276984 (237310@main): <https://commits.webkit.org/237310@main>

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

Comment 14 Kenneth Kufluk 2021-05-04 15:59:12 PDT

Thanks for the investigation and fix! I'm really excited to try it out and see the effect. ❤️🙏