nextcloud / photos

📸 Your memories under your control
GNU Affero General Public License v3.0
516 stars 59 forks source link

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

Closed provokateurin closed 1 month ago

provokateurin commented 1 month ago

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 commented 1 month ago

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

artonge commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

This is indeed a lot cleaner, please review :)

artonge commented 1 month ago

/backport to stable29

artonge commented 1 month ago

/backport to stable28