Skip to content

fix(video): honour the client's requested color range - #146

Merged
hgaiser merged 5 commits into
hgaiser:mainfrom
lutyjj:fix/client-color-range
Aug 2, 2026
Merged

fix(video): honour the client's requested color range#146
hgaiser merged 5 commits into
hgaiser:mainfrom
lutyjj:fix/client-color-range

Conversation

@lutyjj

@lutyjj lutyjj commented Jul 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes #145.

We hardcode full_range = false in the color converter and the encoder VUI, and never read x-nv-video[0].encoderCscMode. Some clients ask for full range (mode 3) and decode accordingly, so our limited-range data lands with black at 16 instead of 0.

Sunshine reads bit 0 of the same attribute:

colorspace.full_range = (config.encoderCscMode & 0x1);

Was noticed before in #46 (full_range: false vs the client's forced PL_COLOR_LEVELS_FULL), and doesn't look like it got fixed in #47.

The remaining bits pick an SDR colorspace. We always encode Rec.709, which is fine as long as the VUI says so, so I left that alone but log a warning on an explicit non-709 request. An absent attribute isn't treated as a request.

Steam logo used to be white on a grey background, now white on proper black.

@lutyjj
lutyjj force-pushed the fix/client-color-range branch 4 times, most recently from 12ae524 to 7d2d29d Compare July 29, 2026 16:44
@lutyjj

lutyjj commented Jul 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Related to pixelforge#24. Limited as the SDR default is right, this just makes us read what the client actually asked for instead of assuming it.

@lutyjj

lutyjj commented Jul 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@hgaiser two things that might belong in pixelforge rather than here:

  1. ColorDescription has no with_* methods, so this uses struct-update syntax at 7 sites. Can add ColorDescription::with_full_range there if it'll look better, would just need a pin bump.

  2. The other encoderCscMode bits (Rec.601, BT.2020 SDR) can't be expressed today: ColorSpace has no 601 variant and the shader only carries BT709 and BT2020 matrices. So this just logs a warning if a client asks for one.

@lutyjj

lutyjj commented Jul 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

found a regression, putting into WIP for now

something is not right when trying to stream it with HDR AV1. the game starts in SDR regardless, then after the first warning screen it switches to HDR, and whole screen gets green. This seems to be unrelated to this PR though. If you reconnect (or new key frame arrives) the stream recovers.

upd: doesn't seem to be related to PR. Works fine with HEVC.

@lutyjj
lutyjj force-pushed the fix/client-color-range branch from 7d2d29d to bdcb89f Compare July 29, 2026 21:14
@lutyjj
lutyjj marked this pull request as draft July 29, 2026 21:14
@lutyjj
lutyjj force-pushed the fix/client-color-range branch 5 times, most recently from ad36df4 to 1160f92 Compare July 29, 2026 22:06
@lutyjj
lutyjj marked this pull request as ready for review July 29, 2026 22:10
@hgaiser

hgaiser commented Aug 1, 2026

Copy link
Copy Markdown
Owner

@hgaiser two things that might belong in pixelforge rather than here:

1. `ColorDescription` has no `with_*` methods, so this uses struct-update syntax at 7 sites. Can add `ColorDescription::with_full_range` there if it'll look better, would just need a pin bump.

2. The other `encoderCscMode` bits (Rec.601, BT.2020 SDR) can't be expressed today: `ColorSpace` has no 601 variant and the shader only carries BT709 and BT2020 matrices. So this just logs a warning if a client asks for one.

Regarding 1: I agree, it makes sense to make a builder pattern for ColorDescription 👍

@hgaiser

hgaiser commented Aug 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

@lutyjj do you want to work on the pixelforge PR before merging this?

Also: is_hdr could maybe also be a member function of ColorDescription

@lutyjj

lutyjj commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

@hgaiser yep, will work on it today/tomorrow :)

@lutyjj
lutyjj marked this pull request as draft August 1, 2026 17:02
lutyjj added a commit to lutyjj/pixelforge that referenced this pull request Aug 1, 2026
@lutyjj
feat: add ColorDescription::with_full_range and is_hdr
2af716a 
From the hgaiser/moonshine#146 review. The presets pin primaries,
transfer and matrix as a group, and full range is the only field that
changes on its own, so every consumer ends up writing
`ColorDescription { full_range, ..ColorDescription::bt2020_pq() }`.
`with_full_range` gives that the same shape as the EncodeConfig
builders, and ColorDescription is `#[must_use]` now like the other
builder types. `is_hdr` moves the "PQ transfer function is what makes
the stream HDR" check onto the type instead of every consumer redefining
it; the luma range and primaries don't decide it. The H.273 code points
the presets use are named constants now instead of bare numbers.
@lutyjj lutyjj mentioned this pull request Aug 1, 2026
Merged
@lutyjj

