vo_gpu_next: keep overlay-add bitmaps in sRGB - #18392
Conversation
Bitmap overlays took the video's colorspace, so that image subtitles, which are muxed with the video and authored in its space, reproduce correctly. Bitmaps supplied through the overlay-add command are not in that space: they come from the client, are documented only as premultiplied BGRA with no colorspace attached, and are in practice sRGB. Reinterpreting them as PQ or HLG broke any client drawing its interface through overlay-add whenever the file was HDR. Measured on a BT.2020/PQ clip by pushing an sRGB overlay and reading the rendered pixels back, with SDR output: in (192,192,192) -> out (255,255,255) light greys clipped to white in (128,128,128) -> out (190,190,190) in (255, 0, 0) -> out (255,117, 37) primaries skewed Mean error was 36.5/255; the overlay now round-trips exactly. With an HDR target the overlay was passed through untouched instead, so sRGB white went out as PQ 255 and requested 10000 nits. struct sub_bitmaps already carries video_color_space for exactly this question, and the SUBBITMAP_LIBASS branch honors it; only the BGRA branch inferred unconditionally. Set the flag where image subtitles are packed and honor it in the VO, so overlay-add keeps the default assigned a few lines above. Image subtitles are unaffected. Refs mpv-player#10678
|
Locking overlay-add to sRGB seems incorrect. It makes more sense to allow it to be tagged. |
|
I agree that tagging is the better long term answer, and that is what #10678 asks for. I went with sRGB here because it seemed like the smaller change, but I am happy to implement the tag instead. |
|
I can confirm this patch fixes the issue I mentioned I encountered (#10678 (comment)) and I think sRGB is the more reasonable default for script-injected images so that a naive script doesn't have to worry about colorspace to display static or UI images. One area where tagging would help though would be |
|
This will break thumbfast. I don't really understand why it's expected that overlays will be srgb, if you're showing seek preview then the preview also came from the same file no? So they should both be the same colorspace already |
The previous commit, mpv-player#18392, made bitmaps from overlay-add sRGB. That was right for content a client drew itself and wrong for content taken from the video: thumbfast passes thumbnails in unconverted and expects them tone mapped along with the video. The command carried no colorspace information, so one behavior had to serve both. Add an optional colorspace argument taking srgb or video. srgb is the default and keeps the behavior that commit settled on. video uses the video's exact colorspace and tone maps the bitmap with it. Existing callers are unaffected. All overlay ids share one packed texture, so each part carries its own colorspace. Appending the parts a colorspace at a time leaves each group a single run of the array, which is the form pl_overlay takes, and the VO draws one overlay per group over the shared texture. Ids with different colorspaces can then be mixed. The grouping costs one pass per colorspace and no extra memory. MP_CMD_DEF_MAX_ARGS allowed eleven arguments, raise it to twelve. A bitmap declared as being in the video's colorspace is left exactly as the video, without the --image-subs-hdr-peak override that keeps image subtitles from blowing out. A thumbnail has to match the video it came from, and clamping it to a different peak means it cannot. Verified by printing pl_color_space where it is assigned: for an overlay tagged video every field matches the video exactly, primaries, transfer and the float luminances alike, while an srgb-tagged overlay of the same pixels keeps BT.709/sRGB.
The previous commit, mpv-player#18392, made bitmaps from overlay-add sRGB. That was right for content a client drew itself and wrong for content taken from the video: thumbfast passes thumbnails in unconverted and expects them tone mapped along with the video. The command carried no colorspace information, so one behavior had to serve both. Add an optional colorspace argument taking srgb or video. srgb is the default and keeps the behavior that commit settled on. video uses the video's exact colorspace and tone maps the bitmap with it. Existing callers are unaffected. All overlay ids share one packed texture, so each part carries its own colorspace. Appending the parts a colorspace at a time leaves each group a single run of the array, which is the form pl_overlay takes, and the VO draws one overlay per group over the shared texture. Ids with different colorspaces can then be mixed. The grouping costs one pass per colorspace and no extra memory. MP_CMD_DEF_MAX_ARGS allowed eleven arguments, raise it to twelve. A bitmap declared as being in the video's colorspace is left exactly as the video, without the --image-subs-hdr-peak override that keeps image subtitles from blowing out. A thumbnail has to match the video it came from, and clamping it to a different peak means it cannot. Verified by printing pl_color_space where it is assigned: for an overlay tagged video every field matches the video exactly, primaries, transfer and the float luminances alike, while an srgb-tagged overlay of the same pixels keeps BT.709/sRGB.
thumbfast is relying on undocumented, gpu-next specific behavior that 704afb8 unintentionally changed in the first place that isn't present on any other VOs. The overlay is external content and many scripts use it to render UI elements or thumbnails from other unrelated files, so it is reasonable that it should not use the same colorspace as the video by default. thumbfast needs code change either way if non-hdr overlay needs to be fixed: either rely on #18394 or use tonemapped screenshots. |
|
@na-na-hi It's still better to make colorspace explicit for overlays, instead of relying on implicit behaviors that aren't documented anywhere |
|
sRGB is the only default that makes sense for bgra8, it's been the default on
In my testing thumbfast is already broken, both when it tonemaps and when it doesn't. It needs an update anyways, so scripts expecting sRGB should not be made to update in service of a script that already needs to update when the proper fix is merged. When thumbfast is tonemapping, it only converts the transfer function not the primaries (vf contains transfer=bt709 but no primaries=bt709), so I get an sRGB thumbnail which is visibly desaturated. When the ffmpeg mpv is built against does not have libzimg, mpv can't use the zscale filter so thumbfast gives up on tonemapping. This fallback is where
They might be in the same colorspace, but trying to offload the conversion to mpv is not reliable because HDR formats need an HDR-aware resampler. For instance, in my example, I have an HDR file with PQ-transfer. The rescale isn't PQ-aware, so a white wordmark in a video comes out cyan because the video's color representation is non-linear. A proper solution for HDR thumbnails needs:
|
|
Guys, I'm too slow. I started implementing this yesterday, and now we have two PR already. Sorry for that, I will do better. I will resolve the above issues. |
posilash
changed the title
6 participants
Bitmaps supplied through the
overlay-addcommand are reinterpreted in thevideo's colorspace under
vo_gpu_next. Any client that draws its interfaceas a bitmap and hands it to mpv through that command therefore gets it
mangled whenever the file is HDR.
vo_gpudoes not do this, so the twovideo outputs disagree.
This affects clients generally: anything pushing a
BGRA surface through
overlay-addis subject to it, including script-driventhumbnail overlays. The case I ran into is Nuvio Desktop, a media player
whose entire player interface, seek bar, buttons, timestamps, title is
rendered as an HTML page, captured to a premultiplied BGRA surface, and
pushed to mpv with
overlay-add, because the interface must composite overthe video inside a single embedded window. On HDR files the whole interface
is visibly wrong: light greys clip to white, darker greys to black, the text is fringing and artifacting and colors shift.
Measured on a BT.2020/PQ clip by pushing an sRGB overlay and reading the
rendered pixels back, with SDR output:
Mean error was 36.5 of 255 before the change, and exact after it. With an
HDR target the overlay was instead passed through untouched, so sRGB white
was emitted as PQ 255, which requests 10000 nits.
struct sub_bitmapsalready carriesvideo_color_spacefor exactly thisquestion, and the
SUBBITMAP_LIBASSbranch honors it. Only theSUBBITMAP_BGRAbranch inferred unconditionally. The flag is now set whereimage subtitles are packed and honored in the video output, so image
subtitles keep their existing behavior and only
overlay-addchanges.Tested on:
e7191f2a65using
vo_gpu_nextwith thex11vkGPU context. Verified against an HDR10 clip with known color patches,and in Nuvio Desktop itself on real HDR content, where the interface renders
correctly with the change applied. Image subtitles were checked separately
and are unchanged.
Without Patch
With Patch Applied
Marked as a request for comments for two reasons. Issue #13381 shows that
colorspace handling for subtitles and the on-screen display is still under
discussion, and issue #10678 asks for an explicit colorspace tag on the
overlay-addcommand rather than inferring sRGB. This change infers, so itmay not be the direction you want. I am happy to rework it either way.
One open question: should bitmaps from
overlay-addalso be scaled to thereference luminance, the way the
SUBBITMAP_LIBASSbranch does? I am glad to add it if thatis the correct behavior.