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

Enable video thumbnail by default #1971

Merged
merged 4 commits into from
Aug 11, 2023
Merged

Conversation

Lingxi-Li
Copy link
Contributor

There is no video thumbnail out of the box and the related doc to enable it is outdated. The PR makes video thumbnail generation default behavior.

Copy link
Member

@ildyria ildyria left a comment

Choose a reason for hiding this comment

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

LGTM, but what if ffmpeg and ffprobe at not at this path ?

also according to:
https://github.com/PHP-FFMpeg/PHP-FFMpeg#how-this-library-works

This should not be necessary. Wouldn't it be better to first check whether they are in the path and then if not hardcode the paths instead ?
You can also use a database migration and access the values with Configs::getValueAsString() if you want to make those path configurable (possibly a better option). :)

@codecov
Copy link

codecov bot commented Aug 7, 2023

Codecov Report

Merging #1971 (2aa301c) into master (5ab140c) will decrease coverage by 0.45%.
The diff coverage is 100.00%.

Additional details and impacted files

@Lingxi-Li
Copy link
Contributor Author

Agree that there are better well-designed alternatives than this quick turnaround. Yet, I don't have any PHP knowledge, so won't go any further.

My installation is set up from the master branch. Per my experience, video thumbnail begins to work only after

  1. editing the Lychee/vendor/lychee-org/php-exif/lib/PHPExif/Adapter/FFprobe.php file and
  2. performing the changes in this PR.

I think "failing fast right at start" is better than "feature malfunctioning silently with no obvious clue". The latter usually leads to more difficult and longer troubleshooting sessions. So I consider this quick turnaround an improvement over existing code.

@Lingxi-Li
Copy link
Contributor Author

@ildyria Looks like the fixer PR automation is broken. I manually added a commit to fix the code style.

@ildyria
Copy link
Member

ildyria commented Aug 7, 2023

I fixed your PR.
Could you do a git pull and then do artisan migrate and try to see if the thumbnail generation is working ?

@ildyria
Copy link
Member

ildyria commented Aug 7, 2023

@qwerty287 given that I contaminated this PR, could you review it too please ?

@ildyria ildyria requested a review from a team August 7, 2023 18:43
@ildyria ildyria added this to the 4.10.1 milestone Aug 7, 2023
@ildyria ildyria added the Review: easy Easy review expected: probably just need a quick to go through. label Aug 7, 2023
@Lingxi-Li
Copy link
Contributor Author

@ildyria I did a brand-new installation with the changes and video thumbnails are working out of the box 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review: easy Easy review expected: probably just need a quick to go through.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants