vo: add `video-target-params` property

[kasper93]

EDIT: removed images, because it is evolving.

[kasper93]

I'm not happy with the amount of mpv<->libplacebo glue it requires. And in general the duplication of code between both is really noticeable. Probably it is not a topic for this PR, but we could consider making libplacebo core dependency of mpv at some point and at least use more robust color params structs etc. and avoid duplication/mapping of them. //CC @haasn

[kasper93]

Although not relevant to this PR, I think there are too many spaces next to the HDR metadata.

It is standard indentation after prefix in stats.lua, I don't see better way to handle it, single space would be too little. This works better with all the fonts and so on, I think.

[kasper93]

If using native hwdec, average-bpp is not available.

Fixed, this is old bug, need to check hw_subfmt. Still may not be the same as the source video itself, it is after decoding, well as all other values.

[kasper93]

color-depth seems to only work with video-target-params, video-params/color-depth is always unavailable

Expected, for video-params use average-bpp. Supporting mp_bit_encoding fully is out of scope of this PR. May be done it the future as part of bigger refactoring.

[kasper93]

If is a non-PQ video and target-trc is not set, video-target-params/gamma is auto, is this the expected behavior?

I've seen it too. Probably should be set, but it comes from libplacebo. I think for consistency libplacebo should set all target parameters to which it inferred internally. Same for target peak which is set only sometimes, currently.

[kasper93]

If is a non-PQ video and target-trc is not set, video-target-params/gamma is auto, is this the expected behavior?

@arm64-v9a: You can test with https://code.videolan.org/videolan/libplacebo/-/merge_requests/557 and this diff

diff --git a/video/out/vo_gpu_next.c b/video/out/vo_gpu_next.c
index a55612f088..265fcf2547 100644
--- a/video/out/vo_gpu_next.c
+++ b/video/out/vo_gpu_next.c
@@ -1120,7 +1120,11 @@ static void draw_frame(struct vo *vo, struct vo_frame *frame)
     }
 
     // Render frame
