feat: optionally generate thumbnails for invalid images #11126
Conversation
| @@ -45,7 +46,8 @@ export class MediaRepository implements IMediaRepository { | |||
| } | |||
|
|
|||
| async generateThumbnail(input: string | Buffer, output: string, options: ThumbnailOptions): Promise<void> { | |||
| const pipeline = sharp(input, { failOn: 'error', limitInputPixels: false }) | |||
| // some invalid images can still be processed by sharp, but we want to fail on them by default to avoid crashes | |||
| const pipeline = sharp(input, { failOn: PROCESS_INVALID_IMAGES ? 'none' : 'error', limitInputPixels: false }) | |||
There was a problem hiding this comment.
Not really sure if using an import from constants is the right choice here. Might also include it in the options object, which would make it better testable. But I guess that's something that shouldn't be configurable in the UI?
There was a problem hiding this comment.
For testing, I think it should be a field in options. We don't need to expose it in the UI since this is a more niche scenario.
There was a problem hiding this comment.
Done that and added a test case. Not too sure about accessing the env directly in the service, let me know if you have other suggestions :)
server/src/constants.ts
Outdated
| @@ -27,6 +27,8 @@ export const WEB_ROOT = process.env.IMMICH_WEB_ROOT || '/usr/src/app/www'; | |||
| const HOST_SERVER_PORT = process.env.IMMICH_PORT || '2283'; | |||
| export const DEFAULT_EXTERNAL_DOMAIN = 'http://localhost:' + HOST_SERVER_PORT; | |||
|
|
|||
| export const PROCESS_INVALID_IMAGES = process.env.IMMICH_PROCESS_INVALID_IMAGES === 'true'; | |||
There was a problem hiding this comment.
Would this need to be documented as well (independent from where the option will be in the end)?
There was a problem hiding this comment.
Yes, it should be added here.
|
Do you have the invalid image to attach to the PR so we can test in the future? |
github-actions
bot
added
the
documentation
Sure! https://drive.google.com/file/d/1pfCPCEJkA_d-glKWOgou3rGbHixA8AK6/view?usp=sharing |
|
What is the reason for making this configurable/not defaulting it to on? |
It seems like that having it on by default could cause issues like: #10452 |
|
I tried to drop the file you posted into the instance. Of course, the thumbnail doesn't get generated, but it doesn't seem to crash the instance. |
Exactly, and with |
|
It isn't guaranteed that an invalid image crashes the server, but there's a higher risk if it's routed to a foreign loader like ImageMagick, libraw or libheif. I've had it happen for a HEIF image in the past and it went away when I changed this setting to fail on error. The default is actually to fail on warning. |
First PR here, there are some questions. Came from a suggestion on discord.