Revert "Support animated PNGs in media attachments (#28516)" (#33334)

app/models/media_attachment.rb

@@ -58,7 +58,6 @@ 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
@@ -103,7 +102,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
-        '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
+        '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
         'c:v' => 'h264',
         'c:a' => 'aac',
         'b:a' => '192k',
@@ -297,7 +296,7 @@ class MediaAttachment < ApplicationRecord
     private
 
     def file_styles(attachment)
-      if attachment.instance.animated_image? || VIDEO_CONVERTIBLE_MIME_TYPES.include?(attachment.instance.file_content_type)
+      if attachment.instance.file_content_type == 'image/gif' || 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
@@ -311,8 +310,8 @@ class MediaAttachment < ApplicationRecord
     end
 
     def file_processors(instance)
-      if instance.animated_image?
-        [:gifv_transcoder, :blurhash_transcoder]
+      if instance.file_content_type == 'image/gif'
+        [:gif_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)
@@ -323,17 +322,6 @@ 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/gifv_transcoder'
+require_relative '../lib/paperclip/gif_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 -flatten -depth 8 -compress none RGB:-', source: File.expand_path(@file.path)).unpack('C*')
+        pixels   = convert(':source -depth 8 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

@@ -0,0 +1,126 @@
+# 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

@@ -1,20 +0,0 @@
-# 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 metadata and generates styles' do
+    it 'saves media attachment with correct file and size metadata' do
       expect(media)
         .to be_persisted
         .and be_processing_complete
@@ -98,28 +98,18 @@ RSpec.describe MediaAttachment, :attachment_processing do
           file: be_present,
           type: eq('image'),
           file_content_type: eq(content_type),
-          file_file_name: end_with(extension),
-          blurhash: have_attributes(size: eq(36))
+          file_file_name: end_with(extension)
         )
 
+      # 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_metadata)
+      expect(media.file.meta.deep_symbolize_keys)
         .to include(
           original: include(
             width: eq(600),
@@ -132,60 +122,6 @@ 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
 
@@ -201,10 +137,10 @@ RSpec.describe MediaAttachment, :attachment_processing do
     it_behaves_like 'static 600x400 image', 'image/png', '.png'
   end
 
-  describe 'gif' do
-    let(:media) { Fabricate(:media_attachment, file: attachment_fixture('600x400.gif')) }
+  describe 'monochrome jpg' do
+    let(:media) { Fabricate(:media_attachment, file: attachment_fixture('monochrome.png')) }
 
-    it_behaves_like 'static 600x400 image', 'image/gif', '.gif'
+    it_behaves_like 'static 600x400 image', 'image/png', '.png'
   end
 
   describe 'webp' do
@@ -225,12 +161,6 @@ 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) }
@@ -239,15 +169,51 @@ RSpec.describe MediaAttachment, :attachment_processing do
   end
 
   describe 'animated gif' do
-    let(:media) { Fabricate(:media_attachment, file: attachment_fixture('600x400-animated.gif')) }
+    let(:media) { Fabricate(:media_attachment, file: attachment_fixture('avatar.gif')) }
 
-    it_behaves_like 'animated 600x400 image'
+    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
   end
 
-  describe 'animated png' do
-    let(:media) { Fabricate(:media_attachment, file: attachment_fixture('600x400-animated.png')) }
+  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 },
+    ]
 
-    it_behaves_like 'animated 600x400 image'
+    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
   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('600x400.gif', 'image/gif') } }
+      let(:params) { { file: fixture_file_upload('attachment.gif', 'image/gif') } }
 
       it_behaves_like 'a successful media upload', 'image'
     end