lutyjj commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

@hgaiser pixelforge side is up: hgaiser/pixelforge#30.

I see though that moonshine is using old pin of pixelforge and would require a little bit more work to unblock this PR :)

@hgaiser

hgaiser commented Aug 1, 2026 via email •
edited
Loading

Copy link
Copy Markdown
Owner

@lutyjj lutyjj mentioned this pull request Aug 1, 2026
Closed
lutyjj added a commit to lutyjj/pixelforge that referenced this pull request Aug 2, 2026
@lutyjj
feat: add ColorDescription::with_full_range and is_hdr
0c14370 
From the hgaiser/moonshine#146 review. The presets pin primaries,
transfer and matrix as a group, and full range is the only field that
changes on its own, so every consumer ends up writing
`ColorDescription { full_range, ..ColorDescription::bt2020_pq() }`.
`with_full_range` gives that the same shape as the EncodeConfig
builders, and ColorDescription is `#[must_use]` now like the other
builder types. `is_hdr` moves the "PQ transfer function is what makes
the stream HDR" check onto the type instead of every consumer redefining
it; the luma range and primaries don't decide it. The H.273 code points
the presets use are named constants now instead of bare numbers.
@lutyjj
lutyjj force-pushed the fix/client-color-range branch from 7083d1b to c08c2d5 Compare August 2, 2026 17:41
@lutyjj
lutyjj marked this pull request as ready for review August 2, 2026 17:43
lutyjj added 2 commits August 2, 2026 21:58
@lutyjj @hgaiser
fix(video): honour the client's requested color range
02d576c 
The color converter and the encoder VUI both hardcoded limited-range luma,
and `x-nv-video[0].encoderCscMode` was never read. A client that requests
full range decodes by expanding 16-235 to 0-255, so limited-range data
arrives with black lifted to 16 and white capped below 255.

Take bit 0 of the attribute as the range request, matching Sunshine's
`colorspace_from_client_config`, and thread it to both the converter's
quantisation and the VUI so the data and the signal agree.

Decide the HDR mode sent over the control stream from the transfer function
rather than by comparing the whole `ColorDescription` against
`bt2020_pq()`, whose `full_range` is always false. The luma range is
independent of whether a stream is HDR, so the struct comparison reported
SDR for every full-range HDR session and the client never left SDR.

The key-frame SEI gate asks the same question and is deliberately left as a
struct comparison: correcting it newly enables AV1 metadata injection for
full-range clients, and that path produces streams the client cannot
decode. It is tracked separately.

The remaining bits of the field select an SDR colorspace. Only Rec.709 is
encoded. Rec.601 doubles as the protocol default for clients that never
chose one, so it logs quietly; an explicit request for anything else is
warned about rather than silently mis-encoded.
@lutyjj @hgaiser
style(video): declare full_range in the same order it is built
47d3b4b
lutyjj and others added 3 commits August 2, 2026 21:58
@lutyjj @hgaiser
refactor(video): use pixelforge ColorDescription builder and is_hdr
6902401 
with_full_range replaces struct-update syntax at seven sites and the
open-coded transfer-function check moves onto the type, as requested in
review. Needs a pixelforge pin at or past the commit that added the API
(pixelforge PR 30).
@lutyjj @hgaiser
chore: update pixelforge for the ColorDescription API
a8e4e0c 
No tag past v0.7.2 yet, so pin the merge commit; swap for a tag once
one is cut.
@hgaiser
chore: update pixelforge to v0.8.0
6274fe7
@hgaiser
hgaiser force-pushed the fix/client-color-range branch from c08c2d5 to 6274fe7 Compare August 2, 2026 20:00
@hgaiser
hgaiser merged commit 8b3aa79 into hgaiser:main Aug 2, 2026
5 checks passed
@hgaiser

hgaiser commented Aug 2, 2026

Copy link
Copy Markdown
Owner

Just tested this, works good as far as I can tell. Thanks!

@lutyjj
lutyjj deleted the fix/client-color-range branch August 4, 2026 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug] Raised blacks for whole stream compared to Sunshine

2 participants

@lutyjj @hgaiser