Make VideoHandler support optional (#1439)

* Make VideoHandler support optional * Changes recommended by nagmat84 * Add a testcase for video upload without ffmpeg * Fix new testcase * Fix a typo * Update tests/Feature/PhotosAddHandlerTestAbstract.php Co-authored-by: Matthias Nagel <[email protected]> * Check for error message that FFmpeg is disabled * Fixed Google Motion Photo test Co-authored-by: Matthias Nagel <[email protected]>
LycheeOrg · Aug 6, 2022 · ba99f20 · ba99f20
1 parent 1373d05
commit ba99f20
Show file tree

Hide file tree

Showing 2 changed files with 118 additions and 25 deletions.
diff --git a/app/Actions/Photo/Strategies/AddStandaloneStrategy.php b/app/Actions/Photo/Strategies/AddStandaloneStrategy.php
@@ -89,36 +89,45 @@ public function do(): Photo
  $this->photo->original_checksum = StreamStat::createFromLocalFile($this->sourceFile)->checksum;
 
  try {
- // Load source image if it is a supported photo format
- if ($this->photo->isPhoto()) {
- $this->sourceImage = new ImageHandler();
- $this->sourceImage->load($this->sourceFile);
- } elseif ($this->photo->isVideo()) {
- $videoHandler = new VideoHandler();
- $videoHandler->load($this->sourceFile);
- $position = is_numeric($this->photo->aperture) ? floatval($this->photo->aperture) / 2 : 0.0;
- $this->sourceImage = $videoHandler->extractFrame($position);
- } else {
- // If we have a raw file, we try to treat it as an image, as
- // Imagick supports a lot of other image-like file types
- // But if it fails we don't mind.
- try {
+ try {
+ if ($this->photo->isPhoto()) {
+ // Load source image if it is a supported photo format
+ $this->sourceImage = new ImageHandler();
+ $this->sourceImage->load($this->sourceFile);
+ } elseif ($this->photo->isVideo()) {
+ $videoHandler = new VideoHandler();
+ $videoHandler->load($this->sourceFile);
+ $position = is_numeric($this->photo->aperture) ? floatval($this->photo->aperture) / 2 : 0.0;
+ $this->sourceImage = $videoHandler->extractFrame($position);
+ } else {
  $this->sourceImage = new ImageHandler();
  $this->sourceImage->load($this->sourceFile);
- } catch (\Throwable) {
- $this->sourceImage = null;
  }
+ } catch (\Throwable $e) {
+ // This may happen for videos if FFmpeg is not available to
+ // extract a still frame, or for raw files (Imagick may be
+ // able to convert them to jpeg, but there are no
+ // guarantees, and Imagick may not be available).
+ $this->sourceImage = null;
+
+ // Log an error without failing.
+ Handler::reportSafely($e);
  }
 
  // Handle Google Motion Pictures
  // We must extract the video stream from the original (local)
  // file and stash it away, before the original file is moved into
  // its (potentially remote) final position
  if ($this->parameters->exifInfo->microVideoOffset !== 0) {
- $tmpVideoFile = new TemporaryLocalFile(GoogleMotionPictureHandler::FINAL_VIDEO_FILE_EXTENSION, $this->sourceFile->getBasename());
- $gmpHandler = new GoogleMotionPictureHandler();
- $gmpHandler->load($this->sourceFile, $this->parameters->exifInfo->microVideoOffset);
- $gmpHandler->saveVideoStream($tmpVideoFile);
+ try {
+ $tmpVideoFile = new TemporaryLocalFile(GoogleMotionPictureHandler::FINAL_VIDEO_FILE_EXTENSION, $this->sourceFile->getBasename());
+ $gmpHandler = new GoogleMotionPictureHandler();
+ $gmpHandler->load($this->sourceFile, $this->parameters->exifInfo->microVideoOffset);
+ $gmpHandler->saveVideoStream($tmpVideoFile);
+ } catch (\Throwable $e) {
+ Handler::reportSafely($e);
+ $tmpVideoFile = null;
+ }
  } else {
  $tmpVideoFile = null;
  }

diff --git a/tests/Feature/PhotosAddHandlerTestAbstract.php b/tests/Feature/PhotosAddHandlerTestAbstract.php
@@ -12,6 +12,7 @@
 
 namespace Tests\Feature;
 
+use App\Models\Configs;
 use Carbon\Carbon;
 use Illuminate\Support\Facades\DB;
 use Tests\Feature\Base\PhotoTestBase;
@@ -287,19 +288,49 @@ public function testGoogleMotionPhotoUpload(): void
  * Tests Google Motion Photo upload with a file which has a broken
  * video stream.
  *
+ * We still expect the still part of the photo to be generated, but
+ * the photo won't be recognized as a Google Motion Photo and the
+ * video part is expected to be missing.
+ *
+ * This is in line with our best effort approach.
+ *
+ * Moreover, the logs should contain an error message, telling the user
+ * what went wrong.
+ *
  * @return void
  */
  public function testBrokenGoogleMotionPhotoUpload(): void
  {
  $this->assertHasExifToolOrSkip();
  $this->assertHasFFMpegOrSkip();
 
- $this->photos_tests->upload(
- TestCase::createUploadedFile(TestCase::SAMPLE_FILE_GMP_BROKEN_IMAGE),
- null,
- 500,
- 'MediaFileOperationException'
+ DB::table('logs')->delete();
+
+ $response = $this->photos_tests->upload(
+ TestCase::createUploadedFile(TestCase::SAMPLE_FILE_GMP_BROKEN_IMAGE)
  );
+ // Size variants are generated, because they are extracted from the
+ // still part of the GMP, not the video part.
+ $response->assertJson([
+ 'album_id' => null,
+ 'title' => 'google_motion_photo_broken',
+ 'type' => TestCase::MIME_TYPE_IMG_JPEG,
+ 'size_variants' => [
+ 'original' => ['width' => 2016, 'height' => 1512],
+ 'medium2x' => null,
+ 'medium' => ['width' => 1440, 'height' => 1080],
+ 'small2x' => ['width' => 960, 'height' => 720],
+ 'small' => ['width' => 480, 'height' => 360],
+ 'thumb2x' => ['width' => 400, 'height' => 400],
+ 'thumb' => ['width' => 200, 'height' => 200],
+ ],
+ 'live_photo_url' => null,
+ ]);
+
+ $logCount = DB::table('logs')
+ ->where('text', 'like', '%ffprobe%failed%')
+ ->count();
+ self::assertNotEquals(0, $logCount);
  }
 
  /**
@@ -344,4 +375,57 @@ public function testTrickyVideoUpload(): void
  ],
  ]);
  }
+
+ /**
+ * Tests video upload without ffmpeg or exiftool.
+ *
+ * @return void
+ */
+ public function testVideoUploadWithoutFFmpeg(): void
+ {
+ $hasExifTool = Configs::getValueAsInt(self::CONFIG_HAS_EXIF_TOOL);
+ Configs::set(self::CONFIG_HAS_EXIF_TOOL, 0);
+
+ $hasFFMpeg = Configs::getValueAsInt(TestCase::CONFIG_HAS_FFMPEG);
+ Configs::set(TestCase::CONFIG_HAS_FFMPEG, 0);
+
+ DB::table('logs')->delete();
+
+ $response = $this->photos_tests->upload(
+ TestCase::createUploadedFile(TestCase::SAMPLE_FILE_GAMING_VIDEO)
+ );
+ $response->assertJson([
+ 'album_id' => null,
+ 'title' => 'gaming',
+ 'type' => TestCase::MIME_TYPE_VID_MP4,
+ 'size_variants' => [
+ 'original' => [
+ 'width' => 0,
+ 'height' => 0,
+ 'filesize' => 66781184,
+ ],
+ 'medium2x' => null,
+ 'medium' => null,
+ 'small2x' => null,
+ 'small' => null,
+ 'thumb2x' => null,
+ 'thumb' => null,
+ ],
+ ]);
+
+ // In the test suite we cannot really ensure that FFMpeg is not
+ // available, because the executable is still part of the test
+ // environment.
+ // Hence, we can only disable it (see above), but cannot be sure
+ // that it isn't called accidentally.
+ // As a second-best approach, we check at least for the existence
+ // of an error message in the log.
+ $logCount = DB::table('logs')
+ ->where('text', 'like', '%FFmpeg%disabled%')
+ ->count();
+ self::assertEquals(1, $logCount);
+
+ Configs::set(self::CONFIG_HAS_FFMPEG, $hasFFMpeg);
+ Configs::set(self::CONFIG_HAS_EXIF_TOOL, $hasExifTool);
+ }
 }