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(server): motion photo asset linking & management #8512

Closed
wants to merge 22 commits into from

Conversation

Ynng
Copy link
Contributor

@Ynng Ynng commented Apr 4, 2024

Description

How Has This Been Tested?

Manually tested locally with MP, MVIMG and apple HEIF motion photo formats.
The video portion of each test photo was uploaded first before the image is uploaded.
The video asset is successfully hidden, and motion photo is linked properly.

Also tested with external library. The same motion photo is added to an external library and uploaded through the web gui. Immich correctly created 2 separate video assets.

Discussion

This PR introduces a nuanced approach regarding the handling of video assets from motion photos in external libraries, potentially raising discussion.

Specifically, I mark video assets derived from motion photos in external libraries isExternal = false, despite their library being EXTERNAL. (what a mouthful)

It only makes sense to keep these video assets in the same external library as their corresponding motion photo, even though they are physically stored in the encoded-video folder internally.

I'm interested to hear what your thoughts are

@carloslockward
Copy link

Tested this. Seems to be working as intended


if (asset.livePhotoVideoId !== motionAsset.id) {
await this.assetRepository.update({ id: asset.id, livePhotoVideoId: motionAsset.id });
Copy link

@carloslockward carloslockward Apr 5, 2024

Choose a reason for hiding this comment

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

Should this be done after deleting the old live photo asset asset.livePhotoVideoId?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original code was in this order (update livePhotoVideoId before deleting old motion video assets)
I think this works because the livePhotoVideoId update is not reflected in asset immediately.

open to discussion

Choose a reason for hiding this comment

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

Mhh yeah is see the original has this order aswell. Perhaps I am missing something but I think its wrong. What if the update to livePhotoVideoId happens before reaching the deletion part? It could delete the new livePhotoVideoId that was just updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I doubt that's possible.
We don't re-fetch or update the existing asset object from the database

@Ynng Ynng marked this pull request as ready for review April 6, 2024 06:10
@Ynng Ynng marked this pull request as draft April 6, 2024 09:22
@Ynng Ynng changed the title fix(server): link existing motion asset with motion photo fix(server): link existing video asset with motion photo, update storage usage properly, and prevents cross library linking Apr 6, 2024
@Ynng Ynng changed the title fix(server): link existing video asset with motion photo, update storage usage properly, and prevents cross library linking fix(server): properly link motion photos with video assets Apr 6, 2024
@Ynng Ynng marked this pull request as ready for review April 6, 2024 11:01
@Ynng Ynng marked this pull request as draft April 7, 2024 05:00
@Ynng Ynng changed the title fix(server): properly link motion photos with video assets fix(server): motion photo asset linking & management across libraries Apr 7, 2024
@Ynng Ynng changed the title fix(server): motion photo asset linking & management across libraries fix(server): motion photo asset linking & management Apr 7, 2024
@Ynng Ynng marked this pull request as ready for review April 7, 2024 09:15
@Ynng Ynng marked this pull request as draft April 7, 2024 09:38
@Ynng Ynng marked this pull request as ready for review April 7, 2024 09:47
@Ynng
Copy link
Contributor Author

Ynng commented Apr 7, 2024

Sorry for the back and forth between draft and PR but I think this PR is ready for review

@jrasm91
Copy link
Contributor

jrasm91 commented Apr 8, 2024

Man, this is kind of annoying. It deals with excluding external library quota stuff and also live motion linking changes. Those should be two separate, unrelated pull requests.

@jrasm91
Copy link
Contributor

jrasm91 commented Apr 8, 2024

I spent awhile looking at this pull request, but there are just too many different things going on here for me to feel comfortable merging it. This PR touches library scanning, quotas, live photo linking (several different scenarios here), as well as updates several queries that might have performance implications. I think most of the code is correct, but the way it is all lumped together makes it hard to review, at least for me personally. I'd be much more likely to merge and include it if you would submit separate pull requests for:

  1. migrating getByChecksum from ownerId to libraryId
  2. linking existing android motion videos (what I think this PR was originally about)
  3. fixing offline motion video files for external libraries.

@carloslockward
Copy link

carloslockward commented Apr 8, 2024

I spent awhile looking at this pull request, but there are just too many different things going on here for me to feel comfortable merging it. This PR touches library scanning, quotas, live photo linking (several different scenarios here), as well as updates several queries that might have performance implications. I think most of the code is correct, but the way it is all lumped together makes it hard to review, at least for me personally. I'd be much more likely to merge and include it if you would submit separate pull requests for:

  1. migrating getByChecksum from ownerId to libraryId
  2. linking existing android motion videos (what I think this PR was originally about)
  3. fixing offline motion video files for external libraries.

@jrasm91 Also:
4. Update storage usage on motion asset creation so that deleting motion assets reflects correctly on the storage usage.

@Ynng
Copy link
Contributor Author

Ynng commented Apr 8, 2024

Will split into smaller RPs

@Ynng Ynng closed this Apr 8, 2024
@jrasm91
Copy link
Contributor

jrasm91 commented Apr 8, 2024

Awesome, thank you for being understanding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment