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

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