From 2d4fb25c34596dd2a616d2ef1c3b5d19df9b18ff Mon Sep 17 00:00:00 2001 From: Bill Thiede Date: Sun, 15 Dec 2013 13:40:09 -0800 Subject: [PATCH] images: fix Decode when resize + rotate + max W/H. Adds more tests to cover rotations with resize when used with MaxWidth/MaxHeight, previously only ScaledWidth/ScaledHeight were tested. Improve tests to compare bounds when determining equality, otherwise an image sized 0x0 is equal to all other images. Sort test image filenames so test order is stable and obvious. Keep more data in memory when indexing images upon receive. Some largish CR2 files need more data or the EXIF parsing will fail. Should address some or all of https://camlistore.org/issue/274 Change-Id: I80d90c33538c9d62ce4480ccb58c003e18ee6629 --- pkg/images/bench_test.go | 2 +- pkg/images/images.go | 61 ++++++++++++++++++++++++++++++--------- pkg/images/images_test.go | 51 ++++++++++++++++++++++++++++++-- pkg/index/receive.go | 5 +++- 4 files changed, 102 insertions(+), 17 deletions(-) diff --git a/pkg/images/bench_test.go b/pkg/images/bench_test.go index 30e3a7e0f..13f6a8dc3 100644 --- a/pkg/images/bench_test.go +++ b/pkg/images/bench_test.go @@ -27,7 +27,7 @@ func benchRescale(b *testing.B, w, h, thumbW, thumbH int) { o := &DecodeOpts{MaxWidth: thumbW, MaxHeight: thumbH} b.ResetTimer() for i := 0; i < b.N; i++ { - _ = rescale(im, o) + _ = rescale(im, o, false) } } diff --git a/pkg/images/images.go b/pkg/images/images.go index 47871a2d0..3af88cfe4 100644 --- a/pkg/images/images.go +++ b/pkg/images/images.go @@ -205,14 +205,19 @@ func ScaledDimensions(w, h, mw, mh int) (newWidth int, newHeight int) { return } -func rescale(im image.Image, opts *DecodeOpts) image.Image { +func rescale(im image.Image, opts *DecodeOpts, swapDimensions bool) image.Image { mw, mh := opts.MaxWidth, opts.MaxHeight mwf, mhf := opts.ScaleWidth, opts.ScaleHeight b := im.Bounds() // only do downscaling, otherwise just serve the original image - if !opts.wantRescale(b) { + if !opts.wantRescale(b, swapDimensions) { return im } + + if swapDimensions { + mw, mh = mh, mw + } + // ScaleWidth and ScaleHeight overrule MaxWidth and MaxHeight if mwf > 0.0 && mwf <= 1 { mw = int(mwf * float32(b.Dx())) @@ -220,7 +225,6 @@ func rescale(im image.Image, opts *DecodeOpts) image.Image { if mhf > 0.0 && mhf <= 1 { mh = int(mhf * float32(b.Dy())) } - // If it's gigantic, it's more efficient to downsample first // and then resize; resizing will smooth out the roughness. // (trusting the moustachio guys on that one). @@ -233,12 +237,36 @@ func rescale(im image.Image, opts *DecodeOpts) image.Image { return resize.Resize(im, b, mw, mh) } -func (opts *DecodeOpts) wantRescale(b image.Rectangle) bool { - return opts != nil && - (opts.MaxWidth > 0 && opts.MaxWidth < b.Dx() || - opts.MaxHeight > 0 && opts.MaxHeight < b.Dy() || - opts.ScaleWidth > 0.0 && opts.ScaleWidth < float32(b.Dx()) || - opts.ScaleHeight > 0.0 && opts.ScaleHeight < float32(b.Dy())) +func (opts *DecodeOpts) wantRescale(b image.Rectangle, swapDimensions bool) bool { + if opts == nil { + return false + } + + // In rescale Scale* trumps Max* so we assume the same relationship here. + + // Floating point compares probably only allow this to work if the values + // were specified as the literal 1 or 1.0, computed values will likely be + // off. If Scale{Width,Height} end up being 1.0-epsilon we'll rescale + // when it probably wouldn't even be noticible but that's okay. + if opts.ScaleWidth == 1.0 && opts.ScaleHeight == 1.0 { + return false + } + if opts.ScaleWidth > 0 && opts.ScaleWidth < 1.0 || + opts.ScaleHeight > 0 && opts.ScaleHeight < 1.0 { + return true + } + + w, h := b.Dx(), b.Dy() + if swapDimensions { + w, h = h, w + } + + // Same size, don't rescale. + if opts.MaxWidth == w && opts.MaxHeight == h { + return false + } + return opts.MaxWidth > 0 && opts.MaxWidth < w || + opts.MaxHeight > 0 && opts.MaxHeight < h } func (opts *DecodeOpts) forcedRotate() bool { @@ -318,8 +346,8 @@ func Decode(r io.Reader, opts *DecodeOpts) (image.Image, Config, error) { ex, err := exif.Decode(tr) maybeRescale := func() (image.Image, Config, error) { im, format, err := image.Decode(io.MultiReader(&buf, r)) - if err == nil && opts.wantRescale(im.Bounds()) { - im = rescale(im, opts) + if err == nil && opts.wantRescale(im.Bounds(), false) { + im = rescale(im, opts, false) c.Modified = true } c.Format = format @@ -379,8 +407,15 @@ func Decode(r io.Reader, opts *DecodeOpts) (image.Image, Config, error) { return nil, c, err } rescaled := false - if opts.wantRescale(im.Bounds()) { - im = rescale(im, opts) + // Orientation changing rotations should have their dimensions swapped + // when scaling. + var swapDimensions bool + switch angle { + case 90, -90: + swapDimensions = true + } + if opts.wantRescale(im.Bounds(), swapDimensions) { + im = rescale(im, opts, swapDimensions) rescaled = true } im = flip(rotate(im, angle), flipMode) diff --git a/pkg/images/images_test.go b/pkg/images/images_test.go index f4c830451..1b81c3060 100644 --- a/pkg/images/images_test.go +++ b/pkg/images/images_test.go @@ -21,6 +21,7 @@ import ( "image/jpeg" "os" "path/filepath" + "sort" "strings" "testing" "time" @@ -31,6 +32,9 @@ import ( const datadir = "testdata" func equals(im1, im2 image.Image) bool { + if !im1.Bounds().Eq(im2.Bounds()) { + return false + } for y := 0; y < im1.Bounds().Dy(); y++ { for x := 0; x < im1.Bounds().Dx(); x++ { r1, g1, b1, a1 := im1.At(x, y).RGBA() @@ -79,6 +83,7 @@ func sampleNames(t *testing.T) []string { if err != nil { t.Fatal(err) } + sort.Strings(samples) return samples } @@ -175,8 +180,29 @@ func TestRescale(t *testing.T) { smallIm := smallStraightFImage(t) + gotB, wantB := rescaledIm.Bounds(), smallIm.Bounds() + if !gotB.Eq(wantB) { + t.Errorf("(scale) %v bounds not equal, got %v want %v", name, gotB, wantB) + } if !equals(rescaledIm, smallIm) { - t.Fatalf("%v not properly rescaled", name) + t.Errorf("(scale) %v pixels not equal", name) + } + + _, err = f.Seek(0, os.SEEK_SET) + if err != nil { + t.Fatal(err) + } + + rescaledIm, _, err = Decode(f, &DecodeOpts{MaxWidth: 2000, MaxHeight: 40}) + if err != nil { + t.Fatal(err) + } + gotB = rescaledIm.Bounds() + if !gotB.Eq(wantB) { + t.Errorf("(max) %v bounds not equal, got %v want %v", name, gotB, wantB) + } + if !equals(rescaledIm, smallIm) { + t.Errorf("(max) %v pixels not equal", name) } } @@ -203,8 +229,29 @@ func TestRescaleEXIF(t *testing.T) { t.Fatal(err) } + gotB, wantB := rescaledIm.Bounds(), smallStraightF.Bounds() + if !gotB.Eq(wantB) { + t.Errorf("(scale) %v bounds not equal, got %v want %v", name, gotB, wantB) + } if !equals(rescaledIm, smallStraightF) { - t.Fatalf("%v not properly rescaled", name) + t.Errorf("(scale) %v pixels not equal", name) + } + + _, err = f.Seek(0, os.SEEK_SET) + if err != nil { + t.Fatal(err) + } + rescaledIm, _, err = Decode(f, &DecodeOpts{MaxWidth: 2000, MaxHeight: 40}) + if err != nil { + t.Fatal(err) + } + + gotB = rescaledIm.Bounds() + if !gotB.Eq(wantB) { + t.Errorf("(max) %v bounds not equal, got %v want %v", name, gotB, wantB) + } + if !equals(rescaledIm, smallStraightF) { + t.Errorf("(max) %v pixels not equal", name) } } } diff --git a/pkg/index/receive.go b/pkg/index/receive.go index 37c901f6d..d2faa847f 100644 --- a/pkg/index/receive.go +++ b/pkg/index/receive.go @@ -264,7 +264,10 @@ func (ix *Index) populateFile(b *schema.Blob, mm *mutationMap) (err error) { var copyDest io.Writer = sha1 var imageBuf *keepFirstN // or nil if strings.HasPrefix(mime, "image/") { - imageBuf = &keepFirstN{N: 256 << 10} + // Emperically derived 1MiB assuming CR2 images require more than any + // other filetype we support: + // https://gist.github.com/wathiede/7982372 + imageBuf = &keepFirstN{N: 1 << 20} copyDest = io.MultiWriter(copyDest, imageBuf) } size, err := io.Copy(copyDest, reader)