fix(server): use correct file extension for motion photo videos #8659
Conversation
lukashass
changed the title
|
@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. |
|
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.
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.
@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.
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.
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.
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.
lukashass
changed the title
| } | ||
|
|
||
| public async down(): Promise<void> { | ||
| // TODO |
| @@ -438,7 +438,7 @@ export class MetadataService { | |||
| checksum, | |||
| ownerId: asset.ownerId, | |||
| originalPath: motionPath, | |||
| originalFileName: asset.originalFileName, | |||
| originalFileName: `${path.parse(asset.originalFileName).name}.mp4`, | |||
Closes #8658
Set
originalFileNameextension of motion assets tomp4TODO