From f88c1dd974d6ed19496a082f27105edb73dd4492 Mon Sep 17 00:00:00 2001 From: Major Date: Wed, 29 Jul 2026 10:26:22 +0200 Subject: [PATCH 1/2] Embed face regions in videos instead of API re-linking (issue #30) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The v1.0.4 fix preserved video people by re-creating them over POST /faces after re-upload. That is Immich-internal only and never touches the file. Nurgak's follow-up ("faces still not embedded in videos") is exactly that: nothing lands in the video file. Immich's metadata face importer actually reads RegionInfo from a video container (verified live), so videos can embed the same MWG regions as images and re-import them when "Import faces from metadata" is on — the same round trip photos already rely on, and portable (survives sharing, readable by digiKam). - HasFaceRegionsToEmbed now accepts mp4/mov/m4v; other containers skip - regionOrientation reads the video Rotation tag (no EXIF Orientation) and maps 0/90/270 to orientation 1/6/8, feeding the existing rasterRegion inverse; 180/non-cardinal is left un-embedded (Immich does not re-orient 180 video regions, verified live) rather than misplace a box - removes the API re-link path (recreateFaces, CreateFace, the finalizeReplacement hook, the Faces uploader field) so the two mechanisms cannot double-create faces --- ARCHITECTURE.md | 2 +- README.md | 7 +- src/api/faces.go | 26 ----- src/api/faces_test.go | 42 -------- src/main.go | 2 +- src/model/people.go | 21 +--- src/model/people_test.go | 15 ++- src/process/faceOrientation_test.go | 50 ++++++++++ src/process/faces.go | 142 ++++++++------------------ src/process/pipeline.go | 2 +- src/process/recreateFaces_test.go | 148 ---------------------------- src/process/uploader.go | 19 ---- src/process/uploader_test.go | 121 ----------------------- src/utils.go | 2 +- 14 files changed, 115 insertions(+), 484 deletions(-) create mode 100644 src/process/faceOrientation_test.go delete mode 100644 src/process/recreateFaces_test.go diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 0d5d2c3..ef693d5 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -23,7 +23,7 @@ With `-faces`, the asset's face boxes are fetched from `GET /api/faces` (the v3 Two mechanisms keep the round trip stable. First, faces with `sourceType: exif` are echoes of the file's own regions (the server's metadata import); they are dropped whenever a detected face carries the same normalized name, because counting both would grow the region set by one on every replace. The match is by name rather than person ID since names are how the importer links regions back to people — with two person records sharing a name, the echo can land on the record recognition did not pick. Echoes still count when they are a name's only face (server without ML). Second, the coordinate comparison widens its per-axis tolerance to two raster pixels: Immich's importer floors region corners to whole pixels, which on small images shifts a round-tripped coordinate by more than the base tolerance. Face edits also get their own pre-upload freshness check (mirroring the `updatedAt` one, which face operations do not bump): the boxes are re-fetched right before upload and the asset is skipped if they moved. -Videos take a different path: MWG regions are an image-only container concept, and Immich re-detects faces on a re-uploaded video's thumbnail but drops the person link (the boxes come back as fresh unnamed clusters). So instead of embedding metadata, the replace flow re-links people over the API — after the new asset exists, every face on the old asset that carries a person is recreated on the new one with `POST /faces` (`recreateFaces` in process/faces.go, run from `finalizeReplacement` before the old asset is trashed, where the source links still live). Boxes are copied verbatim from `GET /api/faces` — no orientation math, since both sides speak the same pixel space Immich reported. People already linked on the target are skipped so repeated runs do not stack duplicates, and unnamed clusters are preserved too (createFace needs a person, so only faceless boxes are dropped). The endpoint arrived with manual face tagging in Immich 1.127, and faces it creates are `sourceType: manual` (they survive later detection jobs since 1.127); a `404` from it means the server is older, and the step is skipped with a warning rather than failing the replace. +Videos embed the same MWG regions, through the same `BuildFaceRegions` path — Immich's metadata importer reads `RegionInfo` from a video container just as it does from an image (verified against a live server), so a re-uploaded video's people survive exactly like a photo's. The one difference is the orientation source: a video has no EXIF `Orientation`, so `regionOrientation` reads the QuickTime `Rotation` tag and maps it to the equivalent orientation value (`videoRotationToOrientation`: 0°→1, 90°→6, 270°→8) that feeds the same `rasterRegion` inverse. A 180° or non-cardinal rotation returns not-anchorable and the video's regions are skipped: Immich was observed to re-orient 90°/270° video regions on import but not 180°, so writing a 180° region would misplace the box. Only `mp4`/`mov`/`m4v` are eligible (`SupportsVideoMetadataEmbedding`); other containers exiftool cannot write are skipped. External-library assets (`libraryId` set; null means internal) are skipped on replace runs: API uploads always land in the internal library, so a replacement would migrate the asset and duplicate it at the next library scan. Read-only modes (dry-run, export) still process them. The `libraryId` semantics hold on every server the client can address: the plural `/api/assets` routes and nullable `libraryId` both arrived in Immich 1.106. The practical floors are higher anyway — `/api/assets/copy` (used by every replace that yields a new ID) first appears around v2.2, and `/api/server/about` (auto-detection) in 1.113 — so a set `libraryId` reliably means external wherever the tool can operate. diff --git a/README.md b/README.md index 4fec97c..37463a7 100644 --- a/README.md +++ b/README.md @@ -143,7 +143,6 @@ On Immich 1.113+ you can scope the API key to exactly what the tool needs (older | `asset.delete` | Trash the old original after a verified replacement | | `album.read` | Resolve `-album` / `-album all` selections | | `face.read` | Fetch face boxes — only needed with `-faces` | -| `face.create` | Re-link people on a re-uploaded video — only with `-faces` | Read-only modes need less: `-dry-run` and `-export-dir` never write to the server, so they only require `server.about`, `asset.read`, `asset.download`, and `album.read` (drop `album.read` too if you only pass asset IDs; add `face.read` if you combine them with `-faces`). @@ -261,7 +260,7 @@ Images use the full tag set below. Supported video containers (`mp4`, `mov`, `m4 | Location | `IPTC:City`, `XMP-photoshop:City`, `IPTC:Province-State`, `XMP-photoshop:State`, `IPTC:Country-PrimaryLocationName`, `XMP-photoshop:Country` | Dual IPTC + XMP-photoshop | | DateTime | `DateTimeOriginal`, `OffsetTimeOriginal`, `TimeZoneOffset`, `XMP-exif:DateTimeOriginal`, `XMP-xmp:CreateDate` | See below; XMP uses ISO 8601 | | Camera | `Make`, `Model`, `LensModel` | Only written if file has no existing value | -| Faces | `XMP-mwg-rs:RegionInfo` (MWG face regions) | Opt-in via `-faces`; images embed MWG regions, videos re-link people via the API; see below | +| Faces | `XMP-mwg-rs:RegionInfo` (MWG face regions) | Opt-in via `-faces`; images and supported videos, named people only; see below | ### Face regions (`-faces`) @@ -271,8 +270,8 @@ With `-faces`, every person you have **named** in Immich is written into the fil - Regions are written in the stored image's coordinate space, applying the exact inverse of the orientation transform Immich uses on import, so rotated photos round-trip correctly. - When the file's regions disagree with Immich, the whole `RegionInfo` structure is **replaced** — Immich is the source of truth, like for every other synced tag. When Immich has no named faces for an asset, existing file regions are left untouched (never cleared). - Round-trip bonus: with Immich's _"Import faces from metadata"_ server setting enabled, the names embedded by `-faces` are re-imported when the replaced file is scanned — face names survive the replace. Re-imported regions (`sourceType: exif`) are recognized as echoes of the file's own content and never counted next to a detected face of the same name (names are how the importer links regions to people), so repeated runs converge instead of duplicating regions. -- **Videos** cannot carry MWG regions, so `-faces` preserves them a different way: after a video is re-uploaded, every assigned person on the old asset is re-linked to the new one over Immich's faces API (the same call the web UI's manual tagging uses). This covers unnamed clusters too, and needs **Immich 1.127+** — older servers are skipped with a warning. Without it, a re-uploaded video keeps its re-detected boxes but loses the person names. -- The key needs the `face.read` permission, plus `face.create` for the video path (see [API key permissions](#api-key-permissions)). +- **Supported videos** (`mp4`, `mov`, `m4v`) embed the same MWG regions as images — exiftool writes XMP into the container, and Immich re-imports them when _"Import faces from metadata"_ is enabled, exactly as for photos. Regions are anchored through the video's display `Rotation` (0°, 90°, 270°); a 180° or non-cardinal rotation is left un-embedded rather than risk a misplaced box, and other containers are skipped. +- The key needs the `face.read` permission (see [API key permissions](#api-key-permissions)). - Incremental-cache caveats: the `-all` state cache keys on the set of _names_, so correcting a face's box or reassigning a face between two already-named people does not invalidate it — use `-force` to re-check. Toggling `-faces` on or off changes the cache key, so the first run after a toggle re-checks the library once. ### DateTime and timezone handling diff --git a/src/api/faces.go b/src/api/faces.go index 2cbe9fa..5a2bfd8 100644 --- a/src/api/faces.go +++ b/src/api/faces.go @@ -1,9 +1,6 @@ package api import ( - "bytes" - "encoding/json" - "io" "net/http" "net/url" @@ -26,26 +23,3 @@ func (c *ImmichClient) GetAssetFaces(assetID string) ([]model.AssetFaceResponse, } return faces, nil } - -// CreateFace links a person to an asset at a pixel box (POST /faces), the same -// endpoint the web UI uses for manual face tagging. It preserves a named person -// on a re-uploaded asset when metadata regions cannot — notably videos, where -// Immich re-detects boxes on the new thumbnail but loses the person link. -func (c *ImmichClient) CreateFace(face model.CreateFaceRequest) error { - jsonBody, err := json.Marshal(face) - if err != nil { - return err - } - req, err := c.newRequest(http.MethodPost, "/faces", bytes.NewReader(jsonBody)) - if err != nil { - return err - } - req.Header.Set("Content-Type", "application/json") - resp, err := c.doRequest(req) - if err != nil { - return err - } - _, _ = io.Copy(io.Discard, resp.Body) - resp.Body.Close() - return nil -} diff --git a/src/api/faces_test.go b/src/api/faces_test.go index 16257bc..69212a9 100644 --- a/src/api/faces_test.go +++ b/src/api/faces_test.go @@ -2,7 +2,6 @@ package api import ( "encoding/json" - "errors" "net/http" "net/http/httptest" "testing" @@ -57,44 +56,3 @@ func TestGetAssetFacesServerError(t *testing.T) { t.Fatal("expected error on server failure") } } - -func TestCreateFace(t *testing.T) { - var received model.CreateFaceRequest - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.Method != http.MethodPost || r.URL.Path != "/api/faces" { - t.Fatalf("unexpected request: %s %s", r.Method, r.URL.Path) - } - if err := json.NewDecoder(r.Body).Decode(&received); err != nil { - t.Fatalf("decode body: %v", err) - } - w.WriteHeader(http.StatusCreated) - })) - defer server.Close() - - c := NewImmichClient(server.URL, "key") - face := model.CreateFaceRequest{ - AssetID: "new-asset", PersonID: "p1", - X: 100, Y: 50, Width: 200, Height: 200, - ImageWidth: 1000, ImageHeight: 500, - } - if err := c.CreateFace(face); err != nil { - t.Fatalf("unexpected error: %v", err) - } - if received != face { - t.Fatalf("server received %+v, want %+v", received, face) - } -} - -func TestCreateFaceUnsupportedEndpoint(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - http.Error(w, "not found", http.StatusNotFound) - })) - defer server.Close() - - c := NewImmichClient(server.URL, "key") - err := c.CreateFace(model.CreateFaceRequest{AssetID: "a", PersonID: "p"}) - var status *StatusError - if !errors.As(err, &status) || status.StatusCode != http.StatusNotFound { - t.Fatalf("expected a 404 StatusError (server too old to skip gracefully), got %v", err) - } -} diff --git a/src/main.go b/src/main.go index f8cc319..11785b9 100644 --- a/src/main.go +++ b/src/main.go @@ -128,7 +128,7 @@ func run() int { return 0 } - uploader := &process.ModernUploader{Client: client, ResolveDuplicate: cfg.ResolveDuplicate, VerifyUpload: cfg.VerifyUpload, Faces: cfg.Faces} + uploader := &process.ModernUploader{Client: client, ResolveDuplicate: cfg.ResolveDuplicate, VerifyUpload: cfg.VerifyUpload} if cfg.ExportDir != "" { cfg.ExportDir = resolveExportDir(cfg) diff --git a/src/model/people.go b/src/model/people.go index 9296247..e3df10b 100644 --- a/src/model/people.go +++ b/src/model/people.go @@ -25,22 +25,6 @@ type AssetFaceResponse struct { Person *PersonResponse `json:"person"` } -// CreateFaceRequest is the POST /faces payload that links a person to an asset -// at a pixel box. The coordinates and image dimensions mirror what -// GetAssetFaces returns, so a box read from one asset re-creates verbatim on -// another. Immich records it with sourceType "manual", which survives later ML -// detection jobs (Immich 1.127+). -type CreateFaceRequest struct { - AssetID string `json:"assetId"` - PersonID string `json:"personId"` - X int `json:"x"` - Y int `json:"y"` - Width int `json:"width"` - Height int `json:"height"` - ImageWidth int `json:"imageWidth"` - ImageHeight int `json:"imageHeight"` -} - // NamedVisibleName returns a person's write-eligible region name and whether // they have one: visible and named, mirroring what Immich itself is willing to // import from file regions. It is the single definition of that eligibility, @@ -71,7 +55,10 @@ func NamedPeopleNames(asset AssetResponse) []string { // are written. It answers the presence question without allocating and sorting // the name list, for the per-asset -all pre-filter. func HasFaceRegionsToEmbed(asset AssetResponse) bool { - if IsVideoAsset(asset) { + // Videos carry MWG regions only in containers exiftool can write; the + // rotation-anchoring guard that decides whether a given video is embeddable + // runs later, once the file's Rotation tag is known. + if IsVideoAsset(asset) && !SupportsVideoMetadataEmbedding(asset) { return false } for _, person := range asset.People { diff --git a/src/model/people_test.go b/src/model/people_test.go index 3d0b5ee..f1a8a40 100644 --- a/src/model/people_test.go +++ b/src/model/people_test.go @@ -2,13 +2,24 @@ package model import "testing" -func TestHasFaceRegionsToEmbedExcludesVideos(t *testing.T) { +func TestHasFaceRegionsToEmbedAllowsSupportedVideos(t *testing.T) { asset := AssetResponse{ OriginalMimeType: "video/mp4", People: []PersonResponse{{ID: "p1", Name: "Alice"}}, } + if !HasFaceRegionsToEmbed(asset) { + t.Fatal("mp4 videos with named people are region-embeddable") + } +} + +func TestHasFaceRegionsToEmbedExcludesUnsupportedVideos(t *testing.T) { + asset := AssetResponse{ + OriginalMimeType: "video/x-matroska", + OriginalFileName: "clip.mkv", + People: []PersonResponse{{ID: "p1", Name: "Alice"}}, + } if HasFaceRegionsToEmbed(asset) { - t.Fatal("videos cannot hold XMP-mwg-rs regions; must be excluded even with named people") + t.Fatal("containers exiftool cannot write must be excluded even with named people") } } diff --git a/src/process/faceOrientation_test.go b/src/process/faceOrientation_test.go new file mode 100644 index 0000000..04ec490 --- /dev/null +++ b/src/process/faceOrientation_test.go @@ -0,0 +1,50 @@ +package process + +import ( + "testing" + + "github.com/majorfi/immich-exif/exif" + "github.com/majorfi/immich-exif/model" +) + +func TestVideoRotationToOrientation(t *testing.T) { + cases := []struct { + rotation int + want int + ok bool + }{ + {0, 1, true}, + {90, 6, true}, + {270, 8, true}, + {-90, 8, true}, // normalized to 270 + {360, 1, true}, // normalized to 0 + {450, 6, true}, // normalized to 90 + {180, 0, false}, // Immich does not re-orient 180 video regions + {45, 0, false}, // non-cardinal + } + for _, c := range cases { + got, ok := videoRotationToOrientation(c.rotation) + if got != c.want || ok != c.ok { + t.Fatalf("rotation %d: got (%d,%v), want (%d,%v)", c.rotation, got, ok, c.want, c.ok) + } + } +} + +func TestRegionOrientationImageUsesExifOrientation(t *testing.T) { + asset := model.AssetResponse{OriginalMimeType: "image/jpeg"} + existing := exif.ExifTagMap{"Orientation": float64(6)} + got, ok := regionOrientation(asset, existing) + if !ok || got != 6 { + t.Fatalf("image should use EXIF Orientation 6, got (%d,%v)", got, ok) + } +} + +func TestRegionOrientationVideoUsesRotation(t *testing.T) { + asset := model.AssetResponse{OriginalMimeType: "video/mp4"} + if got, ok := regionOrientation(asset, exif.ExifTagMap{"Rotation": float64(90)}); !ok || got != 6 { + t.Fatalf("video rotation 90 should map to orientation 6, got (%d,%v)", got, ok) + } + if _, ok := regionOrientation(asset, exif.ExifTagMap{"Rotation": float64(180)}); ok { + t.Fatal("video rotation 180 must report not-anchorable") + } +} diff --git a/src/process/faces.go b/src/process/faces.go index c7b1619..da7531f 100644 --- a/src/process/faces.go +++ b/src/process/faces.go @@ -17,13 +17,18 @@ func wantsFaceRegions(cfg *model.Config, asset model.AssetResponse) bool { // appendFaceRegionChange fetches the asset's face boxes and appends the // region rewrite when the file disagrees with Immich, returning the regions // being embedded (nil when the file already matches) so they can be re-checked -// before upload. The file's own Orientation and pixel dimensions anchor the +// before upload. The file's own orientation and pixel dimensions anchor the // regions, so this must run after the exif read. A file that reports no pixel -// dimensions gets no regions rather than misanchored ones. +// dimensions, or a video whose rotation cannot be safely anchored, gets no +// regions rather than misanchored ones. func appendFaceRegionChange(client *api.ImmichClient, cfg *model.Config, asset model.AssetResponse, existing exif.ExifTagMap, changes []exif.TagChange) ([]exif.TagChange, []exif.FaceRegion, error) { if !wantsFaceRegions(cfg, asset) { return changes, nil, nil } + orientation, ok := regionOrientation(asset, existing) + if !ok { + return changes, nil, nil + } faces, err := client.GetAssetFaces(asset.ID) if err != nil { if isPermissionDenied(err) { @@ -31,7 +36,7 @@ func appendFaceRegionChange(client *api.ImmichClient, cfg *model.Config, asset m } return nil, nil, err } - regions := exif.BuildFaceRegions(faces, intTag(existing, "Orientation")) + regions := exif.BuildFaceRegions(faces, orientation) change := exif.CompareFaceRegions(regions, intTag(existing, "ImageWidth"), intTag(existing, "ImageHeight"), existing) if change == nil { return changes, nil, nil @@ -40,6 +45,34 @@ func appendFaceRegionChange(client *api.ImmichClient, cfg *model.Config, asset m return changes, regions, nil } +// regionOrientation returns the EXIF-orientation value to anchor face regions +// with, and whether they can be anchored at all. Images use their EXIF +// Orientation. Videos have no EXIF Orientation; their display rotation is read +// from the QuickTime Rotation tag and mapped to the equivalent orientation. +func regionOrientation(asset model.AssetResponse, existing exif.ExifTagMap) (int, bool) { + if model.IsVideoAsset(asset) { + return videoRotationToOrientation(intTag(existing, "Rotation")) + } + return intTag(existing, "Orientation"), true +} + +// videoRotationToOrientation maps a QuickTime display rotation to the EXIF +// orientation whose inverse transform Immich applies to video regions on +// import (verified against a live server): 0->1, 90->6, 270->8. A 180 or +// non-cardinal rotation returns ok=false — Immich does not re-orient 180 video +// regions the way it does 90/270, so those are skipped rather than misanchored. +func videoRotationToOrientation(rotation int) (int, bool) { + switch ((rotation % 360) + 360) % 360 { + case 0: + return 1, true + case 90: + return 6, true + case 270: + return 8, true + } + return 0, false +} + // statusCodeOf extracts the HTTP status from an API error, reporting false when // the error is not an *api.StatusError (e.g. a transport failure). func statusCodeOf(err error) (int, bool) { @@ -51,114 +84,21 @@ func statusCodeOf(err error) (int, bool) { } // isPermissionDenied reports whether an API error carries a 401/403 status, the -// signature of an API key missing a face permission (face.read for the GET, -// face.create for the POST). +// signature of an API key missing the face.read permission. func isPermissionDenied(err error) bool { code, ok := statusCodeOf(err) return ok && (code == http.StatusUnauthorized || code == http.StatusForbidden) } -// isNotFound reports whether an API error carries a 404, which for the faces -// endpoint means the server predates manual face tagging (Immich 1.127). -func isNotFound(err error) bool { - code, ok := statusCodeOf(err) - return ok && code == http.StatusNotFound -} - -// errFacePreserveUnsupported marks a server too old for the createFace endpoint -// (introduced with manual face tagging in Immich 1.127). The caller warns and -// keeps going instead of failing the replacement. -var errFacePreserveUnsupported = errors.New("server does not support the faces endpoint (needs Immich 1.127+)") - -// recreateFaces re-links the source asset's assigned people onto the target -// asset via POST /faces. It backs the video path of -faces: a video's regions -// cannot be embedded as metadata, and on re-upload Immich re-detects boxes on -// the new thumbnail but drops the person link. Every face carrying a person is -// recreated verbatim (named or not — createFace requires a personId, so faces -// with none are skipped); a person already linked on the target is left alone so -// repeated runs do not stack duplicates. Returns the number of faces created. -func recreateFaces(client *api.ImmichClient, sourceID, targetID string) (int, error) { - // A 404 on this first call means the server predates the faces endpoint - // (both the GET and the POST arrived with manual tagging in 1.127); the - // source asset always exists, so it is never a missing-asset 404. Skip with a - // warning instead of failing the replace. Past this point the endpoint is - // proven to exist, so a later 404 is a real error and stays fatal. - sourceFaces, err := client.GetAssetFaces(sourceID) - if err != nil { - if isNotFound(err) { - return 0, errFacePreserveUnsupported - } - return 0, err - } - pending := facesWithPerson(sourceFaces) - if len(pending) == 0 { - return 0, nil - } - - targetFaces, err := client.GetAssetFaces(targetID) - if err != nil { - return 0, err - } - linked := linkedPersonIDs(targetFaces) - - created := 0 - for _, face := range pending { - if linked[face.Person.ID] { - continue - } - req := model.CreateFaceRequest{ - AssetID: targetID, - PersonID: face.Person.ID, - X: face.BoundingBoxX1, - Y: face.BoundingBoxY1, - Width: face.BoundingBoxX2 - face.BoundingBoxX1, - Height: face.BoundingBoxY2 - face.BoundingBoxY1, - ImageWidth: face.ImageWidth, - ImageHeight: face.ImageHeight, - } - if err := client.CreateFace(req); err != nil { - if isNotFound(err) { - return created, errFacePreserveUnsupported - } - return created, err - } - linked[face.Person.ID] = true - created++ - } - return created, nil -} - -// facesWithPerson keeps only the faces Immich has assigned to a person; a face -// without one has no personId to re-link. -func facesWithPerson(faces []model.AssetFaceResponse) []model.AssetFaceResponse { - var kept []model.AssetFaceResponse - for _, face := range faces { - if face.Person != nil && face.Person.ID != "" { - kept = append(kept, face) - } - } - return kept -} - -// linkedPersonIDs is the set of people already having a face on an asset. -func linkedPersonIDs(faces []model.AssetFaceResponse) map[string]bool { - linked := map[string]bool{} - for _, face := range faces { - if face.Person != nil && face.Person.ID != "" { - linked[face.Person.ID] = true - } - } - return linked -} - // faceRegionsStale reports whether the server's faces moved away from the // regions this run is about to embed. -func faceRegionsStale(client *api.ImmichClient, assetID string, existing exif.ExifTagMap, written []exif.FaceRegion) (bool, error) { - faces, err := client.GetAssetFaces(assetID) +func faceRegionsStale(client *api.ImmichClient, asset model.AssetResponse, existing exif.ExifTagMap, written []exif.FaceRegion) (bool, error) { + orientation, _ := regionOrientation(asset, existing) + faces, err := client.GetAssetFaces(asset.ID) if err != nil { return false, err } - fresh := exif.BuildFaceRegions(faces, intTag(existing, "Orientation")) + fresh := exif.BuildFaceRegions(faces, orientation) return !exif.FaceRegionsMatch(fresh, written, intTag(existing, "ImageWidth"), intTag(existing, "ImageHeight")), nil } diff --git a/src/process/pipeline.go b/src/process/pipeline.go index bf445f1..bb43857 100644 --- a/src/process/pipeline.go +++ b/src/process/pipeline.go @@ -170,7 +170,7 @@ func ProcessAsset(client *api.ImmichClient, uploader Uploader, cfg *model.Config // when no region is being written: the file already matched, so a mid-run // face move is not ours to guard and must not block an unrelated exif write. if len(faceRegions) > 0 { - stale, staleErr := faceRegionsStale(client, assetID, existing, faceRegions) + stale, staleErr := faceRegionsStale(client, *asset, existing, faceRegions) if staleErr != nil { return fail("re-check faces before upload: %v", staleErr) } diff --git a/src/process/recreateFaces_test.go b/src/process/recreateFaces_test.go deleted file mode 100644 index a3a5abe..0000000 --- a/src/process/recreateFaces_test.go +++ /dev/null @@ -1,148 +0,0 @@ -package process - -import ( - "encoding/json" - "errors" - "net/http" - "net/http/httptest" - "sync" - "testing" - - "github.com/majorfi/immich-exif/api" - "github.com/majorfi/immich-exif/model" -) - -// recreateFacesServer serves GET /api/faces?id= from a per-asset map and -// records every POST /api/faces body. A non-zero postStatus overrides the -// created response, to exercise the too-old (404) path. -func recreateFacesServer(t *testing.T, facesByAsset map[string][]model.AssetFaceResponse, postStatus int) (*httptest.Server, *[]model.CreateFaceRequest) { - t.Helper() - var mu sync.Mutex - var posted []model.CreateFaceRequest - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path != "/api/faces" { - t.Fatalf("unexpected path: %s", r.URL.Path) - } - switch r.Method { - case http.MethodGet: - w.Header().Set("Content-Type", "application/json") - json.NewEncoder(w).Encode(facesByAsset[r.URL.Query().Get("id")]) - case http.MethodPost: - if postStatus != 0 { - http.Error(w, "nope", postStatus) - return - } - var req model.CreateFaceRequest - if err := json.NewDecoder(r.Body).Decode(&req); err != nil { - t.Fatalf("decode POST body: %v", err) - } - mu.Lock() - posted = append(posted, req) - mu.Unlock() - w.WriteHeader(http.StatusCreated) - default: - t.Fatalf("unexpected method: %s", r.Method) - } - })) - return server, &posted -} - -func personFace(personID string, x1, y1, x2, y2, iw, ih int) model.AssetFaceResponse { - face := model.AssetFaceResponse{ - BoundingBoxX1: x1, BoundingBoxY1: y1, BoundingBoxX2: x2, BoundingBoxY2: y2, - ImageWidth: iw, ImageHeight: ih, - } - if personID != "" { - face.Person = &model.PersonResponse{ID: personID} - } - return face -} - -func TestRecreateFacesCopiesAssignedFaces(t *testing.T) { - source := []model.AssetFaceResponse{ - personFace("alice", 100, 50, 300, 250, 1000, 500), - personFace("bob", 400, 60, 500, 160, 1000, 500), - personFace("", 1, 1, 2, 2, 10, 10), // unassigned — no personId, must be skipped - } - server, posted := recreateFacesServer(t, map[string][]model.AssetFaceResponse{ - "old": source, - "new": nil, // fresh upload, ML has not run yet - }, 0) - defer server.Close() - - created, err := recreateFaces(api.NewImmichClient(server.URL, "key"), "old", "new") - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if created != 2 { - t.Fatalf("expected 2 faces created, got %d", created) - } - if len(*posted) != 2 { - t.Fatalf("expected 2 POSTs, got %d", len(*posted)) - } - alice := (*posted)[0] - if alice.AssetID != "new" || alice.PersonID != "alice" || alice.Width != 200 || alice.Height != 200 || alice.X != 100 || alice.Y != 50 { - t.Fatalf("alice face not recreated verbatim: %+v", alice) - } -} - -func TestRecreateFacesSkipsPeopleAlreadyLinked(t *testing.T) { - server, posted := recreateFacesServer(t, map[string][]model.AssetFaceResponse{ - "old": {personFace("alice", 100, 50, 300, 250, 1000, 500), personFace("bob", 0, 0, 10, 10, 1000, 500)}, - "new": {personFace("alice", 0, 0, 10, 10, 800, 400)}, // recognition already re-linked alice - }, 0) - defer server.Close() - - created, err := recreateFaces(api.NewImmichClient(server.URL, "key"), "old", "new") - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if created != 1 { - t.Fatalf("expected only bob created, got %d", created) - } - if len(*posted) != 1 || (*posted)[0].PersonID != "bob" { - t.Fatalf("expected a single bob POST, got %+v", *posted) - } -} - -func TestRecreateFacesNoAssignedFacesDoesNothing(t *testing.T) { - server, posted := recreateFacesServer(t, map[string][]model.AssetFaceResponse{ - "old": {personFace("", 1, 1, 2, 2, 10, 10)}, - }, 0) - defer server.Close() - - created, err := recreateFaces(api.NewImmichClient(server.URL, "key"), "old", "new") - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if created != 0 || len(*posted) != 0 { - t.Fatalf("expected no work, got created=%d posts=%d", created, len(*posted)) - } -} - -func TestRecreateFacesTooOldServer(t *testing.T) { - server, _ := recreateFacesServer(t, map[string][]model.AssetFaceResponse{ - "old": {personFace("alice", 100, 50, 300, 250, 1000, 500)}, - "new": nil, - }, http.StatusNotFound) - defer server.Close() - - _, err := recreateFaces(api.NewImmichClient(server.URL, "key"), "old", "new") - if !errors.Is(err, errFacePreserveUnsupported) { - t.Fatalf("expected errFacePreserveUnsupported on a 404, got %v", err) - } -} - -func TestRecreateFacesTooOldServerOnGet(t *testing.T) { - // A server predating the faces endpoint 404s the GET too, before any POST is - // reached; that must still skip gracefully rather than fail the replace. - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - http.Error(w, "not found", http.StatusNotFound) - })) - defer server.Close() - - _, err := recreateFaces(api.NewImmichClient(server.URL, "key"), "old", "new") - if !errors.Is(err, errFacePreserveUnsupported) { - t.Fatalf("expected errFacePreserveUnsupported on a GET /faces 404, got %v", err) - } -} diff --git a/src/process/uploader.go b/src/process/uploader.go index c40a735..1b5e43e 100644 --- a/src/process/uploader.go +++ b/src/process/uploader.go @@ -1,7 +1,6 @@ package process import ( - "errors" "fmt" "strings" @@ -24,7 +23,6 @@ type ModernUploader struct { Client *api.ImmichClient ResolveDuplicate bool VerifyUpload bool - Faces bool } func (u *ModernUploader) Upload(filePath string, asset *model.AssetResponse, emitter model.EventEmitter) (UploadOutcome, error) { @@ -126,23 +124,6 @@ func (u *ModernUploader) finalizeReplacement(filePath string, asset *model.Asset } } - // Videos cannot carry MWG face regions, so the person links are re-created on - // the new asset over the API before the old one is trashed — the old asset is - // where those links still live. - if u.Faces && model.IsVideoAsset(*asset) { - created, err := recreateFaces(u.Client, asset.ID, targetID) - switch { - case errors.Is(err, errFacePreserveUnsupported): - emitter.EmitProgress(model.ProgressEvent{AssetID: asset.ID, Filename: asset.OriginalFileName, Step: "Skipping video face preservation: server predates the faces endpoint (needs Immich 1.127+)"}) - case isPermissionDenied(err): - return fmt.Errorf("face preservation denied — -faces needs the API key's face.create permission (old asset %s NOT deleted, new asset %s left in place): %w", model.ShortID(asset.ID), model.ShortID(targetID), err) - case err != nil: - return fmt.Errorf("preserve faces failed (old asset %s NOT deleted, new asset %s left in place): %w", model.ShortID(asset.ID), model.ShortID(targetID), err) - case created > 0: - emitter.EmitProgress(model.ProgressEvent{AssetID: asset.ID, Filename: asset.OriginalFileName, Step: fmt.Sprintf("Preserved %d face(s) on %s", created, model.ShortID(targetID))}) - } - } - // Always trash, never permanently delete: checksum verification proves the // rewritten bytes arrived intact, not that exiftool produced a valid file, // and the trash window is the only recovery path for that failure mode. diff --git a/src/process/uploader_test.go b/src/process/uploader_test.go index 5b6dc53..45b77c9 100644 --- a/src/process/uploader_test.go +++ b/src/process/uploader_test.go @@ -7,7 +7,6 @@ import ( "net/http/httptest" "os" "path/filepath" - "slices" "strings" "testing" "time" @@ -97,126 +96,6 @@ func TestModernUploaderArchivedAssetRestoresVisibilityBeforeDelete(t *testing.T) } } -// videoReplaceServer handles a full created-status replace for a video asset -// whose old copy has one assigned face, answering POST /api/faces with -// facePostStatus so the caller can drive the skip vs fail hook branches. -func videoReplaceServer(t *testing.T, calls *[]string, facePostStatus int) (*httptest.Server, *[]model.CreateFaceRequest) { - t.Helper() - var posted []model.CreateFaceRequest - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - *calls = append(*calls, r.Method+" "+r.URL.Path) - switch { - case r.Method == http.MethodPost && r.URL.Path == "/api/assets": - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusCreated) - _, _ = w.Write([]byte(`{"id":"new-asset-id","status":"created"}`)) - case r.Method == http.MethodPut && r.URL.Path == "/api/assets/copy": - w.WriteHeader(http.StatusNoContent) - case r.Method == http.MethodGet && r.URL.Path == "/api/faces": - w.Header().Set("Content-Type", "application/json") - if r.URL.Query().Get("id") == "old-asset-id" { - _ = json.NewEncoder(w).Encode([]model.AssetFaceResponse{ - {BoundingBoxX1: 10, BoundingBoxY1: 20, BoundingBoxX2: 110, BoundingBoxY2: 140, ImageWidth: 640, ImageHeight: 360, Person: &model.PersonResponse{ID: "p1"}}, - }) - return - } - _ = json.NewEncoder(w).Encode([]model.AssetFaceResponse{}) - case r.Method == http.MethodPost && r.URL.Path == "/api/faces": - var req model.CreateFaceRequest - if err := json.NewDecoder(r.Body).Decode(&req); err != nil { - t.Errorf("decode POST /api/faces body: %v", err) - } - posted = append(posted, req) - w.WriteHeader(facePostStatus) - case r.Method == http.MethodDelete && r.URL.Path == "/api/assets": - w.WriteHeader(http.StatusNoContent) - default: - t.Fatalf("unexpected request: %s %s", r.Method, r.URL.Path) - } - })) - return server, &posted -} - -func videoAsset() *model.AssetResponse { - return &model.AssetResponse{ - ID: "old-asset-id", - OriginalFileName: "clip.mp4", - OriginalMimeType: "video/mp4", - FileCreatedAt: time.Now().UTC(), - FileModifiedAt: time.Now().UTC(), - } -} - -func TestModernUploaderVideoTooOldForFacesSkipsAndStillTrashes(t *testing.T) { - var calls []string - server, _ := videoReplaceServer(t, &calls, http.StatusNotFound) - defer server.Close() - - tempDir := t.TempDir() - filePath := filepath.Join(tempDir, "clip.mp4") - if err := os.WriteFile(filePath, []byte("data"), 0644); err != nil { - t.Fatalf("write file: %v", err) - } - - uploader := &ModernUploader{Client: api.NewImmichClient(server.URL, "key"), Faces: true} - if _, err := uploader.Upload(filePath, videoAsset(), &noopEmitter{}); err != nil { - t.Fatalf("a 404 from the faces endpoint must not fail the replace: %v", err) - } - if !slices.Contains(calls, "POST /api/faces") { - t.Fatal("expected a face-create attempt for a video with -faces") - } - if !slices.Contains(calls, "DELETE /api/assets") { - t.Fatal("old asset must still be trashed when face preservation is unsupported") - } -} - -func TestModernUploaderVideoFacePermissionDeniedDoesNotTrash(t *testing.T) { - var calls []string - server, _ := videoReplaceServer(t, &calls, http.StatusForbidden) - defer server.Close() - - tempDir := t.TempDir() - filePath := filepath.Join(tempDir, "clip.mp4") - if err := os.WriteFile(filePath, []byte("data"), 0644); err != nil { - t.Fatalf("write file: %v", err) - } - - uploader := &ModernUploader{Client: api.NewImmichClient(server.URL, "key"), Faces: true} - _, err := uploader.Upload(filePath, videoAsset(), &noopEmitter{}) - if err == nil { - t.Fatal("a face.create permission denial must fail the replace") - } - if slices.Contains(calls, "DELETE /api/assets") { - t.Fatal("old asset must NOT be trashed when face preservation is denied") - } -} - -func TestModernUploaderVideoPreservesFacesThenTrashes(t *testing.T) { - var calls []string - server, posted := videoReplaceServer(t, &calls, http.StatusCreated) - defer server.Close() - - tempDir := t.TempDir() - filePath := filepath.Join(tempDir, "clip.mp4") - if err := os.WriteFile(filePath, []byte("data"), 0644); err != nil { - t.Fatalf("write file: %v", err) - } - - uploader := &ModernUploader{Client: api.NewImmichClient(server.URL, "key"), Faces: true} - if _, err := uploader.Upload(filePath, videoAsset(), &noopEmitter{}); err != nil { - t.Fatalf("preserving faces must not fail the replace: %v", err) - } - if len(*posted) != 1 { - t.Fatalf("expected one preserved face, got %d", len(*posted)) - } - if got := (*posted)[0]; got.PersonID != "p1" || got.AssetID != "new-asset-id" { - t.Fatalf("face re-linked to the wrong person/asset: %+v", got) - } - if !slices.Contains(calls, "DELETE /api/assets") { - t.Fatal("old asset must still be trashed once faces are preserved") - } -} - func TestModernUploaderDoesNotDeleteOldAssetWhenArchiveVisibilityUpdateFails(t *testing.T) { var calls []string diff --git a/src/utils.go b/src/utils.go index 823b702..d1c6360 100644 --- a/src/utils.go +++ b/src/utils.go @@ -260,7 +260,7 @@ func maybeResolveDuplicatesNow(ctx context.Context, client *api.ImmichClient, cf fmt.Printf("\nRe-running unresolved duplicates with -resolve-duplicate (%d assets)\n", len(assetIDs)) resolveCfg := buildResolveDuplicateFollowUpConfig(cfg) - resolveUploader := &process.ModernUploader{Client: client, ResolveDuplicate: true, VerifyUpload: cfg.VerifyUpload, Faces: cfg.Faces} + resolveUploader := &process.ModernUploader{Client: client, ResolveDuplicate: true, VerifyUpload: cfg.VerifyUpload} return runPipeline(ctx, client, resolveUploader, resolveCfg, assetIDs) } From 4650fd6ea29ac0fe53ac3afd8a5ba78fb73bdfd8 Mon Sep 17 00:00:00 2001 From: Major Date: Wed, 29 Jul 2026 11:33:24 +0200 Subject: [PATCH 2/2] Address review: drop dead rotation normalization, cover the video path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - videoRotationToOrientation: exiftool always reports Rotation normalized to 0/90/180/270, so the ((r%360)+360)%360 guard was dead; plain switch - add pipeline tests: a 90° video embeds RegionInfo through the mapped orientation (swapped coords), and a 180° video embeds nothing and never fetches faces --- src/process/faceOrientation_test.go | 3 - src/process/faces.go | 13 ++-- src/process/pipelineFaces_test.go | 103 ++++++++++++++++++++++++++++ 3 files changed, 110 insertions(+), 9 deletions(-) diff --git a/src/process/faceOrientation_test.go b/src/process/faceOrientation_test.go index 04ec490..60cd9b8 100644 --- a/src/process/faceOrientation_test.go +++ b/src/process/faceOrientation_test.go @@ -16,9 +16,6 @@ func TestVideoRotationToOrientation(t *testing.T) { {0, 1, true}, {90, 6, true}, {270, 8, true}, - {-90, 8, true}, // normalized to 270 - {360, 1, true}, // normalized to 0 - {450, 6, true}, // normalized to 90 {180, 0, false}, // Immich does not re-orient 180 video regions {45, 0, false}, // non-cardinal } diff --git a/src/process/faces.go b/src/process/faces.go index da7531f..6ebc230 100644 --- a/src/process/faces.go +++ b/src/process/faces.go @@ -56,13 +56,14 @@ func regionOrientation(asset model.AssetResponse, existing exif.ExifTagMap) (int return intTag(existing, "Orientation"), true } -// videoRotationToOrientation maps a QuickTime display rotation to the EXIF -// orientation whose inverse transform Immich applies to video regions on -// import (verified against a live server): 0->1, 90->6, 270->8. A 180 or -// non-cardinal rotation returns ok=false — Immich does not re-orient 180 video -// regions the way it does 90/270, so those are skipped rather than misanchored. +// videoRotationToOrientation maps a QuickTime display rotation (exiftool +// reports it already normalized to 0/90/180/270) to the EXIF orientation whose +// inverse transform Immich applies to video regions on import (verified against +// a live server): 0->1, 90->6, 270->8. A 180 or non-cardinal rotation returns +// ok=false — Immich does not re-orient 180 video regions the way it does +// 90/270, so those are skipped rather than misanchored. func videoRotationToOrientation(rotation int) (int, bool) { - switch ((rotation % 360) + 360) % 360 { + switch rotation { case 0: return 1, true case 90: diff --git a/src/process/pipelineFaces_test.go b/src/process/pipelineFaces_test.go index b48a610..20f73f6 100644 --- a/src/process/pipelineFaces_test.go +++ b/src/process/pipelineFaces_test.go @@ -79,6 +79,109 @@ func TestProcessAssetWritesFaceRegions(t *testing.T) { } } +func TestProcessAssetWritesVideoFaceRegionsAnchoredByRotation(t *testing.T) { + var facesCalls atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/api/faces" { + facesCalls.Add(1) + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode([]model.AssetFaceResponse{{ + BoundingBoxX1: 100, BoundingBoxY1: 50, BoundingBoxX2: 300, BoundingBoxY2: 250, + ImageWidth: 1000, ImageHeight: 500, + Person: &model.PersonResponse{ID: "p1", Name: "Alice"}, + }}) + return + } + if strings.HasSuffix(r.URL.Path, "/original") { + w.Write([]byte("fake-video-data")) + return + } + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(model.AssetResponse{ + ID: "asset-1", + OriginalFileName: "clip.mp4", + OriginalMimeType: "video/mp4", + Checksum: sha1HexOf("fake-video-data"), + People: []model.PersonResponse{{ID: "p1", Name: "Alice"}}, + }) + })) + defer server.Close() + + var written []string + defer withMockExiftool( + // A video has no EXIF Orientation; its display rotation is 90°. + func(string) (exif.ExifTagMap, error) { + return exif.ExifTagMap{"ImageWidth": float64(640), "ImageHeight": float64(360), "Rotation": float64(90)}, nil + }, + func(_ string, args []string) error { + written = args + return nil + }, + )() + + client := api.NewImmichClient(server.URL, "key") + cfg := &model.Config{DryRun: true, Faces: true} + result := ProcessAsset(client, nil, cfg, "asset-1", 1, 1, &noopEmitter{}, nil) + if result.Status != model.StatusSuccess { + t.Fatalf("expected success, got %s: %s", result.Status, result.Message) + } + // Rotation 90 -> orientation 6: rasterRegion maps (x,y)->(y,1-x) and swaps W/H, + // so the displayed-space center (0.2,0.3) w/h (0.2,0.4) becomes (0.3,0.8) (0.4,0.2). + want := "-XMP-mwg-rs:RegionInfo={AppliedToDimensions={W=640,H=360,Unit=pixel},RegionList=[{Area={X=0.30000,Y=0.80000,W=0.40000,H=0.20000,Unit=normalized},Name=Alice,Type=Face}]}" + if len(written) != 1 || written[0] != want { + t.Fatalf("unexpected write args:\n got %v\nwant %s", written, want) + } +} + +func TestProcessAssetSkipsVideoFaceRegionsWhenRotation180(t *testing.T) { + var facesCalls atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/api/faces" { + facesCalls.Add(1) + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode([]model.AssetFaceResponse{{ + BoundingBoxX1: 100, BoundingBoxY1: 50, BoundingBoxX2: 300, BoundingBoxY2: 250, + ImageWidth: 1000, ImageHeight: 500, + Person: &model.PersonResponse{ID: "p1", Name: "Alice"}, + }}) + return + } + if strings.HasSuffix(r.URL.Path, "/original") { + w.Write([]byte("fake-video-data")) + return + } + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(model.AssetResponse{ + ID: "asset-1", OriginalFileName: "clip.mp4", OriginalMimeType: "video/mp4", + Checksum: sha1HexOf("fake-video-data"), + People: []model.PersonResponse{{ID: "p1", Name: "Alice"}}, + }) + })) + defer server.Close() + + var written []string + defer withMockExiftool( + func(string) (exif.ExifTagMap, error) { + return exif.ExifTagMap{"ImageWidth": float64(640), "ImageHeight": float64(360), "Rotation": float64(180)}, nil + }, + func(_ string, args []string) error { written = args; return nil }, + )() + + client := api.NewImmichClient(server.URL, "key") + cfg := &model.Config{DryRun: true, Faces: true} + result := ProcessAsset(client, nil, cfg, "asset-1", 1, 1, &noopEmitter{}, nil) + // No other metadata to embed and faces skipped (180° un-anchorable) -> nothing written. + if len(written) != 0 { + t.Fatalf("a 180° video must not embed face regions, got %v", written) + } + if facesCalls.Load() != 0 { + t.Fatalf("faces should not be fetched for an un-anchorable rotation, got %d calls", facesCalls.Load()) + } + if result.Status != model.StatusSkipped { + t.Fatalf("expected skip when nothing is embeddable, got %s: %s", result.Status, result.Message) + } +} + func TestProcessAssetFaceRegionsAlreadyMatch(t *testing.T) { var facesCalls atomic.Int32 server := facesAssetServer(