From 9912e63d69baf261611dbe641b57b45dcd1fe590 Mon Sep 17 00:00:00 2001 From: Marcello Costagliola Date: Tue, 6 Oct 2026 15:12:18 +0200 Subject: [PATCH] Bound the work of previewing an attachment A video's preview and a picture's thumbnail are made inside the request that posts the message, and nothing bounded how long either could take. - The video preview filter also selects any frame from 5 seconds on. Rails' filter takes the second frame it selects, which a video with a single keyframe and no scene change only gives at its end, so ffmpeg decoded all of it. - TimeLimitedVideoPreviewer gives ffmpeg 10 seconds of wall-clock time, kills it past that, and reports a failed preview, so the message is posted without one. - Pictures and videos above 250 megapixels, or whose size couldn't be read, get no preview: decoding costs in proportion to the pixels, however small the file. - The view shows a preview only if it was made when the message was posted. Its URL used to make it on view, so a preview that failed or was skipped would be attempted again on every view. The cached presentation's version goes up, so cached messages pick this up. - A video's poster is made, when the message is posted, at the size the view shows it. The full-size WebP made until now wasn't shown anywhere, and encoding it costs in proportion to the frame's pixels. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Bj8KnxpTf9sj2Ysa8aLAVa --- .../messages/attachment_presentation.rb | 16 ++++- app/models/message/attachment.rb | 17 +++++- app/views/messages/_message.html.erb | 2 +- config/initializers/active_storage.rb | 15 +++++ lib/rails_ext/time_limited_video_previewer.rb | 27 ++++++++ test/controllers/messages_caching_test.rb | 16 +++++ test/helpers/messages_helper_test.rb | 37 +++++++++++ test/models/message/attachment_test.rb | 61 +++++++++++++++++++ 8 files changed, 186 insertions(+), 5 deletions(-) create mode 100644 lib/rails_ext/time_limited_video_previewer.rb diff --git a/app/helpers/messages/attachment_presentation.rb b/app/helpers/messages/attachment_presentation.rb index ac21c85..82ce92d 100644 --- a/app/helpers/messages/attachment_presentation.rb +++ b/app/helpers/messages/attachment_presentation.rb @@ -17,11 +17,16 @@ class Messages::AttachmentPresentation attr_reader :message, :context delegate :tag, :link_to, :broadcast_image_tag, :rails_blob_path, :url_for, to: :context + # Previews are made when the message is posted, within limits. One that wasn't, because the file was too large or + # the preview failed, isn't attempted again from here: its URL would make it on every view. def render_preview - if message.attachment.video? + case + when message.attachment.video? video_preview_tag - else + when message.attachment.representation(:thumb).processed? lightboxed_image_preview_tag + else + render_link end end @@ -30,11 +35,16 @@ class Messages::AttachmentPresentation inline_media_dimension_constraints(width, height) do tag.video \ - src: rails_blob_path(message.attachment), poster: url_for(message.attachment.preview(format: :webp, resize_to_limit: [ Message::THUMBNAIL_MAX_WIDTH, Message::THUMBNAIL_MAX_HEIGHT ])), + src: rails_blob_path(message.attachment), poster: video_poster_url, controls: true, preload: :none, width: "100%", height: "100%", class: "message__attachment" end end + def video_poster_url + poster = message.attachment.preview(:poster) + url_for(poster) if poster.processed? + end + def lightboxed_image_preview_tag width, height = preview_dimensions diff --git a/app/models/message/attachment.rb b/app/models/message/attachment.rb index 4b895ea..7946710 100644 --- a/app/models/message/attachment.rb +++ b/app/models/message/attachment.rb @@ -4,9 +4,14 @@ module Message::Attachment THUMBNAIL_MAX_WIDTH = 1200 THUMBNAIL_MAX_HEIGHT = 800 + # Decoding a picture, or a video's frame, costs in proportion to its pixels, however small the file. The largest + # phone photos have 200 million. + THUMBNAIL_MAX_PIXELS = 250_000_000 + included do has_one_attached :attachment do |attachable| attachable.variant :thumb, resize_to_limit: [ THUMBNAIL_MAX_WIDTH, THUMBNAIL_MAX_HEIGHT ] + attachable.variant :poster, format: :webp, resize_to_limit: [ THUMBNAIL_MAX_WIDTH, THUMBNAIL_MAX_HEIGHT ] end end @@ -32,13 +37,23 @@ module Message::Attachment # A file that ffmpeg or libvips can't decode is still the message: post it without a preview. def process_attachment_thumbnail + return if too_many_pixels_to_preview? + case when attachment.video? - attachment.preview(format: :webp).processed + attachment.preview(:poster).processed when attachment.representable? attachment.representation(:thumb).processed end rescue ActiveStorage::PreviewError, Vips::Error => error Rails.logger.warn "Posted #{attachment.filename} without a preview: #{error.class}: #{error.message.lines.first&.chomp}" end + + # Without a size, the analyzer couldn't read the file's header, and the previewer wouldn't either. + def too_many_pixels_to_preview? + if attachment.image? || attachment.video? + width, height = attachment.metadata.values_at(:width, :height) + width.nil? || height.nil? || width * height > THUMBNAIL_MAX_PIXELS + end + end end diff --git a/app/views/messages/_message.html.erb b/app/views/messages/_message.html.erb index d0c491f..77732cd 100644 --- a/app/views/messages/_message.html.erb +++ b/app/views/messages/_message.html.erb @@ -1,7 +1,7 @@ <%# Be sure to check/update messages/_template.html.erb when changing this file %> <%# Bump this version when the message presentation filters change what they emit. Editing this line changes the template digest, which busts BOTH this fragment cache and the collection cache that keys on this partial's digest (helper Ruby changes alone don't). %> -<% cache [ message, "presentation-v3" ] do %> +<% cache [ message, "presentation-v4" ] do %> <%= message_tag message do %>

<%= local_datetime_tag message.created_at, style: :date %>

diff --git a/config/initializers/active_storage.rb b/config/initializers/active_storage.rb index f2d77e1..ee10622 100644 --- a/config/initializers/active_storage.rb +++ b/config/initializers/active_storage.rb @@ -1,5 +1,20 @@ +require "rails_ext/time_limited_video_previewer" + ActiveSupport.on_load(:active_storage_blob) do ActiveStorage::DiskController.after_action only: :show do response.set_header("Cache-Control", "max-age=3600, public") end end + +Rails.application.configure do + # Rails' filter takes the second of the frames it selects (the first one, keyframes, scene changes), so a video + # with a single keyframe and no scene change is decoded to its end. Selecting any frame from 5 seconds on stops + # it there. + config.active_storage.video_preview_arguments = + "-vf 'select=eq(n\\,0)+eq(key\\,1)+gt(scene\\,0.015)+gte(t\\,5),loop=loop=-1:size=2,trim=start_frame=1'" \ + " -frames:v 1 -f image2" + + config.active_storage.previewers = config.active_storage.previewers.map do |previewer| + previewer == ActiveStorage::Previewer::VideoPreviewer ? TimeLimitedVideoPreviewer : previewer + end +end diff --git a/lib/rails_ext/time_limited_video_previewer.rb b/lib/rails_ext/time_limited_video_previewer.rb new file mode 100644 index 0000000..3e4bb08 --- /dev/null +++ b/lib/rails_ext/time_limited_video_previewer.rb @@ -0,0 +1,27 @@ +# Rails waits for ffmpeg however long it takes, and a video's preview is drawn inside the request that posts it. +# This previewer gives ffmpeg a wall-clock limit, kills it past that, and reports a failed preview, so the message +# is posted without one. +class TimeLimitedVideoPreviewer < ActiveStorage::Previewer::VideoPreviewer + TIME_LIMIT = 10 # seconds + + private + def capture(*argv, to:) + to.binmode + + open_tempfile do |err| + IO.popen(argv, in: IO::NULL, err: err) do |out| + Timeout.timeout(TIME_LIMIT) { IO.copy_stream(out, to) } + rescue Timeout::Error + Process.kill :KILL, out.pid + raise ActiveStorage::PreviewError, "#{argv.first} took longer than #{TIME_LIMIT} seconds" + end + err.rewind + + unless $?.success? + raise ActiveStorage::PreviewError, "#{argv.first} failed (status #{$?.exitstatus}): #{err.read.to_s.chomp}" + end + end + + to.rewind + end +end diff --git a/test/controllers/messages_caching_test.rb b/test/controllers/messages_caching_test.rb index 131a078..5c9d8c4 100644 --- a/test/controllers/messages_caching_test.rb +++ b/test/controllers/messages_caching_test.rb @@ -27,6 +27,22 @@ class MessagesCachingTest < ActionDispatch::IntegrationTest end end + test "a page of messages loads whether their previews were made along with the messages, not one at a time" do + room = rooms(:watercooler) + 2.times do |copy| + { "moon.jpg" => "image/jpeg", "alpha-centuri.mov" => "video/quicktime" }.each do |file, content_type| + room.messages.create_with_attachment! creator: users(:david), client_message_id: "#{copy}-#{file}", attachment: fixture_file_upload(file, content_type) + end + end + + assert_no_queries_match(/"active_storage_\w+"\."(id|blob_id|record_id)" = \?/) do + get room_messages_url(room) + end + assert_response :success + assert_select "img[src*='moon.jpg']", count: 2 + assert_select "video[poster]", count: 2 + end + private def with_memory_cache old_cache = Rails.cache diff --git a/test/helpers/messages_helper_test.rb b/test/helpers/messages_helper_test.rb index d2655a8..f648827 100644 --- a/test/helpers/messages_helper_test.rb +++ b/test/helpers/messages_helper_test.rb @@ -24,4 +24,41 @@ class MessagesHelperTest < ActionView::TestCase assert_match /]*>example<\/a>/, presentation assert_match /bold<\/strong>/, presentation end + + test "message_presentation shows an image's thumbnail made when it was posted" do + presentation = view.message_presentation(attachment_message("moon.jpg", "image/jpeg", processed: true)) + + assert_match %r{]+src="[^"]*/representations/[^"]*moon\.jpg"}, presentation + end + + test "message_presentation links an image whose thumbnail wasn't made, rather than making it on view" do + presentation = view.message_presentation(attachment_message("moon.jpg", "image/jpeg", processed: false)) + + assert_no_match %r{/representations/}, presentation + assert_match %r{moon\.jpg}, presentation + end + + test "message_presentation gives a video the poster made when it was posted" do + presentation = view.message_presentation(attachment_message("alpha-centuri.mov", "video/quicktime", processed: true)) + + assert_match %r{]+poster="[^"]*/representations/[^"]*alpha-centuri}, presentation + end + + test "message_presentation shows a video whose poster wasn't made without one, rather than making it on view" do + presentation = view.message_presentation(attachment_message("alpha-centuri.mov", "video/quicktime", processed: false)) + + assert_match %r{]+src="[^"]*alpha-centuri\.mov"}, presentation + assert_no_match %r{poster=|/representations/}, presentation + end + + private + def attachment_message(file, content_type, processed:) + attributes = { creator: users(:jason), client_message_id: "0015", attachment: fixture_file_upload(file, content_type) } + + if processed + rooms(:pets).messages.create_with_attachment!(attributes) + else + rooms(:pets).messages.create!(attributes) + end + end end diff --git a/test/models/message/attachment_test.rb b/test/models/message/attachment_test.rb index 6925eb0..33153a3 100644 --- a/test/models/message/attachment_test.rb +++ b/test/models/message/attachment_test.rb @@ -34,7 +34,68 @@ class Message::AttachmentTest < ActiveSupport::TestCase assert_not message.attachment.preview(format: :webp).image.attached? end + test "creating a message makes the video poster that the room shows" do + message = create_attachment_message("alpha-centuri.mov", "video/quicktime") + + assert_no_difference -> { ActiveStorage::VariantRecord.count } do + message.reload.attachment.preview(:poster).processed + end + end + + test "video previews are drawn by the previewer with a time limit, from a frame within the first 5 seconds" do + assert_includes ActiveStorage.previewers, TimeLimitedVideoPreviewer + assert_not_includes ActiveStorage.previewers, ActiveStorage::Previewer::VideoPreviewer + assert_includes ActiveStorage.video_preview_arguments, "gte(t\\,5)" + end + + test "creating a message gives up on a video preview that takes longer than the time limit" do + started = nil + message = nil + + with_ffmpeg_sleeping(5.seconds) do + stub_const(TimeLimitedVideoPreviewer, :TIME_LIMIT, 0.2) do + started = Process.clock_gettime(Process::CLOCK_MONOTONIC) + message = create_attachment_message("alpha-centuri.mov", "video/quicktime") + end + end + + assert_operator Process.clock_gettime(Process::CLOCK_MONOTONIC) - started, :<, 2 + assert_not message.reload.attachment.preview(format: :webp).image.attached? + end + + test "creating a message makes no thumbnail of an image with more pixels than the limit" do + moon = Vips::Image.new_from_file(file_fixture("moon.jpg").to_s) + + stub_const(Message::Attachment, :THUMBNAIL_MAX_PIXELS, moon.width * moon.height) do + assert create_attachment_message("moon.jpg", "image/jpeg").attachment.representation(:thumb).processed? + end + + stub_const(Message::Attachment, :THUMBNAIL_MAX_PIXELS, moon.width * moon.height - 1) do + assert_not create_attachment_message("moon.jpg", "image/jpeg").attachment.representation(:thumb).processed? + end + end + + test "creating a message makes no preview of a video with more pixels than the limit" do + stub_const(Message::Attachment, :THUMBNAIL_MAX_PIXELS, 1) do + message = create_attachment_message("alpha-centuri.mov", "video/quicktime") + assert_not message.reload.attachment.preview(format: :webp).image.attached? + end + end + private + def with_ffmpeg_sleeping(duration) + Tempfile.create("ffmpeg") do |ffmpeg| + ffmpeg.write "#!/bin/sh\n[ \"$1\" = \"-version\" ] && exit 0\nexec sleep #{duration.to_i}\n" + ffmpeg.close + File.chmod 0o755, ffmpeg.path + + previous, ActiveStorage.paths[:ffmpeg] = ActiveStorage.paths[:ffmpeg], ffmpeg.path + yield + ensure + ActiveStorage.paths[:ffmpeg] = previous + end + end + def create_attachment_message(file, content_type) rooms(:hq).messages.create_with_attachment! \ creator: users(:david),