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): use correct file extension for motion photo videos #8659
base: main
Are you sure you want to change the base?
fix(server): use correct file extension for motion photo videos #8659
Conversation
@alextran1502 I saw that since #7679 the file extension for the archive download is only taken from |
Yeah, IMO |
On the other hand I'm wondering if the video should be downloaded at all, since it seems to still be embedded in the downloaded jpg and thus downloaded twice. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should probably fix the original filename which is wrong, no?
The video is extracted from the jpg on upload, so the filename is kind of correct (since it says "original"). It is a little ambiguous, that's why I'm questioning the download of the separate video altogether 🤔 |
So from what I'm reading there are motion photo formats which
This means the correct fix for the issue is to only download a separate video when the jpg does not contain the video, right? How would we detect that? I could think of two ways:
|
@@ -96,7 +96,8 @@ export class DownloadService { | |||
|
|||
const { originalPath, originalFileName } = asset; | |||
|
|||
let filename = originalFileName; | |||
const extension = parse(originalPath).ext; | |||
let filename = `${parse(originalFileName).name}${extension}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does it have the wrong extension to begin with? I think I would rather fix the root cause instead of patching it everywhere it is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jrasm91 I agree (sort of). Hence my comments #8659 (comment) and #8659 (comment).
What is your opinion on that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iOS doesn't have it embedded but Android does. Either way getting the extracted video might be nice when you download it. The name for the motion asset should come from the original when it is uploaded, but that should not include the extension...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A filename includes the extension (by definition), which makes it confusing as that does not match what users perceive as such.
Having said that I'd say its fair to transform the (original)filename of files when converted (thus, if I am following, never having the incorrect extension for these records at time of download).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure the old extension is coming from here:
https://github.com/immich-app/immich/blob/main/server/src/services/metadata.service.ts#L441
If you are going to fix anything, it should be here instead. It is probably possible to write a migration to fix affected files as well.
IMO we should store the correct filename and extension when the asset is created and then never touch or need to touch/modify if moving forward. The current solution also introduces the possibility of the extension case changing between upload and download.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great imo. Once the tests are passing I think we'll be good to go.
} | ||
|
||
public async down(): Promise<void> { | ||
// TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably can leave blank
@@ -438,7 +438,7 @@ export class MetadataService { | |||
checksum, | |||
ownerId: asset.ownerId, | |||
originalPath: motionPath, | |||
originalFileName: asset.originalFileName, | |||
originalFileName: `${path.parse(asset.originalFileName).name}.mp4`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perfect
Closes #8658
Set
originalFileName
extension of motion assets tomp4
TODO