-
-
Notifications
You must be signed in to change notification settings - Fork 527
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
feat(workers): allows videoWorker to use ytdlp command line arguments… #1117
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Looks good to me, but we need to update the documentation in docs/docs/03-configuration.md
.
const ytDlpArguments = [url]; | ||
if (serverConfig.crawler.maxVideoDownloadSize > 0) { | ||
ytDlpArguments.push( | ||
"-f", | ||
`best[filesize<${serverConfig.crawler.maxVideoDownloadSize}M]`, | ||
); | ||
} | ||
|
||
ytDlpArguments.push(...serverConfig.crawler.ytDlpArguments.split(",")); | ||
ytDlpArguments.push("-o", assetPath); | ||
ytDlpArguments.push("--no-playlist"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we maybe move this to the default of the command?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
game for whatever works for y'all. if there's a good way to represent an array with the config library you're using, i'm all for it.
packages/shared/config.ts
Outdated
@@ -49,6 +49,7 @@ const allEnv = z.object({ | |||
CRAWLER_VIDEO_DOWNLOAD_MAX_SIZE: z.coerce.number().default(50), | |||
CRAWLER_VIDEO_DOWNLOAD_TIMEOUT_SEC: z.coerce.number().default(10 * 60), | |||
CRAWLER_ENABLE_ADBLOCKER: stringBool("true"), | |||
YTDLP_ARGS: z.string().default(""), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do the parsing here as well.
YTDLP_ARGS: z.string().default(""), | |
YTDLP_ARGS: z.string().transform(t => t.split(",").filter(a => a)).default([]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works for me. i'll make those changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually that won't work if the default is empty array as zod is going to always expect that to be a string.
YTDLP_ARGS: z.string().transform(t => t.split(",").filter(a => a)).default('') // works but now the type is ambiguous
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about:
YTDLP_ARGS: z.string().default('').transform(t => t.split(",").filter(a => a))
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that'll do it.
Btw, did you happen to check if any of the ytldb arguments can contain commas? Because that's the delimiter we're using and it might end up being problematic if they do. |
I'll check, I'm also happy to pass certain ones through. I really only need subtitles and login. |
Yeah unfortunately there are.
which would be problematic for my approach :( |
Let's try to find another delimiter then? Maybe |
Happy to code that up. I'll see if I can find a common pattern. Parsing the flags isn't too hard:
It's always going to be a |
Let's just keep it simple and instead of splitting by commas, we can split by |
554b5f6
to
3d8a742
Compare
… specified in the config
3d8a742
to
0182110
Compare
Fixed up! |
Added! |
… specified in the config