Explain why -faces embedded no regions (issue #30)

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)
+	}
+}

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

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,