timothy/ersatztv
1
0
Fork 0
Code Issues 57 Pull Requests 6 Actions Packages Projects Releases 9 Wiki Activity

Anamorphic HDR sources are excluded from the QSV OpenCL tonemap (runtime sar vs calculated SAR) #660

New Issue
Open
opened 2026-07-26 22:37:00 +02:00 by timothy · 1 comment
timothy commented 2026-07-26 22:37:00 +02:00
Owner
Copy Link
Copy Source

Accepted residual from #505 (PR #654), filed so it is tracked rather than living only in a decision record.

What was done and why

#505 moved QSV HDR tonemapping onto the GPU via VA-API→OpenCL, which required the downscale to run in ScaleVaapiFilter instead of ScaleQsvFilter. Those two filters get their sample aspect ratio from different sources:

source of SAR
ScaleQsvFilter the SAR C# calculates — VideoStream.SampleAspectRatio (ErsatzTV.FFmpeg/MediaStream.cs), which has an explicit fallback when SAR is missing or 0:0
ScaleVaapiFilter ffmpeg's runtime sar expression (scale_vaapi=iw*sar:ih, ErsatzTV.FFmpeg/Filter/Vaapi/ScaleVaapiFilter.cs)

When the decoded frame leaves SAR unspecified, those are not the same value. Rather than ship an anamorphic HDR graph nobody had run on real hardware, QsvPipelineBuilder.UseOpenClTonemap returns false for videoStream.IsAnamorphic.

Current impact: none — this is an optimization gap, not a defect

Anamorphic HDR sources fall back to the software tonemap, which is exactly what they got before #505. Nothing regressed; they simply don't get the ~60% CPU saving the OpenCL path delivers. Anamorphic + HDR is a rare combination (HDR is overwhelmingly modern progressive square-pixel content), which is why this was judged not worth blocking #505 on.

The exclusion is in UseOpenClTonemap alongside the other reachability guards, with the reasoning in a comment.

Why it wasn't just fixed

It cannot be validated without a real anamorphic HDR sample on the Intel host (jazz, 192.168.1.29). The failure mode is a wrong-dimensions / wrong-proportions picture, not an error — so exit code 0 proves nothing here, exactly as with the vpp_qsv=tonemap no-op that #505 uncovered. Guessing at it and asserting "it built fine" would repeat the mistake #505 exists to correct.

What a fix looks like

Either:

  • pass videoStream.SampleAspectRatio (the calculated one) into this scale path and pin it explicitly — e.g. emit a setsar ahead of scale_vaapi so the runtime sar variable holds the calculated value; or
  • confirm by measurement that ffmpeg's runtime sar is already correct for our sources, and drop the exclusion.

