diff --git a/src/process/faceSkipReason_test.go b/src/process/faceSkipReason_test.go new file mode 100644 index 0000000..ebd2994 --- /dev/null +++ b/src/process/faceSkipReason_test.go @@ -0,0 +1,141 @@ +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", + }, + { + // 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}, + 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..05fa6b9 100644 --- a/src/process/faces.go +++ b/src/process/faces.go @@ -21,28 +21,48 @@ 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) { + // 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 } 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,