-
-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
fix media uploads with ffmpeg 5 #21191
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.
Can you fix the linter complaint?
Nah, I wouldn't ask them to do it, it's unrelated to their change (annoyingly, the linter complains about already-existing issues anywhere in changed files, not just in the parts that were modified). |
No big deal, it's a simple fix |
app/models/media_attachment.rb
Outdated
@@ -133,7 +134,7 @@ class MediaAttachment < ApplicationRecord | |||
convert_options: { | |||
output: { | |||
'loglevel' => 'fatal', | |||
vf: 'scale=\'min(400\, iw):min(400\, ih)\':force_original_aspect_ratio=decrease', | |||
'vf': 'scale=\'min(400\, iw):min(400\, ih)\':force_original_aspect_ratio=decrease', |
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.
Rubocop wants you to use
'vf': 'scale=\'min(400\, iw):min(400\, ih)\':force_original_aspect_ratio=decrease', | |
'vf' => 'scale=\'min(400\, iw):min(400\, ih)\':force_original_aspect_ratio=decrease', |
(string-based hashes instead of symbol-based hashes)
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.
Ah, thank you, it's been a long time since I've Ruby'd
Is this change backwards-compatible with older versions of FFMPEG? If so, which versions? Is there a link to the FFMPEG change in question that broke this, for posterity/reference? |
I looked, but I was not able to find any specific mention of this breakage. The change logs are extremely terse: https://github.com/FFmpeg/FFmpeg/blob/1ff9c07fa696443d2d243e45d5794c8b19946a1b/Changelog#L57 But as I mentioned in the issue, I tested that adding |
Making a few online searches, I could not find when this argument was introduced exactly, but I could find references to the
It is! But the comment was more about not introducing unrelated changes and e.g. muddying the output of |
Oh I see, ok, I've reverted the fix then |
Fixes #21154. This was tested by running the ffmpeg command manually in a docker container on Ubuntu 20.04 and Debian 11, and this Ruby change was tested on my Fedora 36 server's mastodon instance.