From 5c42f47617d311219d06e082e4daa41e671903c8 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Tue, 1 Oct 2019 01:19:11 +0200 Subject: Fix records not being indexed sometimes (#12024) It's possible that after commit callbacks were not firing when exceptions occurred in the process. Also, the default Sidekiq strategy does not push indexing jobs immediately, which is not necessary and could be part of the issue too. --- app/models/account.rb | 2 +- app/models/account_stat.rb | 2 +- app/models/application_record.rb | 6 ++++++ app/models/favourite.rb | 2 +- app/models/status.rb | 2 +- app/models/tag.rb | 2 +- 6 files changed, 11 insertions(+), 5 deletions(-) (limited to 'app/models') diff --git a/app/models/account.rb b/app/models/account.rb index 55fe53fae..01d45e36c 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -129,7 +129,7 @@ class Account < ApplicationRecord delegate :chosen_languages, to: :user, prefix: false, allow_nil: true - update_index('accounts#account', :self) if Chewy.enabled? + update_index('accounts#account', :self) def local? domain.nil? diff --git a/app/models/account_stat.rb b/app/models/account_stat.rb index 6d1097cec..1351f7d8a 100644 --- a/app/models/account_stat.rb +++ b/app/models/account_stat.rb @@ -16,7 +16,7 @@ class AccountStat < ApplicationRecord belongs_to :account, inverse_of: :account_stat - update_index('accounts#account', :account) if Chewy.enabled? + update_index('accounts#account', :account) def increment_count!(key) update(attributes_for_increment(key)) diff --git a/app/models/application_record.rb b/app/models/application_record.rb index c1b873da6..5d7d3a096 100644 --- a/app/models/application_record.rb +++ b/app/models/application_record.rb @@ -5,6 +5,12 @@ class ApplicationRecord < ActiveRecord::Base include Remotable + class << self + def update_index(_type_name, *_args, &_block) + super if Chewy.enabled? + end + end + def boolean_with_default(key, default_value) value = attributes[key] diff --git a/app/models/favourite.rb b/app/models/favourite.rb index 17f8c9fa6..bf0ec4449 100644 --- a/app/models/favourite.rb +++ b/app/models/favourite.rb @@ -13,7 +13,7 @@ class Favourite < ApplicationRecord include Paginable - update_index('statuses#status', :status) if Chewy.enabled? + update_index('statuses#status', :status) belongs_to :account, inverse_of: :favourites belongs_to :status, inverse_of: :favourites diff --git a/app/models/status.rb b/app/models/status.rb index 5e7474577..078a64566 100644 --- a/app/models/status.rb +++ b/app/models/status.rb @@ -39,7 +39,7 @@ class Status < ApplicationRecord # will be based on current time instead of `created_at` attr_accessor :override_timestamps - update_index('statuses#status', :proper) if Chewy.enabled? + update_index('statuses#status', :proper) enum visibility: [:public, :unlisted, :private, :direct, :limited], _suffix: :visibility diff --git a/app/models/tag.rb b/app/models/tag.rb index 9aca3983f..82786daa8 100644 --- a/app/models/tag.rb +++ b/app/models/tag.rb @@ -49,7 +49,7 @@ class Tag < ApplicationRecord after_save :save_account_tag_stat - update_index('tags#tag', :self) if Chewy.enabled? + update_index('tags#tag', :self) def account_tag_stat super || build_account_tag_stat -- cgit From 62f60e86c281ae1cd0356a7630dbb76069e2ffde Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Wed, 2 Oct 2019 04:59:37 +0200 Subject: Fix account counters being overwritten by parallel writes (#12045) --- app/models/account_stat.rb | 22 +++++++++ ...1001213028_add_lock_version_to_account_stats.rb | 15 ++++++ db/schema.rb | 3 +- spec/models/account_stat_spec.rb | 53 ++++++++++++++++++++++ 4 files changed, 92 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20191001213028_add_lock_version_to_account_stats.rb (limited to 'app/models') diff --git a/app/models/account_stat.rb b/app/models/account_stat.rb index 1351f7d8a..c84e4217c 100644 --- a/app/models/account_stat.rb +++ b/app/models/account_stat.rb @@ -11,6 +11,7 @@ # created_at :datetime not null # updated_at :datetime not null # last_status_at :datetime +# lock_version :integer default(0), not null # class AccountStat < ApplicationRecord @@ -20,10 +21,26 @@ class AccountStat < ApplicationRecord def increment_count!(key) update(attributes_for_increment(key)) + rescue ActiveRecord::StaleObjectError + begin + reload_with_id + rescue ActiveRecord::RecordNotFound + # Nothing to do + else + retry + end end def decrement_count!(key) update(key => [public_send(key) - 1, 0].max) + rescue ActiveRecord::StaleObjectError + begin + reload_with_id + rescue ActiveRecord::RecordNotFound + # Nothing to do + else + retry + end end private @@ -33,4 +50,9 @@ class AccountStat < ApplicationRecord attrs[:last_status_at] = Time.now.utc if key == :statuses_count attrs end + + def reload_with_id + self.id = find_by!(account: account).id if new_record? + reload + end end diff --git a/db/migrate/20191001213028_add_lock_version_to_account_stats.rb b/db/migrate/20191001213028_add_lock_version_to_account_stats.rb new file mode 100644 index 000000000..47f37cca2 --- /dev/null +++ b/db/migrate/20191001213028_add_lock_version_to_account_stats.rb @@ -0,0 +1,15 @@ +require Rails.root.join('lib', 'mastodon', 'migration_helpers') + +class AddLockVersionToAccountStats < ActiveRecord::Migration[5.2] + include Mastodon::MigrationHelpers + + disable_ddl_transaction! + + def up + safety_assured { add_column_with_default :account_stats, :lock_version, :integer, allow_null: false, default: 0 } + end + + def down + remove_column :account_stats, :lock_version + end +end diff --git a/db/schema.rb b/db/schema.rb index 557b777e0..2d516965c 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2019_09_27_232842) do +ActiveRecord::Schema.define(version: 2019_10_01_213028) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -97,6 +97,7 @@ ActiveRecord::Schema.define(version: 2019_09_27_232842) do t.datetime "created_at", null: false t.datetime "updated_at", null: false t.datetime "last_status_at" + t.integer "lock_version", default: 0, null: false t.index ["account_id"], name: "index_account_stats_on_account_id", unique: true end diff --git a/spec/models/account_stat_spec.rb b/spec/models/account_stat_spec.rb index a94185109..8adc0d1d6 100644 --- a/spec/models/account_stat_spec.rb +++ b/spec/models/account_stat_spec.rb @@ -1,4 +1,57 @@ require 'rails_helper' RSpec.describe AccountStat, type: :model do + describe '#increment_count!' do + it 'increments the count' do + account_stat = AccountStat.create(account: Fabricate(:account)) + expect(account_stat.followers_count).to eq 0 + account_stat.increment_count!(:followers_count) + expect(account_stat.followers_count).to eq 1 + end + + it 'increments the count in multi-threaded an environment' do + account_stat = AccountStat.create(account: Fabricate(:account), statuses_count: 0) + increment_by = 15 + wait_for_start = true + + threads = Array.new(increment_by) do + Thread.new do + true while wait_for_start + AccountStat.find(account_stat.id).increment_count!(:statuses_count) + end + end + + wait_for_start = false + threads.each(&:join) + + expect(account_stat.reload.statuses_count).to eq increment_by + end + end + + describe '#decrement_count!' do + it 'decrements the count' do + account_stat = AccountStat.create(account: Fabricate(:account), followers_count: 15) + expect(account_stat.followers_count).to eq 15 + account_stat.decrement_count!(:followers_count) + expect(account_stat.followers_count).to eq 14 + end + + it 'decrements the count in multi-threaded an environment' do + account_stat = AccountStat.create(account: Fabricate(:account), statuses_count: 15) + decrement_by = 10 + wait_for_start = true + + threads = Array.new(decrement_by) do + Thread.new do + true while wait_for_start + AccountStat.find(account_stat.id).decrement_count!(:statuses_count) + end + end + + wait_for_start = false + threads.each(&:join) + + expect(account_stat.reload.statuses_count).to eq 5 + end + end end -- cgit From 575dc11cb2045c32b8fc0325de7bc321bd4728aa Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Wed, 2 Oct 2019 20:04:46 +0200 Subject: Fix needlessly indexing unsearchable statuses into ElasticSearch (#12041) --- app/chewy/statuses_index.rb | 8 ++++---- app/models/status.rb | 10 ++++++---- 2 files changed, 10 insertions(+), 8 deletions(-) (limited to 'app/models') diff --git a/app/chewy/statuses_index.rb b/app/chewy/statuses_index.rb index b7365d5ca..f5735421c 100644 --- a/app/chewy/statuses_index.rb +++ b/app/chewy/statuses_index.rb @@ -31,19 +31,19 @@ class StatusesIndex < Chewy::Index }, } - define_type ::Status.unscoped.without_reblogs.includes(:media_attachments) do + define_type ::Status.unscoped.kept.without_reblogs.includes(:media_attachments), delete_if: ->(status) { status.searchable_by.empty? } do crutch :mentions do |collection| - data = ::Mention.where(status_id: collection.map(&:id)).pluck(:status_id, :account_id) + data = ::Mention.where(status_id: collection.map(&:id)).where(account: Account.local).pluck(:status_id, :account_id) data.each.with_object({}) { |(id, name), result| (result[id] ||= []).push(name) } end crutch :favourites do |collection| - data = ::Favourite.where(status_id: collection.map(&:id)).pluck(:status_id, :account_id) + data = ::Favourite.where(status_id: collection.map(&:id)).where(account: Account.local).pluck(:status_id, :account_id) data.each.with_object({}) { |(id, name), result| (result[id] ||= []).push(name) } end crutch :reblogs do |collection| - data = ::Status.where(reblog_of_id: collection.map(&:id)).pluck(:reblog_of_id, :account_id) + data = ::Status.where(reblog_of_id: collection.map(&:id)).where(account: Account.local).pluck(:reblog_of_id, :account_id) data.each.with_object({}) { |(id, name), result| (result[id] ||= []).push(name) } end diff --git a/app/models/status.rb b/app/models/status.rb index 078a64566..3504ac370 100644 --- a/app/models/status.rb +++ b/app/models/status.rb @@ -129,12 +129,14 @@ class Status < ApplicationRecord REAL_TIME_WINDOW = 6.hours def searchable_by(preloaded = nil) - ids = [account_id] + ids = [] + + ids << account_id if local? if preloaded.nil? - ids += mentions.pluck(:account_id) - ids += favourites.pluck(:account_id) - ids += reblogs.pluck(:account_id) + ids += mentions.where(account: Account.local).pluck(:account_id) + ids += favourites.where(account: Account.local).pluck(:account_id) + ids += reblogs.where(account: Account.local).pluck(:account_id) else ids += preloaded.mentions[id] || [] ids += preloaded.favourites[id] || [] -- cgit From ca22a22d7f4db867ad0045a7978e3d8dcd251a69 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Thu, 3 Oct 2019 01:09:12 +0200 Subject: Fix performance of GIF re-encoding (#12057) * Change animated GIF detection to not shell out to ImageMagick Signed-off-by: Eugen Rochko * Change video encoding parameters to limit to 10800 video frames Signed-off-by: Eugen Rochko * Limit GIF image size further Signed-off-by: Eugen Rochko * Always strip metadata from video files * Fix code style issues --- app/models/concerns/attachmentable.rb | 4 +- app/models/media_attachment.rb | 33 ++++++++--- lib/paperclip/gif_transcoder.rb | 101 +++++++++++++++++++++++++++++++++- lib/paperclip/video_transcoder.rb | 2 + 4 files changed, 128 insertions(+), 12 deletions(-) (limited to 'app/models') diff --git a/app/models/concerns/attachmentable.rb b/app/models/concerns/attachmentable.rb index 246c2c27c..3bbc6453c 100644 --- a/app/models/concerns/attachmentable.rb +++ b/app/models/concerns/attachmentable.rb @@ -6,6 +6,7 @@ module Attachmentable extend ActiveSupport::Concern MAX_MATRIX_LIMIT = 16_777_216 # 4096x4096px or approx. 16MB + GIF_MATRIX_LIMIT = 921_600 # 1280x720px included do before_post_process :set_file_extensions @@ -42,8 +43,9 @@ module Attachmentable next if attachment.blank? || !/image.*/.match?(attachment.content_type) || attachment.queued_for_write[:original].blank? width, height = FastImage.size(attachment.queued_for_write[:original].path) + matrix_limit = attachment.content_type == 'image/gif' ? GIF_MATRIX_LIMIT : MAX_MATRIX_LIMIT - raise Mastodon::DimensionsValidationError, "#{width}x#{height} images are not supported, must be below #{MAX_MATRIX_LIMIT} sqpx" if width.present? && height.present? && (width * height >= MAX_MATRIX_LIMIT) + raise Mastodon::DimensionsValidationError, "#{width}x#{height} images are not supported" if width.present? && height.present? && (width * height > matrix_limit) end end diff --git a/app/models/media_attachment.rb b/app/models/media_attachment.rb index c4932f2ef..630dab55a 100644 --- a/app/models/media_attachment.rb +++ b/app/models/media_attachment.rb @@ -65,6 +65,17 @@ class MediaAttachment < ApplicationRecord file_geometry_parser: FastGeometryParser, blurhash: BLURHASH_OPTIONS, }, + + original: { + keep_same_format: true, + convert_options: { + output: { + 'map_metadata' => '-1', + 'c:v' => 'copy', + 'c:a' => 'copy', + }, + }, + }, }.freeze AUDIO_STYLES = { @@ -86,14 +97,15 @@ class MediaAttachment < ApplicationRecord output: { 'loglevel' => 'fatal', 'movflags' => 'faststart', - 'pix_fmt' => 'yuv420p', - 'vf' => 'scale=\'trunc(iw/2)*2:trunc(ih/2)*2\'', - 'vsync' => 'cfr', - 'c:v' => 'h264', - 'b:v' => '500K', - 'maxrate' => '1300K', - 'bufsize' => '1300K', - 'crf' => 18, + 'pix_fmt' => 'yuv420p', + 'vf' => 'scale=\'trunc(iw/2)*2:trunc(ih/2)*2\'', + 'vsync' => 'cfr', + 'c:v' => 'h264', + 'maxrate' => '1300K', + 'bufsize' => '1300K', + 'frames:v' => 60 * 60 * 3, + 'crf' => 18, + 'map_metadata' => '-1', }, }, }.freeze @@ -103,7 +115,7 @@ class MediaAttachment < ApplicationRecord original: VIDEO_FORMAT, }.freeze - IMAGE_LIMIT = 8.megabytes + IMAGE_LIMIT = 10.megabytes VIDEO_LIMIT = 40.megabytes belongs_to :account, inverse_of: :media_attachments, optional: true @@ -244,7 +256,9 @@ class MediaAttachment < ApplicationRecord def set_meta meta = populate_meta + return if meta == {} + file.instance_write :meta, meta end @@ -287,6 +301,7 @@ class MediaAttachment < ApplicationRecord def reset_parent_cache return if status_id.nil? + Rails.cache.delete("statuses/#{status_id}") end end diff --git a/lib/paperclip/gif_transcoder.rb b/lib/paperclip/gif_transcoder.rb index cbab6fd99..64f12f963 100644 --- a/lib/paperclip/gif_transcoder.rb +++ b/lib/paperclip/gif_transcoder.rb @@ -1,5 +1,103 @@ # frozen_string_literal: true +class GifReader + attr_reader :animated + + EXTENSION_LABELS = [0xf9, 0x01, 0xff].freeze + GIF_HEADERS = %w(GIF87a GIF89a).freeze + + class GifReaderException; 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).unpack('C')[0] >= 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).unpack('C')[0]) + 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 webm @@ -19,8 +117,7 @@ module Paperclip private def needs_convert? - num_frames = identify('-format %n :file', file: file.path).to_i - options[:style] == :original && num_frames > 1 + options[:style] == :original && GifReader.animated?(file.path) end end end diff --git a/lib/paperclip/video_transcoder.rb b/lib/paperclip/video_transcoder.rb index c3504c17c..66f7feda5 100644 --- a/lib/paperclip/video_transcoder.rb +++ b/lib/paperclip/video_transcoder.rb @@ -6,7 +6,9 @@ module Paperclip class VideoTranscoder < Paperclip::Processor def make meta = ::Av.cli.identify(@file.path) + attachment.instance.type = MediaAttachment.types[:gifv] unless meta[:audio_encode] + options[:format] = File.extname(attachment.instance.file_file_name)[1..-1] if options[:keep_same_format] Paperclip::Transcoder.make(file, options, attachment) end -- cgit