guardian / grid

The Guardian’s image management system
https://www.theguardian.com/info/developer-blog/2015/aug/12/open-sourcing-grid-image-service
Apache License 2.0
1.44k stars 120 forks source link

quickfix: show crops with a discrepancy between s3 & es due to recropping with clean aspect ratio #4124

Closed andrew-nowak closed 1 year ago

andrew-nowak commented 1 year ago

The problem

The crops are returned in API response from media-api and cropper (so are in S3 and ES) but some of the dimensions are repeated in the cropper response - eg. in our reported example, Cropper tells us there's a 666 width and a 667 width - odd! and in the Media API response for the same image there is only a 667 width, which means that this crop runs afoul of this check: https://github.com/guardian/grid/blob/main/kahuna/public/js/image/controller.js#L241

but the next question is: why do we have both a 667 width and a 666 width for the same crop id (so same dimensions, same co-ordinates)? Should be impossible - this is a portrait image, so the width is calculated by round(aspectratio*cropheight). cropheight is 1000, and aspectratio is image width / image height -> so 0.6661721068249258 * 1000 -> 666

except that the first crop response listed in the media-api response has aspectRatio set in the specification - 2:3. At some point, somehow, someone has run a crop with aspect ratio set to 2:3 (not entirely sure how, it's not an option in the bar and not a ?customRatio that I can see any other tool setting in the Guardian), which happens to cover all the pixels in the image, so counts as the same crop as running crop full frame. When grid gets a crop request with an explicit aspect ratio, it calculates with width based on that instead - (2 / 3) * 1000 -> 0.66666... * 1000 -> 667

You can repeat this yourself quite easily in TEST; crop an image using one of the set aspect ratios, then recrop in the freeform setting, but pick the exact same co-ordinates/dimensions. It's very likely that the pixels will round slightly differently, and then the crop will be dropped from the UI

The (quick) fix

This PR fixes by reversing the filtering condition instead of requiring every asset present in S3 to also be present in ES, require every asset present in ES to be present in S3.

A future fix

Prevent crop resizing from using the "clean" (2:3) aspect ratio - always use the "dirty" aspect ratio calculated by dividing the original image pixel dimensions. Then there can never be any disagreement between different cropping attempts at the same crop ID.

prout-bot commented 1 year ago

Seen on auth, collections, leases, cropper (created by @andrew-nowak and merged by @paperboyo 9 minutes and 50 seconds ago) Please check your changes!

prout-bot commented 1 year ago

Seen on image-loader (created by @andrew-nowak and merged by @paperboyo 9 minutes and 55 seconds ago) Please check your changes!

prout-bot commented 1 year ago

Seen on usage, kahuna (created by @andrew-nowak and merged by @paperboyo 10 minutes ago) Please check your changes!

prout-bot commented 1 year ago

Seen on metadata-editor (created by @andrew-nowak and merged by @paperboyo 10 minutes and 7 seconds ago) Please check your changes!

prout-bot commented 1 year ago

Seen on thrall (created by @andrew-nowak and merged by @paperboyo 10 minutes and 11 seconds ago) Please check your changes!

prout-bot commented 1 year ago

Seen on media-api (created by @andrew-nowak and merged by @paperboyo 10 minutes and 21 seconds ago) Please check your changes!