spaam / svtplay-dl

Small command-line program to download videos from some streaming sites.
https://svtplay-dl.se
MIT License
718 stars 120 forks source link

srt file won't get deleted after merge #1427

Closed Sopor closed 10 months ago

Sopor commented 3 years ago

When i run svtplay-dl.exe like this it won't delete the srt file:

svtplay-dl.exe -S -M https://www.viafree.se/program/drama/providence/sasong-1/avsnitt-1

svtplay-dl 4.7-4-gb3fd51d

bohelax commented 2 years ago

Fine, as long as you don't force the delete.

I always use -S -M, but keep the .srt file(s). Depending on where (on what device) I do the playback I can choose which subtitles are used (they can differ in size when displayed).

svtplay-dl 4.8

spaam commented 2 years ago

Fine, as long as you don't force the delete. what do you mean? this issue was fixed in 4.8 and it will be deleted.

bohelax commented 2 years ago

Well, my point was that I wanted to keep the .srt file

In 4.8 the delete of the .srt file was indeed deleted and when trying --keep-original I end up with:

bin>svtplay-dl.py -S -M https://www.svtplay.se/video/32988966/politikbyran/pol ikbyran-slaget-om-budgeten --keep-original INFO: Outfile: politikbyran.slaget.om.budgeten-10980a1-svtplay.srt INFO: Outfile: politikbyran.slaget.om.budgeten-10980a1-svtplay.mp4 INFO: Selected to download dash, bitrate: 5458 format: h264 [450/450][========================================] ETA: 0:00:00 [450/450][========================================] ETA: 0:00:00 INFO: Merge audio, video and subtitle into politikbyran.slaget.om.budgeten-109 a1-svtplay.mp4 INFO: Determining the language of the subtitle(s). INFO: Language code: swe INFO: Merging done, keeping original files. Traceback (most recent call last): File "C:\Users\AdminW10\AppData\Local\Programs\Python\Python39\lib\runpy.py" line 197, in _run_module_as_main return _run_code(code, main_globals, None, File "C:\Users\AdminW10\AppData\Local\Programs\Python\Python39\lib\runpy.py" line 87, in _run_code exec(code, run_globals) File "C:\DAPdownload\ffmpeg\bin\svtplay-dl.py__main__.py", line 15, in <mod e> File "C:\DAPdownload\ffmpeg\bin\svtplay-dl.py\svtplay_dl__init.py", line , in main File "C:\DAPdownload\ffmpeg\bin\svtplay-dl.py\svtplay_dl\utils\getmedia.py", ine 63, in get_media File "C:\DAPdownload\ffmpeg\bin\svtplay-dl.py\svtplay_dl\utils\getmedia.py", ine 224, in get_one_media File "C:\DAPdownload\ffmpeg\bin\svtplay-dl.py\svtplay_dl\postprocess\init_ py", line 115, in merge FileExistsError: [WinError 183] Det går inte att skapa en fil som redan finns: C:\DAPdownload\ffmpeg\bin\politikbyran.slaget.om.budgeten-10980a1-svtplay. mp' -> 'C:\DAPdownload\ffmpeg\bin\politikbyran.slaget.om.budgeten-10980a1- tplay.mp4' 76.3 seconds bin>

and I stuck with the four files

politikbyran.slaget.om.budgeten-10980a1-svtplay.srt politikbyran.slaget.om.budgeten-10980a1-svtplay.m4a politikbyran.slaget.om.budgeten-10980a1-svtplay.mp4 politikbyran.slaget.om.budgeten-10980a1-svtplay.temp

and to keep the .srt file (with the merged .mp4) I need to delete two files and rename another to get what I used to get (apart from getting an error message every time).

This is a step backwards (for me). The --help does not contain anything that will restore the 'old' behavior.

iwconfig commented 2 years ago

This wasn't a bug. When I implemented the -M functionality, years ago now, I did this intentionally so that you could have the subtitle both merged and external if you want it that way. I don't understand why this is an issue really. Just don't use -S together with -M and maybe mention this distinction in the help screen.

spaam commented 2 years ago

bah i guess i should just revert everything and only have the features i use. if people want other features they have to maintain it 🤔

iwconfig commented 2 years ago

Well, I don't really care tbh. I just wanted to point this out because I don't understand why this was an issue to begin with.

iwconfig commented 2 years ago

Alright, i just did a PR reverting this and added some clarifications, check out #1444 and share your thoughts if you have any objections.

Another thought, maybe because of situations like this one it is appropriate to introduce some code conventions like for example making sure to write comments sufficiently

iwconfig commented 2 years ago

OK so, I wasn't sure but I had it in the back of my mind that this has been mentioned in the help screen before. I did a little digging and it turns out I was right. In 938ae7d9b9 for version 2.9 it was removed, without any indication of why this happened.

Before that commit it was

merge subtitle with video/audio file with corresponding ISO639-3 language code. this invokes --remux automatically. use with -S for external also.

and now it is like the above without the last sentence (in bold). Which is strange because the next commit, 0201222, made sure -S also is set to True when -M is True.

This is what I did from the beginning which I also commented about in that commit, but what the issue for that commit was I don't recall at all. However, with 4.9 I believe this is not needed (and it shouldn't be), hence I removed it in #1444.

I not sure why this happened, but presumably by accident. Anyway, I think it might have affected this situation where a minor issue is solved by removing something that's actually a (simple) feature and not a bug. Correct me if I'm wrong.

Maybe @spaam can shed some light on this?

EDIT:

More digging! Apparently I also mentioned this in my PR where this subtitle merging thing first came about.

See #414. Quote:

-M, --merge-subtitle removes the external subtitles after the merge, but they do persist if used with -S.

bohelax commented 2 years ago

I am sorry I didn't post this as a new issue. Since it was still open I didn't know that it already had been implemented.

Also I didn't realize it would become a problem, since the old behavior seemed fine to me:

-S : downloads and keeps the subtitle file. -S -M : downloads and keeps the subtitle file, and merges it too. -M : downloads, but does not keep the subtitle file, and merges it too.

It really is a hazle having to copy the subtile file somewhere before it is deleted, every time.

//BoH

iwconfig commented 2 years ago
Sopor commented 10 months ago

This is what i see after merging, but the srt file is not removed:

INFO: Merge audio, video and subtitle into chelsea.detective.s02e02.2.golden.years-490bf89-svtplay.mkv
INFO: Determining the language of the subtitle(s).
INFO: Language code: swe
INFO: Merging done, removing old files.

If it is merged it should also be removed.

iwconfig commented 10 months ago

This is what i see after merging, but the srt file is not removed:

INFO: Merge audio, video and subtitle into chelsea.detective.s02e02.2.golden.years-490bf89-svtplay.mkv
INFO: Determining the language of the subtitle(s).
INFO: Language code: swe
INFO: Merging done, removing old files.

If it is merged it should also be removed.

So skip -S. Pretty simple. How is that problematic? You want it to say "merging done and all belonging files except any subtitles are removed"?

But anyhow though, I'll share my honest view on this non-issue by quoting myself saying

Well, I don't really care tbh.

Feel free to change it to your liking