+#if PL_API_VER >= 315
+    if (!pl_render_image_mix_ex(p->rr, &mix, &target, &params)) {
+#else
     if (!pl_render_image_mix(p->rr, &mix, &target, &params)) {
+#endif
         MP_ERR(vo, "Failed rendering frame!\n");
         goto done;
     }

[kasper93]

Although not relevant to this PR, I think there are too many spaces next to the HDR metadata.

After looking at it indeed it was bit much. Reduced a little, but still with a gap, so it is not too crowded. I don't know ASS well, probably this could be aligned in some nicer table like layout, but dunno.

[Obegg]

Although not relevant to this PR, I think there are too many spaces next to the HDR metadata.

After looking at it indeed it was bit much. Reduced a little, but still with a gap, so it is not too crowded. I don't know ASS well, probably this could be aligned in some nicer table like layout, but dunno.

Unrelated but you did say "ASS" and I was thinking you meant to add subtitle info inside stats.lua too, so this could be an idea... stats.lua already shows video and audio stats so why not subtitles stats too?

[llyyr]

Unrelated but you did say "ASS" and I was thinking you meant to add subtitle info inside stats.lua too, so this could be an idea...

stats.lua, the osd as well as the osc are in ASS and rendered with libass

[kasper93]

so why not subtitles stats too?

There is not much interesting things to be shown about current selected subtitle. Well maybe, language and type. It is possible, but I think noone really though it would be useful before.

[Hrxn]

But now Video Out is between Video and Audio.. and they both belong together imho, as properties of a file..

How about adding Picture Type and Frame number? Very useful for anime screenshots

Why only anime screenshots, though? 🤔

[kasper93]

I feel it would be more aesthetically pleasing to put Video Out under Video?

I see your point, but as already pointed Video and Audio should be together. Else we have weird mess. If something like Audio Out is added, we can reorder it for symmetry, but now I would like to keep current order.

[kasper93]

How about adding Picture Type and Frame number? Very useful for anime screenshots

It may sound strange from me, because I added to this page a lot of things that some may consider useless, but I also want to keep it still readable or at least not too busy. Maybe it is too late, but we can't add thing endlessly.

I agree frame number would actually be useful. I will try to make it in one line, something like Frame: I (12/123). Need to see what looks nice.

[llyyr]

Here's my bikeshedding contribution since we're adding many numbers to stats.lua and modifying the layout a bit:

• Consider renaming "Video Out" to just Display or something
• Maybe only list specified and estimated display fps and file fps if they differ?
• I don't really think it's necessary to show bit depth information for pixel formats. You can already deduce this just from the pixel format itself, so if they can't then "bpp" isn't going to mean much to them either. I don't think this adds any new information, only more numbers.
• For displaying codecs, maybe strip it down to just the decoder name instead of also showing the superfluous description? It's easily the thing using up the most space for no good reason
• Put putting audio sample rate and bitrate on the same row.

[kasper93]

* Consider renaming "Video Out" to just Display or something

Changed to Display:,

* Maybe only list specified and estimated display fps and file fps if they differ?

Added some logic to merge those if needed.

* I don't really think it's necessary to show bit depth information for pixel formats. You can already deduce this just from the pixel format itself, so if they can't then "bpp" isn't going to mean much to them either. I don't think this adds any new information, only more numbers.

I'm not sure about that, we can remove it.

* For displaying codecs, maybe strip it down to just the decoder name instead of also showing the superfluous description? It's easily the thing using up the most space for no good reason

Fixed it to not do nested desc with name. Now should be better.

* Put putting audio sample rate and bitrate on the same row.

I don't touch audio atm. Also I think for symmetry with video it should have it's own line.

@llyyr ^

Perhaps "bpp" should only be showed when using hwdec. The formats of hwdec can sometimes be confusing, while the formats of swdec are always straightforward.

@arm64-v9a Not sure about that. For hwdec it already adds more to the lines, if anything it should be removed. I don't want some logic to show it only sometimes. Either we remove it or show always. I agree that for most formats it is obvious, so maybe indeed it is not needed.

[kasper93]

@llyyr @arm64-v9a @Hrxn Ok, I'm out of ideas for now. Take a look and let me know what do you think now.

[llyyr]

There's some refactoring you can do but I believe they should be their own PR. This one has already gotten massive so it's fine to stop here I think before it gets too annoying to review for others.

Just some small nitpicks to things added in this PR:

• We don't need to show [x:0, y:0, w:0, h:0] when no video-crop is inserted
• Frame and Bitrate can be on the same row to save some space.

Otherwise, this looks good to me visually at least. I'll go over the code in a bit

[kasper93]

There's some refactoring you can do but I believe they should be their own PR.

Yes, I noticed few things while going through the code. But I need to stop or it will never be approved.

We don't need to show [x:0, y:0, w:0, h:0] when no video-crop is inserted

This is blocked by https://github.com/mpv-player/mpv/pull/12307 I can pull the commit from there, but I hope at least this one will be merged soon.

Frame and Bitrate can be on the same row to save some space.

Frame in it's full form can be `Frame: 3213 / 312412 (P) Interlaced", not sure I want add more there. For me bitrate doesn't fit anywhere if I'm being honest.

[kasper93]

I will remove bpp inferred from pixel format as this doesn't mean much, but for color-depth I would like to still keep it, because it is the target depth that libplacebo uses (and dither to), regardless of pixel format. See:

[kasper93]

To be more explicit, I put it in own label.

Let me know if you still think it is useless.

[llyyr]

I preferred the old (8b) one than it having its own label, people shouldn't be watching stuff with stats.lua toggled on but we should still avoid clutter as much as possible

edit: actually yeah, it's already shown on vo passes, so we can remove it I think

[kasper93]

Removed.

[kasper93]

@arm64-v9a: Unrelated to PR, but what font (and settings) are you using?

[Andarwinux]

Is there anything currently blocking the merge this PR?

[kasper93]

Rebased only.

[kasper93]

Added pl_frames_infer_mix

[kasper93]

We could merge this if there are no objections, to get feedback form the field.

[haasn]

Well, I am a bit unhappy introducing all this code duplication. I was thinking we could wait until after #12002 is done and libplacebo is made mandatory dependency, and instead add the pl colorspace fields everywhere

[kasper93]

Well, I am a bit unhappy introducing all this code duplication.

What duplication exactly? There are two mapping functions added and that's all. I can remove something if you think is too much.

wait until after https://github.com/mpv-player/mpv/pull/12002 is done and libplacebo is made mandatory dependency, and instead add the pl colorspace fields everywhere

I would agree if it wasn't essentially mean to wait indefinitely. I don't expect this to happen soon. And frankly it is quite unrelated.

[llyyr]

Perhaps the stats.lua parts of this can be merged, leaving the parts that require libplacebo code duplication to be done after we've made it mandatory?