-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
feat: original-sized previews for non-web-friendly images #14446
base: main
Are you sure you want to change the base?
Conversation
f2b631a
to
54b91b4
Compare
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.
Thank you for the PR! I mostly looked at the server code, where I think it needs some more polishing.
Re: caching, this also affects the case where the user zooms in and no extracted image exists, right? It will load the preview again.
076d59d
to
a7a9b4d
Compare
a7a9b4d
to
34592b3
Compare
any more comments from maintainers to get this merged, so that I can continue with previews of other images? |
@eligao no rush here |
I think we should wait until we're done with hot fixes for the current release. |
For a DNG image with a resolution of 5376x3956 and 41.3MiB in size, the following variants are generated using the default settings:
These generated variants seem larger than expected and don't match the configured settings, with the preview and original being identical. It's also strange that the resolution has slightly changed. |
nice catch, the file sizes are indeed wrong, let me fix that and add checks to tests. |
Hi again @eligao! We've been talking about the PR and think there are a few changes it'll need for it to be merge-able.
|
ebcfe9c
to
e22a123
Compare
return true; | ||
} | ||
|
||
async cloneExif(input: string, output: string): Promise<boolean> { |
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.
Copying all of the metadata seems unnecessary. It adds to the size of the output, and there are surely metadata entries that are correct for the RAW, but wrong for the extracted image.
Can you narrow down which field(s) specifically need to be added? I'd like to see if we can avoid that exiftool.read call and just apply the data in exifInfo that we already have from metadata extraction.
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.
The size added is neglible, a full set of EXIF without embedded images are usually <20KB as I tested from CR2 and ARW, adding ~1% size to a 2MB fullsize preview image.
My purpose is to preserve integrity of EXIF fields in the fullsize preview, as if it was taken with JPEG+RAW enabled or extracted from camera vendors' editors.
It is a nice catch there might be some extra RAW-specific fields, I'll stick to what we have in ExifEntity
now and come up with a cleaner solution later.
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 still don't see what the point is to including all the EXIF fields in the image. This is a generated image with a specific purpose. It doesn't need to have anything beyond what's need to display it correctly. If anything, the presence of rich metadata in the image is a downside that prevents sharing due to metadata leakage.
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.
This fullsize image extracted from RAW can be more than just specifically for preview. I'm thinking of adding features like "download as JPG" and "remove RAW but keep JPG to cleanup space" for RAW files later, which would be very handy for workflows, and it's important to preserve full EXIF fields.
As for EXIF leaking, this is indeed an issue when a shared link disallows downloading and hides EXIF but original JPG files can be acquired via thumbnail?size=fullsize
, let me address this issue.
But anyway, this cloneExif()
is already removed for now.
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 see what you're going for, but I think this should be handled when those features are actually implemented. It can be done as part of the request to download them, etc.
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.
Good point, let me keep only colorspace and orientation EXIF in this PR.
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.
Also I have added Permission.ASSET_DOWNLOAD
requirement for thumbnail?size=fullsize
to prevent leaking EXIF via original files
server/src/services/media.service.ts
Outdated
this.mediaRepository.generateThumbhash(data, thumbnailOptions), | ||
this.mediaRepository.generateThumbnail(data, { ...image.thumbnail, ...thumbnailOptions }, thumbnailPath), | ||
this.mediaRepository.generateThumbnail(data, { ...image.preview, ...thumbnailOptions }, previewPath), | ||
shouldConvertFullsize && |
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'd prefer making this a list that you conditionally append to, then waiting for that list with Promise.all
.
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.
Good suggestion, but it would break inferred type of outputs[0]
@@ -11775,6 +11777,9 @@ | |||
"extractEmbedded": { | |||
"type": "boolean" | |||
}, | |||
"fullsizePreview": { |
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 was thinking of something more like "fullsize": {"enabled": false, "format": "jpeg"}
, where format
is ignored if using the extracted preview.
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.
to make it simple and stupid, I think it is intuitive for "fullsize" to just follow "preview" settings.
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.
In the past, we've gotten complaints about image settings being shared between different files, and there may be other options related to the fullsize image added down the line. For example, a quality option might be added if users are concerned about file size and want 90 or 95 quality instead of 100, Safari users might complain because they don't need fullsize images for HEIF, etc.
I think the config field should be an object to make it easier to add settings in the future. The format is maybe superfluous in a vacuum, but the fact that one setting (format) is shared between the preview and the fullsize and the other two (quality, resolution) are not can lead to confusion. Having them completely separate will be less ambiguous.
return true; | ||
} | ||
|
||
async cloneExif(input: string, output: string): Promise<boolean> { |
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.
The size added is neglible, a full set of EXIF without embedded images are usually <20KB as I tested from CR2 and ARW, adding ~1% size to a 2MB fullsize preview image.
My purpose is to preserve integrity of EXIF fields in the fullsize preview, as if it was taken with JPEG+RAW enabled or extracted from camera vendors' editors.
It is a nice catch there might be some extra RAW-specific fields, I'll stick to what we have in ExifEntity
now and come up with a cleaner solution later.
server/src/services/media.service.ts
Outdated
this.mediaRepository.generateThumbhash(data, thumbnailOptions), | ||
this.mediaRepository.generateThumbnail(data, { ...image.thumbnail, ...thumbnailOptions }, thumbnailPath), | ||
this.mediaRepository.generateThumbnail(data, { ...image.preview, ...thumbnailOptions }, previewPath), | ||
shouldConvertFullsize && |
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.
Good suggestion, but it would break inferred type of outputs[0]
delete exifTagsToWrite['Orientation']; | ||
} | ||
const result = await exiftool.write(output, exifTagsToWrite, { | ||
ignoreMinorErrors: true, |
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.
tedious, inaccurate and incomplete, but works for now.
@@ -11775,6 +11777,9 @@ | |||
"extractEmbedded": { | |||
"type": "boolean" | |||
}, | |||
"fullsizePreview": { |
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.
to make it simple and stupid, I think it is intuitive for "fullsize" to just follow "preview" settings.
8d43411
to
4cee0c7
Compare
image.extractEmbedded
in options is set, it will attempt to extract the JPG/Preview from RAW images.extractEmbedded
is not set, it will fall back to creating a full-sized image from RAW.AssetFileType.converted
file.AssetMediaSize.Original
size is available under theviewThumbnail
APIAssetMediaSize.Original
preview via theviewThumbnail
APISome loose ends: