Correct quality precedence and cache-hit header consistency

ROADMAP.md

@@ -21,10 +21,10 @@ merely parsed, with the exceptions listed under **Known gaps** below.
   the EXIF `Copyright` and `Artist` fields from the source and splices a minimal EXIF segment into the encoded
   result. That mechanism only exists for JPEG. PNG and WebP can carry EXIF too, and the same approach would work
   for them; nobody has needed it yet.
-- **`preserve_hdr` on libvips below 8.16** — the `gainmap` keep flag does not exist there, and naming it makes the
-  encode fail rather than degrade. The published image ships 8.16.1, so this only affects a build against an older
-  system libvips. A runtime version check that drops the flag would fix it; the cost is a `vips_version` call at
-  startup and a branch in the suffix builder.
+- **`preserve_hdr` on libvips below 8.16** — the `gainmap` keep flag does not exist there. imgforge checks the
+  runtime version once and drops the flag rather than naming it, so the request succeeds and loses only the gain
+  map; the high bit-depth half still works. The drop is logged. The published image ships 8.16.1 and is
+  unaffected.
 - **`webp_options` preset** — only libvips' own preset names reach the encoder. Others are ignored rather than
   failing, because an unknown name makes libvips reject the whole encode.
 

doc/5_processing_options.md

@@ -44,7 +44,7 @@ Unrecognised directive *names* are ignored rather than rejected, so a typo silen
 | `strip_metadata`        | `sm`       | `bool`                                                      | Drops encoder metadata when supported by the output format.                                                  |
 | `strip_color_profile`   | `scp`      | `bool`                                                      | Drops the embedded colour profile, leaving other metadata alone.                                             |
 | `keep_copyright`        | `kcr`      | `bool`                                                      | Retains the EXIF copyright and artist tags across a metadata strip. JPEG output only.                        |
-| `preserve_hdr`          | `ph`       | `bool`                                                      | Keeps a high bit-depth image high bit-depth and carries its gain map through. Needs libvips 8.16+.           |
+| `preserve_hdr`          | `ph`       | `bool`                                                      | Keeps a high bit-depth image high bit-depth and carries its gain map through. Gain maps need libvips 8.16+; older builds keep the depth and drop the map. |
 | `enforce_thumbnail`     | `eth`      | `bool`                                                      | Uses the source's embedded EXIF thumbnail instead of the full image when one is present.                     |
 | `jpeg_options`          | `jpgo`     | `progressive:no_subsample:trellis:dering:scans:quant_table` | Advanced JPEG encoder switches.                                                                              |
 | `png_options`           | `pngo`     | `interlaced:quantize:colors`                                | Advanced PNG encoder switches.                                                                               |
@@ -248,7 +248,7 @@ A result too large for its output container is scaled down to fit rather than ha
 
 - **`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.
-- **`preserve_hdr`** keeps a high bit-depth source at its own depth when the output format can carry it (PNG, TIFF, AVIF, HEIF) and retains the gain map that makes the image HDR, even while other metadata is being stripped. The gain-map flag needs libvips 8.16 or later; on an older build, enabling it makes the encode fail.
+- **`preserve_hdr`** keeps a high bit-depth source at its own depth when the output format can carry it (PNG, TIFF, AVIF, HEIF) and retains the gain map that makes the image HDR, even while other metadata is being stripped. The gain-map half needs libvips 8.16 or later, where the `gainmap` keep flag was added. On an older build imgforge detects the runtime version and drops that flag rather than failing: the request succeeds, keeps its bit depth, and loses only the gain map. A successful response on such a build is therefore not proof that the gain map survived — the drop is logged when it happens.
 - **`enforce_thumbnail`** uses the source's embedded EXIF thumbnail in place of the full image whenever one is present, which turns a large JPEG into a very cheap request. The thumbnail is usually a few hundred pixels wide, so the result is only as good as that; a thumbnail that will not decode falls back to the full image rather than failing.
 
 ### `background`

src/processing/mod.rs

@@ -151,14 +151,10 @@ pub fn process_image(
         }
     }
 
-    let quality = parsed_options
-        .quality
-        .or_else(|| parsed_options.save.format_quality.get(&output_format).copied())
-        .unwrap_or(85);
     let mut output_vec = save::save_image_with_options(
         img,
         &output_format,
-        quality,
+        parsed_options.quality_for(&output_format),
         &parsed_options.save,
         page_height.filter(|_| save::format_supports_animation(&output_format)),
     )?;

src/processing/options/mod.rs

@@ -25,6 +25,9 @@ use names::*;
 use std::str::FromStr;
 use tracing::debug;
 
+/// Encoder quality when nothing in the request or the configuration names one.
+pub const DEFAULT_QUALITY: u8 = 85;
+
 /// Represents a single image processing option from the URL path.
 #[derive(Debug, Clone)]
 pub struct ProcessingOption {
@@ -45,8 +48,11 @@ pub struct ParsedOptions {
     pub crop: Option<Crop>,
     /// Optional output image format.
     pub format: Option<String>,
-    /// Optional output image quality (1-100).
+    /// Output image quality (1-100) named by the URL.
     pub quality: Option<u8>,
+    /// Server-configured quality, used only when neither the URL's `quality`
+    /// nor its `format_quality` says anything.
+    pub default_quality: Option<u8>,
     /// Optional background color for transparent areas or extending.
     pub background: Option<[u8; 4]>, // RGBA array
     /// Optional target width (used with `resize` if no explicit resize type).
@@ -176,7 +182,8 @@ impl ParsedOptions {
             blur: None,
             crop: None,
             format: None,
-            quality: defaults.quality,
+            quality: None,
+            default_quality: defaults.quality,
             background: None,
             width: None,
             height: None,
@@ -229,6 +236,20 @@ impl ParsedOptions {
         }
     }
 
+    /// The encoder quality for an output format.
+    ///
+    /// imgproxy's precedence, which is what a URL author expects: the URL's own
+    /// `quality` first, then its per-format `format_quality`, then whatever the
+    /// server configured, and finally the built-in default. Seeding the URL's
+    /// `quality` from the configuration instead would have made a configured
+    /// default silently outrank a `format_quality` the URL asked for.
+    pub fn quality_for(&self, format: &str) -> u8 {
+        self.quality
+            .or_else(|| self.save.format_quality.get(format).copied())
+            .or(self.default_quality)
+            .unwrap_or(DEFAULT_QUALITY)
+    }
+
     /// The zoom factors in effect, defaulting to no zoom.
     pub fn zoom_factors(&self) -> Zoom {
         self.zoom.unwrap_or_default()

src/processing/save.rs

@@ -247,7 +247,12 @@ fn metadata_keep(options: &SaveOptions) -> String {
     // A gain map is what makes an HDR image high dynamic range; it is neither
     // descriptive metadata nor a colour profile, so neither strip option should
     // take it away when the request explicitly asked to preserve it.
-    if options.preserve_hdr.unwrap_or(false) {
+    //
+    // The flag itself only exists from libvips 8.16. Naming it on an older
+    // build makes the option-string parser reject the whole encode, so a
+    // deployment linked against an older system libvips would fail every
+    // request that set preserve_hdr rather than merely losing the gain map.
+    if options.preserve_hdr.unwrap_or(false) && supports_gainmap_flag() {
         flags.push("gainmap");
     }
 
@@ -258,6 +263,20 @@ fn metadata_keep(options: &SaveOptions) -> String {
     }
 }
 
+/// Whether this libvips knows the `gainmap` metadata flag, added in 8.16.
+fn supports_gainmap_flag() -> bool {
+    static SUPPORTED: OnceLock<bool> = OnceLock::new();
+    *SUPPORTED.get_or_init(|| {
+        // vips_version(0) is the major number and vips_version(1) the minor.
+        let (major, minor) = unsafe { (bindings::vips_version(0), bindings::vips_version(1)) };
+        let supported = major > 8 || (major == 8 && minor >= 16);
+        if !supported {
+            debug!("libvips {major}.{minor} has no gainmap keep flag; preserve_hdr will not retain one");
+        }
+        supported
+    })
+}
+
 /// Builds a libvips save suffix: `.png[option,option=value]`.
 struct Suffix {
     parts: Vec<String>,

src/processing/tests/options_parse_tests.rs

@@ -1218,3 +1218,50 @@ fn disable_animation_outranks_an_explicit_page_count() {
         "an animated output with no disable should read every frame"
     );
 }
+
+/// The URL's own `quality` beats its `format_quality`, which beats whatever the
+/// server configured. Seeding the URL's quality from the configuration instead
+/// would have made a configured default silently outrank a `format_quality` the
+/// URL asked for.
+#[test]
+fn test_quality_precedence_runs_url_first_then_format_then_configuration() {
+    use crate::processing::options::{parse_all_options_with_defaults, OptionDefaults};
+
+    let configured = OptionDefaults {
+        quality: Some(50),
+        ..OptionDefaults::default()
+    };
+
+    let format_quality = || ProcessingOption {
+        name: "format_quality".to_string(),
+        args: vec!["webp".to_string(), "70".to_string()],
+    };
+
+    // Nothing in the URL: the configured default applies.
+    let parsed = parse_all_options_with_defaults(Vec::new(), configured).unwrap();
+    assert_eq!(parsed.quality_for("webp"), 50);
+
+    // A per-format quality outranks the configured default.
+    let parsed = parse_all_options_with_defaults(vec![format_quality()], configured).unwrap();
+    assert_eq!(parsed.quality_for("webp"), 70);
+    // ...for that format only.
+    assert_eq!(parsed.quality_for("jpeg"), 50);
+
+    // An explicit quality outranks both.
+    let parsed = parse_all_options_with_defaults(
+        vec![
+            format_quality(),
+            ProcessingOption {
+                name: "quality".to_string(),
+                args: vec!["95".to_string()],
+            },
+        ],
+        configured,
+    )
+    .unwrap();
+    assert_eq!(parsed.quality_for("webp"), 95);
+
+    // With nothing configured at all, the built-in default stands.
+    let parsed = parse_all_options(Vec::new()).unwrap();
+    assert_eq!(parsed.quality_for("webp"), 85);
+}

src/processing/tests/save_tests.rs

@@ -211,12 +211,17 @@ fn stripping_one_kind_of_metadata_keeps_the_other() {
     );
 
     // A gain map is what makes an image HDR, so preserving HDR has to keep it
-    // even while everything else is being stripped.
+    // even while everything else is being stripped — on a libvips that has the
+    // flag. On an older one the flag is dropped rather than named, because
+    // naming it would make the option-string parser reject the whole encode.
+    init_vips();
     options.strip_metadata = Some(true);
     options.preserve_hdr = Some(true);
-    assert_eq!(
-        save::save_suffix("avif", 80, &options, None).unwrap(),
-        ".avif[Q=80,compression=av1,effort=7,subsample-mode=auto,keep=gainmap]"
+    let suffix = save::save_suffix("avif", 80, &options, None).unwrap();
+    assert!(
+        suffix == ".avif[Q=80,compression=av1,effort=7,subsample-mode=auto,keep=gainmap]"
+            || suffix == ".avif[Q=80,compression=av1,effort=7,subsample-mode=auto,keep=none]",
+        "unexpected suffix: {suffix}"
     );
 }
 

src/service/cache_key.rs

@@ -23,6 +23,17 @@ use crate::processing::options::OptionDefaults;
 use sha2::{Digest, Sha256};
 use std::borrow::Cow;
 
+/// Bumped whenever a release changes the bytes an unchanged URL produces.
+///
+/// A disk or hybrid cache outlives the version that filled it, and imgforge has
+/// no TTL — an entry survives until capacity evicts it. Without this, upgrading
+/// leaves frequently requested URLs serving their old output indefinitely, and
+/// the operator's only recourse is to change every `cachebuster` or wipe the
+/// cache. 0.18.0 changes the result of, among others, any URL using
+/// `format_quality` with a configured quality, an uncentred `crop`, a
+/// watermark, `brightness`/`contrast`, or a rotation with a resize.
+const OUTPUT_VERSION: &str = "v2";
+
 /// Everything outside the URL path that changes the response bytes, or that
 /// decides whether the response may be produced at all.
 #[derive(Debug, Clone, Copy)]
@@ -84,15 +95,16 @@ pub struct CacheKeyParts<'a> {
 }
 
 pub fn processed_cache_key<'a>(parts: CacheKeyParts<'a>) -> Cow<'a, str> {
-    // A raw response is the untouched source, so the format decision and the
-    // result ceiling cannot apply to it — but the limits describing the source
-    // very much can, and they are the only thing standing between a tightened
-    // policy and the bytes already in the cache.
+    // A raw response is the untouched source: nothing is processed, so neither
+    // the format decision, the result ceiling, nor the output version can apply
+    // to it — the bytes are the origin's, and they have not changed. The limits
+    // describing the *source* are the exception, and they are the only thing
+    // standing between a tightened policy and the bytes already in the cache.
     if parts.is_raw {
         return source_limits(parts, Cow::Owned(source_scoped(parts)));
     }
 
-    let base = if parts.has_explicit_format {
+    let base: Cow<'a, str> = if parts.has_explicit_format {
         Cow::Owned(source_scoped(parts))
     } else {
         match parts.negotiated_format {
@@ -135,7 +147,7 @@ pub fn processed_cache_key<'a>(parts: CacheKeyParts<'a>) -> Cow<'a, str> {
         None => base,
     };
 
-    source_limits(parts, base)
+    source_limits(parts, Cow::Owned(format!("{OUTPUT_VERSION}:{base}")))
 }
 
 /// The request path, scoped to the source it actually resolves to.

src/service/mod.rs

@@ -166,10 +166,12 @@ pub async fn process_path(state: Arc<AppState>, request: ProcessRequest<'_>) ->
     }
     let vary = vary_headers(config);
 
-    // Resolved before the cache lookup: a source the deployment no longer
-    // permits must stop being served, and a persistent cache would otherwise
-    // keep answering for it long after `IMGFORGE_ALLOWED_SOURCES` was tightened
-    // — the request never reaches the check because it never reaches the fetch.
+    // Resolved before the cache lookup for two reasons. A source the deployment
+    // no longer permits must stop being served, and a persistent cache would
+    // otherwise keep answering for it long after `IMGFORGE_ALLOWED_SOURCES` was
+    // tightened — the request never reaches the check because it never reaches
+    // the fetch. And the canonical header has to name the same URL on a hit as
+    // on a miss.
     let decoded_url = resolve_source_url(config, &url_parts)?;
 
     // The watermark's URL is part of the request too, so it is checked where
@@ -234,18 +236,17 @@ pub async fn process_path(state: Arc<AppState>, request: ProcessRequest<'_>) ->
     if let Some(cached_image) = cached_image {
         debug!("Image found in cache for path={}", path);
 
-        // A cache hit has no source response to draw on, so the origin's own
-        // caching headers are not available; the configured policy still is,
-        // and the entity tag comes from the bytes either way.
-        // The origin's delivery metadata was stored with the entry, so a hit
-        // keeps saying what the origin said — a passthrough `no-store` must
-        // not vanish the moment the cache starts answering.
+        // The origin's delivery metadata was stored with the entry and the
+        // source URL is resolved either way, so a hit keeps saying what the
+        // origin said — a passthrough `no-store` must not vanish the moment
+        // the cache starts answering — and keeps naming the same canonical
+        // address a miss would.
         let cached_source = SourceMetadata {
             cache_control: (!cached_image.origin_cache_control.is_empty())
                 .then(|| cached_image.origin_cache_control.clone()),
             last_modified: (!cached_image.origin_last_modified.is_empty())
                 .then(|| cached_image.origin_last_modified.clone()),
-            url: None,
+            url: Some(decoded_url.clone()),
         };
         let headers = DeliveryHeaders::for_cache_hit(config, &cached_source, &cached_image.etag, &vary);