Fix #3400: Smarter thumbnail generation for videos

app/logical/ffmpeg.rb

@@ -0,0 +1,25 @@
+class FFmpeg
+  attr_reader :file
+
+  # Operate on a file with FFmpeg.
+  # @param file [File, String] a webm, mp4, gif, or apng file
+  def initialize(file)
+    @file = file.is_a?(String) ? File.open(file) : file
+  end
+
+  # Generate a .jpg preview image for a video or animation. Generates
+  # thumbnails intelligently by avoiding blank frames.
+  #
+  # @return [MediaFile] the preview image
+  def smart_video_preview
+    vp = Tempfile.new(["video-preview", ".jpg"], binmode: true)
+
+    # https://ffmpeg.org/ffmpeg.html#Main-options
+    # https://ffmpeg.org/ffmpeg-filters.html#thumbnail
+    ffmpeg_out, status = Open3.capture2e("ffmpeg -i #{file.path} -vf thumbnail=300 -frames:v 1 -y #{vp.path}")
+    raise "ffmpeg failed: #{ffmpeg_out}" if !status.success?
+    Rails.logger.debug(ffmpeg_out)
+
+    MediaFile.open(vp)
+  end
+end

app/logical/media_file/image.rb

@@ -38,11 +38,15 @@ class MediaFile::Image < MediaFile
   # @see https://github.com/jcupitt/libvips/wiki/HOWTO----Image-shrinking
   # @see http://jcupitt.github.io/libvips/API/current/Using-vipsthumbnail.md.html
   def preview(width, height)
-    output_file = Tempfile.new(["image-preview", ".jpg"])
-    resized_image = image.thumbnail_image(width, height: height, **THUMBNAIL_OPTIONS)
-    resized_image.jpegsave(output_file.path, **JPEG_OPTIONS)
+    if is_animated?
+      FFmpeg.new(file).smart_video_preview
+    else
+      output_file = Tempfile.new(["image-preview", ".jpg"])
+      resized_image = image.thumbnail_image(width, height: height, **THUMBNAIL_OPTIONS)
+      resized_image.jpegsave(output_file.path, **JPEG_OPTIONS)
 
-    MediaFile::Image.new(output_file)
+      MediaFile::Image.new(output_file)
+    end
   end
 
   def crop(width, height)

app/logical/media_file/ugoira.rb

@@ -36,6 +36,7 @@ class MediaFile::Ugoira < MediaFile
   # XXX should take width and height and resize image
   def convert
     raise NotImplementedError, "can't convert ugoira to webm: ffmpeg or mkvmerge not installed" unless self.class.videos_enabled?
+    raise RuntimeError, "can't convert ugoira to webm: no ugoira frame data was provided" unless frame_data.present?
 
     Dir.mktmpdir("ugoira-#{md5}") do |tmpdir|
       output_file = Tempfile.new(["ugoira-conversion", ".webm"], binmode: true)
@@ -87,10 +88,8 @@ class MediaFile::Ugoira < MediaFile
   end
 
   def preview_frame
-    tempfile = Tempfile.new("ugoira-preview", binmode: true)
-    zipfile.entries.first.extract(tempfile.path) { true } #  'true' means overwrite the existing tempfile.
-    MediaFile.open(tempfile)
+    FFmpeg.new(convert).smart_video_preview
   end
 
-  memoize :zipfile, :preview_frame, :dimensions
+  memoize :zipfile, :preview_frame, :dimensions, :convert
 end

app/logical/media_file/video.rb

@@ -32,9 +32,7 @@ class MediaFile::Video < MediaFile
   end
 
   def preview_frame
-    vp = Tempfile.new(["video-preview", ".jpg"], binmode: true)
-    video.screenshot(vp.path, seek_time: 0)
-    MediaFile.open(vp.path)
+    FFmpeg.new(file).smart_video_preview
   end
 
   memoize :video, :preview_frame, :dimensions, :duration, :has_audio?

config/initializers/inflections.rb

@@ -5,6 +5,7 @@
 # locales as you wish. All of these examples are active by default:
 ActiveSupport::Inflector.inflections(:en) do |inflect|
   inflect.uncountable "general"
+  inflect.acronym "FFmpeg"
 #   inflect.plural /^(ox)$/i, '\1en'
 #   inflect.singular /^(ox)en/i, '\1'
 #   inflect.irregular 'person', 'people'

test/unit/media_file_test.rb

@@ -33,7 +33,8 @@ class MediaFileTest < ActiveSupport::TestCase
 
     should "determine the correct dimensions for a ugoira file" do
       skip unless MediaFile.videos_enabled?
-      assert_equal([60, 60], MediaFile.open("test/files/valid_ugoira.zip").dimensions)
+      frame_data = JSON.parse(File.read("test/files/ugoira.json"))
+      assert_equal([60, 60], MediaFile.open("test/files/ugoira.zip", frame_data: frame_data).dimensions)
     end
 
     should "determine the correct dimensions for a flash file" do
@@ -57,7 +58,8 @@ class MediaFileTest < ActiveSupport::TestCase
       assert_equal([512, 512], mf.dimensions)
       assert_equal([512, 512], mf.dimensions)
 
-      mf = MediaFile.open("test/files/valid_ugoira.zip")
+      frame_data = JSON.parse(File.read("test/files/ugoira.json"))
+      mf = MediaFile.open("test/files/ugoira.zip", frame_data: frame_data)
       assert_equal([60, 60], mf.dimensions)
       assert_equal([60, 60], mf.dimensions)
     end
@@ -92,7 +94,7 @@ class MediaFileTest < ActiveSupport::TestCase
     end
 
     should "determine the correct extension for a ugoira file" do
-      assert_equal(:zip, MediaFile.open("test/files/valid_ugoira.zip").file_ext)
+      assert_equal(:zip, MediaFile.open("test/files/ugoira.zip").file_ext)
     end
 
     should "determine the correct extension for a flash file" do
@@ -113,12 +115,18 @@ class MediaFileTest < ActiveSupport::TestCase
   end
 
   context "#preview" do
-    should "generate a preview image" do
+    should "generate a preview image for a static image" do
       assert_equal([150, 101], MediaFile.open("test/files/test.jpg").preview(150, 150).dimensions)
       assert_equal([113, 150], MediaFile.open("test/files/test.png").preview(150, 150).dimensions)
       assert_equal([150, 150], MediaFile.open("test/files/test.gif").preview(150, 150).dimensions)
     end
 
+    should "generate a preview image for an animated image" do
+      skip unless MediaFile.videos_enabled?
+      assert_equal([86, 52], MediaFile.open("test/files/test-animated-86x52.gif").preview(150, 150).dimensions)
+      assert_equal([150, 150], MediaFile.open("test/files/apng/normal_apng.png").preview(150, 150).dimensions)
+    end
+
     should "generate a preview image for a video" do
       skip unless MediaFile.videos_enabled?
       assert_equal([150, 150], MediaFile.open("test/files/test-512x512.webm").preview(150, 150).dimensions)

test/unit/upload_service_test.rb

@@ -60,8 +60,6 @@ class UploadServiceTest < ActiveSupport::TestCase
 
           assert_not_nil(@upload.context["ugoira"])
           assert_operator(File.size(file.path), :>, 0)
-
-          file.close
         end
       end
     end