Merge "images: fix Decode when resize + rotate + max W/H."

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)
 	}
 }
 

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)

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)
 		}
 	}
 }

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)