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

fix(SizeMetadataProvider): Swap the width and height if the image is rotated #2474

Merged
merged 1 commit into from
May 14, 2024

Conversation

provokateurin
Copy link
Member

Hi,
I'm not sure if this fixes make sense, but there is a problem with the current size calculation: The width and height are reported for the original unrotated version instead of the rotated version. This leads to the problem that clients relying on the WebDAV metadata or the rich object parameters when sending an image in Talk will display the image with the wrong dimensions. I'm not sure if this is a valid problem, but I don't see another way as not all places expose the full exif metadata (Talk) and letting the clients handle it is a bit tedious.

To fix this I just added this code to detect the rotation and swap the dimensions if necessary, but I'm not happy with this fix. It only works with the exif module enabled and the exif metadata is already read in the ExifMetadataProvider so this is duplicate. I think this fix would be cleaner if the functionality was moved to the ExifMetadataProvider so the exif data is only read once. If the exif module is not available we can still fallback to the current method of getting the image dimensions, but swapping the dimensions won't work.

@provokateurin provokateurin added bug Something isn't working 3. to review Waiting for reviews labels May 13, 2024
@provokateurin provokateurin requested review from artonge and skjnldsv May 13, 2024 09:08
@provokateurin
Copy link
Member Author

provokateurin commented May 13, 2024

Is it necessary to somehow trigger the metadata generation again since the cached version might be wrong?

@artonge
Copy link
Collaborator

artonge commented May 13, 2024

I think you can read the already generated metadata from a provider. So if the size provider is executed after the exif one, you can use the exif data from the size provider.
Could this help have a cleaner code?

@provokateurin
Copy link
Member Author

Yeah that was also an idea I had, but I wasn't sure if that is a good way to do it. If you want I can go that route as it should definitely make the fix a lot cleaner. Do you generally agree that this is a bug or not?

@artonge
Copy link
Collaborator

artonge commented May 13, 2024

If you want I can go that route as it should definitely make the fix a lot cleaner.

I think that would be better, yes :).

Do you generally agree that this is a bug or not?

Yes, if the size does not match, then this is an issue. Just to be sure that I understand the issue, when the image is rotated, then the width is stored as the height in the EXIF data, right?

If so, then I think inverting width and height makes sense.

@provokateurin
Copy link
Member Author

Yes, if the size does not match, then this is an issue. Just to be sure that I understand the issue, when the image is rotated, then the width is stored as the height in the EXIF data, right?

If the image has a rotation set in the EXIF data the resulting photos-size metadata will be incorrect because it doesn't account for the rotation. This means the metadata will be the inverse of how a client will display the image because it will interprete the rotation while the metadata doesn't.

@provokateurin provokateurin force-pushed the fix/size-metadata-provider/rotation branch from 301da1a to 369b473 Compare May 14, 2024 07:47
@provokateurin
Copy link
Member Author

This is indeed a lot cleaner, please review :)

…rotated

Signed-off-by: provokateurin <kate@provokateurin.de>
@provokateurin provokateurin force-pushed the fix/size-metadata-provider/rotation branch from 369b473 to edafa9f Compare May 14, 2024 07:50
@artonge
Copy link
Collaborator

artonge commented May 14, 2024

/backport to stable29

@artonge
Copy link
Collaborator

artonge commented May 14, 2024

/backport to stable28

@backportbot backportbot bot added the backport-request Pending backport by the backport-bot label May 14, 2024
@skjnldsv skjnldsv merged commit f85947f into master May 14, 2024
42 checks passed
@skjnldsv skjnldsv deleted the fix/size-metadata-provider/rotation branch May 14, 2024 16:25
@backportbot backportbot bot removed the backport-request Pending backport by the backport-bot label May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants