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 URL Enhancements and Video Verification after Download #26

Merged
merged 4 commits into from
Jan 22, 2024

Conversation

movchan74
Copy link
Contributor

Summary:
This Pull Request enhances our YouTube integration by allowing more complex URL parameters and improving error handling for invalid video content.

Key Changes:

  1. Extended URL Parameter Support: Now supports additional parameters in YouTube URLs, such as 't=4s&ab_channel=A24'. Currently, the extra parameters are ignored, we will add proper support for the video start time parameter later.
  2. Improved Error Handling for Invalid Videos: Added video-checking post-download. If the downloaded content is not a valid video, the system now raises a VideoReadingError, ensuring that only valid video content is processed and error messages are more meaningful.

and improved errors for invalid video URL
Copy link
Collaborator

@evanderiel evanderiel left a comment

Choose a reason for hiding this comment

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

Good but let's not use regex for parsing URLs

aana/models/core/video.py Show resolved Hide resolved
aana/models/core/video_source.py Outdated Show resolved Hide resolved
aana/tests/test_video_source.py Outdated Show resolved Hide resolved
@movchan74
Copy link
Contributor Author

@evanderiel I removed VideoSource in favor of using yt-dlp for all video downloads. Please, take a look again.

@movchan74 movchan74 requested a review from evanderiel January 8, 2024 12:25
Copy link
Contributor

@HRashidi HRashidi left a comment

Choose a reason for hiding this comment

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

LTGM

Just a question about the video downloader
If the video is not exist or the url is not valid, does it creates a empty file or throw exception. If possible we can use different error message in the UI.

aana/tests/test_video.py Show resolved Hide resolved
@movchan74
Copy link
Contributor Author

movchan74 commented Jan 9, 2024

LTGM

Just a question about the video downloader If the video is not exist or the url is not valid, does it creates a empty file or throw exception. If possible we can use different error message in the UI.

@HRashidi If it cannot download the video, it will throw DeownloadException and nothing will be saved.

Copy link
Collaborator

@evanderiel evanderiel 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, but please fix the usedforsecurity parameter with md5 & resolve comments before merging

aana/tests/test_video.py Outdated Show resolved Hide resolved
aana/utils/video.py Outdated Show resolved Hide resolved
aana/models/core/video.py Show resolved Hide resolved
@movchan74 movchan74 requested a review from evanderiel January 22, 2024 11:05
@movchan74 movchan74 merged commit d555e9a into main Jan 22, 2024
2 checks passed
@movchan74 movchan74 deleted the youtube_extra_params branch January 22, 2024 20:31
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.

3 participants