option: add --watch-later-blacklist-properties option

[CrendKing]

This option allows user to skip backing up specific properties for watch later feature. The "all" special value skips all properties.

I chose to do "skipping" to preserve backward compatibility. Let me know if the opposite is desired.

Since the list of properties is fixed and fairly short, I don't think it is worth sorting or using hash set to accelerate the lookup.

Do I also need to update documentation?

Related issue: #4641

[garoto]

Previously: #6541, #6373

[Dudemanguy]

I'd prefer the term "blacklist" here (is this not hip anymore?) personally. Seems clearer.

Do I also need to update documentation?

Yes.

[CrendKing]

Previously: #6541, #6373

Wow, didn't know I'm at least the 3rd attempt. If I read it correctly, @wnoun wants to always write the value, but ignore when reading, which is what #6541 did, but @ghost (a deleted account?) wants the not write to begin with, which is what this PR and #6373 did?

So which direction are we proceeding? What is the benefit of having those properties in the watch-later but not using them? I can see the argument be because user could change his mind and remove this new option, he can get them back. Counter argument would be it complicates already complex logic of processing options at read time. I'm OK either way.

Yes.

Will do once settled.

[Dudemanguy]

Yeah, it's a little strange that this never got implemented.

So which direction are we proceeding?

I would greatly argue in favor of simply not writing the properties to begin with. It's much cleaner.

[CrendKing]

Question: in mp_get_resume_defaults(), the mpctx->resume_defaults is calculated and stored in the context. This function is called during mp_initialize(). Later mpctx->resume_defaults is used in mp_write_watch_later_conf().

Does this mean the property values from mpctx could change over its lifetime? Based on my own experience, no matter what I change in mpv.conf, once the player starts playing, it always uses the same config. Same even if I play next file in playlist or reload the same file.

If that's true, can't we move the code logic of mp_get_resume_defaults() to mp_write_watch_later_conf() and get rid of mpctx->resume_defaults?

[Dudemanguy]

mpctx->resume_defaults just holds the default values for all the properties (i.e. what you set in your config file, command line, etc.) Its only use with watch_later is to not write properties that are already equal to the default. Proprieties in mpv definitely can change across runtime, but the defaults are the defaults. They do not change during runtime.

There's no need to worry about it here.

[garoto]

I would greatly argue in favor of simply not writing the properties to begin with. It's much cleaner.

It was also wm4's favored approach judging from his comment on @jgreco's PR (/pull/6373). Seems to make the most sense from my layman perspective.

[CrendKing]

Ah, I misunderstood what "defaults" meant. They needed to call mp_get_resume_defaults() in mp_initialize() because it must happen before loading user's config in mp_input_load_config().

Updated to address comments.

It was also wm4's favored approach

Who is wm4? ghost? Why is his account deleted?

[garoto]

Who is wm4? ghost? Why is his account deleted?

ugh, yes he is the ghost account you see on the linked PRs. wm4 was the principal maintainer. He deleted his account because of day-to-day internal project struggles.

[CrendKing]

Are you that new?

Start to know mpv last Friday ;-)

Also let me know or feel free to change if the option name is too long or verbose.

[Dudemanguy]

Seems reasonable. The all keyword is a good idea given the big length of the backup_properties array. Nitpick: when you squash the commits, could you prefix it with options: instead of config:?

[CrendKing]

Squashed. One more question: like you said, the list of properties is large, and can change over time. How does user know what properties are available for this option? I'd love to include a permalink to backup_properties in configfiles.c from the doc, but then it won't be up-to-date. If we link master, then it won't be perma. Should I just link to the file and instruct user to look for backup_properties? What if the file itself is moved (like what happened in b19414f)?

[Dudemanguy]

I don't think what properties actually get saved in watch_later are documented anywhere. Perhaps it should be but no need to worry about it here. Anyways LGTM.

[CrendKing]

OK. Can any of you merge? I'm new I don't have access. Thanks.

[CrendKing]

@garoto @Dudemanguy What's going on? Nobody can merge this? Why is this approved but no action? Is there something I need to do at this point?

[Akemi]

it will be merged when it's merged. please don't get impatient.

[jeeb]

Also FYI, responses are generally much better on IRC, so if you want to ping on a PR or patch set, I'd recommend joining the IRC channel if possible.

I did find some things about this patch set and am in process of reviewing it. Currently checking what is the seemingly least bad way of handling this, as I did get some ideas from some of the d3d11 option addition which I did quite some time ago.

[CrendKing]

I apologize. I thought there was something I'm supposed to do and I forgot. I'm not in a rush or pushy, just wondering what's going on.

Feel free to change anything to your liking.

[jeeb]

Alright, sorry for taking a sweet time, kind of been a tiring month+. There are some general comments which touch upon the code, as well as a somewhat larger idea on how to make this feel a bit less bad :) .

So, my main issue where my brain would just not like this was the constant looping over the blacklist. Not to mention the loop being within the semi-complex property writing loop already :) Thus an idea sparked out.

1. Make the list of strings a list of structs.
2. Have the struct contain a flag skip_writing.
3. In the best case we'd register for updates for that option, but I think for now even calling a function that first resets everything to "don't skip" first, and then goes through the set values would be quite nice in the beginnings of mp_write_watch_later_conf (since those are usually the shorter list of strings). Warn in case of unknown properties being set.
4. Instead of looping through properties that are set to be skipped in the loop going over the properties, just...

   for (int i = 0; backup_properties[i].name; i++) {
       if (backup_properties[i].skip_writing) {
           continue;
       }
   }

5. ???
6. Profit.

The first bit of this idea to just show how possible it is can be seen in jeeb@4daccf7 . How does something like that sound? It would nicely separate the parsing of the option from the looping and writing.

Alternatively one could just make a dynamic list a la the char * allocation in mp_get_resume_defaults, and only add to that list things that are actually requested, but that is somewhat more complex :P .

Also the special value of all is making this harder than it should be, you could just make it a separate bool and if that is set, early exit. Although watch-later-skip-properties VS watch-later-skip-all-properties kind of naming/split is kind of meh, I do agree.

P.S. This sort of string-matching stuff kind of makes me wish we had a hash map here somewhere :/ (and we probably do, and I just don't know about it).

[jeeb]

Unfortunately, strcmp completely ignores the length of strings, just if the initial bit matches or not.

I think there's two ways around this:

1. strncmp, and set the length of the string that's being checked against, (in this case it'd be 3 for "all", or in case of dynamic variables strlen).
2. Using the bstr string wrapper. Really nice if you get into it, see misc/bstr.h

[CrendKing]

String literals are always null-terminated, so strcmp will behave as strncmp(... , 3), which is safe. Plus the "else" part is pretty much the same and needs strcmp anyway. Plus there are other cases strncmp is worse when used on literals (e.g. if the string is long and developer miscount the length, or if the literal is changed in a future patch and the person forgot to update the length).

I did a quick search and found https://stackoverflow.com/questions/448563/am-i-correct-that-strcmp-is-equivalent-and-safe-for-literals. I'm sure this must have been discussed many times.

[CrendKing]

The first bit of this idea to just show how possible it is can be seen in jeeb@4daccf7 .

I'm OK either way. If you want to take over the PR, feel free to do so. I just want to see this happen.

But either way, it is gonna be a O(m * n) loop without hash map or sorting.

Also the special value of all is making this harder than

How? Just loop the input array to find "all" first. Ideally if people use "all", that should be the only value, so O(1). Worst case would be O(n), not making the whole run time worse.

Although watch-later-skip-properties VS watch-later-skip-all-properties kind of naming/split is kind of meh

I don't think this feature warrant two options, let alone two options with very similar names.

<code comments>

I mostly copied the handling of reset-on-next-file. I don't know any pre and post condition of those macros, so went with safe route.

So let me know what do you want to do (or want me to do).

[h1z1]

Brace yourselves, thread was posted to entitled twats in r/linux. Shitfest is a bit of an understatement.

[CrendKing]

@jeeb After cleanup with your suggestion, it doesn't look bad now. Like I said, without hashmap or sorting, there will be a nested loop, either before the main logic or inside the main loop. However, my version avoid changing the data structure of backup_properties.

Would you consider OK if I leave a comment saying if more metadata are associated with these backup properties, we should retrospectively change the data structure and loop. But for now let's leave as is?

