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

Implement video-length filter using ffprobe #6246

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

Skaronator
Copy link

@Skaronator Skaronator commented Sep 28, 2024

This implements two new filter options to filter videos based on their actual length before downloading them.

This fixes #4928

One interesting issue I noticed is that Motion JPEG exist. This is a video format that often only contains a single frame. Hence, I implemented the minimum frame logic.

@Skaronator Skaronator changed the title Implement video-llength filter using ffprobe Implement video-length filter using ffprobe Sep 28, 2024
@shelvacu
Copy link

I see a few problems with this approach. The request sent by ffprobe will not have any of the headers (most importantly cookies), the retry logic is missing, and this will 'double up' every request.

The proper (much more annoying to implement) way to do this would be to include ffmpeg as a library, and then do the requests in python and pass the data to the ffmpeg library.

I think it might make sense to include this nearly as-is (it still does something useful), but I think it should be marked with warnings in the documentation (maybe even rename the options something like experimental-min-length) and log a warning if headers["Cookies"] is set

@Skaronator
Copy link
Author

Thanks for the feedback. I'll look into it

@Skaronator
Copy link
Author

@shelvacu do you have an example link/page that explicitly requires specific headers? Thanks

@Skaronator Skaronator marked this pull request as draft October 21, 2024 07:31
@shelvacu
Copy link

Every site that requires a login will have to send a Cookie header. Other than that I'm not sure. I would suspect some sites insist on a Referer header but I'm not sure how to check which extractors add that.

@Skaronator
Copy link
Author

I've implemented the same retry mechanism as the standard HTTP downloader. The code has been moved to a separate file for better readability, and I've also added the header logic.

Please feel free to test it out, and any feedback would be greatly appreciated, @shelvacu @mikf

@Skaronator Skaronator marked this pull request as ready for review October 25, 2024 13:51
Copy link

@shelvacu shelvacu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I hope this gets merged.

@Skaronator
Copy link
Author

Fixed an issue while handling an error. Otherwise the code so far is stable in my longer tests.

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

Successfully merging this pull request may close these issues.

Implement ffprobe as an additional metadata filter option
2 participants