Implement the Pro options that were only ever cheap - #74
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: 55d514be0a
ℹ️ 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 (requested_width, requested_height) = match aspect_ratio { | ||
| Some(aspect_ratio) => aspect_ratio.correct(requested_width, requested_height), | ||
| None => (requested_width, requested_height), | ||
| }; | ||
|
|
||
| let width = requested_width.min(src_width).max(1); | ||
| let height = requested_height.min(src_height).max(1); |
There was a problem hiding this comment.
When both requested crop dimensions exceed the source, applying the aspect-ratio correction before these independent clamps can discard the correction even when a valid crop is possible. For example, a 100×100 source with crop:1000:1000 and car:2 is corrected to 1000×500 and then clamped to 100×100, whereas the default shrinking behavior could produce 100×50. Clamp the resolved crop window to the source before applying the shrinking correction.
Useful? React with 👍 / 👎.
| let mut adders = [ | ||
| f64::from(colorize.color[0]) * opacity, | ||
| f64::from(colorize.color[1]) * opacity, | ||
| f64::from(colorize.color[2]) * opacity, | ||
| ]; |
There was a problem hiding this comment.
With a 16-bit source, preserve_hdr:true, and a high-bit-depth output such as PNG, these adders are still expressed in the 8-bit 0–255 range while the image channels span 0–65535. Consequently, colorize:1:ff0000 produces [255,0,0] in a 16-bit image—nearly black rather than full red. The requested color must be scaled to the source band range before blending; the duotone shadow offset has the same range assumption.
Useful? React with 👍 / 👎.
55d514b to
c834c40
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c834c40caf
ℹ️ 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".
| pub struct WatermarkSize { | ||
| pub width: u32, | ||
| pub height: u32, | ||
| } |
There was a problem hiding this comment.
Imgproxy-compatible watermark_size URLs include a third enlarge argument, but this type has nowhere to store it and parse silently ignores it. Consequently, both wms:100:100:false and wms:100:100:true follow the unconditional scaling in size_watermark, enlarging a smaller logo even when the URL forbids it and producing different output for migrated URLs. Preserve the flag and cap the scale at the watermark's natural size when it is false.
AGENTS.md reference: AGENTS.md:L1-L2
Useful? React with 👍 / 👎.
| match img.get_format() { | ||
| Ok(ops::BandFormat::Ushort) | Ok(ops::BandFormat::Short) => 65535.0, | ||
| _ => 255.0, |
There was a problem hiding this comment.
For a signed 16-bit frame that reaches tone mapping, BandFormat::Short tops out at 32767, not 65535. Grouping it with Ushort makes values such as colorize:1:808080 calculate an adder around 32896 and then clip to 32767 on the cast back, turning mid-gray into white; duotone shadow offsets clip similarly. Fresh evidence after the prior high-bit-depth fix is this new match arm explicitly grouping Short with Ushort; handle the signed range separately.
Useful? React with 👍 / 👎.
c834c40 to
11f4818
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11f48182b7
ℹ️ 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".
| if colorize.keep_alpha { | ||
| rejoin_alpha(washed, alpha) | ||
| } else { | ||
| Ok(washed) |
There was a problem hiding this comment.
When the source has transparency and keep_alpha is omitted or false, any positive colorize opacity drops the alpha band entirely, making every pixel fully opaque; for example, colorize:0.01:ff0000 turns a fully transparent pixel opaque instead of moving its alpha only slightly toward the wash color. Blend the original alpha toward the requested color's alpha using the same opacity, reserving the unchanged-alpha path for keep_alpha:true.
AGENTS.md reference: AGENTS.md:L1-L2
Useful? React with 👍 / 👎.
11f4818 to
7f192ee
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f192ee74d
ℹ️ 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".
| fn blend(base: &VipsImage, tinted: &VipsImage, intensity: f64) -> Result<VipsImage, TransformError> { | ||
| let format = base.get_format(); | ||
|
|
||
| let kept = ops::linear(base, &mut [1.0 - intensity; 3], &mut [0.0; 3]).map_err(vips("Error blending tone"))?; |
There was a problem hiding this comment.
For a one-band BW source, or a two-band grayscale-plus-alpha source after split_alpha, base has one band while this call supplies three coefficients; libvips linear requires either one coefficient or one per input band. Consequently, monochrome and duotone fail here, while colorize fails at its analogous three-coefficient call, even though to_processing deliberately leaves BW/Grey16 sources grayscale. Promote the color portion to three bands before applying these RGB effects.
Useful? React with 👍 / 👎.
| let scale = f64::from(width) / wm_w; | ||
| (scale, scale) | ||
| } | ||
| (width, height) => (f64::from(width) / wm_w, f64::from(height) / wm_h), |
There was a problem hiding this comment.
When both dimensions are nonzero, applying independent horizontal and vertical scales stretches the watermark instead of fitting it within the requested box. For example, a natural 100×50 logo with wms:100:100 becomes 100×100 rather than remaining 100×50. Imgproxy-compatible watermark_size preserves the watermark's aspect ratio, so this branch should select one fit scale rather than two independent factors.
AGENTS.md reference: AGENTS.md:L1-L2
Useful? React with 👍 / 👎.
| debug!("Applying watermark with options: {:?}", watermark_opts); | ||
| img = watermark::apply_watermark(img, source, watermark_opts, options.resizing_algorithm.as_deref())?; | ||
| let placement = watermark::WatermarkPlacement { | ||
| size: options.watermark_size, |
There was a problem hiding this comment.
For requests combining watermark_size with dpr greater than 1, the parsed dimensions are passed through unchanged, whereas imgproxy defines watermark width and height as DPR-scaled dimensions. Thus wms:40:20/dpr:2 renders a 40×20 watermark instead of 80×40, making migrated high-density-image URLs produce undersized overlays. Scale the explicit watermark dimensions during DPR normalization before constructing this placement.
AGENTS.md reference: AGENTS.md:L1-L2
Useful? React with 👍 / 👎.
7f192ee to
9fd8418
Compare
9fd8418 to
7fcbd91
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! 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". |
7fcbd91 to
9d120f6
Compare
9d120f6 to
e677932
Compare
e677932 to
23838be
Compare
23838be to
3cb07e4
Compare
3cb07e4 to
449bb3b
Compare
449bb3b to
346fdbe
Compare
346fdbe to
0d415c4
Compare
0d415c4 to
2f742e5
Compare
2f742e5 to
6f94105
Compare
6f94105 to
60704d7
Compare
60704d7 to
c52e314
Compare
c52e314 to
da909a2
Compare
The roadmap listed these as unimplemented with a note that none of them was hard. That is not a defensible place for an option to sit, so they are implemented: crop_aspect_ratio, monochrome, duotone, colorize, watermark_size and watermark_rotate. imgproxy charges for all six. The three tone effects share a shape — derive a colour per pixel, then blend it over the original by an intensity — and differ only in how the colour is derived, so they live together in transform::tone rather than scattered through effects where they would read as unrelated filters. All three leave alpha alone: folding a tint into opacity would fade the image rather than colour it. What is left on that list needs something libvips does not provide — an object detector, a quality search loop, a CSS parser, a video decoder.
da909a2 to
6fe0f42
Compare
1 participant
Stacked on #73. Last of the stack.
The roadmap listed these as unimplemented with a note that none of them was hard. That is not a defensible place for an option to sit, so they are implemented — and imgproxy charges for all six.
crop_aspect_ratio(car) corrects the crop area's shape without moving it. Shrinks the long axis by default, which can never ask for pixels the source does not have;enlargegrows the short axis instead.monochrome(mc) scales a base colour by each pixel's luminance, so the result keeps the image's tonal structure and loses only its hue.duotone(dt) interpolates between two colours across the tonal range.colorize(col) ignores luminance and washes a flat colour over everything.watermark_size(wms) andwatermark_rotate(wmr) size and turn the watermark explicitly. Rotation comes after sizing, so the requested size describes the watermark rather than its bounding box once turned.Structure
The three tone effects share a shape — derive a colour per pixel, then blend it over the original by an intensity — and differ only in how the colour is derived, so they live together in a new
transform::tonerather than scattered througheffectswhere they would read as unrelated filters. All three leave alpha alone: folding a tint into opacity would fade the image rather than colour it.What is left
autoquality, object detection,gradient,unsharp_masking,style,dpi,hashsum,watermark_text,fallback_image_urland thevideo_thumbnail_*family stay unimplemented. Each needs something libvips does not provide on its own — a detector, a quality search loop, a CSS parser, a video decoder — and the roadmap now says so rather than calling any of them easy.Validation