From cc04fa4ed71c3ebe5d61718f97b8b31996d9dd55 Mon Sep 17 00:00:00 2001 From: kamil4 Date: Wed, 3 Aug 2022 17:14:03 -0500 Subject: [PATCH 1/8] Make VideoHandler support optional --- .../Photo/Strategies/AddStandaloneStrategy.php | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/app/Actions/Photo/Strategies/AddStandaloneStrategy.php b/app/Actions/Photo/Strategies/AddStandaloneStrategy.php index e3dfb36eef9..1792663968d 100644 --- a/app/Actions/Photo/Strategies/AddStandaloneStrategy.php +++ b/app/Actions/Photo/Strategies/AddStandaloneStrategy.php @@ -94,10 +94,16 @@ public function do(): Photo $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); + // We try to extract a video frame using FFmpeg; however, + // that one's optional, so we need to be able to recover. + try { + $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); + } catch (\Throwable) { + $this->sourceImage = null; + } } 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 From cfbaf3d085922937a3d92ec15b09cac2d134f047 Mon Sep 17 00:00:00 2001 From: kamil4 Date: Wed, 3 Aug 2022 22:36:08 -0500 Subject: [PATCH 2/8] Changes recommended by nagmat84 --- .../Strategies/AddStandaloneStrategy.php | 47 ++++++++++--------- 1 file changed, 25 insertions(+), 22 deletions(-) diff --git a/app/Actions/Photo/Strategies/AddStandaloneStrategy.php b/app/Actions/Photo/Strategies/AddStandaloneStrategy.php index 1792663968d..07b57e721f5 100644 --- a/app/Actions/Photo/Strategies/AddStandaloneStrategy.php +++ b/app/Actions/Photo/Strategies/AddStandaloneStrategy.php @@ -89,31 +89,29 @@ 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()) { - // We try to extract a video frame using FFmpeg; however, - // that one's optional, so we need to be able to recover. - 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); - } catch (\Throwable) { - $this->sourceImage = null; - } - } 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 { + } 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 @@ -121,10 +119,15 @@ public function do(): Photo // 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; } From 5e5217b7403d219d0e4203a3f1ba5730d5368a2a Mon Sep 17 00:00:00 2001 From: kamil4 Date: Wed, 3 Aug 2022 22:51:42 -0500 Subject: [PATCH 3/8] Add a testcase for video upload without ffmpeg --- .../Feature/PhotosAddHandlerTestAbstract.php | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/tests/Feature/PhotosAddHandlerTestAbstract.php b/tests/Feature/PhotosAddHandlerTestAbstract.php index b04c7d18aea..a0cfec597df 100644 --- a/tests/Feature/PhotosAddHandlerTestAbstract.php +++ b/tests/Feature/PhotosAddHandlerTestAbstract.php @@ -344,4 +344,37 @@ public function testTrickyVideoUpload(): void ], ]); } + + /** + * Tests video upload with 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); + + $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, + ], + ], + ]); + + Configs::set(self::CONFIG_HAS_FFMPEG, $hasFFMpeg); + Configs::set(self::CONFIG_HAS_EXIF_TOOL, $hasExifTool); + } } From d3572c5b4fdd1ff904878f8a0c6538ec2de1cc50 Mon Sep 17 00:00:00 2001 From: kamil4 Date: Wed, 3 Aug 2022 23:01:02 -0500 Subject: [PATCH 4/8] Fix new testcase --- tests/Feature/PhotosAddHandlerTestAbstract.php | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/Feature/PhotosAddHandlerTestAbstract.php b/tests/Feature/PhotosAddHandlerTestAbstract.php index a0cfec597df..cf281a6fc3f 100644 --- 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; From cb38db6eda9e5ad9d36394fa70f79a3b1cabd566 Mon Sep 17 00:00:00 2001 From: kamil4 Date: Wed, 3 Aug 2022 23:09:50 -0500 Subject: [PATCH 5/8] Fix a typo --- tests/Feature/PhotosAddHandlerTestAbstract.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Feature/PhotosAddHandlerTestAbstract.php b/tests/Feature/PhotosAddHandlerTestAbstract.php index cf281a6fc3f..37754b2767e 100644 --- a/tests/Feature/PhotosAddHandlerTestAbstract.php +++ b/tests/Feature/PhotosAddHandlerTestAbstract.php @@ -347,7 +347,7 @@ public function testTrickyVideoUpload(): void } /** - * Tests video upload with ffmpeg or exiftool. + * Tests video upload without ffmpeg or exiftool. * * @return void */ From 8dcbd13780a9ac3e17db846aec1b732c7f8a111b Mon Sep 17 00:00:00 2001 From: Kamil Iskra Date: Thu, 4 Aug 2022 11:02:00 -0500 Subject: [PATCH 6/8] Update tests/Feature/PhotosAddHandlerTestAbstract.php Co-authored-by: Matthias Nagel --- tests/Feature/PhotosAddHandlerTestAbstract.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/Feature/PhotosAddHandlerTestAbstract.php b/tests/Feature/PhotosAddHandlerTestAbstract.php index 37754b2767e..c41bf64b3b6 100644 --- a/tests/Feature/PhotosAddHandlerTestAbstract.php +++ b/tests/Feature/PhotosAddHandlerTestAbstract.php @@ -372,6 +372,12 @@ public function testVideoUploadWithoutFFmpeg(): void 'height' => 0, 'filesize' => 66781184, ], + 'medium2x' => null, + 'medium' => null, + 'small2x' => null, + 'small' => null, + 'thumb2x' => null, + 'thumb' => null, ], ]); From b7f84c747bbc46878d54a2f233591a6ffbc98df9 Mon Sep 17 00:00:00 2001 From: Matthias Nagel Date: Thu, 4 Aug 2022 18:30:46 +0200 Subject: [PATCH 7/8] Check for error message that FFmpeg is disabled --- tests/Feature/PhotosAddHandlerTestAbstract.php | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/Feature/PhotosAddHandlerTestAbstract.php b/tests/Feature/PhotosAddHandlerTestAbstract.php index c41bf64b3b6..c388eef9028 100644 --- a/tests/Feature/PhotosAddHandlerTestAbstract.php +++ b/tests/Feature/PhotosAddHandlerTestAbstract.php @@ -359,6 +359,8 @@ public function testVideoUploadWithoutFFmpeg(): void $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) ); @@ -381,6 +383,18 @@ public function testVideoUploadWithoutFFmpeg(): void ], ]); + // 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); } From 05fbd50786ce394df59723bff73fbc5819e8678e Mon Sep 17 00:00:00 2001 From: Matthias Nagel Date: Thu, 4 Aug 2022 18:47:42 +0200 Subject: [PATCH 8/8] Fixed Google Motion Photo test --- .../Feature/PhotosAddHandlerTestAbstract.php | 40 ++++++++++++++++--- 1 file changed, 35 insertions(+), 5 deletions(-) diff --git a/tests/Feature/PhotosAddHandlerTestAbstract.php b/tests/Feature/PhotosAddHandlerTestAbstract.php index c388eef9028..70ffaeb7baf 100644 --- a/tests/Feature/PhotosAddHandlerTestAbstract.php +++ b/tests/Feature/PhotosAddHandlerTestAbstract.php @@ -288,6 +288,15 @@ 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 @@ -295,12 +304,33 @@ 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); } /**