davidde / mpv-autosub

Fully automatic subtitle downloading for the MPV media player
https://mpv.io/
MIT License
295 stars 42 forks source link

feature: disable fetching subs for audio files #12

Closed gdmka closed 4 years ago

gdmka commented 4 years ago

Hi @davidde ! First of all, thanks for a great script. Given mpv is my go to player for music and video, i've noticed autosub acts funny while playing lossless or lossy audio — it downloads unrelated subtitles (ofc only when auto is true). To mitigate the annoyance a check of file type was added so autosub skips known audio formats. b and n keys would still trigger subtitle download.

Let me know what you think.

davidde commented 4 years ago

Thank you for taking the time to file such a complete PR. However, there are still a few problems with this:

I really appreciate you're aiming to fix a problem in the general case, making it useful for everyone, but could you share your specific case (e.g. file format, length, etc.)?

gdmka commented 4 years ago
* In general, audio files would be shorter than 15 minutes, so that check would eliminate most audio files. Did you alter the script in any way that got rid of that check, or are you just working with really long audio files?

Not necessarily. Lots of live records, classical music, opera ship as a single file. I'm not even talking about bootlegs here.

* Since MPV does not distinguish between mp4/m4a/mov and some others, this would not work for all listed file formats.

Wasn't aware. Thanks for letting me know. Sure my PR needs to resemble that.

but could you share your specific case (e.g. file format, length, etc.)?

Sure. I own a lot of CD-rips performed with EAC that are in fact a single flac/ape/wv file with a cue sheet metadata. cue files are supported by mpv, directing it to navigate lossless audio file with chapters. But with autosub on, the 15 minute check is bypassed as mpv buffers a single file surpassing the limit.

Check out the logfile. The catch is at line 45 [timeline] Total duration: 3063.333333 log.txt

davidde commented 4 years ago

Great, sounds good to me.

Just a couple more questions since I don't have such audio. Is cue an audio only format? Could it be listed a single time for all types instead of separately for each audio format? Does mp.msg.warn('active_format = ' .. active_format) return cue or cue/flac?

gdmka commented 4 years ago

Cue is a text file containing metadata. It holds an index of the tracks contained in the audio file. The reason to have those pairs is because cue actually points to a file on disk denoted by a specific format.

Great Wikipedia example

Does mp.msg.warn('active_format = ' .. active_format) return cue or cue/flac?

It returns cue/flac when a cue is used to launch playback. But if someone wants to listen to the whole file w/o jumping tracks (and for that reason starts a .flac) then _activeformat property returns plain flac.

I handle both cases basically.

davidde commented 4 years ago

Thanks, that makes it a lot clearer.

This gives me excellent opportunity to refactor some stuff that would be better off bundled with this function. I'll leave this open until then, so you can check if I haven't killed off some of your functionality. I'll let you know when it's up, and thanks for the help.

davidde commented 4 years ago

@gdmka Feel free to check if it still works for you.

gdmka commented 4 years ago

@davidde looks good from here But i see an opportunity for a little tweak.

Screen Shot 2020-06-20 at 13 24 30

It sounds valid to me to check file format type right before the script hits rescan_external_files so that redundant call and a warning just go away not polluting the shell output.

UPD: here's how i managed around it https://github.com/gdmka/mpv-autosub/commit/4464d50d32099f2182b161b35cf6e6d8389bcce6 If you want me to open a new PR — let me know

davidde commented 4 years ago

I see what you mean. The reason I put it behind the rescan, is because the rescan is still useful when auto is false; in that case it can still enable the right subtitles that might be off by default.

So this means the function would indeed need to be cut in half again; those that never need subs (like audio and web streaming) and all the rest.

davidde commented 4 years ago

In the long term, I will probably move setting sub-auto and slang to mpv.conf, obsoleting the rescan. Meaning it's probably not worth obsessing over this now.