Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: 加速缩略图生成时间 #1818

Closed
wants to merge 3 commits into from
Closed

feat: 加速缩略图生成时间 #1818

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c60cdb1
fix(Thumb): Don't generate / get thumb for file size equal to 0
01101sam Aug 14, 2023
51e419a
feat(Thumb): Speed up thumbnail generation
01101sam Aug 14, 2023
0c82134
fix(thumb - speedup): Consistent with author's request changes
01101sam Aug 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion models/file.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -463,7 +463,7 @@ func (file *File) GetPosition() string {
// `True` does not guarantee the load request will success in next step, but the client
// should try to load and fallback to default placeholder in case error returned.
func (file *File) ShouldLoadThumb() bool {
return file.MetadataSerialized[ThumbStatusMetadataKey] != ThumbStatusNotAvailable
return file.MetadataSerialized[ThumbStatusMetadataKey] != ThumbStatusNotAvailable && file.Size != 0
}

// return sidecar thumb file name
Expand Down
27 changes: 17 additions & 10 deletions pkg/filesystem/image.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import (
"github.com/cloudreve/Cloudreve/v3/pkg/filesystem/response"
"github.com/cloudreve/Cloudreve/v3/pkg/thumb"
"github.com/cloudreve/Cloudreve/v3/pkg/util"
)
)

/* ================
图像处理相关
Expand Down Expand Up @@ -117,7 +117,20 @@ 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)) {
Copy link
Member

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.

Copy link
Contributor Author

@01101sam 01101sam Aug 14, 2023 •
edited
Loading

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

// Provide file source path for local policy files
var err error
url, src := "", ""
if conf.SystemConfig.Mode == "slave" || file.GetPolicy().Type == "local" {
src = file.SourceName
} else {
Copy link
Member

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:

  1. Generator specific logic should keeps in thumb package. This image.go should be generator-agnostic.

  2. 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.

Copy link
Contributor Author

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.

url, err = fs.Handler.Source(ctx, file.SourceName, 300, false, 0)
if err != nil {
util.Log().Warning("failed to get slave file download url: %w", err)
}
}

// Only check max size for file that will touch local fs
if url == "" && file.Size > uint64(model.GetIntSetting("thumb_max_src_size", 31457280)) {
_ = updateThumbStatus(file, model.ThumbStatusNotAvailable)
return errors.New("file too large")
}
Expand All @@ -128,17 +141,11 @@ func (fs *FileSystem) generateThumbnail(ctx context.Context, file *model.File) e
// 获取文件数据
source, err := fs.Handler.Get(newCtx, file.SourceName)
if err != nil {
return fmt.Errorf("faield to fetch original file %q: %w", file.SourceName, err)
return fmt.Errorf("failed to fetch original file %q: %w", file.SourceName, err)
}
defer source.Close()

// Provide file source path for local policy files
src := ""
if conf.SystemConfig.Mode == "slave" || file.GetPolicy().Type == "local" {
src = file.SourceName
}

thumbRes, err := thumb.Generators.Generate(ctx, source, src, file.Name, model.GetSettingByNames(
thumbRes, err := thumb.Generators.Generate(ctx, source, src, url, file, model.GetSettingByNames(
"thumb_width",
"thumb_height",
"thumb_builtin_enabled",
Expand Down
4 changes: 2 additions & 2 deletions pkg/thumb/builtin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package thumb
import (
"context"
"fmt"
"image"
"image"
"image/gif"
"image/jpeg"
"image/png"
Expand Down Expand Up @@ -157,7 +157,7 @@ func (image *Thumb) CreateAvatar(uid uint) error {

type Builtin struct{}

func (b Builtin) Generate(ctx context.Context, file io.Reader, src, name string, options map[string]string) (*Result, error) {
func (b Builtin) Generate(ctx context.Context, file io.Reader, src, url, name string, options map[string]string) (*Result, error) {
img, err := NewThumbFromFile(file, name)
if err != nil {
return nil, err
Expand Down
5 changes: 4 additions & 1 deletion pkg/thumb/ffmpeg.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ type FfmpegGenerator struct {
lastRawExts string
}

func (f *FfmpegGenerator) Generate(ctx context.Context, file io.Reader, src, name string, options map[string]string) (*Result, error) {
func (f *FfmpegGenerator) Generate(ctx context.Context, file io.Reader, src, url, name string, options map[string]string) (*Result, error) {
ffmpegOpts := model.GetSettingByNames("thumb_ffmpeg_path", "thumb_ffmpeg_exts", "thumb_ffmpeg_seek", "thumb_encode_method", "temp_path")

if f.lastRawExts != ffmpegOpts["thumb_ffmpeg_exts"] {
Expand All @@ -41,6 +41,9 @@ func (f *FfmpegGenerator) Generate(ctx context.Context, file io.Reader, src, nam
)

tempInputPath := src
if url != "" {
tempInputPath = url
}
if tempInputPath == "" {
// If not local policy files, download to temp folder
tempInputPath = filepath.Join(
Expand Down
2 changes: 1 addition & 1 deletion pkg/thumb/libreoffice.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ type LibreOfficeGenerator struct {
lastRawExts string
}

func (l *LibreOfficeGenerator) Generate(ctx context.Context, file io.Reader, src string, name string, options map[string]string) (*Result, error) {
func (l *LibreOfficeGenerator) Generate(ctx context.Context, file io.Reader, src, url, name string, options map[string]string) (*Result, error) {
sofficeOpts := model.GetSettingByNames("thumb_libreoffice_path", "thumb_libreoffice_exts", "thumb_encode_method", "temp_path")

if l.lastRawExts != sofficeOpts["thumb_libreoffice_exts"] {
Expand Down
18 changes: 12 additions & 6 deletions pkg/thumb/pipeline.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import (
type Generator interface {
// Generate generates a thumbnail for a given reader. Src is the original file path, only provided
// for local policy files.
Generate(ctx context.Context, file io.Reader, src string, name string, options map[string]string) (*Result, error)
Generate(ctx context.Context, file io.Reader, src, url, name string, options map[string]string) (*Result, error)

// Priority of execution order, smaller value means higher priority.
Priority() int
Expand Down Expand Up @@ -63,18 +63,24 @@ func RegisterGenerator(generator Generator) {
sort.Sort(Generators)
}

func (p GeneratorList) Generate(ctx context.Context, file io.Reader, src, name string, options map[string]string) (*Result, error) {
inputFile, inputSrc, inputName := file, src, name
func (p GeneratorList) Generate(ctx context.Context, fileReader io.Reader, src, url string, file *model.File, options map[string]string) (*Result, error) {
inputFile, inputSrc, inputUrl, inputName := fileReader, src, url, file.Name
for _, generator := range p {
if model.IsTrueVal(options[generator.EnableFlag()]) {
res, err := generator.Generate(ctx, inputFile, inputSrc, inputName, options)
if generator.EnableFlag() == "thumb_ffmpeg_enabled" &&
file.Size > uint64(model.GetIntSetting("thumb_max_src_size", 31457280)) {
util.Log().Debug("Generator %s failed to generate thumbnail for %s since file too large, passing through to next generator.", reflect.TypeOf(generator).String(), inputName)
continue
}

res, err := generator.Generate(ctx, inputFile, inputSrc, inputUrl, inputName, options)
if errors.Is(err, ErrPassThrough) {
util.Log().Debug("Failed to generate thumbnail using %s for %s: %s, passing through to next generator.", reflect.TypeOf(generator).String(), name, err)
util.Log().Debug("Failed to generate thumbnail using %s for %s: %s, passing through to next generator.", reflect.TypeOf(generator).String(), inputName, err)
continue
}

if res != nil && res.Continue {
util.Log().Debug("Generator %s for %s returned continue, passing through to next generator.", reflect.TypeOf(generator).String(), name)
util.Log().Debug("Generator %s for %s returned continue, passing through to next generator.", reflect.TypeOf(generator).String(), inputName)

// defer cleanup funcs
for _, cleanup := range res.Cleanup {
Expand Down
2 changes: 1 addition & 1 deletion pkg/thumb/vips.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ type VipsGenerator struct {
lastRawExts string
}

func (v *VipsGenerator) Generate(ctx context.Context, file io.Reader, src, name string, options map[string]string) (*Result, error) {
func (v *VipsGenerator) Generate(ctx context.Context, file io.Reader, src, url, name string, options map[string]string) (*Result, error) {
vipsOpts := model.GetSettingByNames("thumb_vips_path", "thumb_vips_exts", "thumb_encode_quality", "thumb_encode_method", "temp_path")

if v.lastRawExts != vipsOpts["thumb_vips_exts"] {
Expand Down
Loading