Support animated PNGs in media attachments (#28516)

app/models/media_attachment.rb

@@ -58,6 +58,7 @@ class MediaAttachment < ApplicationRecord
   ).freeze
 
   IMAGE_MIME_TYPES             = %w(image/jpeg image/png image/gif image/heic image/heif image/webp image/avif).freeze
+  IMAGE_ANIMATED_MIME_TYPES    = %w(image/png image/gif).freeze
   IMAGE_CONVERTIBLE_MIME_TYPES = %w(image/heic image/heif image/avif).freeze
   VIDEO_MIME_TYPES             = %w(video/webm video/mp4 video/quicktime video/ogg).freeze
   VIDEO_CONVERTIBLE_MIME_TYPES = %w(video/webm video/quicktime).freeze
@@ -102,7 +103,7 @@ class MediaAttachment < ApplicationRecord
         'preset' => 'veryfast',
         'movflags' => 'faststart', # Move metadata to start of file so playback can begin before download finishes
         'pix_fmt' => 'yuv420p', # Ensure color space for cross-browser compatibility
-        'vf' => 'crop=floor(iw/2)*2:floor(ih/2)*2', # h264 requires width and height to be even. Crop instead of scale to avoid blurring
+        'filter_complex' => 'drawbox=t=fill:c=white[bg];[bg][0]overlay,crop=trunc(iw/2)*2:trunc(ih/2)*2', # Remove transparency. h264 requires width and height to be even; crop instead of scale to avoid blurring
         'c:v' => 'h264',
         'c:a' => 'aac',
         'b:a' => '192k',
@@ -296,7 +297,7 @@ class MediaAttachment < ApplicationRecord
     private
 
     def file_styles(attachment)
-      if attachment.instance.file_content_type == 'image/gif' || VIDEO_CONVERTIBLE_MIME_TYPES.include?(attachment.instance.file_content_type)
+      if attachment.instance.animated_image? || VIDEO_CONVERTIBLE_MIME_TYPES.include?(attachment.instance.file_content_type)
         VIDEO_CONVERTED_STYLES
       elsif IMAGE_CONVERTIBLE_MIME_TYPES.include?(attachment.instance.file_content_type)
         IMAGE_CONVERTED_STYLES
@@ -310,8 +311,8 @@ class MediaAttachment < ApplicationRecord
     end
 
     def file_processors(instance)
-      if instance.file_content_type == 'image/gif'
-        [:gif_transcoder, :blurhash_transcoder]
+      if instance.animated_image?
+        [:gifv_transcoder, :blurhash_transcoder]
       elsif VIDEO_MIME_TYPES.include?(instance.file_content_type)
         [:transcoder, :blurhash_transcoder, :type_corrector]
       elsif AUDIO_MIME_TYPES.include?(instance.file_content_type)
@@ -322,6 +323,17 @@ class MediaAttachment < ApplicationRecord
     end
   end
 
+  def animated_image?
+    if processing_complete?
+      gifv?
+    elsif IMAGE_ANIMATED_MIME_TYPES.include?(file_content_type)
+      @animated_image = FastImage.animated?(file.queued_for_write[:original].path) unless defined?(@animated_image)
+      @animated_image
+    else
+      false
+    end
+  end
+
   private
 
   def set_unknown_type

config/application.rb

@@ -28,7 +28,7 @@ require_relative '../lib/redis/namespace_extensions'
 require_relative '../lib/paperclip/url_generator_extensions'
 require_relative '../lib/paperclip/attachment_extensions'
 
-require_relative '../lib/paperclip/gif_transcoder'
+require_relative '../lib/paperclip/gifv_transcoder'
 require_relative '../lib/paperclip/media_type_spoof_detector_extensions'
 require_relative '../lib/paperclip/transcoder'
 require_relative '../lib/paperclip/type_corrector'

lib/paperclip/blurhash_transcoder.rb

