diff --git a/app/models/media_attachment.rb b/app/models/media_attachment.rb index 76815dede5..6708cd7793 100644 --- a/app/models/media_attachment.rb +++ b/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 diff --git a/config/application.rb b/config/application.rb index 59afafd5be..e4e9680e66 100644 --- a/config/application.rb +++ b/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' diff --git a/lib/paperclip/blurhash_transcoder.rb b/lib/paperclip/blurhash_transcoder.rb index fe58d1bce1..b4ff4a12a0 100644 --- a/lib/paperclip/blurhash_transcoder.rb +++ b/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 diff --git a/lib/paperclip/gif_transcoder.rb b/lib/paperclip/gif_transcoder.rb new file mode 100644 index 0000000000..32bdb8a863 --- /dev/null +++ b/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 diff --git a/lib/paperclip/gifv_transcoder.rb b/lib/paperclip/gifv_transcoder.rb deleted file mode 100644 index 4bb27e3b75..0000000000 --- a/lib/paperclip/gifv_transcoder.rb +++ /dev/null @@ -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 diff --git a/spec/fixtures/files/600x400-animated.gif b/spec/fixtures/files/600x400-animated.gif deleted file mode 100644 index 1a773c0af1..0000000000 Binary files a/spec/fixtures/files/600x400-animated.gif and /dev/null differ diff --git a/spec/fixtures/files/600x400-animated.png b/spec/fixtures/files/600x400-animated.png deleted file mode 100644 index 6430fceabb..0000000000 Binary files a/spec/fixtures/files/600x400-animated.png and /dev/null differ diff --git a/spec/fixtures/files/600x400.gif b/spec/fixtures/files/600x400.gif deleted file mode 100644 index bab39cb6f7..0000000000 Binary files a/spec/fixtures/files/600x400.gif and /dev/null differ diff --git a/spec/fixtures/files/attachment.gif b/spec/fixtures/files/attachment.gif new file mode 100644 index 0000000000..89dd73ad3e Binary files /dev/null and b/spec/fixtures/files/attachment.gif differ diff --git a/spec/models/media_attachment_spec.rb b/spec/models/media_attachment_spec.rb index cce4b30479..5f91ae0967 100644 --- a/spec/models/media_attachment_spec.rb +++ b/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 diff --git a/spec/requests/api/v1/media_spec.rb b/spec/requests/api/v1/media_spec.rb index 3340e26b98..d7d0b92f11 100644 --- a/spec/requests/api/v1/media_spec.rb +++ b/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