Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUGFIX: Use autorotate filter to apply resize adjustments properly to images with exif-orientations #3147

Merged
merged 1 commit into from
Nov 18, 2020

Conversation

mficzel
Copy link
Member

@mficzel mficzel commented Oct 27, 2020

The autorotate is applied before calculating the resize dimensions to work with correct size values.

How to test:

Resolves: #3148

@mficzel mficzel requested a review from daniellienert October 27, 2020 17:44
@mficzel mficzel marked this pull request as draft October 27, 2020 17:45
@mficzel
Copy link
Member Author

mficzel commented Oct 27, 2020

@daniellienert Not sure wether this is the right way to handle images with exif-orientation but for me it worked. Another question would be wether this is a bugfix or a feature and which branch to target.

@mficzel mficzel changed the title BUGFIX: Use autorotate filter images to apply resize adjustments properly to images that used exif-orientations BUGFIX: Use autorotate filter to apply resize adjustments properly to images with exif-orientations Oct 28, 2020
Copy link
Member

@jonnitto jonnitto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and it works. But why you want to put this only to master?

@mficzel
Copy link
Member Author

mficzel commented Oct 28, 2020

@jonnitto that is why i asked what to target and wether this is a bug or a feature

@jonnitto
Copy link
Member

From an editorial perspective, this is a bugfix

@mficzel
Copy link
Member Author

mficzel commented Oct 28, 2020

I just noticed that the x-y dimensions calculated for the rotated images are also wrong. Maybe the autorotation has to be added earlier in the process.

Added the autorotation also to the dimension calculation to ensure that correct sizes are calculated.

@mficzel mficzel force-pushed the bugfix/autorotateImages branch 2 times, most recently from 0d18af9 to 74ec6f4 Compare October 28, 2020 19:36
…erly to images that used exif-orientations

The autorotate is applied before calculating the dimensions to work with correct size values.
@mficzel mficzel force-pushed the bugfix/autorotateImages branch from 74ec6f4 to c4a293d Compare October 28, 2020 19:39
@mficzel mficzel changed the base branch from master to 4.3 October 28, 2020 19:39
@mficzel mficzel marked this pull request as ready for review October 28, 2020 19:39
@mficzel
Copy link
Member Author

mficzel commented Oct 28, 2020

FYI: I rebased this to 4.3 as bugfix

@@ -456,6 +456,9 @@ protected function resize(ImagineImageInterface $image, string $mode = ImageInte
throw new \InvalidArgumentException('Invalid mode specified', 1574686891);
}

$autorotateFilter = new \Imagine\Filter\Basic\Autorotate();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am currently trying to untangle the complete image processing workflow ... Wouldn't it make sense to put it into a separate rotate adjustment? Is there no scenario where you don't want to have it rotated or where it is breaking? Should the adjustment be configurable?

Copy link
Member Author

@mficzel mficzel Oct 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daniellienert The autorotate filter does nothing if no exif orientation is set and in other cases it applies the transformation to the image so following filters do not have to deal with it. (https://github.com/avalanche123/Imagine/blob/develop/src/Filter/Basic/Autorotate.php) as far as i see this filter is meant to be applied to all images.

The only way that could cause problems i can imagine is when some code would transfer the original exif tags to the generated image-variant. Not sure wether such things exist and i would assume that the result would be broken in that case today aswell because the calculations are performed on flipped dimensions.

But i am far from sure that this is the correct way to apply this filter. The only thing i know is that this filter has to be applied before calculating dimensions and crops to support exif-orientations.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found this in the notes of the Autorotate filter Your attention please: This filter requires the use of the ExifMetadataReader to work. and in ExifMetadataReader it states Using the exif metadata reader adds a significant overhead to image processing.

Still think that it is still more important to properly read images and doubt that the exif parsing can be that significant.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yap. We have quite some projects where editors complain about this bug

@mficzel
Copy link
Member Author

mficzel commented Nov 2, 2020

Close and reopen to trigger travis

@mficzel mficzel closed this Nov 2, 2020
@mficzel mficzel reopened this Nov 2, 2020
@jonnitto
Copy link
Member

@daniellienert Is it ok for you if we merge this one?

@daniellienert
Copy link
Member

👍🏼

@jonnitto jonnitto merged commit d5f051b into neos:4.3 Nov 18, 2020
Copy link
Member

@kdambekalns kdambekalns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A post-merge "yay, and it looks good" from my side…

markusguenther pushed a commit to markusguenther/neos-development-collection that referenced this pull request Mar 25, 2021
BUGFIX: Use autorotate filter to apply resize adjustments properly to images with exif-orientations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Images with exif-orientations are not resized properly.
4 participants