-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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: 加速缩略图生成时间 #1818
feat: 加速缩略图生成时间 #1818
Conversation
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.
Thanks for the RP, I left some comments. Overall, it is not easy to implement under current generator interface.
pkg/filesystem/image.go
Outdated
} | ||
} | ||
|
||
if src == "" && file.Size > uint64(model.GetIntSetting("thumb_max_src_size", 31457280)) { |
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 will break max_src_size
check.
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.
('s on purpose as mentioned above)
@@ -117,11 +117,6 @@ func (fs *FileSystem) generateThumbnail(ctx context.Context, file *model.File) e | |||
defer cancel() | |||
// TODO: check file size | |||
|
|||
if file.Size > uint64(model.GetIntSetting("thumb_max_src_size", 31457280)) { |
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.
unhappy path first - better to check max size before obtain workers.
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.
TLDR: I moved this code to below that local download checking, generating thumbnail shouldn't have a max size limit on.
So you want max size check to be the first priority?
Because by using url method size can be ignored and unlimited.
By letting size check for local (the code below) this could safely confirmes that we're not downloading huge files at local.
If this line puts in first this could break how this generate by url
method goes.
For example,
if I increase the max size to 10GB, (e.g. Some kind of movie)
when I failed to get the source file, the logic will fallbacks to local download mode
that's the place that the max size is worth for checking to avoid downloading huge files to local vm (As mentioned above)
In another way, if you check the max size first, this could cause lots of file that over the max size can't generate the thumbnail and forced ppl to increase in settings.
If they don't have big enough hard disk in local, this could cause OOM / full hard disk problem.
Update: I add that only checks for url, if not then it should be local file / file that will fetch to local.
For more, see pipeline.go
pkg/filesystem/image.go
Outdated
} else { | ||
ffmpegFormats := strings.Split(model.GetSettingByName("thumb_ffmpeg_exts"), ",") | ||
vipsFormats := strings.Split(model.GetSettingByName("thumb_vips_exts"), ",") | ||
// 如果文件格式支持 ffmpeg 或 vips 生成缩略图,则使用源文件链接 |
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 include VIPS? src
is not even used in VIPS generator.
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.
Thanks for letting me know, didn't notice that when writiing this.
As documented of vips
, url can also be used to generate thumbnail
Faked by ChatGPT, fuck
@@ -136,6 +131,24 @@ func (fs *FileSystem) generateThumbnail(ctx context.Context, file *model.File) e | |||
src := "" | |||
if conf.SystemConfig.Mode == "slave" || file.GetPolicy().Type == "local" { | |||
src = file.SourceName | |||
} else { |
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 does not seems to be a good abstraction:
-
Generator specific logic should keeps in
thumb
package. Thisimage.go
should be generator-agnostic. -
src
parameter in thumb generator is originally for file path. I understand in ffmpeg it happens can also be file URL, but it will make the interface definition inconsistent.
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.
ok, I'll move it into thumb
package, but this also means it needs sort of changing function sign too.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Fix: #1817
大概
3-5s
就能生成了顺便修复下给文件大小为0的文件生成缩图导致上传后不生成