Close the roadmap's remaining known gaps - #73
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eecf92ff79
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let mut header = Vec::with_capacity(body.len() + 18); | ||
| header.extend_from_slice(b"VP8X"); | ||
| header.extend_from_slice(&10u32.to_le_bytes()); | ||
| header.push(0b0000_1000); |
There was a problem hiding this comment.
When keep_copyright is used with a transparent lossless WebP whose original container contains only a VP8L chunk, this synthesized VP8X header advertises EXIF but not the alpha feature encoded in the VP8L header. The resulting container has inconsistent feature flags, so strict WebP readers may reject it or treat transparent pixels as opaque. Derive and set the VP8X alpha bit when the VP8L bitstream reports transparency.
Useful? React with 👍 / 👎.
| } else if is_png(&encoded) { | ||
| attach_to_png(&encoded, &tiff) | ||
| } else if is_webp(&encoded) { | ||
| attach_to_webp(&encoded, &tiff) |
There was a problem hiding this comment.
For the newly supported PNG and WebP paths, max_bytes is enforced by the encoder before this post-processing step adds the EXIF chunk. If the bare encode fits the limit but the tagged result does not, the request returns an oversized response even though lowering the encoding quality could have met the limit. Include the attachment overhead in the size loop or re-check the final bytes after attaching.
Useful? React with 👍 / 👎.
eecf92f to
d90d944
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d90d9448df
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "soea" => Some(Self::SouthEast), | ||
| "sowe" => Some(Self::SouthWest), | ||
| "fp" => Some(Self::FocusPoint), | ||
| "sm" => Some(Self::Smart), |
There was a problem hiding this comment.
When a URL uses extend:true:sm or extend_aspect_ratio:true:sm, the shared gravity parser now accepts it, but both extension paths pass the value to calc_position, where Smart falls through to the center-anchor branch instead of invoking smartcrop. The request therefore succeeds while silently behaving as ce; restrict sm to crop/fill contexts, as is already done for watermark positioning, or define explicit extension semantics.
Useful? React with 👍 / 👎.
d90d944 to
e6bad1d
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6bad1d513
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| - **`strip_metadata`** drops the descriptive tags (EXIF, XMP, IPTC) and leaves the colour profile alone. **`strip_color_profile`** does the reverse. Set both to drop everything. | ||
| - **`keep_copyright`** carries the EXIF `Copyright` and `Artist` tags across a `strip_metadata`. libvips has no copyright granularity in its `keep` flags — they are `none|exif|xmp|iptc|icc|other|gainmap|all` — so imgforge reads the two fields from the source and splices a minimal EXIF segment back into the encoded result. That mechanism is JPEG-only; other output formats strip as normal, and the option is a no-op for them. | ||
| - **`keep_copyright`** carries the EXIF `Copyright` and `Artist` tags across a `strip_metadata`. libvips has no copyright granularity in its `keep` flags — they are `none|exif|xmp|iptc|icc|other|gainmap|all` — so imgforge reads the two fields from the source and writes a minimal EXIF block back into the encoded result: an APP1 segment for JPEG, an `eXIf` chunk for PNG, and an `EXIF` chunk for WebP, synthesising the extended header WebP needs to carry one. Formats that cannot hold EXIF at all strip as normal, and the option is a no-op for them. |
There was a problem hiding this comment.
This expanded PNG/WebP support is contradicted by the quick-reference row at line 46, which still states that keep_copyright is JPEG-only. Users relying on the table will incorrectly conclude that the option is a no-op for PNG and WebP, so update that row to match the behavior documented here.
Useful? React with 👍 / 👎.
e6bad1d to
78372c4
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
78372c4 to
242c4a0
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 242c4a07f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| `keep_copyright` (`kcr`) carries the EXIF copyright across a metadata strip, for every container that can hold | ||
| EXIF: an APP1 segment for JPEG, an `eXIf` chunk for PNG, and an `EXIF` chunk for WebP, synthesising the extended |
There was a problem hiding this comment.
For strip_metadata:true plus keep_copyright:true with TIFF, AVIF, or HEIF output, these supported containers can carry EXIF, but attach_copyright only handles JPEG, PNG, and WebP and returns the stripped encoding unchanged for every other format. Consequently the new claim that copyright is retained for “every container that can hold EXIF” is false; either add preservation for the remaining EXIF-capable outputs or explicitly limit the claim to the three implemented formats.
Useful? React with 👍 / 👎.
242c4a0 to
c2bd8eb
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2bd8eb712
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| #[test] | ||
| fn test_smart_gravity_finds_the_subject_a_centre_crop_would_miss() { |
There was a problem hiding this comment.
These new tests exercise processing helpers directly, but the diff adds no coverage under tests/ for the handler-level behavior introduced here, including smart-gravity URL handling and the PNG/WebP copyright response bytes. That leaves routing, option propagation, HTTP error mapping, and final encoded responses unchecked despite the repository explicitly requiring the extended integration suite to be expanded whenever processing changes.
AGENTS.md reference: AGENTS.md:L21-L21
Useful? React with 👍 / 👎.
c2bd8eb to
79a4df5
Compare
79a4df5 to
05ef538
Compare
05ef538 to
11d5dd8
Compare
11d5dd8 to
dcf1a96
Compare
22d8559 to
c3e549e
Compare
c3e549e to
78b4b0f
Compare
78b4b0f to
a108bcd
Compare
a108bcd to
27376c0
Compare
cdce7c5 to
f9d0905
Compare
f9d0905 to
d97273a
Compare
Three entries were left in the roadmap's own "Known gaps" list, one of them flagged there as cheap. They are gone. keep_copyright now works for every container that can carry EXIF, not just JPEG: an eXIf chunk for PNG and an EXIF chunk for WebP, which also means synthesising the VP8X extended header a simple WebP lacks and libwebp requires before it will carry metadata at all. Both writers replace an existing block rather than appending one — a reader takes the first it finds, and the first attempt left the copyright stranded behind whatever the encoder had already written. gravity:sm hands the window choice to libvips' smartcrop, for crop and for the implicit crop a fill performs. The roadmap called this the one Pro option that would be cheap; it was, so leaving it undone was not defensible. An unrecognised webp_options preset is now refused instead of accepted and then dropped on the way to the encoder, where a typo silently produced a different image than the URL asked for.
d97273a to
45c512a
Compare
1 participant
Stacked on #72. Three entries were left in the roadmap's own "Known gaps" list, one of them flagged there as cheap.
keep_copyrightbeyond JPEGlibvips'
keepflags have no copyright granularity, so retaining the field across a metadata strip means writing it back afterwards. That existed for JPEG only. PNG now gets aneXIfchunk and WebP anEXIFchunk — which also means synthesising theVP8Xextended header a simple WebP lacks and the container spec requires before it carries metadata at all.Both writers replace an existing block rather than appending one. The first attempt appended, and the copyright came back unreadable: a reader takes the first block it finds, and libvips had already written one.
The PNG chunk needs a CRC-32; it is computed bitwise (a copyright string is a few dozen bytes, so a table would cost more than it saves) and checked in a test against the published
IENDconstant.Smart gravity
gravity:smhands the window choice to libvips'smartcrop, which scores the image for the region a viewer's eye would settle on — the one thing a geometric anchor cannot do, and the answer when no fixed anchor is right for every image in a catalogue. It applies tocropand to the implicit crop afillperforms. imgproxy gates this behind its Pro tier.The roadmap called it the one Pro entry that would be cheap. It was, so leaving it undone was not defensible.
webp_optionspresetAn unrecognised name was accepted and then dropped on the way to the encoder, so a typo silently produced a different image. It is refused now, like every other bad argument. The suffix builder still matches against the known set rather than interpolating on trust — a preset carrying
],losslesswould otherwise close the bracket and set an option the request never asked for.Notes for review
The smart-gravity tests needed an image with exactly one thing worth looking at; a uniform image gives an attention map indistinguishable from noise. They assert the smart crop is measurably darker than the centre crop, i.e. that it actually found the subject.
Validation