From acfd8c97cd3e4645107158181d18aef5e7fbc8f0 Mon Sep 17 00:00:00 2001 From: Major Date: Thu, 30 Jul 2026 09:31:36 +0200 Subject: [PATCH 1/2] Explain why -faces embedded no regions (issue #30) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A -faces run that wrote no face region said nothing at all: five separate guards returned silently, so "no Face regions line" was indistinguishable between "no named people", "rotation cannot be anchored", "file has no dimensions", "face boxes carry no named person", and an unwritable video container. Nurgak's report is exactly this ambiguity — his video shows date changes and no face line, and the output cannot say which guard hit. appendFaceRegionChange now returns a reason for each of those, surfaced as a "Face regions (skipped) -> " entry in the diff block (the one place a piped or -y run still prints) and appended to the skip message when there was nothing else to embed. The faces fetch deliberately stays ahead of the dimension guard so a missing face.read permission is still reported loudly. --- src/process/faceSkipReason_test.go | 133 +++++++++++++++++++++++++++++ src/process/faces.go | 36 ++++++-- src/process/pipeline.go | 13 ++- 3 files changed, 171 insertions(+), 11 deletions(-) create mode 100644 src/process/faceSkipReason_test.go diff --git a/src/process/faceSkipReason_test.go b/src/process/faceSkipReason_test.go new file mode 100644 index 0000000..0505c44 --- /dev/null +++ b/src/process/faceSkipReason_test.go @@ -0,0 +1,133 @@ +package process + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/majorfi/immich-exif/api" + "github.com/majorfi/immich-exif/exif" + "github.com/majorfi/immich-exif/model" +) + +func faceSkipServer(faces []model.AssetFaceResponse) *httptest.Server { + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(faces) + })) +} + +// Every guard in appendFaceRegionChange must name itself: a -faces run that +// embeds nothing has to say which condition stopped it. +func TestAppendFaceRegionChangeExplainsEverySkip(t *testing.T) { + namedPerson := []model.PersonResponse{{ID: "p1", Name: "Alice"}} + usableFace := model.AssetFaceResponse{ + BoundingBoxX1: 10, BoundingBoxY1: 20, BoundingBoxX2: 110, BoundingBoxY2: 140, + ImageWidth: 1000, ImageHeight: 500, Person: &model.PersonResponse{ID: "p1", Name: "Alice"}, + } + sizedTags := exif.ExifTagMap{"ImageWidth": float64(1920), "ImageHeight": float64(1080)} + + cases := []struct { + name string + asset model.AssetResponse + tags exif.ExifTagMap + faces []model.AssetFaceResponse + wantReason string + }{ + { + name: "no named person", + asset: model.AssetResponse{ID: "a", OriginalMimeType: "image/jpeg"}, + tags: sizedTags, + wantReason: "no named, visible person", + }, + { + name: "unsupported video container", + asset: model.AssetResponse{ID: "a", OriginalMimeType: "video/x-matroska", OriginalFileName: "c.mkv", People: namedPerson}, + tags: sizedTags, + wantReason: "container cannot hold face regions", + }, + { + name: "video rotated 180", + asset: model.AssetResponse{ID: "a", OriginalMimeType: "video/mp4", People: namedPerson}, + tags: exif.ExifTagMap{"ImageWidth": float64(1920), "ImageHeight": float64(1080), "Rotation": float64(180)}, + faces: []model.AssetFaceResponse{usableFace}, + wantReason: "rotation 180° cannot be anchored", + }, + { + name: "file has no pixel dimensions", + asset: model.AssetResponse{ID: "a", OriginalMimeType: "image/jpeg", People: namedPerson}, + tags: exif.ExifTagMap{}, + faces: []model.AssetFaceResponse{usableFace}, + wantReason: "no pixel dimensions", + }, + { + name: "face boxes carry no usable person", + asset: model.AssetResponse{ID: "a", OriginalMimeType: "image/jpeg", People: namedPerson}, + tags: sizedTags, + faces: []model.AssetFaceResponse{{BoundingBoxX2: 10, BoundingBoxY2: 10, ImageWidth: 100, ImageHeight: 100}}, + wantReason: "carry a named person", + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + server := faceSkipServer(c.faces) + defer server.Close() + client := api.NewImmichClient(server.URL, "key") + + _, regions, reason, err := appendFaceRegionChange(client, &model.Config{Faces: true}, c.asset, c.tags, nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(regions) != 0 { + t.Fatalf("expected no regions, got %d", len(regions)) + } + if !strings.Contains(reason, c.wantReason) { + t.Fatalf("reason %q does not mention %q", reason, c.wantReason) + } + }) + } +} + +func TestAppendFaceRegionChangeSilentWithoutFacesFlag(t *testing.T) { + server := faceSkipServer(nil) + defer server.Close() + + _, _, reason, err := appendFaceRegionChange( + api.NewImmichClient(server.URL, "key"), + &model.Config{Faces: false}, + model.AssetResponse{ID: "a", OriginalMimeType: "image/jpeg"}, + exif.ExifTagMap{}, nil, + ) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if reason != "" { + t.Fatalf("a run without -faces must not report a face skip, got %q", reason) + } +} + +func TestAppendFaceRegionChangeNoReasonWhenRegionsMatch(t *testing.T) { + face := model.AssetFaceResponse{ + BoundingBoxX1: 100, BoundingBoxY1: 50, BoundingBoxX2: 300, BoundingBoxY2: 250, + ImageWidth: 1000, ImageHeight: 500, Person: &model.PersonResponse{ID: "p1", Name: "Alice"}, + } + server := faceSkipServer([]model.AssetFaceResponse{face}) + defer server.Close() + + asset := model.AssetResponse{ID: "a", OriginalMimeType: "image/jpeg", People: []model.PersonResponse{{ID: "p1", Name: "Alice"}}} + tags := exif.ExifTagMap{"ImageWidth": float64(4000), "ImageHeight": float64(2000)} + + changes, regions, reason, err := appendFaceRegionChange(api.NewImmichClient(server.URL, "key"), &model.Config{Faces: true}, asset, tags, nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(changes) != 1 || len(regions) != 1 { + t.Fatalf("expected one region change, got %d changes / %d regions", len(changes), len(regions)) + } + if reason != "" { + t.Fatalf("a successful embed must report no skip reason, got %q", reason) + } +} diff --git a/src/process/faces.go b/src/process/faces.go index 6ebc230..cf218f8 100644 --- a/src/process/faces.go +++ b/src/process/faces.go @@ -21,28 +21,46 @@ func wantsFaceRegions(cfg *model.Config, asset model.AssetResponse) bool { // regions, so this must run after the exif read. A file that reports no pixel // 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 +// The third result is a human-readable reason when -faces was asked for but no +// region could be written. Every guard below used to return silently, which made +// a run that embedded nothing indistinguishable from one that had nothing to do. +func appendFaceRegionChange(client *api.ImmichClient, cfg *model.Config, asset model.AssetResponse, existing exif.ExifTagMap, changes []exif.TagChange) ([]exif.TagChange, []exif.FaceRegion, string, error) { + if !cfg.Faces { + return changes, nil, "", nil + } + if !model.HasFaceRegionsToEmbed(asset) { + if model.IsVideoAsset(asset) { + return changes, nil, "this video container cannot hold face regions", nil + } + return changes, nil, "Immich lists no named, visible person on this asset", nil } orientation, ok := regionOrientation(asset, existing) if !ok { - return changes, nil, nil + return changes, nil, fmt.Sprintf("video rotation %d° cannot be anchored (only 0°, 90° and 270° are)", intTag(existing, "Rotation")), nil } + // The faces fetch stays ahead of the dimension guard so a missing face.read + // permission is still reported loudly, even for a file with no dimensions. faces, err := client.GetAssetFaces(asset.ID) if err != nil { if isPermissionDenied(err) { - return nil, nil, fmt.Errorf("faces read denied — the -faces flag needs the API key's face.read permission: %w", err) + return nil, nil, "", fmt.Errorf("faces read denied — the -faces flag needs the API key's face.read permission: %w", err) } - return nil, nil, err + return nil, nil, "", err + } + rasterWidth, rasterHeight := intTag(existing, "ImageWidth"), intTag(existing, "ImageHeight") + if rasterWidth <= 0 || rasterHeight <= 0 { + return changes, nil, "the file reports no pixel dimensions to anchor regions against", nil } regions := exif.BuildFaceRegions(faces, orientation) - change := exif.CompareFaceRegions(regions, intTag(existing, "ImageWidth"), intTag(existing, "ImageHeight"), existing) + if len(regions) == 0 { + return changes, nil, fmt.Sprintf("none of Immich's %d face box(es) carry a named person with dimensions", len(faces)), nil + } + change := exif.CompareFaceRegions(regions, rasterWidth, rasterHeight, existing) if change == nil { - return changes, nil, nil + return changes, nil, "", nil } changes = append(changes, *change) - return changes, regions, nil + return changes, regions, "", nil } // regionOrientation returns the EXIF-orientation value to anchor face regions diff --git a/src/process/pipeline.go b/src/process/pipeline.go index bb43857..7f8485c 100644 --- a/src/process/pipeline.go +++ b/src/process/pipeline.go @@ -90,16 +90,25 @@ func ProcessAsset(client *api.ImmichClient, uploader Uploader, cfg *model.Config } changes := exif.CompareAssetMetadata(*asset, existing) - changes, faceRegions, err := appendFaceRegionChange(client, cfg, *asset, existing, changes) + changes, faceRegions, faceSkip, err := appendFaceRegionChange(client, cfg, *asset, existing, changes) if err != nil { return fail("fetch faces: %v", err) } exifArgs := exif.CollectExifArgs(changes) if len(exifArgs) == 0 { - return model.ProcessResult{AssetID: assetID, Status: model.StatusSkipped, Message: "metadata already matches", ExifMatched: true} + message := "metadata already matches" + if faceSkip != "" { + message += "; no face regions written: " + faceSkip + } + return model.ProcessResult{AssetID: assetID, Status: model.StatusSkipped, Message: message, ExifMatched: true} } diffEntries := exif.CollectDiffEntries(changes) + // Surface the reason inside the diff block: it is the one place a piped or + // -y run still prints, so "-faces wrote nothing" is never silent. + if faceSkip != "" { + diffEntries = append(diffEntries, model.DiffEntry{Tag: "Face regions", Symbol: model.DiffChange, Old: "(skipped)", New: faceSkip}) + } action := emitter.EmitDiff(model.DiffEvent{ AssetID: assetID, Filename: asset.OriginalFileName, From 81df679e7011c645abee7aa152937f5947bd85c0 Mon Sep 17 00:00:00 2001 From: Major Date: Thu, 30 Jul 2026 09:41:22 +0200 Subject: [PATCH 2/2] Do not blame the container for a video with no named people MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit HasFaceRegionsToEmbed returns false for a video for two different reasons — an unwritable container, or nobody named on the asset — and the skip reason branched on IsVideoAsset, so a perfectly supported .mp4/.MOV with no named people was told "this video container cannot hold face regions". That is the exact case this diagnostic PR is meant to clarify, and it would send the reporter chasing a container problem that does not exist. Branch on IsUnsupportedVideoAsset instead, and cover the supported-video-no-people case. --- src/process/faceSkipReason_test.go | 8 ++++++++ src/process/faces.go | 4 +++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/process/faceSkipReason_test.go b/src/process/faceSkipReason_test.go index 0505c44..ebd2994 100644 --- a/src/process/faceSkipReason_test.go +++ b/src/process/faceSkipReason_test.go @@ -48,6 +48,14 @@ func TestAppendFaceRegionChangeExplainsEverySkip(t *testing.T) { tags: sizedTags, wantReason: "container cannot hold face regions", }, + { + // A writable container with nobody named must not be blamed on the + // container: that would send the user chasing the wrong problem. + name: "supported video without named people", + asset: model.AssetResponse{ID: "a", OriginalMimeType: "video/quicktime", OriginalFileName: "IMG_4827.MOV"}, + tags: sizedTags, + wantReason: "no named, visible person", + }, { name: "video rotated 180", asset: model.AssetResponse{ID: "a", OriginalMimeType: "video/mp4", People: namedPerson}, diff --git a/src/process/faces.go b/src/process/faces.go index cf218f8..05fa6b9 100644 --- a/src/process/faces.go +++ b/src/process/faces.go @@ -29,7 +29,9 @@ func appendFaceRegionChange(client *api.ImmichClient, cfg *model.Config, asset m return changes, nil, "", nil } if !model.HasFaceRegionsToEmbed(asset) { - if model.IsVideoAsset(asset) { + // Only an unwritable container blames the container: a supported video + // with no named people must report that, not a false "unsupported" steer. + if model.IsUnsupportedVideoAsset(asset) { return changes, nil, "this video container cannot hold face regions", nil } return changes, nil, "Immich lists no named, visible person on this asset", nil