[wnoun]

what I want is an option like watch-later-properties instead of a hard coded backup_properties, as if we can customize reset-on-next-file

[CrendKing]

I wished it started as a configurable option as you said. However, to change it now means

1. we need to default the option to the full list, but conventionally most list-like options default to no value.
2. we have to assume people being OK to the current behavior as baseline. Then imagine people just want to remove fullscreen from the list. They have to put watch-later-properties=osd-level|speed|options/edition|pause|volume|mute|audio-delay|ontop|border|gamma|brightness|contrast|saturation|hue|options/deinterlace|vf|af|panscan|options/aid|options/vid|options/sid|sub-delay|sub-speed|sub-pos|sub-visibility|sub-scale|sub-use-margins|sub-ass-force-margins|sub-ass-vsfilter-aspect-compat|sub-style-override|ab-loop-a|ab-loop-b|options/video-aspect-override in their mpv.conf.

[wnoun]

for list options, we have the -remove suffix, so it can be simplified to watch-later-properties-remove=fullscreen. even if without the suffix, the blacklist solution is not better when I want to exclude all options except the fullscreen.

we need to default the option to the full list, but conventionally most list-like options default to no value.

No, the the option default should depend on its usage and suitable for most people, the display-tags is a good example.

[CrendKing]

I didn't notice that suffix until now. I'm OK either way. I don't know how to proceed with this PR. If I change the design, will you (@wnoun) review it? Otherwise the other folks might not like that design and this PR will never go anywhere.

[wnoun]

the problem is how to implement it. one possible implementation is to save the key-value list of properties and their default values to mpctx->resume_defaults in mp_get_resume_defaults, so you don't need to synchronize properties and their default values when writing watch-later files. it's simple, but the disadvantage is that modifying the watch-later-properties option after playback is initialized has no effect. it's unlikely to make it respond to runtime changes without backing up all options first.

[CrendKing]

I think the biggest problem is that this conversation even exist. I created this PR more than a month ago. And we still haven't figure out how to implement a small and seemingly normal feature. It is very discouraging, at least to me, to want to contribute anymore to this project.

Who is the leader figure of this project? According to garoto, wm4 was once the leader but no longer. So who is going to decide how a feature is implemented and give shipit? The oldest open PR is dated March 2018, and I don't know how many were closed due to lack of activity. Akimi said "it will be merged when it's merged", which is true but also a meaningless tautology.

wnoun, I actually like your redesign idea on this feature better, but where were you for the last 29 days when the previous design was discussed? And where are @Dudemanguy and @jeeb now who agreed on the previous design? Shouldn't these design questions be sorted out by you guys who have authority on the project's direction/future/path? jeeb recommended to use IRC, but the project is hosted here in GitHub. So no member of the project ever browse the list of pending PRs unless getting pinged?

If any of you is interested to push through this, be my guest. I'm done with it.

[Dudemanguy]

I'm sorry you feel this way, but mpv is a purely volunteer project run on member's free time. We don't have any sort of formalities, deadlines, schedules or anything like that. Different projects have different rules, but I do not believe a month of waiting is a particularly long time. Also, it is out of line to attack wnoun who is not a member of the project.

[CrendKing]

Oh, I meant absolutely no offense to wnoun, or any of you. I'm sorry if I chose wrong words or bad syntax. I'm not native English speaker. Let me explain more.

I just feel this form of project management is not friendly to an "outsider". Of course anyone could have their opinion, but then who should I listen to? If all of you folks are on equal, and everyone thinks his idea has sound ground, then there won't be progress. If I'm "insider", I'll just choose an approach and do it despite of some objection. But I'm not. Also, considering features this small that takes only 5 minutes to write and maybe 5 minutes to review, I don't think a month is a good turnaround period, not to mention we are not even close to be done here.

I'm not an expert on this, but I think the parallel here is Python vs Linux. After Guido's "departure" from Python, many feel Python lost it's spirit. Many think Python is too rigid on being a "reference implementation" and fear of adopting new features. Whereas for Linux kernel, Linus' presence plays an important role on the project's general direction and path, despite the existence of the Linux kernel community.