Merge pull request #330: bound attachment preview work

Reviewed and merged by GPT on behalf of DHH. Keep both boost-cache and preview-loading regressions.

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,17 @@ 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
 
+    # A preview is processed once the frame is drawn, but the poster is a variant of that frame, which can fail on its own.
+    def video_poster_url
+      poster = message.attachment.preview(:poster)
+      url_for(poster) if poster.processed? && poster.image.variant(poster.variation).processed?
+    end
+
     def lightboxed_image_preview_tag
       width, height = preview_dimensions
 

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

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 %>
     <h2 class="message__day-separator"><%= local_datetime_tag message.created_at, style: :date %></h2>
 

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

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

test/controllers/messages_caching_test.rb

@@ -67,6 +67,22 @@ class MessagesCachingTest < ActionDispatch::IntegrationTest
     assert_equal in_order, css_select("##{dom_id(messages(:fourth), :boosts)} .boost").map { it["id"] }
   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

test/helpers/messages_helper_test.rb

@@ -24,4 +24,52 @@ class MessagesHelperTest < ActionView::TestCase
     assert_match /<a href="https:\/\/example\.com"[^>]*>example<\/a>/, presentation
     assert_match /<strong>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{<img[^>]+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{<span>moon\.jpg</span>}, 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{<video[^>]+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{<video[^>]+src="[^"]*alpha-centuri\.mov"}, presentation
+    assert_no_match %r{poster=|/representations/}, presentation
+  end
+
+  test "message_presentation shows a video whose frame was drawn but whose poster wasn't made without one" do
+    message = attachment_message("alpha-centuri.mov", "video/quicktime", processed: false)
+    message.attachment.preview(format: :jpg).processed
+    assert message.attachment.preview(:poster).processed?
+
+    presentation = view.message_presentation(message.reload)
+
+    assert_match %r{<video[^>]+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

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),