osc.lua: use private options to control SUB/OSD offset#17555
osc.lua: use private options to control SUB/OSD offset#17555kasper93 merged 2 commits intompv-player:masterfrom
Conversation
|
@na-na-hi: 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. |
What you see is what you get. Why do you think screenshots should render different position that what's on the screen?
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?
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...
There is no intent to do that. 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 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 |
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 |
|
Is a script setting It would indeed work with different OSCs since they also set Anyway this PR is also fine. |
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
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. |
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.
How would you plumb that to Also there are different kind of states mouse position is not an option, but I digress. |
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 That sounds reasonable on an OSC level, no? Then warn in option validations: 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: |
|
@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. |
|
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. |
player/command.c
Outdated
| int ret = do_op_udata(&nctx, action, arg); | ||
|
|
||
| if (ret >= 0 && bstr_startswith0(bstr0(path), "user-data/osc/margins")) { | ||
| mpv_node *osc = node_map_get(&mpctx->command_ctx->udata, "osc"); |
There was a problem hiding this comment.
This block can be extracted to a new function
There was a problem hiding this comment.
Is this what you would consider less hacky?
I honestly don't have a strong opinion here. 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).
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.
All solutions will be "hacky" since the OSC pretends to be an independent thing. But that cannot meaningfully be true since in order to achieve visual coherency UI has to collaborate with other visual elements.
sub/osd.c
Outdated
| void osd_set_script_margins(struct osd_state *osd, double t, double b) | ||
| { | ||
| atomic_store(&osd->margin_t, t); | ||
| atomic_store(&osd->margin_b, b); |
There was a problem hiding this comment.
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.
Clearly, I'm fixing it, by creating this PR.
I agree on that. I just don't like how it's done, but anyway.
I will review and clean this patch, once the approach is approved by @na-na-hi. |
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.
It does not adjust subtitle margin, which is what I have issue with.
I do not think depending on Instead, I think this should be their own properties like 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 |
Without relating to the rest of this discussion, I would prefer to consider 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 |
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?
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 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 :)
Correct, glad that at last someone noticed :)
Side note, with the I understand the natural idea to use
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. |
There is concern that with certain user options or script the value of
`{sub,osd}-margin-y` value may be persisted or overwrite user option
when it's changed while OSD is visible and offset is applied.
Fix this by using dedicated option.
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.
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. |
Subtitles are complex subsystem, let's leave the choice to move them with margin on user.
|
Will merge if there are no more comments. |
6 participants
There is concern that with certain user options or script the value of
{sub,osd}-margin-yvalue may be persisted or overwrite user option when it's changed while OSD is visible and offset is applied.Fix this by using dedicated private option only for OSC use.