Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make VideoHandler support optional #1439

Merged
merged 8 commits into from
Aug 6, 2022
Merged

Make VideoHandler support optional #1439

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cc04fa4
Make VideoHandler support optional
kamil4 Aug 3, 2022
cfbaf3d
Changes recommended by nagmat84
kamil4 Aug 4, 2022
5e5217b
Add a testcase for video upload without ffmpeg
kamil4 Aug 4, 2022
d3572c5
Fix new testcase
kamil4 Aug 4, 2022
cb38db6
Fix a typo
kamil4 Aug 4, 2022
8dcbd13
Update tests/Feature/PhotosAddHandlerTestAbstract.php
kamil4 Aug 4, 2022
b7f84c7
Check for error message that FFmpeg is disabled
nagmat84 Aug 4, 2022
05fbd50
Fixed Google Motion Photo test
nagmat84 Aug 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 29 additions & 20 deletions app/Actions/Photo/Strategies/AddStandaloneStrategy.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
34 changes: 34 additions & 0 deletions tests/Feature/PhotosAddHandlerTestAbstract.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

namespace Tests\Feature;

use App\Models\Configs;
use Carbon\Carbon;
use Illuminate\Support\Facades\DB;
use Tests\Feature\Base\PhotoTestBase;
Expand Down Expand Up @@ -344,4 +345,37 @@ 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);

$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,
],
],
kamil4 marked this conversation as resolved.
Show resolved Hide resolved
]);

Configs::set(self::CONFIG_HAS_FFMPEG, $hasFFMpeg);
Configs::set(self::CONFIG_HAS_EXIF_TOOL, $hasExifTool);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should additionally assert that the expected error message, e.g. "FFMpeg is disabled by configuration", can be found in the logs?

To this end we could clear the log table at the beginning of the test with DB::tables('logs)->delete(). At the end of the test we do a count-query and assert that we found an entry, e.g. something like

$numErrorMsg = DB::tables('logs)->where('message', 'like', '%FFMpeg is disabled by configuration%')->count();
self::assertEquals(1, $numErrorMsg);

But maybe this would make the test too strict? I am unsure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't do it myself (testing for log messages seems a little overly prescriptive to me) but feel free to add a commit to that effect to this PR if you want.

}