Skip to content

Conversation

@jrasm91
Copy link
Member

@jrasm91 jrasm91 commented Feb 13, 2025

Image rotation

Description

This PR adds the rotation feature by saving/updating the Orientation exif tag to a sidecar file. Afterwards, it queues jobs to regenerate thumbnails, which now use the saved orientation value. The only thing left to do is change the url so it can bypass the frontend cache which currently relies on asset.checksum, which isn't updated with this type of edit.

API Changes

PUT /api/assets { assetIds: [], orientation: <1,2,3,4,5,6,7,8> }
PUT /api/assets/:id { orientation: <1,2,3,4,5,6,7,8> }

Fixes #8355 (and potentially double rotates other HEIF images, which can at least now be fixed in the web ui directly)

image
image

@mertalev
Copy link
Member

Wouldn't it be better to set the orientation tag in the preview and thumbnail instead of regenerating them?

@jrasm91
Copy link
Member Author

jrasm91 commented Feb 14, 2025

I can see if it is possible to update them by simply changing exif. I was under the impression that it wouldn't change how it displayed it.

@bo0tzz bo0tzz added the preview label Feb 14, 2025
@github-actions
Copy link
Contributor

Deploying preview environment to https://pr-16089.preview.internal.immich.cloud/

@jrasm91
Copy link
Member Author

jrasm91 commented Feb 14, 2025

@mertalev you combined all the jobs together, but thumbhash still needs to be regenerated when orientation changes. I can't be bothered to rewrite this just to prevent regenerating 2 thumbnails.
image

@jrasm91 jrasm91 marked this pull request as ready for review February 14, 2025 16:05
Copy link
Member

@mertalev mertalev left a comment

Choose a reason for hiding this comment

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

Can you debounce the rotation clicks? It's not good if they rotate multiple times and queue multiple thumbnail jobs that run in parallel. Besides being unnecessary work, it can cause all kinds of race conditions.

const processInvalidImages = process.env.IMMICH_PROCESS_INVALID_IMAGES === 'true';

const orientation = useExtracted && asset.exifInfo?.orientation ? Number(asset.exifInfo.orientation) : undefined;
const orientation = asset.exifInfo?.orientation ? Number(asset.exifInfo.orientation) : undefined;
Copy link
Member

Choose a reason for hiding this comment

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

This is how it used to work and it was changed to handle HEIC files correctly.

Copy link
Member

@mertalev mertalev left a comment

Choose a reason for hiding this comment

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

HEIF needs to be handled correctly before this can be merged. It's the dominant format for most libraries; it can't be an afterthought.

@marekStef

This comment was marked as off-topic.

@pmorch
Copy link

pmorch commented Jul 12, 2025

@mertalev wrote:

  • HEIF needs to be handled correctly before this can be merged

  • Can you debounce the rotation clicks?

@jrasm91 wrote:

I can't be bothered to rewrite this just to prevent regenerating 2 thumbnails.

@jrasm91 : Where are you on this? Have you given up on this PR? (Which would be fair enough. This is open source and voluntary after all...)

It seems like very worthy functionality, but it sounds like it needs the finishing touches. Would anybody be able to provide any hints on how to complete this?

@danieldietzler
Copy link
Member

We're starting to pick this up internally again.

@Flowingblood
Copy link

What is the current status of this? Are there any further adjustments that need to be made?

@danieldietzler
Copy link
Member

#24155

@jrasm91 jrasm91 closed this Dec 3, 2025
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 backed up from iOS don't keep correct orientation

8 participants