Skip to content

Leave out images whose file carries an EXIF orientation - #18

Merged
06pratyush merged 2 commits into
mainfrom
eval-orientation
Oct 28, 2026
Merged

06pratyush merged 2 commits into
mainfrom
eval-orientation

Conversation

@shashwat-deep

@shashwat-deep shashwat-deep commented Oct 28, 2026 •

Copy link
Copy Markdown
Owner

Follows Pratyush's review of #10. Stacked on #17 (eval-hashes); once #17 is merged, this PR moves to main.

What changes

  • eval/build_manifest.py leaves out any image whose file carries an EXIF orientation other than 1, or EXIF data that Pillow cannot read, and takes the next image in its split's ranking, checking that one in turn. The browser turns an image by its EXIF orientation, and Open Images' boxes and Rotation value do not say whether they allow for it. No image of the current draw is affected: eval/manifest/ is unchanged.
  • It also prints how many drawn files are larger than 1,024 px on the long side, carry EXIF data, or carry an ICC profile. For the current draw: 2, 2 and 430, and 0 left out. The two large files are the tune images 6ae23606d22bad2d (3780 x 6111) and cce0d9c5636f322e (6000 x 4000), both with EXIF, IPTC, XMP and ICC data, and Rotation 0.
  • run_detect.py, render_copies.py and check_colour.py refuse such a file before the browser starts, whatever the transform, and transforms.upright() refuses it too.
  • The runner gives an image stored turned its own orientation tag in place of any EXIF data the file has, where it used to refuse a turned file with EXIF data. So every image the builder keeps can be served. The 15 turned images of the current draw have no EXIF data and are served byte for byte as before.
  • The orientation is read as Chromium reads it (transforms.exif_orientation()): from the first EXIF segment only, and not from XMP's tiff:Orientation, which Pillow's getexif() also reports. A test checks this in Chromium 156.
  • docs/eval-protocol.md (part B: Data, Exclusions, Orientation and the tiled setting) and eval/README.md no longer say that every CVDF copy is at most 1,024 px with all metadata removed, and list the new exclusion. No evaluation run has been made yet, so this is not a deviation from the protocol.

Checks

  • .venv/bin/python -m pytest eval/tests: 104 passed.
  • Removing each new rule in turn (the builder's exclusion, the runner's refusal, the as_stored condition, the EXIF-only reading, the replacement of EXIF data, the unreadable case, check_colour.py's check) makes a test fail.
  • eval/build_manifest.py on the real metadata leaves eval/manifest/ byte for byte unchanged, and all 1,000 images pass check_orientations().
  • /code-review (max): 9 findings, all fixed in the second commit.

🤖 Generated with Claude Code

shashwat-deep and others added 2 commits October 28, 2026 16:45
The browser turns an image by its EXIF orientation, and Open Images' boxes
and Rotation value do not say whether they allow for it. build_manifest.py
now leaves out any image whose file carries an orientation other than 1 and
takes the next one in its split's ranking; the runner, render_copies.py and
transforms.upright() refuse such a file. The orientation is read from the
EXIF data only: Chromium ignores XMP's tiff:Orientation, which Pillow's
getexif() also reports. No image of the current draw is affected.

build_manifest.py also prints how many drawn files are larger than 1,024 px
or carry EXIF data or an ICC profile, and the protocol and eval README no
longer say that every CVDF copy is at most 1,024 px with all metadata
removed. Follows Pratyush's review of #10.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- The runner gives an image stored turned its own EXIF orientation tag in
  place of any EXIF data the file has, instead of refusing such a file, so
  every image the builder keeps can be served. The current draw is served
  byte for byte as before.
- EXIF data that Pillow cannot read counts as 'unreadable', which the
  builder leaves out and the runner refuses, instead of a crash. Like
  Chromium, only the first EXIF segment counts (checked in Chromium).
- check_colour.py runs the orientation check before Chromium starts.
- The builder fetches and checks only images it has not checked before.
- The refusal message no longer says the page would apply the tag, which
  is wrong for transforms served as PNGs.
- The docs say which counts the builder prints; the made-up Open Images
  files carry EXIF data only where a test needs it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@shashwat-deep
shashwat-deep marked this pull request as ready for review October 28, 2026 12:05
@06pratyush
06pratyush changed the base branch from eval-hashes to main October 28, 2026 17:00

@06pratyush 06pratyush left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. I checked:

  • the exclude-and-replace loop in build_manifest.py: each image left out for its EXIF orientation is replaced by the next one in its split's ranking;
  • the refusals in run_detect.py, render_copies.py, check_colour.py and transforms.upright();
  • the runner replacing the EXIF data of an image stored turned with its own orientation tag;
  • the test that Chromium turns an image by its first EXIF segment only.

The eval tests pass on Windows at the top of the stack (104, none skipped).

@06pratyush
06pratyush merged commit 7e3717b into main Oct 28, 2026
2 checks passed
@06pratyush
06pratyush deleted the eval-orientation branch October 28, 2026 17:01
06pratyush added a commit that referenced this pull request Oct 28, 2026
Resolves the conflict in docs/ai-use.md, where main added the rows for
#17, #18 and #19 and this branch has its own: keeps all four, main's
rows first.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
06pratyush added a commit that referenced this pull request Oct 28, 2026
Resolves the conflict in docs/ai-use.md, where main added the rows for
#15, #17, #18 and #19 and this branch has its own: keeps all five,
main's rows first. The merge brings in only eval/ and docs changes;
nothing under app/ or tests/ changes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants