Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

youtube-dl wrapper breaks when using absolute paths #18

Closed
msglm opened this issue Jan 17, 2021 · 5 comments
Closed

youtube-dl wrapper breaks when using absolute paths #18

msglm opened this issue Jan 17, 2021 · 5 comments

Comments

@msglm
Copy link

msglm commented Jan 17, 2021

when working with absolute paths (and tools that require them like tsp), the tool will break due to an appending of skrubbed- to the begining of the URI. This causes issues by making the URI for mv or ffmpeg to be skrubbed-/home/user/.... The reason for this is that the wrapper relies on youtube-dl's exec command and appending directly to the end of the file. A fix could be appending skrubbed to the middle of the file (i.e test-skrubbed.mp4 from test.mp4). The current nature of how this tool is set up makes it impossible to use with non-relative paths or tools that reqire them without tons of hacking.

@pukkandan
Copy link
Contributor

pukkandan commented Jan 17, 2021

You made 2 issues (accidentally?). Might want to close the other one.

The ytdl wrapper is a very simple implementation. It has a lot of issues and should be deprecated imo.

The fix for this issue is not as easy as you might think. Care must be taken to ensure that the filename ends up as test-skrubbed.mp4 and not test.mp4-skrubbed. Doing this in an OS-agnostic way is difficult (impossible?) using the current implementation.

I would recommend using my fork yt-dlp instead since it has direct integration with this project. This issue as well as #17 will not exist for it. Also, you can download and scrub whole playlists/channels in batch

@msglm
Copy link
Author

msglm commented Jan 17, 2021

"You made 2 issues (accidentally?). Might want to close the other one."
Yeah sorry about that, github cli is a bit buggy when it comes to canceling tasks, so it sent twice.

"The ytdl wrapper is a very simple implementation. It has a lot of issues and should be deprecated imo."
I think it can be saved (I basically rewrote the wrapper in shell script), but in its current state its a total disaster

"Care must be taken to ensure that the filename ends up as test-skrubbed.mp4 and not test.mp4-skrubbed."
Of course, I had modded the program to put -skrubbed at the end only for that to break ffmpeg. What I have done was abandon the wrapper and wrote a shell script that utilizes cut to get the basename and the extention. This would then glue those two together with "-skrubbed-" in between. I would love to make a PR to clean this thing up but I am not familiar with D at all.

"I would recommend using my fork yt-dlp"
I have looked into dlp as a way of fixing this, but I think its a bad idea to basically abandon a main feature of this project. Maybe I will end up learning D and fixing it.

@pukkandan
Copy link
Contributor

The wrapper also just uses an inline script to call sponskrub

spawnShell(`youtube-dl --exec "sponskrub %s -- %s {} skrubbed-{} && rm {} || mv {} skrubbed-{}" %s -- %s`.format(skrub_args, video_id, dl_args, video_id))

You can just modify this with your shell script
Obviously, it wouldn't directly work on windows. Hence why I said:

Doing this in an OS-agnostic way is difficult

@faissaloo
Copy link
Owner

faissaloo commented Jan 18, 2021

Hello there! I presume you were setting the output filename using youtube-dl's -o option, if so it should be fixed in https://github.com/faissaloo/SponSkrub/releases/tag/3.4.1

Nonetheless using @pukkandan's fork is still a good idea. That's not to say I'm deprecating youtube-dl-sponsorblock, but I might do so in future depending on how much adoption @pukkandan's fork gets. The core functionality this project deals with is sponskrub, youtube-dl-sponsorblock just exists for convenience.

I basically rewrote the wrapper in shell script

Funnily enough youtube-dl-sponsorblock was actually originally a bash wrapper script called ydl until sponskrub became more and more advanced and different people with different needs started using it.

@msglm
Copy link
Author

msglm commented Jan 20, 2021

good to know that this issue is being fixed. I ended up adopting youtube-dlp as a fix and its working fine for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants