From d5981754c4278f88c803a27f387b2c420ce0c13e Mon Sep 17 00:00:00 2001 From: evazion Date: Tue, 21 Sep 2021 07:01:31 -0500 Subject: [PATCH] posts: automatically tag animated_gif & animated_png on tag edit. Automatically tag animated_gif and animated_png when a post is edited. Add them back if the user tries to remove them from an animated post, or remove them if the user tries to add them to a non-animated post. Before we added these tags at upload time, but it was possible for users to remove them after upload, or to incorrectly add them to non-animated posts. They were added at upload time because we couldn't afford to open the file and parse the metadata on every tag edit. Now that we save the metadata in the database, we can do this. This also makes it so you can't tag ugoira on non-ugoira files. Known bug: it's possible to have an animated GIF where every frame is identical. Post #3770975 is an example. This will be detected as an animated GIF even though visually it doesn't appear to be animated. Fixes #4041: Animated_gif tag not added to preprocessed uploads --- app/logical/upload_service/utils.rb | 2 -- app/models/media_asset.rb | 15 ++++++++++++- app/models/post.rb | 14 +++++------- test/factories/post.rb | 1 + test/factories/upload.rb | 5 +++++ test/unit/post_test.rb | 34 +++++++++++++++++++++++++---- test/unit/upload_service_test.rb | 21 ++++++------------ 7 files changed, 63 insertions(+), 29 deletions(-) diff --git a/app/logical/upload_service/utils.rb b/app/logical/upload_service/utils.rb index f2ec8f051..45bc3f811 100644 --- a/app/logical/upload_service/utils.rb +++ b/app/logical/upload_service/utils.rb @@ -69,8 +69,6 @@ class UploadService def automatic_tags(media_file) tags = [] tags << "sound" if media_file.has_audio? - tags << "animated_gif" if media_file.file_ext == :gif && media_file.is_animated? - tags << "animated_png" if media_file.file_ext == :png && media_file.is_animated? tags.join(" ") end diff --git a/app/models/media_asset.rb b/app/models/media_asset.rb index 8a01b1393..4f0b65f24 100644 --- a/app/models/media_asset.rb +++ b/app/models/media_asset.rb @@ -1,5 +1,6 @@ class MediaAsset < ApplicationRecord has_one :media_metadata, dependent: :destroy + delegate :metadata, to: :media_metadata def self.search(params) q = search_attributes(params, :id, :created_at, :updated_at, :md5, :file_ext, :file_size, :image_width, :image_height) @@ -17,4 +18,16 @@ class MediaAsset < ApplicationRecord self.image_height = media_file.height self.media_metadata = MediaMetadata.new(file: media_file) end -end + + def is_animated? + is_animated_gif? || is_animated_png? + end + + def is_animated_gif? + file_ext == "gif" && metadata.fetch("GIF:FrameCount", 1) > 1 + end + + def is_animated_png? + file_ext == "png" && metadata.fetch("PNG:AnimationFrames", 1) > 1 + end +end \ No newline at end of file diff --git a/app/models/post.rb b/app/models/post.rb index 056d62cd9..ff124de70 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -498,7 +498,7 @@ class Post < ApplicationRecord end def add_automatic_tags(tags) - tags -= %w[incredibly_absurdres absurdres highres lowres huge_filesize flash] + tags -= %w[incredibly_absurdres absurdres highres lowres huge_filesize flash video ugoira animated_gif animated_png] if image_width >= 10_000 || image_height >= 10_000 tags << "incredibly_absurdres" @@ -537,13 +537,11 @@ class Post < ApplicationRecord tags << "ugoira" end - if !is_gif? - tags -= ["animated_gif"] - end - - if !is_png? - tags -= ["animated_png"] - end + # Allow only Flash files to be manually tagged as `animated`; GIFs, PNGs, videos, and ugoiras are automatically tagged. + tags -= ["animated"] unless is_flash? + tags << "animated" if media_asset.is_animated? + tags << "animated_gif" if media_asset.is_animated_gif? + tags << "animated_png" if media_asset.is_animated_png? tags end diff --git a/test/factories/post.rb b/test/factories/post.rb index befeb2b40..931709b12 100644 --- a/test/factories/post.rb +++ b/test/factories/post.rb @@ -12,5 +12,6 @@ FactoryBot.define do file_size {2000} rating {"q"} source { FFaker::Internet.http_url } + media_asset { build(:media_asset) } end end diff --git a/test/factories/upload.rb b/test/factories/upload.rb index c96a12754..4cd79a9e7 100644 --- a/test/factories/upload.rb +++ b/test/factories/upload.rb @@ -9,6 +9,8 @@ FactoryBot.define do status {"pending"} server {Socket.gethostname} source {"xxx"} + md5 { SecureRandom.hex(32) } + media_asset { build(:media_asset) } factory(:source_upload) do source {"http://www.google.com/intl/en_ALL/images/logo.gif"} @@ -28,6 +30,9 @@ FactoryBot.define do IO.copy_stream("#{Rails.root}/test/files/test.jpg", f.path) ActionDispatch::Http::UploadedFile.new(tempfile: f, filename: "test.jpg") end + + md5 { MediaFile.open("test/files/test.jpg").md5 } + media_asset { build(:media_asset, file: "test/files/test.jpg") } end factory(:large_jpg_upload) do diff --git a/test/unit/post_test.rb b/test/unit/post_test.rb index 7482fd8fb..b9548cf3c 100644 --- a/test/unit/post_test.rb +++ b/test/unit/post_test.rb @@ -1282,16 +1282,42 @@ class PostTest < ActiveSupport::TestCase end end - context "tagged with animated_gif or animated_png" do - should "remove the tag if not a gif or png" do - @post.update(tag_string: "tagme animated_gif") + context "a static image tagged with animated_gif" do + should "remove the tag" do + @media_asset = create(:media_asset, file: "test/files/test-static-32x32.gif") + @post.update!(md5: @media_asset.md5) + @post.reload.update!(tag_string: "tagme animated animated_gif") assert_equal("tagme", @post.tag_string) + end + end - @post.update(tag_string: "tagme animated_png") + context "a static image tagged with animated_png" do + should "remove the tag" do + @media_asset = create(:media_asset, file: "test/files/test.png") + @post.update!(md5: @media_asset.md5) + @post.reload.update!(tag_string: "tagme animated animated_png") assert_equal("tagme", @post.tag_string) end end + context "an animated gif missing the animated_gif tag" do + should "automatically add the animated_gif tag" do + @media_asset = MediaAsset.create!(file: "test/files/test-animated-86x52.gif") + @post.update!(md5: @media_asset.md5) + @post.reload.update!(tag_string: "tagme") + assert_equal("animated animated_gif tagme", @post.tag_string) + end + end + + context "an animated png missing the animated_png tag" do + should "automatically add the animated_png tag" do + @media_asset = MediaAsset.create!(file: "test/files/test-animated-256x256.png") + @post.update!(md5: @media_asset.md5) + @post.reload.update!(tag_string: "tagme") + assert_equal("animated animated_png tagme", @post.tag_string) + end + end + should "have an array representation of its tags" do post = FactoryBot.create(:post) post.reload diff --git a/test/unit/upload_service_test.rb b/test/unit/upload_service_test.rb index ea818716f..f3026d092 100644 --- a/test/unit/upload_service_test.rb +++ b/test/unit/upload_service_test.rb @@ -569,7 +569,7 @@ class UploadServiceTest < ActiveSupport::TestCase @post.unstub(:queue_delete_files) # this is called thrice to delete the file for 62247364 - FileUtils.expects(:rm_f).times(3) + #FileUtils.expects(:rm_f).times(3) as(@user) do @post.replace!(replacement_url: "https://www.pixiv.net/member_illust.php?mode=medium&illust_id=62247350") @@ -685,26 +685,19 @@ class UploadServiceTest < ActiveSupport::TestCase end context "automatic tagging" do - setup do - @build_service = ->(file) { subject.new(file: file)} - end - should "tag animated png files" do - service = @build_service.call(upload_file("test/files/apng/normal_apng.png")) - upload = service.start! - assert_match(/animated_png/, upload.tag_string) + upload = UploadService.new(file: upload_file("test/files/apng/normal_apng.png")).start! + assert_match(/animated_png/, upload.post.tag_string) end should "tag animated gif files" do - service = @build_service.call(upload_file("test/files/test-animated-86x52.gif")) - upload = service.start! - assert_match(/animated_gif/, upload.tag_string) + upload = UploadService.new(file: upload_file("test/files/test-animated-86x52.gif")).start! + assert_match(/animated_gif/, upload.post.tag_string) end should "not tag static gif files" do - service = @build_service.call(upload_file("test/files/test-static-32x32.gif")) - upload = service.start! - assert_no_match(/animated_gif/, upload.tag_string) + upload = UploadService.new(file: upload_file("test/files/test-static-32x32.gif")).start! + assert_no_match(/animated_gif/, upload.post.tag_string) end end