@@ -23,7 +23,7 @@ module Paperclip
         image = Vips::Image.thumbnail(@file.path, 100)
         [image.width, image.height, image.colourspace(:srgb).extract_band(0, n: 3).to_a.flatten]
       else
-        pixels   = convert(':source -depth 8 RGB:-', source: File.expand_path(@file.path)).unpack('C*')
+        pixels   = convert(':source -flatten -depth 8 -compress none RGB:-', source: File.expand_path(@file.path)).unpack('C*')
         geometry = options.fetch(:file_geometry_parser).from_file(@file)
         [geometry.width, geometry.height, pixels]
       end

lib/paperclip/gif_transcoder.rb

@@ -1,126 +0,0 @@
-# frozen_string_literal: true
-
-class GifReader
-  attr_reader :animated
-
-  EXTENSION_LABELS = [0xf9, 0x01, 0xff].freeze
-  GIF_HEADERS      = %w(GIF87a GIF89a).freeze
-
-  class GifReaderException < StandardError; end
-
-  class UnknownImageType < GifReaderException; end
-
-  class CannotParseImage < GifReaderException; end
-
-  def self.animated?(path)
-    new(path).animated
-  rescue GifReaderException
-    false
-  end
-
-  def initialize(path, max_frames = 2)
-    @path      = path
-    @nb_frames = 0
-
-    File.open(path, 'rb') do |s|
-      raise UnknownImageType unless GIF_HEADERS.include?(s.read(6))
-
-      # Skip to "packed byte"
-      s.seek(4, IO::SEEK_CUR)
-
-      # "Packed byte" gives us the size of the GIF color table
-      packed_byte, = s.read(1).unpack('C')
-
-      # Skip background color and aspect ratio
-      s.seek(2, IO::SEEK_CUR)
-
-      if packed_byte & 0x80 != 0
-        # GIF uses a global color table, skip it
-        s.seek(3 * (1 << ((packed_byte & 0x07) + 1)), IO::SEEK_CUR)
-      end
-
-      # Now read data
-      while @nb_frames < max_frames
-        separator = s.read(1)
-
-        case separator
-        when ',' # Image block
-          @nb_frames += 1
-
-          # Skip to "packed byte"
-          s.seek(8, IO::SEEK_CUR)
-          packed_byte, = s.read(1).unpack('C')
-
-          if packed_byte & 0x80 != 0
-            # Image uses a local color table, skip it
-            s.seek(3 * (1 << ((packed_byte & 0x07) + 1)), IO::SEEK_CUR)
-          end
-
-          # Skip lzw min code size
-          raise InvalidValue unless s.read(1).unpack1('C') >= 2
-
-          # Skip image data sub-blocks
-          skip_sub_blocks!(s)
-        when '!' # Extension block
-          skip_extension_block!(s)
-        when ';' # Trailer
-          break
-        else
-          raise CannotParseImage
-        end
-      end
-    end
-
-    @animated = @nb_frames > 1
-  end
-
-  private
-
-  def skip_extension_block!(file)
-    if EXTENSION_LABELS.include?(file.read(1).unpack1('C'))
-      block_size, = file.read(1).unpack('C')
-      file.seek(block_size, IO::SEEK_CUR)
-    end
-
-    # Read until extension block end marker
-    skip_sub_blocks!(file)
-  end
-
-  # Skip sub-blocks up until block end marker
-  def skip_sub_blocks!(file)
-    loop do
-      size, = file.read(1).unpack('C')
-
-      break if size.zero?
-
-      file.seek(size, IO::SEEK_CUR)
-    end
-  end
-end
-
-module Paperclip
-  # This transcoder is only to be used for the MediaAttachment model
-  # to convert animated GIFs to videos
-
-  class GifTranscoder < Paperclip::Processor
-    def make
-      return File.open(@file.path) unless needs_convert?
-
-      final_file = Paperclip::Transcoder.make(file, options, attachment)
-
-      if options[:style] == :original
-        attachment.instance.file_file_name    = "#{File.basename(attachment.instance.file_file_name, '.*')}.mp4"
-        attachment.instance.file_content_type = 'video/mp4'
-        attachment.instance.type              = MediaAttachment.types[:gifv]
-      end
-
-      final_file
-    end
-
-    private
-
-    def needs_convert?
-      GifReader.animated?(file.path)
-    end
-  end
-end

lib/paperclip/gifv_transcoder.rb

@@ -0,0 +1,20 @@
+# frozen_string_literal: true
+
+module Paperclip
+  # This transcoder is only to be used for the MediaAttachment model
+  # to convert animated GIFs and PNGs to videos
+
+  class GifvTranscoder < Paperclip::Processor
+    def make
+      final_file = Paperclip::Transcoder.make(file, options, attachment)
+
+      if options[:style] == :original
+        attachment.instance.file_file_name    = "#{File.basename(attachment.instance.file_file_name, '.*')}.mp4"
+        attachment.instance.file_content_type = 'video/mp4'
+        attachment.instance.type              = MediaAttachment.types[:gifv]
+      end
+
+      final_file
+    end
+  end
+end

spec/models/media_attachment_spec.rb

@@ -90,7 +90,7 @@ RSpec.describe MediaAttachment, :attachment_processing do
       media.destroy
     end
 
-    it 'saves media attachment with correct file and size metadata' do
+    it 'saves metadata and generates styles' do
       expect(media)
         .to be_persisted
         .and be_processing_complete
@@ -98,18 +98,28 @@ RSpec.describe MediaAttachment, :attachment_processing do
           file: be_present,
           type: eq('image'),
           file_content_type: eq(content_type),
-          file_file_name: end_with(extension)
+          file_file_name: end_with(extension),
+          blurhash: have_attributes(size: eq(36))
         )
 
-      # Rack::Mime (used by PublicFileServerMiddleware) recognizes file extension
-      expect(Rack::Mime.mime_type(extension, nil)).to eq content_type
-
       # Strip original file name
       expect(media.file_file_name)
         .to_not start_with '600x400'
 
+      # Generate styles
+      expect(FastImage.size(media.file.path(:original)))
+        .to eq [600, 400]
+      expect(FastImage.size(media.file.path(:small)))
+        .to eq [588, 392]
+
+      # Use extension recognized by Rack::Mime (used by PublicFileServerMiddleware)
+      expect(media.file.path(:original))
+        .to end_with(extension)
+      expect(media.file.path(:small))
+        .to end_with(extension)
+
       # Set meta for original and thumbnail
-      expect(media.file.meta.deep_symbolize_keys)
+      expect(media_metadata)
         .to include(
           original: include(
             width: eq(600),
@@ -122,6 +132,60 @@ RSpec.describe MediaAttachment, :attachment_processing do
             aspect: eq(1.5)
           )
         )
+
+      # Rack::Mime (used by PublicFileServerMiddleware) recognizes file extension
+      expect(Rack::Mime.mime_type(extension, nil)).to eq content_type
+    end
+  end
+
+  shared_examples 'animated 600x400 image' do
+    after do
+      media.destroy
+    end
+
+    it 'saves metadata and generates styles' do
+      expect(media)
+        .to be_persisted
+        .and be_processing_complete
+        .and have_attributes(
+          file: be_present,
+          type: eq('gifv'),
+          file_content_type: eq('video/mp4'),
+          file_file_name: end_with('.mp4'),
+          blurhash: have_attributes(size: eq(36))
+        )
+
+      # Strip original file name
+      expect(media.file_file_name)
+        .to_not start_with '600x400'
+
+      # Transcode to MP4
+      expect(media.file.path(:original))
+        .to end_with('.mp4')
+
+      # Generate static thumbnail
+      expect(FastImage.size(media.file.path(:small)))
+        .to eq [600, 400]
+      expect(FastImage.animated?(media.file.path(:small)))
+        .to be false
+      expect(media.file.path(:small))
+        .to end_with('.png')
+
+      # Set meta for styles
+      expect(media_metadata)
+        .to include(
+          original: include(
+            width: eq(600),
+            height: eq(400),
+            duration: eq(3),
+            frame_rate: '1/1'
+          ),
+          small: include(
+            width: eq(600),
+            height: eq(400),
+            aspect: eq(1.5)
+          )
+        )
     end
   end
 
@@ -137,10 +201,10 @@ RSpec.describe MediaAttachment, :attachment_processing do
     it_behaves_like 'static 600x400 image', 'image/png', '.png'
   end
 
-  describe 'monochrome jpg' do
-    let(:media) { Fabricate(:media_attachment, file: attachment_fixture('monochrome.png')) }
+  describe 'gif' do
+    let(:media) { Fabricate(:media_attachment, file: attachment_fixture('600x400.gif')) }
 
-    it_behaves_like 'static 600x400 image', 'image/png', '.png'
+    it_behaves_like 'static 600x400 image', 'image/gif', '.gif'
   end
 
   describe 'webp' do
@@ -161,6 +225,12 @@ RSpec.describe MediaAttachment, :attachment_processing do
     it_behaves_like 'static 600x400 image', 'image/jpeg', '.jpeg'
   end
 
+  describe 'monochrome jpg' do
+    let(:media) { Fabricate(:media_attachment, file: attachment_fixture('monochrome.png')) }
+
+    it_behaves_like 'static 600x400 image', 'image/png', '.png'
+  end
+
   describe 'base64-encoded image' do
     let(:base64_attachment) { "data:image/jpeg;base64,#{Base64.encode64(attachment_fixture('600x400.jpeg').read)}" }
     let(:media) { Fabricate(:media_attachment, file: base64_attachment) }
@@ -169,51 +239,15 @@ RSpec.describe MediaAttachment, :attachment_processing do
   end
 
   describe 'animated gif' do
-    let(:media) { Fabricate(:media_attachment, file: attachment_fixture('avatar.gif')) }
+    let(:media) { Fabricate(:media_attachment, file: attachment_fixture('600x400-animated.gif')) }
 
-    it 'sets correct file metadata' do
-      expect(media)
-        .to have_attributes(
-          type: eq('gifv'),
-          file_content_type: eq('video/mp4')
-        )
-      expect(media_metadata)
-        .to include(
-          original: include(
-            width: eq(128),
-            height: eq(128)
-          )
-        )
-    end
+    it_behaves_like 'animated 600x400 image'
   end
 
-  describe 'static gif' do
-    fixtures = [
-      { filename: 'attachment.gif', width: 600, height: 400, aspect: 1.5 },
-      { filename: 'mini-static.gif', width: 32, height: 32, aspect: 1.0 },
-    ]
+  describe 'animated png' do
+    let(:media) { Fabricate(:media_attachment, file: attachment_fixture('600x400-animated.png')) }
 
-    fixtures.each do |fixture|
-      context fixture[:filename] do
-        let(:media) { Fabricate(:media_attachment, file: attachment_fixture(fixture[:filename])) }
-
-        it 'sets correct file metadata' do
-          expect(media)
-            .to have_attributes(
-              type: eq('image'),
-              file_content_type: eq('image/gif')
-            )
-          expect(media_metadata)
-            .to include(
-              original: include(
-                width: eq(fixture[:width]),
-                height: eq(fixture[:height]),
-                aspect: eq(fixture[:aspect])
-              )
-            )
-        end
-      end
-    end
+    it_behaves_like 'animated 600x400 image'
   end
 
   describe 'ogg with cover art' do

spec/requests/api/v1/media_spec.rb

@@ -137,7 +137,7 @@ RSpec.describe 'Media' do
     end
 
     context 'with image/gif', :attachment_processing do
-      let(:params) { { file: fixture_file_upload('attachment.gif', 'image/gif') } }
+      let(:params) { { file: fixture_file_upload('600x400.gif', 'image/gif') } }
 
       it_behaves_like 'a successful media upload', 'image'
     end