[RFC] command: let overlay-add declare a colorspace tag - #18394
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
posilash
changed the title
b61665a to
7550dba
Compare
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.
7550dba to
dd62695
Compare
|
@iwalton3 would you mind testing this out. Nothing changes for existing code. The argument is optional and defaults to srgb, which is the behaviour #18392 settles on, so existing clients still work, just now defaults to srgb. To opt in, append the colorspace argument with either option of |
| {"colorspace", OPT_CHOICE(v.i, | ||
| {"srgb", SUB_BITMAP_CSP_SRGB}, | ||
| {"video", SUB_BITMAP_CSP_VIDEO}), | ||
| OPTDEF_INT(SUB_BITMAP_CSP_SRGB)} }}, |
There was a problem hiding this comment.
Is there any reason to not let the user specify any arbitrary colorspace mpv supports now that we're doing this properly?
There was a problem hiding this comment.
Tbh, I personally do not see the usecase for that, but it's not impossible. Explicit primaries/transfer would be easy to add on top, mpv already exposes pl_csp_prim_names/pl_csp_trc_names from --target-prim/--target-trc.
From my pov i think video should be sufficient, it carries the source's HDR metadata, unless there really is a usecase for explicit colorspaces for overlays. Either way, explicit HDR transfers would need to include the peak metadata (from where?), or else the overlay blowout issue would persist. Also, the VO groups parts by making one pass per possible value, which is fine at two, but there are a few hundred possible prim/transfer combinations, so it'd need to collect the distinct values actually present instead.
is there a specific usecase you have in mind?
|
This PR also works for my needs, it's still not enough for thumbfast to send non-sRGB thumbnails reliably. (More details posted on #18392) |
|
That settles the usecase question then @llyyr. The partially converted thumbnail case needs explicit primaries, which neither srgb nor video can express. Worth noting it needs primaries rather than an HDR transfer, so it sidesteps the peak metadata problem entirely. Either way I don't think this blocks it. The enum can gain values, or another optional argument can be appended exactly the way this one was, so explicit primaries/transfer can land on top without disturbing the tag that's already there. So merging this in the meantime seems reasonable to me. |
3 participants
Depends on #18392. That PR is the first commit here and is unchanged, only the second commit,
command: let overlay-add declare a colorspace, is new for review.Why
#18392 settles
overlay-addbitmaps as sRGB, which is right for a client drawing its own interface and wrong for content taken from the video. thumbfast decodes thumbnails and hands them over unconverted, expecting them to be tone mapped along with the video. The command carried no colorspace information, so one behavior had to serve both cases, depending on the situation one case is left broken.What
An optional twelfth argument:
srgbis the default and keeps the behavior #18392 settles on.videouses the video's exact colorspace and tone maps the bitmap with it. Existing callers are unaffected.A bitmap declared as
videois left exactly as the video, without the--image-subs-hdr-peakoverride 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.How
Overlay ids share one packed texture, so each part carries its own colorspace. Appending the parts one 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.
Grouping is one pass per colorspace over the parts, so it stays linear and doesn't need a scratch buffer or sorting.
struct sub_bitmapgains auint8_t, placed so it's in the existing padding,sizeofis 56 before and after, verified withgdb -ex 'ptype /o'.Stereo 3D duplication had to move inside the grouping loop. Leaving it where it was makes the copies land outside every group's range and they don't get drawn.
MP_CMD_DEF_MAX_ARGS
It allowed eleven arguments, an additional one was added, raising it to twelve.
Testing
Tested on Arch Linux,
vo=gpu-next, Vulkan (x11vk), NVIDIA (nvidia-open 610.57.04), against a BT.2020/PQ clip and an SDR clip.srgb, HDR source60,80,100,120,140,160in and outvideo, HDR source19,28,40,54,72,94srgboverlay-removeinvalid parameterpl_color_spacewas also printed where it is assigned: for an overlay taggedvideoevery field matches the video exactly, the primaries, transfer and the float luminances. While ansrgb-tagged overlay keeps BT.709/sRGB.