Note ScaleVaapiFilter is shared with VaapiPipelineBuilder, so changing the filter itself alters the VAAPI pipeline too — prefer adding a step in the QSV path over editing the shared filter (see #505's review, where a shared-filter swap silently broke image-subtitle scaling).

Reference

  • Decision record ffmpeg.qsv-hdr-tonemap-opencl (docs/decisions/records/ffmpeg/) — records this as an accepted residual.
  • Found by the cold review of PR #654 (rated Medium, "currently a hypothesis").

Done-when

  • adversarial review passed
  • a real anamorphic HDR source is transcoded on the Intel QSV host and the output verified for dimensions, SAR and DAR (not just exit status), on both the current software-tonemap path and the proposed OpenCL one
  • either the IsAnamorphic exclusion is removed with that evidence, or the record is updated to say it is permanent and why
  • VaapiPipelineBuilder's behavior is demonstrably unchanged if ScaleVaapiFilter is touched at all
Accepted residual from #505 (PR #654), filed so it is tracked rather than living only in a decision record. ## What was done and why #505 moved QSV HDR tonemapping onto the GPU via VA-API→OpenCL, which required the downscale to run in `ScaleVaapiFilter` instead of `ScaleQsvFilter`. Those two filters get their sample aspect ratio from **different sources**: | | source of SAR | |---|---| | `ScaleQsvFilter` | the SAR **C# calculates** — `VideoStream.SampleAspectRatio` (`ErsatzTV.FFmpeg/MediaStream.cs`), which has an explicit fallback when SAR is missing or `0:0` | | `ScaleVaapiFilter` | ffmpeg's **runtime `sar` expression** (`scale_vaapi=iw*sar:ih`, `ErsatzTV.FFmpeg/Filter/Vaapi/ScaleVaapiFilter.cs`) | When the decoded frame leaves SAR unspecified, those are not the same value. Rather than ship an anamorphic HDR graph nobody had run on real hardware, `QsvPipelineBuilder.UseOpenClTonemap` returns `false` for `videoStream.IsAnamorphic`. ## Current impact: none — this is an optimization gap, not a defect Anamorphic HDR sources fall back to the **software** tonemap, which is exactly what they got before #505. Nothing regressed; they simply don't get the ~60% CPU saving the OpenCL path delivers. Anamorphic + HDR is a rare combination (HDR is overwhelmingly modern progressive square-pixel content), which is why this was judged not worth blocking #505 on. The exclusion is in `UseOpenClTonemap` alongside the other reachability guards, with the reasoning in a comment. ## Why it wasn't just fixed It cannot be validated without a real anamorphic HDR sample on the Intel host (jazz, `192.168.1.29`). The failure mode is a *wrong-dimensions / wrong-proportions* picture, not an error — so exit code 0 proves nothing here, exactly as with the `vpp_qsv=tonemap` no-op that #505 uncovered. Guessing at it and asserting "it built fine" would repeat the mistake #505 exists to correct. ## What a fix looks like Either: - pass `videoStream.SampleAspectRatio` (the calculated one) into this scale path and pin it explicitly — e.g. emit a `setsar` ahead of `scale_vaapi` so the runtime `sar` variable holds the calculated value; or - confirm by measurement that ffmpeg's runtime `sar` is already correct for our sources, and drop the exclusion. Note `ScaleVaapiFilter` is **shared with `VaapiPipelineBuilder`**, so changing the filter itself alters the VAAPI pipeline too — prefer adding a step in the QSV path over editing the shared filter (see #505's review, where a shared-filter swap silently broke image-subtitle scaling). ## Reference - Decision record `ffmpeg.qsv-hdr-tonemap-opencl` (`docs/decisions/records/ffmpeg/`) — records this as an accepted residual. - Found by the cold review of PR #654 (rated Medium, "currently a hypothesis"). ## Done-when - [ ] adversarial review passed - [ ] a real anamorphic HDR source is transcoded on the Intel QSV host and the output verified for **dimensions, SAR and DAR** (not just exit status), on both the current software-tonemap path and the proposed OpenCL one - [ ] either the `IsAnamorphic` exclusion is removed with that evidence, or the record is updated to say it is permanent and why - [ ] `VaapiPipelineBuilder`'s behavior is demonstrably unchanged if `ScaleVaapiFilter` is touched at all
timothy added the enhancementpriority: low labels 2026-07-26 22:37:00 +02:00
timothy commented 2026-07-26 22:37:39 +02:00
Author
Owner
Copy Link
Copy Source

🔗 Session bundle — #505 QSV OpenCL tonemap residuals: #660, #661

Both are accepted residuals recorded in ffmpeg.qsv-hdr-tonemap-opencl when #505 shipped, and both sit in the same QsvPipelineBuilder HDR path. They are independent — #660 needs an anamorphic HDR sample on the Intel host, #661 is a pure compile-time refactor with no hardware requirement — so they can be swept together or taken separately.

Note #661 is a good candidate to do first if both are picked up in one session: it changes no generated command, so landing it clears the noise before #660 starts altering the actual graph.

Per docs/handoffs/chicorytv-issue-queue.md → bundles, check this cluster when claiming either.

🔗 **Session bundle — #505 QSV OpenCL tonemap residuals:** #660, #661 Both are accepted residuals recorded in `ffmpeg.qsv-hdr-tonemap-opencl` when #505 shipped, and both sit in the same `QsvPipelineBuilder` HDR path. They are **independent** — #660 needs an anamorphic HDR sample on the Intel host, #661 is a pure compile-time refactor with no hardware requirement — so they can be swept together or taken separately. Note #661 is a good candidate to do *first* if both are picked up in one session: it changes no generated command, so landing it clears the noise before #660 starts altering the actual graph. Per `docs/handoffs/chicorytv-issue-queue.md` → bundles, check this cluster when claiming either.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
ad-hoc
One-off / ad-hoc work not tracked by a dedicated issue
 api
REST API / HTTP endpoints
 bug
Something isn't working
 ci-cd
Build, test, deploy pipeline
 content
Channel content / schedules / playlists
 dependencies
Dependency updates (Renovate)
 enhancement
New feature or improvement
 frontend
ChicoryTV React SPA frontend
 in-progress
Claimed by an active session — do not pick up
 jellyfin
Jellyfin tuner / IPTV integration
 parked
Excluded from automatic queue pickup; work only when explicitly selected
 priority: high
priority: low
priority: medium
review
Adversarial review finding
 security
Security / vulnerability fix
No labels enhancement priority: low
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
renovate timothy (Timothy)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: timothy/ersatztv#660