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

ActiveStorage: Allow the parameters sent to ffmpeg to be configurable #42471

Merged
merged 1 commit into from Jun 16, 2021

Conversation

brendon
Copy link
Contributor

@brendon brendon commented Jun 13, 2021

Summary

The parameters sent to ffmpeg for generating a video preview image are now configurable under config.active_storage.video_preview_arguments.

This PR also better documents a previous commit from Jonathan Hefner (@jonathanhefner) that changes the default video image preview to use scene detection to generate a better preview.

Other Information

I don't think any additional tests are required since preview generation should be tested under both configurations in the CI suite.

Concerns: #39096

@brendon
Copy link
Contributor Author

brendon commented Jun 13, 2021

Looks like the failure was a transient failure to connect to S3.

Copy link
Member

@jonathanhefner jonathanhefner left a comment

Choose a reason for hiding this comment

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

Thank you for working on this, @brendon!

railties/lib/rails/application/configuration.rb Outdated Show resolved Hide resolved
@pixeltrix
Copy link
Contributor

Looks good but we usually have some configuration tests in railties to ensure that the defaults for upgrading apps and new apps are set correctly - can we add those please.

@brendon
Copy link
Contributor Author

brendon commented Jun 15, 2021

Ok, all the changes are made. Apologies for all the commits. I have to admit I don't know how to fix that. The guidance here didn't seem to work for me.

@brendon brendon force-pushed the ffmpeg-options branch 2 times, most recently from 5a7da2c to 3d3f34d Compare June 15, 2021 02:15
@brendon
Copy link
Contributor Author

brendon commented Jun 15, 2021

Managed to squish everything into one commit. Hopefully the tests are green and this is good to go :)

guides/CHANGELOG.md Outdated Show resolved Hide resolved
The configuration key is `config.active_storage.video_preview_arguments`.

This commit also better documents a previous commit from Jonathan Hefner (@ jonathanhefner) that changes the default video image preview to use scene detection to generate a better preview.
@zzak zzak added the ready PRs ready to merge label Jun 16, 2021
@pixeltrix pixeltrix merged commit dc512b0 into rails:main Jun 16, 2021
@pixeltrix
Copy link
Contributor

@brendon thanks! 👍🏻

@brendon
Copy link
Contributor Author

brendon commented Jun 16, 2021

You're welcome @pixeltrix :) I'll fix up the back-port now :)

@crawler
Copy link
Contributor

crawler commented Jun 26, 2021

@pixeltrix, @zzak Sorry for the disturbance. Maybe, something like this will make sense for the Blob#default_variant_transformations ? For example auto_orient: true and/or gravity: 'Center' as default options can work well for all variants in some projects.

@pixeltrix
Copy link
Contributor

@crawler actually I think a better feature would be to have named transformations, e.g.

config.active_storage.variations = {
  thumbnail: { resize_to_limit: [100, 100] }
}

avatar.variant(:thumbnail).processed.url

This seems fairly easy to implement by extending ActiveStorage::Variation.wrap to map a symbol to a key in the config hash.

Having said that I'm not opposed to also adding default transformations as long as it allows you to override the default file format as well to something like JPEG.

@crawler
Copy link
Contributor

crawler commented Jun 26, 2021

Or maybe allow to set defaults, through some combination with #39135

@pixeltrix
Copy link
Contributor

LOL, didn't know about #39135 🤦🏻

@crawler
Copy link
Contributor

crawler commented Jun 26, 2021

I also just found this 😅

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

Successfully merging this pull request may close these issues.

None yet

5 participants