CogentRedTester / mpv-sub-select

An advanced conditional subtitle track selector for mpv player
MIT License
89 stars 8 forks source link

Blacklisted Track Is Selected #10

Closed george-emerald closed 2 years ago

george-emerald commented 2 years ago

A track that should be blacklisted is selected for some reason. Here is the .json:

[
    {
        "alang": "jpn",
        "slang": ["jpn", "enm", "eng"],
        "blacklist": ["signs", "songs", "translation only"],
        "whitelist": ["commie", "honorifics"]
    },
        {
                "alang": "gre",
                "slang": "no"
        },
        {
                "alang": "eng",
                "slang": ["forced", "no"]
        },
    {
        "alang": "*",
        "slang": "eng"
    }
]

A track with the title "Signs and Songs" is selected on my test file, even though both "signs" and "songs" are in the blacklist.

george-emerald commented 2 years ago

The whitelist seems to be working fine when I test it, so I have no idea why the blacklist isn't working.

george-emerald commented 2 years ago

Just to explain further, instead of the blacklist working as intended, it behaves either as if the blacklist just isn't working/there or as if it deffered to mpv.

george-emerald commented 2 years ago

Just checked again with another file and the whitelist is working exactly as intended so I have no idea why the blacklist isn't.

george-emerald commented 2 years ago

And before you ask here is the log: https://pastebin.com/raw/3HaHZZ2k

george-emerald commented 2 years ago

So after some testing, if I remove the whitelist, the blacklist works. But, I can't find a way to make both of them work at the same time. I tried duplicating "alang": "jpn" to allow for both the whitelist and the blacklist to be present, but it doesn't work. Please help me.

george-emerald commented 2 years ago

I'm guessing there's something wrong with these lines of code in the script:

if pref.whitelist then
        if not title then return false end
        title = title:lower()
        local found = false

        for _,word in ipairs(pref.whitelist) do
            if title:find(word) then found = true end
        end

        if not found then return false end
    end

if pref.blacklist then
       if not title then return true end
       title = title:lower()

       for _,word in ipairs(pref.blacklist) do
           if title:find(word) then return false end
       end
end

return true

They make it so that only the whitelist is read if present and if it's not the blacklist is read, when both should be read. Your .json examples allow both to be present after all.

george-emerald commented 2 years ago

I don't know lua scripting, so I can't fix it myself. Please help!

CogentRedTester commented 2 years ago

Currently the script assumes that either the whitelist or the blacklist option is set, not both. If both are present then the whitelist takes precedence and the blacklist is ignored. This isn't mentioned anywhere because I assumed people would not use both. Notice none of my examples anywhere use both in one option. Edit: actually the main README example does, that's my mistake, sorry.

I am not opposed to adding this feature though it does introduce some extra problems, most significantly the issue of which filter should take precedence, whitelist or blacklist. I may need to add an extra option flag to switch which has precedence, but if you have any ideas please let me know.

george-emerald commented 2 years ago

My request is that both be read, without any taking precedence. So with my .json for example: If I have this (the test file): (1) alang=eng (2) alang=jpn (1) slang=eng Signs And Songs (2) slang=eng Full Subtitles

The second subtitle should be selected, indicating that the blacklist is working.

If I have this (the test file I used for the whitelist): (1) alang=jpn (1) alang=eng Full Subtitles (2) alang=eng Honorifics

The second subtitle should be selected, indicating that the whitelist is working.

And if I have this example: (1) alang=eng (2) alang=jpn (1) slang=eng Signs & Songs [Hatsuyuki] (2) slang=eng Full Subtitles [Hatsuyuki] (3) slang=eng Signs & Songs [Commie] (4) slang=eng Full Subtitles [Commie]

The fourth subtitle should be selected, indicating that both the blacklist and the whitelist are working.

george-emerald commented 2 years ago

Is there no way to do this? I assumed that's how the script would work because of the example in the README.

CogentRedTester commented 2 years ago

Oh yes, that is an easier way of doing it. I actually think I intended for it to be that way originally but forgot about it. I'll have a commit shortly.

CogentRedTester commented 2 years ago

I believe the latest commit should have fixed this.

george-emerald commented 2 years ago

I will check in a bit because I'm currently on my phone. Will get back to you as soon as possible.

george-emerald commented 2 years ago

Also, is there a preference within the whitelist? My whitelist has two entries, so if both are present will it pick the track that matches the first entry or just the first track that matches any entry?

CogentRedTester commented 2 years ago

The first track that matches any entry.

george-emerald commented 2 years ago

Oh OK. Just tested it and sadly it behaves the same. The blacklist is still ignored.

CogentRedTester commented 2 years ago

It's definitely working on my end, are you sure you updated the script?

george-emerald commented 2 years ago

I'll try redownloading.

george-emerald commented 2 years ago

I redownloaded and I'm getting the same results. I'm sure it's the new one because it contains the passes_whitelist and passes_blacklist variables which are new. Did you test it using a file on your end? The commit for my previous issue worked, so I doubt my testing is wrong.

george-emerald commented 2 years ago

The whitelist works and the blacklist doesn't just like before. Are you sure the if statement about the blacklist is still run even if the if statement for the whitelist has been run?

CogentRedTester commented 2 years ago

I did test it with a file yes and I had both the whitelist and blacklist working together as expected. I am currently working on improved debug logging to make it easier for me to diagnose this. When I'm done I'll ask you to update the script and run another --log-file.

george-emerald commented 2 years ago

In my test file only the blacklist needs to work. Not both together. Did you test that?

george-emerald commented 2 years ago

Here is a log created with -v if that helps: https://pastebin.com/raw/DfQJKSZv

CogentRedTester commented 2 years ago

Please make a new log file with the new version of the script.

george-emerald commented 2 years ago

I made you a debug log: https://pastebin.com/raw/1Lf0s8Ld

From reading it it would appear that there are no matches for "alang": "jpn", so I'm guessing you made the whitelist mandatory, as in a track will be selected only if it matches the whitelist. The whitelist should be a preference not something mandatory.

CogentRedTester commented 2 years ago

Sorry could you do another log file with --log-file and --msg-level=sub_select='trace'.

There is some weird stuff going on here.

CogentRedTester commented 2 years ago

I'm guessing you made the whitelist mandatory, as in a track will be selected only if it matches the whitelist. The whitelist should be a preference not something mandatory.

If you specify a whitelist it will be mandatory, I've said so in the README: For a track to be selected it must match any entry in the whitelist and must not match any entry in the blacklist.

If you want to prefer things in the whitelist but still match things not on the whitelist you need another lower priority preference:

[
    {
        "alang": "jpn",
        "slang": ["jpn", "enm", "eng"],
        "blacklist": ["signs", "songs", "translation only"],
        "whitelist": ["commie", "honorifics"]
    },
    {
        "alang": "jpn",
        "slang": ["jpn", "enm", "eng"],
        "blacklist": ["signs", "songs", "translation only"]
    },
        {
                "alang": "gre",
                "slang": "no"
        },
        {
                "alang": "eng",
                "slang": ["forced", "no"]
        },
    {
        "alang": "*",
        "slang": "eng"
    }
]
george-emerald commented 2 years ago

Your suggested .json fixed it so there's no need for another log file. Thanks!

george-emerald commented 2 years ago

Is there any way to prefer the first entry of the white list? Like making 3 seperate preferences?

CogentRedTester commented 2 years ago

Is there any way to prefer the first entry of the white list? Like making 3 seperate preferences?

No the entries in the whitelist and blacklist all have equal priority. If you want priorities then you should make multiple preferences.