Fix #5230: video upload 500 error (StatementInvalid) & empty error panel on page

Fix StatementInvalid exception when uploading https://files.catbox.moe/vxoe2p.mp4.

This was a result of multiple bugs:

* First, generating thumbnails for the video failed. This was because
  the video uses the AV1 codec, which FFmpeg failed to decode. It failed
  because our version of FFmpeg was built without the `--enable-libdav1d`
  flag, so it uses the builtin AV1 decoder, which apparently can't
  handle this particular video (it spews a bunch of errors about "Failed
  to get pixel format" and "missing sequence header" and "failed to get
  reference frame").

* Because generating the thumbnails failed, an exception was raised. We
  tried to save the error message in the upload_media_assets.error
  field. However, this also failed because the error message was 77kb
  long (it contained the entire output of the ffmpeg command), but the
  `upload_media_assets` table had a btree index on the `error` column,
  which meant the maximum length of the error column was limited to
  ~2.7kb. This lead to a StatementInvalid exception being raised.

* Because the StatementInvalid exception was raised while we were trying
  to set the upload media asset's status to `failed`, the upload was
  left stuck in the `processing` state rather than being set to the
  `failed` state.

* Because the upload was stuck in the `processing` state, the upload
  page would hang forever waiting for the upload to complete.

The fixes are to:

* Build FFmpeg with `--enable-libdav1d` to use libdav1d for decoding AV1
  videos instead of the builtin AV1 decoder.

* Remove the index on the `upload_media_assets.error` column so that
  setting overly long error messages won't fail.

* Catch unexpected exceptions in ProcessUploadMediaAssetJob so we can
  mark uploads as failed, even if `process_upload!` itself fails because
  it raises an unexpected exception inside its own exception handler.

* Check that the video is playable with `MediaFile::Video#is_corrupt?` before
  allowing it to be uploaded. This way we can return a better error
  message if we can't generate thumbnails because the video isn't
  playable. This requires decoding the entire video, so it means uploads
  may take several seconds longer for long videos. It's also a security
  risk in case ffmpeg has any bugs.

* Define `MediaAsset#preview!` as raising an exception on error, so
  it's clear that generating thumbnails can fail. Define `MediaAsset#preview`
  as returning nil on error for when we don't care about the cause of
  the error.

app/jobs/process_upload_media_asset_job.rb

@@ -5,5 +5,9 @@ class ProcessUploadMediaAssetJob < ApplicationJob
 
   def perform(upload_media_asset)
     upload_media_asset.process_upload!
+  rescue Exception => e
+    # This should never happen. It will only happen if `process_upload!` raises an unexpected exception inside its own exception handler.
+    upload_media_asset.update!(status: :failed, error: e.message)
+    raise
   end
 end

app/logical/iqdb_client.rb

@@ -98,7 +98,7 @@ class IqdbClient
     # @param file [File] the image to search
     def query_file(file, limit: 20)
       media_file = MediaFile.open(file)
-      preview = media_file.preview(Danbooru.config.small_image_width, Danbooru.config.small_image_width)
+      preview = media_file.preview!(Danbooru.config.small_image_width, Danbooru.config.small_image_width)
       file = HTTP::FormData::File.new(preview)
       request(:post, "query", form: { file: file }, params: { limit: limit })
     end

app/logical/media_file.rb

@@ -200,17 +200,23 @@ class MediaFile
     false
   end
 
-  # Return a preview of the file, sized to fit within the given width and
+  # Return a preview of the file, sized to fit within the given width and height (preserving the aspect ratio).
-  # height (preserving the aspect ratio).
   #
   # @param width [Integer] the max width of the image
   # @param height [Integer] the max height of the image
   # @param options [Hash] extra options when generating the preview
   # @return [MediaFile, nil] a preview file, or nil if we can't generate a preview for this file type (e.g. Flash files)
   def preview(width, height, **options)
+    preview!(width, height, **options)
+  rescue
     nil
   end
 
+  # Like `preview`, but raises an exception if generating the preview fails for any reason.
+  def preview!(width, height, **options)
+    raise NotImplementedError
+  end
+
   # Return a set of AI-inferred tags for this image. Performs an API call to
   # the Autotagger service. The Autotagger service must be running, otherwise
   # it will return an empty list of tags.

app/logical/media_file/image.rb

@@ -5,6 +5,8 @@
 # @see https://github.com/libvips/ruby-vips
 # @see https://libvips.github.io/libvips/API/current
 class MediaFile::Image < MediaFile
+  delegate :thumbnail_image, to: :image
+
   def dimensions
     image.size
   rescue Vips::Error
@@ -61,21 +63,21 @@ class MediaFile::Image < MediaFile
     image.interpretation
   end
 
-  def resize(max_width, max_height, format: :jpeg, quality: 85, **options)
+  def resize!(max_width, max_height, format: :jpeg, quality: 85, **options)
     # @see https://www.libvips.org/API/current/Using-vipsthumbnail.md.html
     # @see https://www.libvips.org/API/current/libvips-resample.html#vips-thumbnail
     if colorspace.in?(%i[srgb rgb16])
-      resized_image = preview_frame.image.thumbnail_image(max_width, height: max_height, import_profile: "srgb", export_profile: "srgb", **options)
+      resized_image = thumbnail_image(max_width, height: max_height, import_profile: "srgb", export_profile: "srgb", **options)
     elsif colorspace == :cmyk
       # Leave CMYK as CMYK for better color accuracy than sRGB.
-      resized_image = preview_frame.image.thumbnail_image(max_width, height: max_height, import_profile: "cmyk", export_profile: "cmyk", intent: :relative, **options)
+      resized_image = thumbnail_image(max_width, height: max_height, import_profile: "cmyk", export_profile: "cmyk", intent: :relative, **options)
     elsif colorspace.in?(%i[b-w grey16]) && has_embedded_profile?
       # Convert greyscale to sRGB so that the color profile is properly applied before we strip it.
-      resized_image = preview_frame.image.thumbnail_image(max_width, height: max_height, export_profile: "srgb", **options)
+      resized_image = thumbnail_image(max_width, height: max_height, export_profile: "srgb", **options)
     elsif colorspace.in?(%i[b-w grey16])
       # Otherwise, leave greyscale without a profile as greyscale because
       # converting it to sRGB would change it from 1 channel to 3 channels.
-      resized_image = preview_frame.image.thumbnail_image(max_width, height: max_height, **options)
+      resized_image = thumbnail_image(max_width, height: max_height, **options)
     else
       raise NotImplementedError
     end
@@ -102,9 +104,9 @@ class MediaFile::Image < MediaFile
     MediaFile::Image.new(output_file)
   end
 
-  def preview(max_width, max_height, **options)
+  def preview!(max_width, max_height, **options)
     w, h = MediaFile.scale_dimensions(width, height, max_width, max_height)
-    resize(w, h, size: :force, **options)
+    preview_frame.resize!(w, h, size: :force, **options)
   end
 
   def preview_frame

app/logical/media_file/ugoira.rb

@@ -30,8 +30,8 @@ class MediaFile::Ugoira < MediaFile
     preview_frame.dimensions
   end
 
-  def preview(width, height, **options)
+  def preview!(width, height, **options)
-    preview_frame.preview(width, height, **options)
+    preview_frame.preview!(width, height, **options)
   end
 
   def duration

app/logical/media_file/video.rb

@@ -11,8 +11,8 @@ class MediaFile::Video < MediaFile
     [video.width, video.height]
   end
 
-  def preview(max_width, max_height, **options)
+  def preview!(max_width, max_height, **options)
-    preview_frame.preview(max_width, max_height, **options)
+    preview_frame.preview!(max_width, max_height, **options)
   end
 
   def is_supported?
@@ -26,6 +26,11 @@ class MediaFile::Video < MediaFile
     end
   end
 
+  # True if decoding the video fails.
+  def is_corrupt?
+    video.playback_info.blank?
+  end
+
   private
 
   def video

app/models/media_asset.rb

@@ -86,17 +86,17 @@ class MediaAsset < ApplicationRecord
     def convert_file(media_file)
       case type
       in :preview
-        media_file.preview(width, height, format: :jpeg, quality: 85)
+        media_file.preview!(width, height, format: :jpeg, quality: 85)
       in :"180x180"
-        media_file.preview(width, height, format: :jpeg, quality: 85)
+        media_file.preview!(width, height, format: :jpeg, quality: 85)
       in :"360x360"
-        media_file.preview(width, height, format: :jpeg, quality: 85)
+        media_file.preview!(width, height, format: :jpeg, quality: 85)
       in :"720x720"
-        media_file.preview(width, height, format: :webp, quality: 75)
+        media_file.preview!(width, height, format: :webp, quality: 75)
       in :sample if media_asset.is_ugoira?
         media_file.convert
       in :sample | :full if media_asset.is_static_image?
-        media_file.preview(width, height, format: :jpeg, quality: 85)
+        media_file.preview!(width, height, format: :jpeg, quality: 85)
       in :original
         media_file
       end
@@ -235,7 +235,7 @@ class MediaAsset < ApplicationRecord
 
         # XXX should do this in parallel with thumbnail generation.
         # XXX shouldn't generate thumbnail twice (very slow for ugoira)
-        media_asset.update!(ai_tags: media_file.preview(360, 360).ai_tags)
+        media_asset.update!(ai_tags: media_file.preview!(360, 360).ai_tags)
         media_asset.update!(media_metadata: MediaMetadata.new(file: media_file))
 
         media_asset.distribute_files!(media_file)

app/models/upload_media_asset.rb

@@ -86,9 +86,10 @@ class UploadMediaAsset < ApplicationRecord
     Source::Extractor.find(source_url, page_url)
   end
 
+  # Calls `process_upload!`
   def async_process_upload!
     if file.present?
-      process_upload!
+      ProcessUploadMediaAssetJob.perform_now(self)
     else
       ProcessUploadMediaAssetJob.perform_later(self)
     end

config/docker/build-base-image.sh

@@ -14,7 +14,7 @@ COMMON_BUILD_DEPS="
   curl ca-certificates build-essential pkg-config git
 "
 RUBY_BUILD_DEPS="libssl-dev zlib1g-dev libgmp-dev"
-FFMPEG_BUILD_DEPS="libvpx-dev nasm"
+FFMPEG_BUILD_DEPS="libvpx-dev libdav1d-dev nasm"
 MOZJPEG_BUILD_DEPS="cmake nasm libpng-dev zlib1g-dev"
 VIPS_BUILD_DEPS="
   libfftw3-dev libwebp-dev liborc-dev liblcms2-dev libpng-dev
@@ -24,7 +24,7 @@ EXIFTOOL_RUNTIME_DEPS="perl perl-modules libarchive-zip-perl"
 DANBOORU_RUNTIME_DEPS="
   ca-certificates mkvtoolnix rclone libpq5 openssl libgmpxx4ldbl
   zlib1g libfftw3-3 libwebp7 libwebpmux3 libwebpdemux2 liborc-0.4.0 liblcms2-2
-  libpng16-16 libexpat1 libglib2.0 libgif7 libexif12 libheif1 libvpx7
+  libpng16-16 libexpat1 libglib2.0 libgif7 libexif12 libheif1 libvpx7 libdav1d6
   libseccomp2 libseccomp-dev libjemalloc2
 "
 COMMON_RUNTIME_DEPS="
@@ -77,7 +77,7 @@ install_ffmpeg() {
   curl -L "$FFMPEG_URL" | tar -C /usr/local/src -xzvf -
   cd /usr/local/src/FFmpeg-n${FFMPEG_VERSION}
 
-  ./configure --disable-ffplay --disable-network --disable-doc --enable-libvpx
+  ./configure --disable-ffplay --disable-network --disable-doc --enable-libvpx --enable-libdav1d
   make -j "$(nproc)"
   cp ffmpeg ffprobe /usr/local/bin
 

db/migrate/20221027000931_remove_error_index_on_upload_media_assets.rb

@@ -0,0 +1,5 @@
+class RemoveErrorIndexOnUploadMediaAssets < ActiveRecord::Migration[7.0]
+  def change
+    remove_index :upload_media_assets, :error, where: "error IS NOT NULL"
+  end
+end

db/structure.sql

@@ -5500,13 +5500,6 @@ CREATE INDEX index_upgrade_codes_on_status ON public.upgrade_codes USING btree (
 CREATE INDEX index_upgrade_codes_on_user_upgrade_id ON public.upgrade_codes USING btree (user_upgrade_id) WHERE (user_upgrade_id IS NOT NULL);
 
 
---
--- Name: index_upload_media_assets_on_error; Type: INDEX; Schema: public; Owner: -
---
-
-CREATE INDEX index_upload_media_assets_on_error ON public.upload_media_assets USING btree (error) WHERE (error IS NOT NULL);
-
-
 --
 -- Name: index_upload_media_assets_on_media_asset_id; Type: INDEX; Schema: public; Owner: -
 --
@@ -6907,6 +6900,7 @@ INSERT INTO "schema_migrations" (version) VALUES
 ('20221003080342'),
 ('20221010035855'),
 ('20221026084655'),
-('20221026084656');
+('20221026084656'),
+('20221027000931');
 
 

test/functional/uploads_controller_test.rb

@@ -179,7 +179,7 @@ class UploadsControllerTest < ActionDispatch::IntegrationTest
         end
       end
 
-      context "for a corrupted image" do
+      context "for a corrupted file" do
         should "fail for a corrupted jpeg" do
           create_upload!("test/files/test-corrupt.jpg", user: @user)
           assert_match("corrupt", Upload.last.error)
@@ -195,6 +195,11 @@ class UploadsControllerTest < ActionDispatch::IntegrationTest
           create_upload!("test/files/test-corrupt.png", user: @user)
           assert_match("corrupt", Upload.last.error)
         end
+
+        should "fail for a corrupted mp4" do
+          create_upload!("test/files/mp4/test-corrupt.mp4", user: @user)
+          assert_match("corrupt", Upload.last.error)
+        end
       end
 
       context "for an unsupported WebP file" do
@@ -330,6 +335,7 @@ class UploadsControllerTest < ActionDispatch::IntegrationTest
         should_upload_successfully("test/files/test-static-32x32.gif")
         should_upload_successfully("test/files/test-animated-86x52.gif")
         should_upload_successfully("test/files/test-300x300.mp4")
+        should_upload_successfully("test/files/test-audio.mp4")
         should_upload_successfully("test/files/webm/test-512x512.webm")
         should_upload_successfully("test/files/test-audio.m4v")
         # should_upload_successfully("test/files/compressed.swf")

test/unit/media_file_test.rb

@@ -202,15 +202,23 @@ class MediaFileTest < ActiveSupport::TestCase
 
     should "determine the duration of the video" do
       file = MediaFile.open("test/files/test-audio.mp4")
+      assert_equal(false, file.is_corrupt?)
       assert_equal(1.002667, file.duration)
       assert_equal(10/1.002667, file.frame_rate)
       assert_equal(10, file.frame_count)
 
       file = MediaFile.open("test/files/test-300x300.mp4")
+      assert_equal(false, file.is_corrupt?)
       assert_equal(5.7, file.duration)
       assert_equal(1.75, file.frame_rate.round(2))
       assert_equal(10, file.frame_count)
     end
+
+    should "detect corrupt videos" do
+      file = MediaFile.open("test/files/mp4/test-corrupt.mp4")
+
+      assert_equal(true, file.is_corrupt?)
+    end
   end
 
   context "for a webm file" do