@na-na-hi: Does this fix your concern?

Does this fix your concern?

This does not address the screenshot issue with sub-margin I mentioned. And it was already considered too much of a hack to fix: #3727 (comment)

These private options are a hack, and there is nothing that prevents third party scripts from using this option. Once this is added third party osc scripts will start using this option anyway, and after a while it can never be removed because it will then break popular third party scripts.

And if they are properly controlled to be only usable by builtin scripts, people will complain that their modified version of osc.lua they put in the script folder does not work.

This does not address the screenshot issue with sub-margin I mentioned.

What you see is what you get. Why do you think screenshots should render different position that what's on the screen?

These private options are a hack

In what way? It resolves the concern of using/overriding user facing options, such that the worry was that they will interrupt user workflows/script.

Now we are using internal interface to communicate from OSC to mpv sub/osd renderer that rendering should be offset. Where is the hack? Do you have other ideas for interface between OSC and mpv?

and there is nothing that prevents third party scripts from using this option. Once this is added third party osc scripts will start using this option anyway, and after a while it can never be removed because it will then break popular third party scripts.

Is there an issue here? I don't understand this point. If an application/script uses private api they can expect breakage. However, I don't understand why we would need to remove those options?

I can even document them if you prefer. But then we are running in circles, because you likely will say that now user scripts will save them and restore...

And if they are properly controlled to be only usable by builtin scripts, people will complain that their modified version of osc.lua they put in the script folder does not work.

There is no intent to do that. M_OPT_PRIVATE is only so it doesn't show in --help and the option itself is redundant from cli point of view and used only for this case of temporary updating the offsets.

I see that you don't like the solution. But do you see any issues with this except what you said? It fixes all concerns with save/restore issues, which arguably is a hack in osc and while the impact is not huge, using dedicated interface is solving this.

I don't understand why you would expect screenshots to have subtitles positioned differently than when you're taking the screenshot, I would find that very unintuitive.

I assume that by private kasper just intended these to not be used in mpv.conf and --list-options, it's natural that OSC forks will also use them even if not documented since they are useful to them, like mp.set_mouse_area.

Anyway, the option is worth enabling by defaut for solving the issue of overlapped subtitles. I think all proposed solutions are fine. My original proposal was to read user-data/osc/margins directy where the C code sets margins though it requires passing mpctx all the way there.

My original proposal was to read user-data/osc/margins directy where the C code sets margins though it requires passing mpctx all the way there.

We never query anything form script to core however. This would be major change in script paradigm for mpv. Where scripts are configuring mpv core to act accordingly, not the other way around. Also I found those options to be OSC related more then mpv, if you use different osc or no-osc then those options would be not useful/working.

And this is clear by the fact that mpctx is not available in subtitle renderer too.

Is a script setting --osd-margin-y-offset to make mpv's core behave differently really different from setting user-data/osc/margins to make mpv's core behave differenty? It's just in a different property.

It would indeed work with different OSCs since they also set user-data/osc/margins to be compatible. That would be the advantage over this PR, less work for all OSC forks.

Anyway this PR is also fine.

What you see is what you get. Why do you think screenshots should render different position that what's on the screen?

I don't understand why you would expect screenshots to have subtitles positioned differently than when you're taking the screenshot, I would find that very unintuitive.

I have already explained it here: #17544 (comment)

And "What you see is what you get" argument is completely bogus. I see osd and osc in the window, but I do not see them in the screenshot. The offset only makes sense in the context of osd and osc, so if they are not shown, it disobeys the user setting of sub-margin-y when taking screenshots and makes no sense.

Do you have other ideas for interface between OSC and mpv?

It can be an internal function that modifies the internal offset values and is only callable from lua scripts. This has been done before for mouse position for example.

And "What you see is what you get" argument is completely bogus. I see osd and osc in the window, but I do not see them in the screenshot. The offset only makes sense in the context of osd and osc, so if they are not shown, it disobeys the user setting of sub-margin-y when taking screenshots and makes no sense.

I disagree. If you request screenshot without OSC, then it may not be rendered, but it shouldn't move other elements that will be rendered. This would be unexpected. While I generally can see your point of view, this I think you are nitpicking just to nitpick.

It can be an internal function that modifies the internal offset values and is only callable from lua scripts. This has been done before for mouse position for example.

How would you plumb that to ad_ass.c? Do you think it's cleaner solution? Could you explain again why adding an option is bad?

Also there are different kind of states mouse position is not an option, but I digress.

It would indeed work with different OSCs since they also set user-data/osc/margins to be compatible. That would be the advantage over this PR, less work for all OSC forks.

I honestly agree, especially as one that maintains an OSC fork.

I have to ask, guido suggested to read watch later options, if it includes sub/osd margin, then disable sub/osd/dynamic_margins options.

That sounds reasonable on an OSC level, no? Then warn in option validations:
option X disabled. need to do X to enable

This makes it entirely a user's decision whether to do this or not, and I'm certain majority of users wouldn't mind the screenshot issue, as ModernZ had this feature for about two years and no complaints.

Also uosc adjust osd margin long before that.

Allowing users to enable/disable those options seems sufficient enough.

Edit:
Nevertheless, it wouldn't be a big deal. Just seems like more work than needed.

@na-na-hi @N-R-K: Is this what you would consider less hacky? I personally find accessing user-data way more out of place, and drilling through tracks, but if that's the expected way, we can go this direction.

Also it's likely that that needs playres to be exported from OSC, because this won't work with subtitles that have different playres.

Download the artifacts for this pull request:

