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

FFmpeg (5.0+) has updated the tag where rotation info is retrieved from #45837

Merged

Conversation

hahmed
Copy link
Contributor

@hahmed hahmed commented Aug 16, 2022

Summary

When running the test locally bin/test test/analyzer/video_analyzer_test.rb:22
(analyzing a rotated video).

I'm seeing it failing for FFmpeg in versions 5.0+ this is due to a change upstream.

The original code which pulls the rotation from tags is still left for
older ffmpeg versions whilst the newer ffmpeg version 5.0+ will use the
side_data to retrieve the rotation info.

Example output for side_data_list (when adding puts here https://github.com/rails/rails/pull/45837/files#diff-e69a5f7b5e4a55a776812dcc33b529c549984a5692048b0ec2683596abd999b4R107)

[{"side_data_type"=>"Display Matrix",   
  "displaymatrix"=>                     
   "\n00000000:            0       65536           0\n00000001:       -65536           0           0\n00000002:            0           0  1073741824\n",
  "rotation"=>-90}] 

Run the command below to see the full output for the rotated_video file.

ffprobe -print_format json -show_streams -show_format activestorage/test/fixtures/files/rotated_video.mp4

Other Information

It's unclear why on CI everything is green but this line https://github.com/rails/buildkite-config/blob/b44bcf075b8683283ee4ab06391c895e608f162e/Dockerfile#L91 looks like
we are using the latest version, I have no idea why upstream is not failing 🤷‍♂️

Commits where "rotate" was removed;

cc/ @eileencodes

@@ -26,7 +26,7 @@ class ActiveStorage::Analyzer::VideoAnalyzerTest < ActiveSupport::TestCase
assert_equal 480, metadata[:width]
assert_equal 640, metadata[:height]
assert_equal [4, 3], metadata[:display_aspect_ratio]
assert_equal 90, metadata[:angle]
assert_includes [90, -90], metadata[:angle]
Copy link
Contributor Author

@hahmed hahmed Aug 16, 2022

Choose a reason for hiding this comment

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

Easy way not to care about the version of ffmpeg? Version < 5 this is 90 and 5.0+ it's -90

Copy link
Member

@eileencodes eileencodes left a comment

Choose a reason for hiding this comment

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

Let's add a changelog in case for someone -90 and -270 represents a behavior change.

@hahmed hahmed force-pushed the ha/active-storage-fix-rotation-test-failing branch 2 times, most recently from 114c4a3 to 78abc57 Compare August 17, 2022 20:36
@hahmed hahmed force-pushed the ha/active-storage-fix-rotation-test-failing branch from 78abc57 to ad68fec Compare August 26, 2022 14:01
@rails-bot rails-bot bot added the railties label Aug 26, 2022
@hahmed
Copy link
Contributor Author

hahmed commented Aug 26, 2022

The tests look unrelated, will see if I can take a look but let me know if there is anything else needed for this 🙂

EDIT: actually, I committed a file by accident, let me remove

The original code which pulls the rotation from tags is still left for
older ffmpeg versions whilst the newer ffmpeg version 5.0+ will use the
side_data to retrieve the rotation info.
@hahmed hahmed force-pushed the ha/active-storage-fix-rotation-test-failing branch from ad68fec to 206a826 Compare August 26, 2022 14:26
@eileencodes eileencodes merged commit 7de6670 into rails:main Sep 27, 2022
yahonda pushed a commit to yahonda/rails that referenced this pull request Jun 23, 2023
…tion-test-failing

FFmpeg (5.0+) has updated the tag where rotation info is retrieved from
yahonda added a commit that referenced this pull request Jun 23, 2023
Merge pull request #45837 from hahmed/ha/active-storage-fix-rotation-…
skipkayhil pushed a commit to skipkayhil/rails that referenced this pull request Jan 9, 2024
…tion-test-failing

FFmpeg (5.0+) has updated the tag where rotation info is retrieved from
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

2 participants