Default chapter ranges

[christoph-heinrich]

I've been wondering about this for a while now and I don't know what the best way of dealing with this would be, so I'll just throw it out there.

There is a default configuration for chapter ranges in uosc.conf, but sometimes it behaves suboptimally. Of course that is to be expected to some degree, but I have modified mine and it has worked pretty well for me over the past two months.

chapter_ranges=^op |^op$| op$|opening$<968638:0.5>.*, ^ed |^ed$| ed$|ending$<968638:0.5>.*|{eof}, sponsor start<3535a5:.5>sponsor end, segment start<3535a5:0.5>segment end, ^sponsors?<3535a5:.5>.*

Now I could make a PR and hope that it works as well for other people as it did for me, but it sure would be great if there was some way of verifying that it works well other then just "try it for a while and see what happens".

We would need some sort of collection of chapter titles that it should match and also titles that it should not match, and it's probably good if people can contribute to that without having to create a PR, so maybe a wiki page like mpv has for user scripts. But the problem with the wiki page is conveniently getting those strings for the actual tests.

Regarding the test itself, it is possible to access functions and global variables from the main lua file directly from a test script as long as they are not defined as local. It is necessary to define some stubs to not have the lua interpreter scream at you when using require uosc in the test script, and any actually required functionality that's shipped by mpv needs to be replicated somehow. I locally have a test.lua and a JSON.lua that I use for testing whatever I'm currently working on in quality-menu. It's fairly primitive as it just barely covers what I needed since creating it and it's far from an actual unit test, but here is a listing in case you want to use parts of it. Btw, I used something similar to test the text wrapping, because starting mpv every time would have been a pain, but it didn't call usoc directly and instead i just copied code.

local mp = {}
function mp.add_key_binding()
end
function mp.get_property()
end
function mp.register_event()
end
function mp.register_script_message()
end
function mp.get_script_name()
end
function mp.commandv()
end
package.loaded["mp"] = mp

local mp_utils = {}
package.loaded["mp.utils"] = mp_utils

local mp_msg = {}
package.loaded["mp.msg"] = mp_msg

local mp_assdraw = {}
package.loaded["mp.assdraw"] = mp_assdraw

local mp_options = {}
function mp_options.read_options()
end
package.loaded["mp.options"] = mp_options

io.input('test3.json')
local json_string = io.read("*a")

local json = require 'JSON'
json = json:decode(json_string)

require 'quality-menu'

opts.include_unknown = true

local vres, ares , vfmt, afmt = process_json(json)

for _,f in ipairs(vres) do
	print(f.label)
end
for _,f in ipairs(ares) do
	print(f.label)
end

The reason I bring it up now is because I've literally just run into the first false positive since using that configuration, and I don't know how to modify it without breaking something else.

[FichteFoll]

But the problem with the wiki page is conveniently getting those strings for the actual tests.

Every wiki on Github is backed by a normal git repository you can clone and interface with using git. It wouldn't be hard to clone a repo and parse one of its markdown sourcefiles for sample chapter names if they adhere to a certain format.

[FichteFoll]

Here's already an example chapter that shouldn't have specific highlighting (using the default config):

https://www.youtube.com/watch?v=xt0Jc8-ohjY

[tomasklaen]

@christoph-heinrich I like your patterns more then the default ones, which are a bit too loose. The @FichteFoll example would not happen with yours for instance.

But I think you're overthinking it. This is just a couple patterns, we should just tweak them when someone comes with a string that should obviously not be matched. Currently, as I've noted, it's a bit too loose, and tries to match anything that could be a range. I think we should instead optimize to never match something that is not a range. IMO it's less confusing if something isn't transformed into a range, than having a UI display some range that doesn't make sense, and leaves the user wondering what is that rectangle there for.

In practice, removing stuff like ^end, which is too broad, and matches the case above. Or I see in your patterns you're a bit loose with matching sponsor blocks by using ^sponsors?. I personally don't use the sponsorblock in mpv, but doesn't it use standard naming for chapters? Isn't this looseness redundant?

[christoph-heinrich]

I don't use sponsorblock, but YouTube videos often have chapters for their sponsor segments. That's what that is for. Sometimes they are called something like "Sponsor: Linode", that's why it is loose. I don't remember that one ever running into a false positive though.

I'll make a PR with my config.

[FichteFoll]

Apparently "Intro" and "OP" are also occasionally used together …

[christoph-heinrich]

That doesn't work? I haven't looked at the new implementation yet, but I've already had a false positive that wouldn't have happened with the old chapter ranges. The chapter was called "Opening the Box" and got recognized as an opening.

[tomasklaen]

@christoph-heinrich my bad 😶. I removed spaces from patterns when copy & pasting them. Fixed now.

@FichteFoll ok I'll split the openings type into two, and fix the issue where timeline doesn't display the tooltip for OP tomorrow.

[FichteFoll]

I guess I didn't specify what the actual and expected behaviors are. Not sure if you understood me correctly.

Actual behavior: Intro and OP are marked as blue
Expected behavior: only OP is marked

[tomasklaen]

Fixed now. Intros and outros are now their own chapter types, and they are disabled by default.