EXIF orientation: bake it in when normalizing images (#103) #111

Merged
steve merged 2 commits from feat/exif-orientation into main 2026-07-22 05:35:46 +00:00
2 changed files with 75 additions and 36 deletions
Showing only changes of commit 8aad2278a0 - Show all commits
+45 -12
View File
@@ -202,22 +202,34 @@ func decodeSafely(raw []byte) (img image.Image, format string, err error) {
// applyOrientation returns img with the EXIF orientation (1..8) baked in, so the
// pixels are upright and no display-time rotation is needed. Orientation 1 (and
// anything out of range) is a no-op. Values 5..8 are 90° rotations, which swap
// the output's width and height. Reads through image.Image.At and writes an RGBA,
// so it works whatever concrete type decode/downscale produced.
// the output's width and height. Copies raw RGBA pixels by byte offset (after a
// one-time conversion if the source isn't already RGBA), so a full-resolution
// rotation doesn't box a color.Color per pixel.
func applyOrientation(img image.Image, o int) image.Image {
if o <= 1 || o > 8 {
return img
}
b := img.Bounds()
w, h := b.Dx(), b.Dy()
swap := o >= 5 // a quarter-turn: output dimensions transpose
dst := image.NewRGBA(image.Rect(0, 0, w, h))
if swap {
dst = image.NewRGBA(image.Rect(0, 0, h, w))
// Work on a concrete RGBA so the transform is a 4-byte copy per pixel rather
// than millions of boxed color.Color values through At/Set. downscale usually
// hands us an *image.RGBA already; convert once if not (e.g. a small JPEG that
Outdated
Review

🔴 Double image allocation on 90° rotations: first RGBA buffer immediately discarded

maintainability, performance · flagged by 4 models

  • internal/imagenorm/imagenorm.go:214-216 — Double image allocation on every 90° rotation. applyOrientation always allocates image.NewRGBA(image.Rect(0, 0, w, h)), then immediately throws it away and allocates image.NewRGBA(image.Rect(0, 0, h, w)) when swap is true (orientations 5–8). For a 2048×1536 image at the downscale bound that wastes ~12 MB of zero-use heap; at max square dimensions it wastes ~16.7 MB. The fix is a simple if/else so only the correctly-sized buffer is ever…

🪰 Gadfly · advisory

🔴 **Double image allocation on 90° rotations: first RGBA buffer immediately discarded** _maintainability, performance · flagged by 4 models_ - **`internal/imagenorm/imagenorm.go:214-216` — Double image allocation on every 90° rotation.** `applyOrientation` always allocates `image.NewRGBA(image.Rect(0, 0, w, h))`, then immediately throws it away and allocates `image.NewRGBA(image.Rect(0, 0, h, w))` when `swap` is true (orientations 5–8). For a 2048×1536 image at the downscale bound that wastes ~12 MB of zero-use heap; at max square dimensions it wastes ~16.7 MB. The fix is a simple `if/else` so only the correctly-sized buffer is ever… <sub>🪰 Gadfly · advisory</sub>
// skipped downscale decodes to YCbCr).
src, ok := img.(*image.RGBA)
if !ok {
b := img.Bounds()
Outdated
Review

🟠 Per-pixel img.At() interface call boxes color.Color, causing millions of heap allocations on rotation

maintainability, performance · flagged by 2 models

  • internal/imagenorm/imagenorm.go:218-239 — manual At/Set per-pixel loop diverges from the draw-package idiom used elsewhere in the file. applyOrientation walks every pixel with img.At/dst.Set (going through the color.Model conversion path), while downscale (line 332) uses draw.CatmullRom.Scale. For an exact remap (rotations/mirrors) point sampling is correct, but the manual nested loop is stylistically inconsistent with the rest of the file and the conversion overhead is…

🪰 Gadfly · advisory

🟠 **Per-pixel img.At() interface call boxes color.Color, causing millions of heap allocations on rotation** _maintainability, performance · flagged by 2 models_ - **`internal/imagenorm/imagenorm.go:218-239` — manual `At`/`Set` per-pixel loop diverges from the `draw`-package idiom used elsewhere in the file.** `applyOrientation` walks every pixel with `img.At`/`dst.Set` (going through the `color.Model` conversion path), while `downscale` (line 332) uses `draw.CatmullRom.Scale`. For an exact remap (rotations/mirrors) point sampling is correct, but the manual nested loop is stylistically inconsistent with the rest of the file and the conversion overhead is… <sub>🪰 Gadfly · advisory</sub>
conv := image.NewRGBA(image.Rect(0, 0, b.Dx(), b.Dy()))
draw.Draw(conv, conv.Bounds(), img, b.Min, draw.Src)
src = conv
}
sb := src.Bounds()
w, h := sb.Dx(), sb.Dy()
// A quarter-turn (5..8) transposes the output; size the one buffer accordingly.
dw, dh := w, h
if o >= 5 {
dw, dh = h, w
}
dst := image.NewRGBA(image.Rect(0, 0, dw, dh))
for y := range h {
for x := range w {
c := img.At(b.Min.X+x, b.Min.Y+y)
var dx, dy int
switch o {
case 2: // mirror horizontal
@@ -235,7 +247,9 @@ func applyOrientation(img image.Image, o int) image.Image {
case 8: // rotate 90° counter-clockwise
dx, dy = y, w-1-x
}
Review

🟡 JPEG fill bytes between markers not skipped in exifOrientation

error-handling · flagged by 1 model

  • internal/imagenorm/imagenorm.go:249-275 — JPEG fill bytes (0xFF) between markers are not skipped The JPEG spec allows any number of fill bytes (0xFF) to precede a marker. The parser at exifOrientation assumes each marker is immediately preceded by exactly one 0xFF byte. If a phone encoder emits padding 0xFF bytes before the APP1 segment, the loop misreads the marker and returns orientation 1 instead of the true value. This is safe (no wrong rotation) but incorrect for a valid…

🪰 Gadfly · advisory

🟡 **JPEG fill bytes between markers not skipped in exifOrientation** _error-handling · flagged by 1 model_ - **`internal/imagenorm/imagenorm.go:249-275` — JPEG fill bytes (`0xFF`) between markers are not skipped** The JPEG spec allows any number of fill bytes (`0xFF`) to precede a marker. The parser at `exifOrientation` assumes each marker is immediately preceded by exactly one `0xFF` byte. If a phone encoder emits padding `0xFF` bytes before the APP1 segment, the loop misreads the marker and returns orientation `1` instead of the true value. This is safe (no wrong rotation) but incorrect for a valid… <sub>🪰 Gadfly · advisory</sub>
dst.Set(dx, dy, c)
si := src.PixOffset(sb.Min.X+x, sb.Min.Y+y)
di := dst.PixOffset(dx, dy)
copy(dst.Pix[di:di+4], src.Pix[si:si+4])
}
}
return dst
1
@@ -252,14 +266,25 @@ func exifOrientation(raw []byte) int {
if len(raw) < 4 || raw[0] != 0xFF || raw[1] != 0xD8 {
return 1
}
for i := 2; i+4 <= len(raw); {
for i := 2; i+1 < len(raw); {
if raw[i] != 0xFF {
return 1 // not aligned on a marker; give up rather than misread
}
// A marker may be preceded by any number of 0xFF fill bytes (JPEG spec);
// skip them so a padded APP1 isn't misread as a marker of value 0xFF.
for i+1 < len(raw) && raw[i+1] == 0xFF {
i++
}
if i+1 >= len(raw) {
return 1
}
Outdated
Review

🟠 Missing TIFF magic number validation in orientationFromApp1

error-handling · flagged by 1 model

  • internal/imagenorm/imagenorm.go:280 — Missing TIFF magic number validation in orientationFromApp1 The function trusts the IFD offset at tiff[4:8] without first checking that bytes tiff[2:4] contain the required TIFF magic number 42 (0x002A little-endian / 0x2A00 big-endian). A malformed APP1 segment that passes the "Exif\x00\x00" prefix check but has arbitrary bytes in the rest of the TIFF header can direct the parser to a bounds-checked but wrong IFD location, potentially…

🪰 Gadfly · advisory

🟠 **Missing TIFF magic number validation in orientationFromApp1** _error-handling · flagged by 1 model_ - **`internal/imagenorm/imagenorm.go:280` — Missing TIFF magic number validation in `orientationFromApp1`** The function trusts the IFD offset at `tiff[4:8]` without first checking that bytes `tiff[2:4]` contain the required TIFF magic number `42` (`0x002A` little-endian / `0x2A00` big-endian). A malformed APP1 segment that passes the `"Exif\x00\x00"` prefix check but has arbitrary bytes in the rest of the TIFF header can direct the parser to a bounds-checked but wrong IFD location, potentially… <sub>🪰 Gadfly · advisory</sub>
marker := raw[i+1]
if marker == 0xD9 || marker == 0xDA {
return 1 // EOI / start-of-scan: no more headers to read
}
if i+4 > len(raw) {
return 1
}
segLen := int(raw[i+2])<<8 | int(raw[i+3])
if segLen < 2 || i+2+segLen > len(raw) {
return 1
1
@@ -292,6 +317,9 @@ func orientationFromApp1(seg []byte) (int, bool) {
default:
return 0, false
}
if bo.Uint16(tiff[2:4]) != 0x2A { // TIFF magic (42); byte order must agree
return 0, false
}
ifd := int(bo.Uint32(tiff[4:8])) // offset to IFD0 from the TIFF start
if ifd < 8 || ifd+2 > len(tiff) {
return 0, false
@@ -305,7 +333,12 @@ func orientationFromApp1(seg []byte) (int, bool) {
if bo.Uint16(tiff[off:off+2]) != 0x0112 { // Orientation tag
continue
}
// SHORT value lives in the first 2 bytes of the entry's value field.
// Orientation is defined as a single SHORT, whose value sits inline in the
// first 2 bytes of the value field. Reject anything else rather than read a
// mistyped entry (a LONG/offset there would be a different number entirely).
if bo.Uint16(tiff[off+2:off+4]) != 3 || bo.Uint32(tiff[off+4:off+8]) != 1 {
return 0, false
}
v := int(bo.Uint16(tiff[off+8 : off+10]))
if v >= 1 && v <= 8 {
return v, true
+30 -24
View File
@@ -269,36 +269,42 @@ func TestNormalizeAppliesExifOrientation(t *testing.T) {
// orientation, wantW/H is the corrected canvas and (mx,my) is where that
// marker must land — derived from the same transform Normalize applies.
cases := []struct {
Review

🟠 Missing test coverage for mirror/transpose orientations 2/4/5/7

maintainability · flagged by 1 model

  • internal/imagenorm/imagenorm_test.go:271-281 — The regression test only exercises 5 of the 8 orientations (0,1,3,6,8). Mirror/transpose cases (2, 4, 5, 7) are implemented in production code but completely untested, so a future refactor could silently break them. Add test cases for the missing orientations.

🪰 Gadfly · advisory

🟠 **Missing test coverage for mirror/transpose orientations 2/4/5/7** _maintainability · flagged by 1 model_ - `internal/imagenorm/imagenorm_test.go:271-281` — The regression test only exercises 5 of the 8 orientations (0,1,3,6,8). Mirror/transpose cases (2, 4, 5, 7) are implemented in production code but completely untested, so a future refactor could silently break them. Add test cases for the missing orientations. <sub>🪰 Gadfly · advisory</sub>
name string
orient int
wantW, wantH int
mx, my int
}{
Review

🟠 No test covers orientations 2/4/5/7 (mirrors + transpose/transverse), the most error-prone switch cases

maintainability · flagged by 1 model

  • internal/imagenorm/imagenorm_test.go:276-280 — no test covers orientations 2, 4, 5, 7. TestNormalizeAppliesExifOrientation only exercises 0/1/3/6/8, and TestExifOrientationParsing only parses 6. The applyOrientation switch (lines 222–237) handles all of 2–8, and the mirror/transpose/transverse variants (2, 4, 5, 7) are the most error-prone cases — a future refactor of the switch would not be caught for half the orientations. Adding cases for 2/4/5/7 would pin the transpose/transver…

🪰 Gadfly · advisory

🟠 **No test covers orientations 2/4/5/7 (mirrors + transpose/transverse), the most error-prone switch cases** _maintainability · flagged by 1 model_ - **`internal/imagenorm/imagenorm_test.go:276-280` — no test covers orientations 2, 4, 5, 7.** `TestNormalizeAppliesExifOrientation` only exercises 0/1/3/6/8, and `TestExifOrientationParsing` only parses 6. The `applyOrientation` switch (lines 222–237) handles all of 2–8, and the mirror/transpose/transverse variants (2, 4, 5, 7) are the most error-prone cases — a future refactor of the switch would not be caught for half the orientations. Adding cases for 2/4/5/7 would pin the transpose/transver… <sub>🪰 Gadfly · advisory</sub>
{0, 40, 24, 10, 6}, // no EXIF → unchanged, marker top-left
{1, 40, 24, 10, 6}, // normal → unchanged
{3, 40, 24, 29, 17}, // 180 → bottom-right
{6, 24, 40, 17, 10}, // 90° CW → top-right (dims swap)
{8, 24, 40, 6, 29}, // 90° CCW → bottom-left (dims swap)
{"none", 0, 40, 24, 10, 6}, // no EXIF → unchanged, marker top-left
{"normal", 1, 40, 24, 10, 6}, // normal → unchanged
{"mirror-h", 2, 40, 24, 29, 6}, // flip horizontal → top-right
{"rotate-180", 3, 40, 24, 29, 17}, // → bottom-right
{"mirror-v", 4, 40, 24, 10, 17}, // flip vertical → bottom-left
{"transpose", 5, 24, 40, 6, 10}, // main diagonal (dims swap)
Outdated
Review

🟡 table test missing t.Run subtests inconsistent with file pattern

maintainability · flagged by 1 model

  • internal/imagenorm/imagenorm.go:214applyOrientation allocates dst with dimensions w×h, then immediately throws it away and allocates again with h×w whenever swap is true (orientations 5–8). This wastes an allocation of w*h*4 bytes for every quarter-turn. Move the allocation into an if/else so only the correct size is created. - internal/imagenorm/imagenorm_test.go:282TestNormalizeAppliesExifOrientation iterates over a table of cases but doesn't wrap them in `t…

🪰 Gadfly · advisory

🟡 **table test missing t.Run subtests inconsistent with file pattern** _maintainability · flagged by 1 model_ - **`internal/imagenorm/imagenorm.go:214`** — `applyOrientation` allocates `dst` with dimensions `w×h`, then immediately throws it away and allocates again with `h×w` whenever `swap` is true (orientations 5–8). This wastes an allocation of `w*h*4` bytes for every quarter-turn. Move the allocation into an `if/else` so only the correct size is created. - **`internal/imagenorm/imagenorm_test.go:282`** — `TestNormalizeAppliesExifOrientation` iterates over a table of cases but doesn't wrap them in `t… <sub>🪰 Gadfly · advisory</sub>
{"rotate-90-cw", 6, 24, 40, 17, 10}, // → top-right (dims swap)
{"transverse", 7, 24, 40, 17, 29}, // anti-diagonal (dims swap)
{"rotate-90-ccw", 8, 24, 40, 6, 29}, // → bottom-left (dims swap)
}
for _, tc := range cases {
out, format, err := Normalize(bytes.NewReader(orientedJPEG(t, tc.orient)), Options{})
if err != nil {
t.Fatalf("orient %d: Normalize err = %v", tc.orient, err)
}
if format != "jpeg" {
t.Errorf("orient %d: format = %q, want jpeg", tc.orient, format)
}
m, _, err := image.Decode(bytes.NewReader(out))
if err != nil {
t.Fatalf("orient %d: decode output: %v", tc.orient, err)
}
if m.Bounds().Dx() != tc.wantW || m.Bounds().Dy() != tc.wantH {
t.Errorf("orient %d: output %dx%d, want %dx%d",
tc.orient, m.Bounds().Dx(), m.Bounds().Dy(), tc.wantW, tc.wantH)
}
if !bright(m.At(m.Bounds().Min.X+tc.mx, m.Bounds().Min.Y+tc.my)) {
t.Errorf("orient %d: white marker not at (%d,%d) — orientation not applied",
tc.orient, tc.mx, tc.my)
}
t.Run(tc.name, func(t *testing.T) {
out, format, err := Normalize(bytes.NewReader(orientedJPEG(t, tc.orient)), Options{})
if err != nil {
t.Fatalf("Normalize err = %v", err)
}
if format != "jpeg" {
t.Errorf("format = %q, want jpeg", format)
}
m, _, err := image.Decode(bytes.NewReader(out))
if err != nil {
t.Fatalf("decode output: %v", err)
}
if m.Bounds().Dx() != tc.wantW || m.Bounds().Dy() != tc.wantH {
t.Errorf("output %dx%d, want %dx%d",
m.Bounds().Dx(), m.Bounds().Dy(), tc.wantW, tc.wantH)
}
if !bright(m.At(m.Bounds().Min.X+tc.mx, m.Bounds().Min.Y+tc.my)) {
t.Errorf("white marker not at (%d,%d) — orientation not applied", tc.mx, tc.my)
}
})
}
}