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

Active Storage test_analyzing_a_video_without_a_video_stream fails using Ubuntu 23.10 #49650

Closed
yahonda opened this issue Oct 16, 2023 · 9 comments · Fixed by #49869
Closed

Comments

@yahonda
Copy link
Member

yahonda commented Oct 16, 2023

Steps to reproduce

Install Ubuntu 23.10

git clone https://github.com/rails/rails
cd rails/activestorage
bundle install
bin/test test/analyzer/video_analyzer_test.rb -n test_analyzing_a_video_without_a_video_stream

Expected behavior

It should pass.

Actual behavior

$ bin/test test/analyzer/video_analyzer_test.rb -n test_analyzing_a_video_without_a_video_stream
/home/yahonda/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/httpclient-2.8.3/lib/httpclient/ssl_config.rb:370: warning: assigned but unused variable - pathlen
/home/yahonda/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/httpclient-2.8.3/lib/httpclient/ssl_config.rb:51: warning: method redefined; discarding old initialize
/home/yahonda/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/httpclient-2.8.3/lib/httpclient/ssl_config.rb:58: warning: method redefined; discarding old add_cert
/home/yahonda/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/httpclient-2.8.3/lib/httpclient/ssl_config.rb:58: warning: method redefined; discarding old add_file
/home/yahonda/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/httpclient-2.8.3/lib/httpclient/ssl_config.rb:58: warning: method redefined; discarding old add_path
Missing service configuration file in test/service/configurations.yml
/home/yahonda/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/turbo-rails-1.3.2/app/models/concerns/turbo/broadcastable.rb:84: warning: `**' interpreted as argument prefix
== 20170806125915 CreateActiveStorageTables: migrating ========================
-- create_table(:active_storage_blobs, {:id=>:primary_key})
   -> 0.0008s
-- create_table(:active_storage_attachments, {:id=>:primary_key})
   -> 0.0007s
-- create_table(:active_storage_variant_records, {:id=>:primary_key})
   -> 0.0005s
== 20170806125915 CreateActiveStorageTables: migrated (0.0022s) ===============

==  ActiveStorageCreateUsers: migrating =======================================
-- create_table(:users)
   -> 0.0004s
==  ActiveStorageCreateUsers: migrated (0.0004s) ==============================

==  ActiveStorageCreateGroups: migrating ======================================
-- create_table(:groups)
   -> 0.0003s
==  ActiveStorageCreateGroups: migrated (0.0003s) =============================

Run options: -n test_analyzing_a_video_without_a_video_stream --seed 7120

# Running:

F

Failure:
ActiveStorage::Analyzer::VideoAnalyzerTest#test_analyzing_a_video_without_a_video_stream [/home/yahonda/src/github.com/rails/rails/activestorage/test/analyzer/video_analyzer_test.rb:67]:
Expected: 1.022
  Actual: 1.0


bin/test test/analyzer/video_analyzer_test.rb:61



Finished in 0.092548s, 10.8052 runs/s, 54.0261 assertions/s.
1 runs, 5 assertions, 1 failures, 0 errors, 0 skips
$

System configuration

Rails version: main branch

Ruby version: ruby 3.2.2 (2023-03-30 revision e51014f9c0) [x86_64-linux]

@yahonda
Copy link
Member Author

yahonda commented Oct 16, 2023

Here is the ffmpeg version installed.

$ apt info ffmpeg
Package: ffmpeg
Version: 7:6.0-6ubuntu1
Priority: optional
Section: universe/video
Origin: Ubuntu
Maintainer: Ubuntu Developers <ubuntu-devel-discuss@lists.ubuntu.com>
Original-Maintainer: Debian Multimedia Maintainers <debian-multimedia@lists.debian.org>
Bugs: https://bugs.launchpad.net/ubuntu/+filebug
Installed-Size: 2,486 kB
Depends: libavcodec60 (>= 7:6.0), libavdevice60 (>= 7:6.0), libavfilter9 (>= 7:6.0), libavformat60 (>= 7:6.0), libavutil58 (>= 7:6.0), libc6 (>= 2.35), libpostproc57 (>= 7:6.0), libsdl2-2.0-0 (>= 2.0.12), libswresample4 (>= 7:6.0), libswscale7 (>= 7:6.0)
Suggests: ffmpeg-doc
Homepage: https://ffmpeg.org/
Task: kubuntu-desktop, kubuntu-full, ubuntustudio-desktop, ubuntukylin-desktop, ubuntu-mate-desktop, ubuntu-budgie-desktop, ubuntu-budgie-desktop-raspi, ubuntucinnamon-desktop, ubuntucinnamon-desktop-raspi
Download-Size: 1,853 kB
APT-Manual-Installed: yes
APT-Sources: http://jp.archive.ubuntu.com/ubuntu mantic/universe amd64 Packages
Description: Tools for transcoding, streaming and playing of multimedia files
 FFmpeg is the leading multimedia framework, able to decode, encode, transcode,
 mux, demux, stream, filter and play pretty much anything that humans and
 machines have created. It supports the most obscure ancient formats up to the
 cutting edge.
 .
 This package contains:
  * ffmpeg: a command line tool to convert multimedia files between formats
  * ffplay: a simple media player based on SDL and the FFmpeg libraries
  * ffprobe: a simple multimedia stream analyzer
  * qt-faststart: a utility to rearrange Quicktime files

$

@yahonda
Copy link
Member Author

yahonda commented Oct 18, 2023

I can say this is likely due to ffmpeg/ffprobe behavior difference between 5.1 and 6.0.

This issue started to reproduce once upgrading Ubuntu 23.04 to 23.10

ffprobe shows different duration between 5.1 and 6.0 for the same file

  • `ffprobe version 5.1.3 installed at Red Hat Enterprise Linux release 9.2 (Plow)
$ ffprobe -v error -show_format -show_streams activestorage/test/fixtures/files/video_without_video_stream.mp4
[STREAM]
... snip ...
duration=1.000000
... snip ...
[/STREAM]
[FORMAT]
... snip ...
duration=1.022000
... snip ...
[/FORMAT]
$
  • ffprobe version 6.0-6ubuntu1 installed at Ubuntu 23.10
$ ffprobe -v error -show_format -show_streams activestorage/test/fixtures/files/video_without_video_stream.mp4
[STREAM]
... snip ...
duration=1.000000
... snip ...
[/STREAM]
[FORMAT]
... snip ...
duration=1.000000
... snip ...
[/FORMAT]
$

@zzak
Copy link
Member

zzak commented Oct 20, 2023

I could reproduce this on my machine with ffprobe 6.0, I think this is a change upstream and we could report it to their bug tracker -- but also that test could be more conservative?

@yahonda
Copy link
Member Author

yahonda commented Oct 28, 2023

I've sent a email to https://ffmpeg.org/pipermail/ffmpeg-user/ mailing list to see if this is an expected behavior change between ffprobe 5.1 and 6.0.

@yahonda
Copy link
Member Author

yahonda commented Oct 29, 2023

@yahonda
Copy link
Member Author

yahonda commented Oct 30, 2023

Informed this is an expected behavior change.

http://ffmpeg.org/pipermail/ffmpeg-user/2023-October/057083.html

Yes, this is an expected change,
take a look at this unambiguous commit:
FFmpeg/FFmpeg@836b800

Note that your sample has an edit list which truncates the available (stts) duration to 1s,
but other durations in the file (mvhd/mdhd/tkhd) are set to longer values - which is wrong (according to 14496-12).

@zzak
Copy link
Member

zzak commented Oct 31, 2023

What do you think of this, or something like it to just round the result?

--- a/activestorage/test/analyzer/video_analyzer_test.rb
+++ b/activestorage/test/analyzer/video_analyzer_test.rb
@@ -64,7 +64,7 @@ class ActiveStorage::Analyzer::VideoAnalyzerTest < ActiveSupport::TestCase

     assert_not_includes metadata, :width
     assert_not_includes metadata, :height
-    assert_equal 1.022000, metadata[:duration]
+    assert_equal 1.0.to_fs, metadata[:duration].to_fs

Do we really care about the accuracy or just trust upstream to do the right thing 🤔

@yahonda
Copy link
Member Author

yahonda commented Oct 31, 2023

I'd like to hear from @jonathanhefner as the author of a197d39e13e

jonathanhefner added a commit to jonathanhefner/rails that referenced this issue Oct 31, 2023
FFmpeg 6.0 now reports the duration of `video_without_video_stream.mp4`
as 1.000000 instead of 1.022000.  This discrepancy was [reported][]
to the FFmpeg mailing list, and the [reply][] indicated that the change
is intentional.

For this test, the exact duration isn't significant.  We merely want to
assert that the metadata includes the duration reported by FFmpeg.
Therefore, this commit changes the assertion to accomodate the duration
reported by FFmpeg 6.0 as well as previous versions.

Fixes rails#49650.

[reported]: https://ffmpeg.org/pipermail/ffmpeg-user/2023-October/057067.html
[reply]: http://ffmpeg.org/pipermail/ffmpeg-user/2023-October/057083.html

Co-authored-by: Yasuo Honda <yasuo.honda@gmail.com>
@jonathanhefner
Copy link
Member

The purpose of a197d39 was to support duration metadata for video formats such as WebM, which store duration in the container rather than the video stream. The test for that still appears to be correct.

That change also happened to provide duration metadata for video files that didn't have a video stream (i.e. audio only), which is why a197d39 had to modify assert_equal(assert_equal({ "analyzed" => true, "identified" => true }, metadata). But the specific duration 1.022000 is not important. We just want to assert that we return the correct duration from FFmpeg. If FFmpeg now claims the correct duration is 1.000000, then it is okay.

So I have submitted #49869 to support the duration reported by FFmpeg 6.0 as well as previous versions. @yahonda, I have added you as a co-author since you spent time investigating this. Thank you! ❤️

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

Successfully merging a pull request may close this issue.

3 participants