Merge pull request #7836 from pixelfed/fix/video-thumbnail-short

Clamp video thumbnail seek and keep storage dispatch on thumbnail failure

app/Jobs/VideoPipeline/VideoThumbnail.php

@@ -83,51 +83,50 @@ class VideoThumbnail implements ShouldBeUniqueUntilProcessing, ShouldQueue
         $base = $media->media_path;
         $path = explode('/', $base);
         $name = last($path);
+
+        // Thumbnail extraction is best-effort. A failure here must never abort the
+        // job, otherwise handle() returns before the MediaStoragePipeline dispatch
+        // below and the video is stranded on local disk with no cdn_url. A clip
+        // shorter than a second also can't be seeked to 1s, so fall back to frame 0.
         try {
             $t = explode('.', $name);
             $t = $t[0].'_thumb.jpeg';
             $i = count($path) - 1;
             $path[$i] = $t;
             $save = implode('/', $path);
-            $video = FFMpeg::open($base)
-                ->getFrameFromSeconds(1)
+
+            $video = FFMpeg::open($base);
+            $seconds = $video->getDurationInSeconds() < 1 ? 0 : 1;
+            $video->getFrameFromSeconds($seconds)
                 ->export()
                 ->toDisk('local')
                 ->save($save);
 
             $media->thumbnail_path = $save;
             $media->save();
-
-            // Isolated from the thumbnail work on purpose. The blurhash is decorative;
-            // MediaStoragePipeline at the end of this method is not, and until now any
-            // failure here skipped it, leaving the video on local disk forever with no
-            // failed_jobs row to show for it (pixelfed#2652).
-            try {
-                $blurhash = Blurhash::generate($media);
-                if ($blurhash) {
-                    $media->blurhash = $blurhash;
-                    $media->save();
-                }
-            } catch (\Throwable $e) {
-                // \Throwable, not \Exception: the failure this guard exists for is a
-                // memory_limit exhaustion, which surfaces as \Error (or a fatal), not
-                // \Exception. Catching only \Exception here would let the very case
-                // that strands the video (pixelfed#2652) slip through and skip the
-                // MediaStoragePipeline dispatch below.
-                if (config('app.dev_log')) {
-                    Log::error('Video blurhash generation failed: '.$e->getMessage());
-                }
+        } catch (\Throwable $e) {
+            // \Throwable, not \Exception: a memory_limit exhaustion surfaces as
+            // \Error, and catching only \Exception here would let it strand the
+            // video by skipping the storage dispatch below (pixelfed#2652).
+            if (config('app.dev_log')) {
+                Log::error('Video thumbnail generation failed: '.$e->getMessage());
             }
+        }
 
-            if (config('media.hls.enabled')) {
-                VideoHlsPipeline::dispatch($media)->onQueue('mmo');
+        try {
+            $blurhash = Blurhash::generate($media);
+            if ($blurhash) {
+                $media->blurhash = $blurhash;
+                $media->save();
             }
-        } catch (\Exception $e) {
+        } catch (\Throwable $e) {
             if (config('app.dev_log')) {
-                Log::error('Video thumbnail generation failed: '.$e->getMessage());
+                Log::error('Video blurhash generation failed: '.$e->getMessage());
             }
+        }
 
-            throw $e;
+        if (config('media.hls.enabled')) {
+            VideoHlsPipeline::dispatch($media)->onQueue('mmo');
         }
 
         if ($media->status_id) {

app/Jobs/VideoPipeline/VideoThumbnailToCloudPipeline.php

@@ -16,6 +16,7 @@ use Illuminate\Queue\InteractsWithQueue;
 use Illuminate\Queue\Middleware\WithoutOverlapping;
 use Illuminate\Queue\SerializesModels;
 use Illuminate\Support\Facades\Cache;
+use Illuminate\Support\Facades\Log;
 use Illuminate\Support\Facades\Storage;
 
 class VideoThumbnailToCloudPipeline implements ShouldBeUniqueUntilProcessing, ShouldQueue
@@ -100,8 +101,10 @@ class VideoThumbnailToCloudPipeline implements ShouldBeUniqueUntilProcessing, Sh
             $i = count($path) - 1;
             $path[$i] = $t;
             $save = implode('/', $path);
-            $video = FFMpeg::open($base)
-                ->getFrameFromSeconds(1)
+
+            $video = FFMpeg::open($base);
+            $seconds = $video->getDurationInSeconds() < 1 ? 0 : 1;
+            $video->getFrameFromSeconds($seconds)
                 ->export()
                 ->toDisk('local')
                 ->save($save);
@@ -134,7 +137,10 @@ class VideoThumbnailToCloudPipeline implements ShouldBeUniqueUntilProcessing, Sh
             if (str_starts_with($media->media_path, 'public/m/_v2/') && str_ends_with($media->media_path, '.mp4')) {
                 Storage::disk('local')->delete($media->media_path);
             }
-        } catch (\Exception) {
+        } catch (\Throwable $e) {
+            if (config('app.dev_log')) {
+                Log::error('Video thumbnail (cloud) generation failed: '.$e->getMessage());
+            }
         }
 
         if ($media->status_id) {

tests/Feature/VideoPipeline/VideoThumbnailStrandedVideoTest.php

@@ -0,0 +1,87 @@
+<?php
+
+use App\Jobs\MediaPipeline\MediaStoragePipeline;
+use App\Jobs\VideoPipeline\VideoThumbnail;
+use App\Models\Media;
+use App\Models\User;
+use Illuminate\Foundation\Testing\LazilyRefreshDatabase;
+use Illuminate\Support\Facades\Bus;
+use ProtoneMedia\LaravelFFMpeg\Support\FFMpeg;
+
+uses(LazilyRefreshDatabase::class);
+
+// Builds the media row the job deserializes. A status_id is set so the job's
+// cache-busting branch runs, matching production.
+function strandedVideoMedia(): Media
+{
+    $user = User::factory()->create();
+    $user->refresh();
+
+    return Media::create([
+        'profile_id' => $user->profile->id,
+        'user_id' => $user->id,
+        'status_id' => null,
+        'media_path' => 'public/m/_v2/1/aa/bb/clip.mp4',
+        'mime' => 'video/mp4',
+        'size' => 1024,
+        'remote_media' => false,
+        'order' => 0,
+    ]);
+}
+
+it('generates a frame-0 thumbnail for a sub-1s video and dispatches storage', function () {
+    Bus::fake();
+
+    $media = strandedVideoMedia();
+
+    $requestedSeconds = null;
+
+    $exporter = Mockery::mock();
+    $exporter->shouldReceive('export')->andReturnSelf();
+    $exporter->shouldReceive('toDisk')->andReturnSelf();
+    $exporter->shouldReceive('save')->andReturn('saved');
+
+    $opened = Mockery::mock();
+    $opened->shouldReceive('getDurationInSeconds')->andReturn(0);
+    $opened->shouldReceive('getFrameFromSeconds')->andReturnUsing(function ($seconds) use (&$requestedSeconds, $exporter) {
+        $requestedSeconds = $seconds;
+
+        return $exporter;
+    });
+
+    $spy = Mockery::mock();
+    $spy->shouldReceive('open')->andReturn($opened);
+    FFMpeg::swap($spy);
+
+    (new VideoThumbnail($media))->handle();
+
+    expect($requestedSeconds)->toBe(0);
+    expect($media->fresh()->thumbnail_path)->toBe('public/m/_v2/1/aa/bb/clip_thumb.jpeg');
+
+    Bus::assertDispatched(MediaStoragePipeline::class);
+});
+
+it('does not strand the video when thumbnail extraction fails', function () {
+    Bus::fake();
+
+    $media = strandedVideoMedia();
+
+    $exporter = Mockery::mock();
+    $exporter->shouldReceive('export')->andReturnSelf();
+    $exporter->shouldReceive('toDisk')->andReturnSelf();
+    $exporter->shouldReceive('save')->andThrow(new RuntimeException('seek past EOF'));
+
+    $opened = Mockery::mock();
+    $opened->shouldReceive('getDurationInSeconds')->andReturn(0);
+    $opened->shouldReceive('getFrameFromSeconds')->andReturn($exporter);
+
+    $spy = Mockery::mock();
+    $spy->shouldReceive('open')->andReturn($opened);
+    FFMpeg::swap($spy);
+
+    (new VideoThumbnail($media))->handle();
+
+    expect($media->fresh()->thumbnail_path)->toBeNull();
+
+    Bus::assertDispatched(MediaStoragePipeline::class);
+});