EXIF orientation: bake it in when normalizing images (#103) #111
@@ -13,7 +13,7 @@ Work is tracked in Gitea issues; the tracking epic links every piece in dependen
|
|||||||
- **Users:** multi-user with ownership. Users own gardens; a garden can be shared with other users as viewer (read) or editor (edit content). Owner additionally shares/deletes. The first registered user is `is_admin` (set race-free inside the INSERT); admin gates instance-wide Settings (`requireAdmin`), the only thing that reads that flag.
|
- **Users:** multi-user with ownership. Users own gardens; a garden can be shared with other users as viewer (read) or editor (edit content). Owner additionally shares/deletes. The first registered user is `is_admin` (set race-free inside the INSERT); admin gates instance-wide Settings (`requireAdmin`), the only thing that reads that flag.
|
||||||
- **Auth:** OIDC-first (Authentik is the primary IdP), local argon2id passwords as an optional fallback.
|
- **Auth:** OIDC-first (Authentik is the primary IdP), local argon2id passwords as an optional fallback.
|
||||||
- **Instance settings (#79):** admin-editable, instance-wide config in a single-row `instance_settings` table — pansy's first DB-stored *instance* state (everything else hangs off a garden/object). Today it holds the agent model + on/off; **secrets never move here** — `OLLAMA_CLOUD_API_KEY` stays in the env so it doesn't land in backups or the undo history. Precedence: Settings value → env → default. The live agent Runner sits behind an `atomic.Pointer` in the API layer (`agentHolder`) with its routes always registered, so a settings change swaps it with no restart and no race against in-flight requests; `/capabilities` reads that pointer, so it reports what's live rather than what was configured at boot. The model/registry knowledge lives in one leaf package (`internal/agentmodel`) that both the runner and the settings validator import — agent imports service, so it can live in neither.
|
- **Instance settings (#79):** admin-editable, instance-wide config in a single-row `instance_settings` table — pansy's first DB-stored *instance* state (everything else hangs off a garden/object). Today it holds the agent model + on/off; **secrets never move here** — `OLLAMA_CLOUD_API_KEY` stays in the env so it doesn't land in backups or the undo history. Precedence: Settings value → env → default. The live agent Runner sits behind an `atomic.Pointer` in the API layer (`agentHolder`) with its routes always registered, so a settings change swaps it with no restart and no race against in-flight requests; `/capabilities` reads that pointer, so it reports what's live rather than what was configured at boot. The model/registry knowledge lives in one leaf package (`internal/agentmodel`) that both the runner and the settings validator import — agent imports service, so it can live in neither.
|
||||||
- **Seed-packet capture (#81):** photograph a packet → a *vision* model (separate `vision_model` setting) reads it into structured fields via one-shot `majordomo.Generate[SeedPacket]` — NOT an agent loop, so the extraction can't touch the garden; it only reads a picture and returns data. The image is normalized to JPEG at the upload boundary (`internal/imagenorm`: decodes HEIC/webp/png/jpeg, since majordomo's stdlib media path can't do HEIC — the iPhone default). The hard part is **catalog matching, not OCR**: a wrong auto-match splits a variety's seed-lot history across duplicate rows, so the service NEVER auto-creates — it surfaces ranked candidates (`matchPlants`) and the user confirms, then `CreateFromPacket` makes the plant (new or existing) + the lot. Plants/lots aren't in the undo history (they're catalog/inventory), so there's no change set to wrap. The extractor is injectable on the service (`WithPacketExtractor`) so the whole path tests hermetically against majordomo's `fake` provider.
|
- **Seed-packet capture (#81):** photograph a packet → a *vision* model (separate `vision_model` setting) reads it into structured fields via one-shot `majordomo.Generate[SeedPacket]` — NOT an agent loop, so the extraction can't touch the garden; it only reads a picture and returns data. The image is normalized to JPEG at the upload boundary (`internal/imagenorm`: decodes HEIC/webp/png/jpeg, since majordomo's stdlib media path can't do HEIC — the iPhone default; it also bakes in the JPEG EXIF orientation, since the re-encode strips EXIF and a phone photo tagged "rotate 90°" would otherwise reach the model sideways). The hard part is **catalog matching, not OCR**: a wrong auto-match splits a variety's seed-lot history across duplicate rows, so the service NEVER auto-creates — it surfaces ranked candidates (`matchPlants`) and the user confirms, then `CreateFromPacket` makes the plant (new or existing) + the lot. Plants/lots aren't in the undo history (they're catalog/inventory), so there's no change set to wrap. The extractor is injectable on the service (`WithPacketExtractor`) so the whole path tests hermetically against majordomo's `fake` provider.
|
||||||
- **Agentic future:** integration with majordomo/executus via typed Go tools (`llm.DefineTool[Args]`) wrapping the same service layer the REST API uses — not MCP/OpenAPI.
|
- **Agentic future:** integration with majordomo/executus via typed Go tools (`llm.DefineTool[Args]`) wrapping the same service layer the REST API uses — not MCP/OpenAPI.
|
||||||
|
|
||||||
## Domain model
|
## Domain model
|
||||||
|
|||||||
@@ -28,6 +28,7 @@ package imagenorm
|
|||||||
|
|
||||||
import (
|
import (
|
||||||
"bytes"
|
"bytes"
|
||||||
|
"encoding/binary"
|
||||||
"errors"
|
"errors"
|
||||||
"fmt"
|
"fmt"
|
||||||
"image"
|
"image"
|
||||||
@@ -104,9 +105,10 @@ var (
|
|||||||
)
|
)
|
||||||
|
|
||||||
// Normalize reads an image of any supported format (JPEG, PNG, HEIC, WebP),
|
// Normalize reads an image of any supported format (JPEG, PNG, HEIC, WebP),
|
||||||
// downscales it to fit opts.MaxDim on its longest edge, and returns it re-encoded
|
// downscales it to fit opts.MaxDim on its longest edge, applies the JPEG EXIF
|
||||||
// as JPEG, plus the decoded format name (e.g. "heic") — handy for logging what a
|
// orientation so the pixels come out upright, and returns it re-encoded as JPEG,
|
||||||
// phone actually sent. On any error the returned bytes are nil and format is "".
|
// plus the decoded format name (e.g. "heic") — handy for logging what a phone
|
||||||
|
// actually sent. On any error the returned bytes are nil and format is "".
|
||||||
//
|
//
|
||||||
// Errors, by cause:
|
// Errors, by cause:
|
||||||
// - input over opts.MaxBytes, or a decoded canvas over maxDecodePixels /
|
// - input over opts.MaxBytes, or a decoded canvas over maxDecodePixels /
|
||||||
@@ -120,13 +122,18 @@ var (
|
|||||||
// pre-decode pixel/dimension check, and a recover around the third-party decoders
|
// pre-decode pixel/dimension check, and a recover around the third-party decoders
|
||||||
// (a malformed HEIC/WebP shouldn't take the process down).
|
// (a malformed HEIC/WebP shouldn't take the process down).
|
||||||
//
|
//
|
||||||
// Two known gaps, both deferred to the upload handler that wires this in (#81):
|
// EXIF orientation: phone cameras store the sensor pixels in one orientation and
|
||||||
// - EXIF ORIENTATION is not applied, so a portrait phone photo tagged
|
// set an EXIF tag to rotate on display, so a JPEG "portrait" photo is really a
|
||||||
// "rotate 90°" comes out sideways. That's best fixed and tested with a real
|
// landscape bitmap tagged "rotate 90°" — and the re-encode below strips EXIF,
|
||||||
// oriented photo end-to-end, which the library has no consumer for yet.
|
// which is exactly why the rotation must be BAKED IN here. applyOrientation does
|
||||||
// - There is no context: image.Decode is CPU-bound and not cancellable
|
// that for the JPEG path (the format phone uploads overwhelmingly arrive in);
|
||||||
// mid-decode, so a caller that needs a hard deadline should run Normalize
|
// other formats carry no JPEG EXIF and their decoders own orientation, so they're
|
||||||
// under its own timeout. The size guards keep the work finite regardless.
|
// left as decoded.
|
||||||
|
//
|
||||||
|
// One known gap, deferred to the upload handler (#81): there is no context —
|
||||||
|
// image.Decode is CPU-bound and not cancellable mid-decode, so a caller that
|
||||||
|
// needs a hard deadline should run Normalize under its own timeout. The size
|
||||||
|
// guards keep the work finite regardless.
|
||||||
func Normalize(r io.Reader, opts Options) (out []byte, format string, err error) {
|
func Normalize(r io.Reader, opts Options) (out []byte, format string, err error) {
|
||||||
// Cap the read at MaxBytes+1 so we can tell "exactly at the cap" from "over".
|
// Cap the read at MaxBytes+1 so we can tell "exactly at the cap" from "over".
|
||||||
// maxBytes() is always a sane positive (default 25 MiB); guard the +1 anyway.
|
// maxBytes() is always a sane positive (default 25 MiB); guard the +1 anyway.
|
||||||
@@ -161,7 +168,11 @@ func Normalize(r io.Reader, opts Options) (out []byte, format string, err error)
|
|||||||
return nil, "", err
|
return nil, "", err
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Downscale first (cheaper to rotate the small image), then bake in the EXIF
|
||||||
|
// orientation so the JPEG we emit is upright. A 90° rotation swaps the sides
|
||||||
|
// but not the longest edge, so the downscale bound still holds after it.
|
||||||
img = downscale(img, opts.maxDim())
|
img = downscale(img, opts.maxDim())
|
||||||
|
img = applyOrientation(img, exifOrientation(raw))
|
||||||
|
|
||||||
var buf bytes.Buffer
|
var buf bytes.Buffer
|
||||||
if err := jpeg.Encode(&buf, img, &jpeg.Options{Quality: jpegQuality}); err != nil {
|
if err := jpeg.Encode(&buf, img, &jpeg.Options{Quality: jpegQuality}); err != nil {
|
||||||
@@ -188,6 +199,155 @@ func decodeSafely(raw []byte) (img image.Image, format string, err error) {
|
|||||||
return img, format, nil
|
return img, format, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// 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. 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
|
||||||
|
}
|
||||||
|
// 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
|
||||||
|
|
|||||||
|
// skipped downscale decodes to YCbCr).
|
||||||
|
src, ok := img.(*image.RGBA)
|
||||||
|
if !ok {
|
||||||
|
b := img.Bounds()
|
||||||
|
gitea-actions
commented
🟠 Per-pixel img.At() interface call boxes color.Color, causing millions of heap allocations on rotation maintainability, performance · flagged by 2 models
🪰 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 {
|
||||||
|
var dx, dy int
|
||||||
|
switch o {
|
||||||
|
case 2: // mirror horizontal
|
||||||
|
dx, dy = w-1-x, y
|
||||||
|
case 3: // rotate 180
|
||||||
|
dx, dy = w-1-x, h-1-y
|
||||||
|
case 4: // mirror vertical
|
||||||
|
dx, dy = x, h-1-y
|
||||||
|
case 5: // transpose (mirror across the main diagonal)
|
||||||
|
dx, dy = y, x
|
||||||
|
case 6: // rotate 90° clockwise
|
||||||
|
dx, dy = h-1-y, x
|
||||||
|
case 7: // transverse (mirror across the anti-diagonal)
|
||||||
|
dx, dy = h-1-y, w-1-x
|
||||||
|
case 8: // rotate 90° counter-clockwise
|
||||||
|
dx, dy = y, w-1-x
|
||||||
|
}
|
||||||
|
gitea-actions
commented
🟡 JPEG fill bytes between markers not skipped in exifOrientation error-handling · flagged by 1 model
🪰 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>
|
|||||||
|
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
|
||||||
|
}
|
||||||
|
gitea-actions
commented
🟠 JPEG marker padding bytes not skipped, causing EXIF miss on spec-valid JPEGs error-handling · flagged by 1 model
🪰 Gadfly · advisory 🟠 **JPEG marker padding bytes not skipped, causing EXIF miss on spec-valid JPEGs**
_error-handling · flagged by 1 model_
- **`internal/imagenorm/imagenorm.go:256` — JPEG marker-padding edge case unhandled.** The JPEG spec allows any number of fill bytes (`0xFF`) to precede a marker. `exifOrientation` assumes exactly one `0xFF` followed immediately by the marker code, so a file like `FF D8 FF FF E1 …` (valid per spec) misaligns: `marker` becomes `0xFF`, `segLen` is read from the next two bytes (`0xE1 0x00` ≈ 57 KiB), the length check fails, and the parser bails out returning `1` (normal). For a portrait photo whose…
<sub>🪰 Gadfly · advisory</sub>
|
|||||||
|
|
||||||
|
// exifOrientation extracts the EXIF Orientation tag (1..8) from raw image bytes,
|
||||||
|
// returning 1 (normal) when it's absent or unparseable — the safe default, since
|
||||||
|
// a wrong guess rotates a correct image. Only the JPEG APP1/Exif path is parsed:
|
||||||
|
// that's the format uploaded phone photos overwhelmingly arrive in, and the other
|
||||||
|
// decoders own their own orientation.
|
||||||
|
func exifOrientation(raw []byte) int {
|
||||||
|
// A JPEG is a run of FFxx marker segments after the SOI (FFD8). Walk them
|
||||||
|
// looking for APP1 (FFE1) carrying "Exif\0\0"; stop at the scan data (SOS).
|
||||||
|
if len(raw) < 4 || raw[0] != 0xFF || raw[1] != 0xD8 {
|
||||||
|
return 1
|
||||||
|
}
|
||||||
|
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
|
||||||
|
}
|
||||||
|
gitea-actions
commented
🟠 Missing TIFF magic number validation in orientationFromApp1 error-handling · flagged by 1 model
🪰 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
|
||||||
|
}
|
||||||
|
if marker == 0xE1 {
|
||||||
|
if o, ok := orientationFromApp1(raw[i+4 : i+2+segLen]); ok {
|
||||||
|
return o
|
||||||
|
}
|
||||||
|
}
|
||||||
|
i += 2 + segLen
|
||||||
|
}
|
||||||
|
return 1
|
||||||
|
}
|
||||||
|
|
||||||
|
// orientationFromApp1 reads the Orientation tag from a JPEG APP1 segment body
|
||||||
|
// (everything after the 2-byte length): "Exif\0\0" then a TIFF block holding
|
||||||
|
// IFD0. Returns (0, false) if the segment isn't Exif or the tag is missing.
|
||||||
|
func orientationFromApp1(seg []byte) (int, bool) {
|
||||||
|
gitea-actions
commented
🟠 EXIF orientation parser does not validate IFD type/count before reading value correctness, error-handling · flagged by 2 models
🪰 Gadfly · advisory 🟠 **EXIF orientation parser does not validate IFD type/count before reading value**
_correctness, error-handling · flagged by 2 models_
- **`internal/imagenorm/imagenorm.go:305-309`** — `orientationFromApp1` reads the Orientation value without validating the IFD entry's **type** and **count**. The EXIF spec requires Orientation to be `SHORT` (type `3`) with count `1`. A malformed entry with a different type (e.g. `LONG`) or count `>1` stores an *offset* in the value field, not the value itself. The current code reads the first two bytes of that offset as an orientation number, which can return a bogus rotation (violating the "sa…
<sub>🪰 Gadfly · advisory</sub>
|
|||||||
|
const prefix = "Exif\x00\x00"
|
||||||
|
if len(seg) < len(prefix)+8 || string(seg[:len(prefix)]) != prefix {
|
||||||
|
return 0, false
|
||||||
|
}
|
||||||
|
tiff := seg[len(prefix):]
|
||||||
|
var bo binary.ByteOrder
|
||||||
|
switch string(tiff[0:2]) {
|
||||||
|
case "II":
|
||||||
|
bo = binary.LittleEndian
|
||||||
|
case "MM":
|
||||||
|
bo = binary.BigEndian
|
||||||
|
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
|
||||||
|
}
|
||||||
|
n := int(bo.Uint16(tiff[ifd : ifd+2]))
|
||||||
|
for k := range n {
|
||||||
|
off := ifd + 2 + k*12 // each IFD entry is 12 bytes
|
||||||
|
if off+12 > len(tiff) {
|
||||||
|
return 0, false
|
||||||
|
}
|
||||||
|
if bo.Uint16(tiff[off:off+2]) != 0x0112 { // Orientation tag
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
// 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
|
||||||
|
}
|
||||||
|
return 0, false
|
||||||
|
}
|
||||||
|
return 0, false
|
||||||
|
}
|
||||||
|
|
||||||
// downscale returns img shrunk so its longest edge is at most maxDim, preserving
|
// downscale returns img shrunk so its longest edge is at most maxDim, preserving
|
||||||
// aspect ratio. An image already within bounds is returned unchanged (no
|
// aspect ratio. An image already within bounds is returned unchanged (no
|
||||||
// re-sampling, no quality loss beyond the JPEG round-trip). Uses Catmull-Rom for
|
// re-sampling, no quality loss beyond the JPEG round-trip). Uses Catmull-Rom for
|
||||||
|
|||||||
@@ -5,6 +5,7 @@ import (
|
|||||||
"encoding/binary"
|
"encoding/binary"
|
||||||
"hash/crc32"
|
"hash/crc32"
|
||||||
"image"
|
"image"
|
||||||
|
"image/color"
|
||||||
"image/jpeg"
|
"image/jpeg"
|
||||||
"image/png"
|
"image/png"
|
||||||
"os"
|
"os"
|
||||||
@@ -200,3 +201,126 @@ func TestNormalizeRejectsPixelBomb(t *testing.T) {
|
|||||||
})
|
})
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// orientedJPEG builds a JPEG whose top-left quadrant is white and the rest black
|
||||||
|
// — a marker to track through a rotation — tagged with the given EXIF orientation
|
||||||
|
// (1..8). The marker lets a test assert the pixels actually moved to where that
|
||||||
|
// orientation says they should.
|
||||||
|
func orientedJPEG(t *testing.T, orient int) []byte {
|
||||||
|
t.Helper()
|
||||||
|
const w, h = 40, 24
|
||||||
|
m := image.NewRGBA(image.Rect(0, 0, w, h))
|
||||||
|
for y := range h {
|
||||||
|
for x := range w {
|
||||||
|
c := color.RGBA{0, 0, 0, 255}
|
||||||
|
if x < w/2 && y < h/2 {
|
||||||
|
c = color.RGBA{255, 255, 255, 255}
|
||||||
|
}
|
||||||
|
m.Set(x, y, c)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
var jb bytes.Buffer
|
||||||
|
if err := jpeg.Encode(&jb, m, &jpeg.Options{Quality: 95}); err != nil {
|
||||||
|
t.Fatalf("encode jpeg: %v", err)
|
||||||
|
}
|
||||||
|
if orient == 0 {
|
||||||
|
return jb.Bytes() // caller wants a plain JPEG with no EXIF
|
||||||
|
}
|
||||||
|
return spliceExifOrientation(t, jb.Bytes(), orient)
|
||||||
|
}
|
||||||
|
|
||||||
|
// spliceExifOrientation inserts a minimal little-endian Exif APP1 segment
|
||||||
|
// carrying just the Orientation tag right after the JPEG SOI marker.
|
||||||
|
func spliceExifOrientation(t *testing.T, jpg []byte, orient int) []byte {
|
||||||
|
t.Helper()
|
||||||
|
var tiff bytes.Buffer
|
||||||
|
tiff.WriteString("II") // little-endian
|
||||||
|
_ = binary.Write(&tiff, binary.LittleEndian, uint16(0x2A))
|
||||||
|
_ = binary.Write(&tiff, binary.LittleEndian, uint32(8)) // IFD0 offset
|
||||||
|
_ = binary.Write(&tiff, binary.LittleEndian, uint16(1)) // one entry
|
||||||
|
_ = binary.Write(&tiff, binary.LittleEndian, uint16(0x0112))
|
||||||
|
_ = binary.Write(&tiff, binary.LittleEndian, uint16(3)) // SHORT
|
||||||
|
_ = binary.Write(&tiff, binary.LittleEndian, uint32(1)) // count
|
||||||
|
_ = binary.Write(&tiff, binary.LittleEndian, uint16(orient)) // value
|
||||||
|
_ = binary.Write(&tiff, binary.LittleEndian, uint16(0)) // value pad
|
||||||
|
_ = binary.Write(&tiff, binary.LittleEndian, uint32(0)) // next IFD
|
||||||
|
payload := append([]byte("Exif\x00\x00"), tiff.Bytes()...)
|
||||||
|
|
||||||
|
segLen := len(payload) + 2
|
||||||
|
seg := []byte{0xFF, 0xE1, byte(segLen >> 8), byte(segLen)}
|
||||||
|
seg = append(seg, payload...)
|
||||||
|
|
||||||
|
out := make([]byte, 0, len(jpg)+len(seg))
|
||||||
|
out = append(out, jpg[:2]...) // SOI
|
||||||
|
out = append(out, seg...)
|
||||||
|
return append(out, jpg[2:]...)
|
||||||
|
}
|
||||||
|
|
||||||
|
func bright(c color.Color) bool {
|
||||||
|
r, g, b, _ := c.RGBA() // 16-bit
|
||||||
|
return (r+g+b)/3 > 0x8000
|
||||||
|
}
|
||||||
|
|
||||||
|
// TestNormalizeAppliesExifOrientation is the #103 regression: a phone photo tagged
|
||||||
|
// "rotate 90°" must come out of Normalize with the pixels upright, not sideways —
|
||||||
|
// the re-encode strips EXIF, so the rotation has to be baked into the bitmap.
|
||||||
|
func TestNormalizeAppliesExifOrientation(t *testing.T) {
|
||||||
|
// The white marker starts centred at (10,6) in the 40x24 source. For each
|
||||||
|
// 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 {
|
||||||
|
gitea-actions
commented
🟠 Missing test coverage for mirror/transpose orientations 2/4/5/7 maintainability · flagged by 1 model
🪰 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
|
||||||
|
}{
|
||||||
|
gitea-actions
commented
🟠 No test covers orientations 2/4/5/7 (mirrors + transpose/transverse), the most error-prone switch cases maintainability · flagged by 1 model
🪰 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>
|
|||||||
|
{"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)
|
||||||
|
gitea-actions
commented
🟡 table test missing t.Run subtests inconsistent with file pattern maintainability · flagged by 1 model
🪰 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 {
|
||||||
|
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)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// TestExifOrientationParsing pins the parser against non-JPEG and no-EXIF inputs,
|
||||||
|
// which must default to 1 (never guess a rotation onto a correct image).
|
||||||
|
func TestExifOrientationParsing(t *testing.T) {
|
||||||
|
if o := exifOrientation(pngBytes(t, 8, 8)); o != 1 {
|
||||||
|
t.Errorf("PNG orientation = %d, want 1 (no JPEG EXIF path)", o)
|
||||||
|
}
|
||||||
|
if o := exifOrientation(orientedJPEG(t, 0)); o != 1 {
|
||||||
|
t.Errorf("JPEG without EXIF orientation = %d, want 1", o)
|
||||||
|
}
|
||||||
|
if o := exifOrientation(orientedJPEG(t, 6)); o != 6 {
|
||||||
|
t.Errorf("JPEG tagged 6 → %d, want 6", o)
|
||||||
|
}
|
||||||
|
if o := exifOrientation([]byte("not an image")); o != 1 {
|
||||||
|
t.Errorf("garbage bytes → %d, want 1", o)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
🔴 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.applyOrientationalways allocatesimage.NewRGBA(image.Rect(0, 0, w, h)), then immediately throws it away and allocatesimage.NewRGBA(image.Rect(0, 0, h, w))whenswapis 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 simpleif/elseso only the correctly-sized buffer is ever…🪰 Gadfly · advisory