Doesn't playres only matter for ASS subtitles? Which are not affected by margins.

Doesn't playres only matter for ASS subtitles? Which are not affected by margins.

Everything is ASS subtitle, and if OSC uses different than default playres it likely will blow up.

Other than the fact that it's an issue that should be fixed. (Responding to the original issue where it was suggested that it's not worth fixing).

Clearly, I'm fixing it, by creating this PR.

Using user-data/osc/margins make it work with all UI scripts that already set this property, which is nice. But this is not a hill I'm interested in dying on.

I agree on that. I just don't like how it's done, but anyway.

Racy, the other thread might load new t but old b. Not asking you to fix it though, just noting to make sure whether it was an intentional decision or not.

I will review and clean this patch, once the approach is approved by @na-na-hi.

I disagree. If you request screenshot without OSC, then it may not be rendered, but it shouldn't move other elements that will be rendered. This would be unexpected. While I generally can see your point of view, this I think you are nitpicking just to nitpick.

This thinking is valid if osc.lua is a 3rd party script that is separate from mpv, considering that mpv is providing a mechanism and does not know what scripts will do with it. However, because osc.lua is built-in and a part of out-of-box experience there are some special considerations.

From a user standpoint, it makes little sense that subtitle positioning on the screenshot is different depending on the subtitle type, osc setting, and osc state. This means the user has to perform certain actions to hide the osc in order to take screenshots that are consistent in position with the other screenshots taken.

I think mpv core is doing what it should, and this issue is purely on the script side to handle. In the case of osc.lua, this means disabling this feature by default.

Also uosc adjust osd margin long before that.

It does not adjust subtitle margin, which is what I have issue with.

@na-na-hi @N-R-K: Is this what you would consider less hacky?

I do not think depending on user-data is good here. It not only makes player core use the data in user-data which is explicitly documented not the case, but the existing clients that set user-data/osc/margin and sub-margin-y at the same time will break because it will be applied twice. Currently many UI scripts set user-data/osc/margin to avoid conflict with console.lua, and they may not want the osd/subtitle reposition feature, and this causes a change of behavior in these scripts.

Instead, I think this should be their own properties like osd-dynamic-margins and sub-dynamic-margins, and explicitly document what they do and that they are runtime dynamic values that are not meant to be saved.

I have never precluded adding some extra options/properties to solve this runtime change issue. But I do have issue with this being some undocumented interface that implies the acknowledgement of osc in the player core. By adding new dedicated properties that are generic in principle like --video-margin-ratio-* (which was made so that osc does not have to change --video-zoom and ``--video-align-* to achieve the same effect), this issue is avoided.

This thinking is valid if osc.lua is a 3rd party script that is separate from mpv

Without relating to the rest of this discussion,

I would prefer to consider osc.lua separate from mpv, because otherwise 3rd party replacement OSCs are not guaranteed to be able to do what osc.lua does.

It has been a trend, and personally my preferance, to not use private or hidden API/arrangements between builtin scripts and mpv, so that any other 3rd party script or libmpv client would be able to officially do whatever the builtin scripts are doing - and therefore be able to replace them with potentially augmented or improved functionality.

That's why mouse-pos became a property instead of an undocumented scripting-only API, etc.

I think mpv core is doing what it should, and this issue is purely on the script side to handle. In the case of osc.lua, this means disabling this feature by default.

What if user wants to have this feature enabled? In which case all your concerns apply, do we not care about this and only default?

It does not adjust subtitle margin, which is what I have issue with.

Then say it directly, instead of trying to materialize some bogus arguments that only derail the discussion and make it harder to understand where is the problem.

Don't mix implementation issues with decision why something should be default or not. Here I clearly fix the valid concerns about options being overwritten, it is invariant to default state of those options.

I do not think depending on user-data is good here. It not only makes player core use the data in user-data which is explicitly documented not the case

I agree, I don't like it either. I did it only to show how bad it would look, but people were unfazed by the amount of hackery there, except for atomic bs :)

but the existing clients that set user-data/osc/margin and sub-margin-y at the same time will break because it will be applied twice.

Correct, glad that at last someone noticed :)

and they may not want the osd/subtitle reposition feature

Side note, with the user-data approach, this is mpv core option and it's imperative that reposition feature is enabled/disabled based on options. Scripts have no say in that.

I understand the natural idea to use margins for that, but I see this as an osc/ui feature to move subs as they want, not core to try to adjust on margins. Margins shouldn't have unintended side effect, when better way of controlling core behavior exist. Same as with boxvideo option.

Instead, I think this should be their own properties like osd-dynamic-margins and sub-dynamic-margins, and explicitly document what they do and that they are runtime dynamic values that are not meant to be saved.
I have never precluded adding some extra options/properties to solve this runtime change issue. But I do have issue with this being some undocumented interface that implies the acknowledgement of osc in the player core.

So... exactly the initial patch? I've restored it and added documentation. See, if you said that those options need to be documented, we would avoid all the song and dance.

What if user wants to have this feature enabled?

If the script manages to fix this (for example, somehow resets subtitle position when screenshot is being taken), then it can be enabled by default. Currently there is no mechanism to do that.

Don't mix implementation issues with decision why something should be default or not.

You asked: "Does this fix your concern?" and my answer is "No", because it does not fix the screenshot concern, which cannot be sensibly solved unless default is reverted. And if it is no longer the default, there will be less incentive to fix the runtime change concern.

This PR is still a good addition for third party osc scripts without consideration of osc.lua, so I am not going to argue against this.

Will merge if there are no more comments.