From 6ca6cbe6a024f6d9f3fd85b03dfe4cadb0589624 Mon Sep 17 00:00:00 2001 From: "Simon L." Date: Tue, 20 Oct 2026 11:48:43 +0200 Subject: [PATCH 1/4] feat(preview): generate video previews on an ffmpeg-over-ip server Add the OC\Preview\MovieRemote provider which runs ffprobe and ffmpeg on an ffmpeg-over-ip server (https://github.com/steelbrain/ffmpeg-over-ip), similar to the Imaginary provider for images. It is configured with preview_ffmpeg_url and preview_ffmpeg_key; neither ffmpeg nor the ffmpeg-over-ip client is needed on the Nextcloud server. OC\Preview\FFmpegOverIpClient implements the client side of the protocol in PHP. The server tunnels all file access of the remote process back over the connection, so the video is streamed from any storage, including S3 and external storages. Only the video itself can be opened, read-only. Streams that cannot seek are copied to a temporary stream first. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 Signed-off-by: Simon L. --- config/config.sample.php | 20 ++ lib/composer/composer/autoload_classmap.php | 2 + lib/composer/composer/autoload_static.php | 2 + lib/private/Preview/FFmpegOverIpClient.php | 340 +++++++++++++++++++ lib/private/Preview/Movie.php | 34 +- lib/private/Preview/MovieRemote.php | 132 +++++++ lib/private/PreviewManager.php | 2 + lib/private/SystemConfig.php | 2 + tests/lib/Preview/FFmpegOverIpClientTest.php | 198 +++++++++++ tests/lib/Preview/MovieRemoteTest.php | 191 +++++++++++ 10 files changed, 910 insertions(+), 13 deletions(-) create mode 100644 lib/private/Preview/FFmpegOverIpClient.php create mode 100644 lib/private/Preview/MovieRemote.php create mode 100644 tests/lib/Preview/FFmpegOverIpClientTest.php create mode 100644 tests/lib/Preview/MovieRemoteTest.php diff --git a/config/config.sample.php b/config/config.sample.php index da3de23d25202..2246ef38bb06b 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -1633,6 +1633,25 @@ */ 'preview_imaginary_key' => 'secret', + /** + * Set the address (``host:port``) of an ffmpeg-over-ip server to generate + * video previews on. Also requires the ``OC\Preview\MovieRemote`` provider + * to be enabled in the ``enabledPreviewProviders`` array. Neither ffmpeg nor + * the ffmpeg-over-ip client is needed on the Nextcloud server in that case. + * + * The server reads the videos from Nextcloud over the same connection, so + * it needs no access to the data directory. The connection is not + * encrypted, so only use it within a trusted network. + * + * See https://github.com/steelbrain/ffmpeg-over-ip + */ + 'preview_ffmpeg_url' => 'ffmpeg-over-ip:5050', + + /** + * The ``authSecret`` configured on the ffmpeg-over-ip server. + */ + 'preview_ffmpeg_key' => 'secret', + /** * Only register providers that have been explicitly enabled * @@ -1646,6 +1665,7 @@ * - ``OC\Preview\Illustrator`` * - ``OC\Preview\JP2`` * - ``OC\Preview\Movie`` + * - ``OC\Preview\MovieRemote`` * - ``OC\Preview\MP3`` * - ``OC\Preview\MSOffice2003`` * - ``OC\Preview\MSOffice2007`` diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 358118e8d059e..1c6012389c898 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -2172,6 +2172,7 @@ 'OC\\Preview\\Db\\Preview' => $baseDir . '/lib/private/Preview/Db/Preview.php', 'OC\\Preview\\Db\\PreviewMapper' => $baseDir . '/lib/private/Preview/Db/PreviewMapper.php', 'OC\\Preview\\EMF' => $baseDir . '/lib/private/Preview/EMF.php', + 'OC\\Preview\\FFmpegOverIpClient' => $baseDir . '/lib/private/Preview/FFmpegOverIpClient.php', 'OC\\Preview\\Font' => $baseDir . '/lib/private/Preview/Font.php', 'OC\\Preview\\GIF' => $baseDir . '/lib/private/Preview/GIF.php', 'OC\\Preview\\Generator' => $baseDir . '/lib/private/Preview/Generator.php', @@ -2193,6 +2194,7 @@ 'OC\\Preview\\MarkDown' => $baseDir . '/lib/private/Preview/MarkDown.php', 'OC\\Preview\\MimeIconProvider' => $baseDir . '/lib/private/Preview/MimeIconProvider.php', 'OC\\Preview\\Movie' => $baseDir . '/lib/private/Preview/Movie.php', + 'OC\\Preview\\MovieRemote' => $baseDir . '/lib/private/Preview/MovieRemote.php', 'OC\\Preview\\Office' => $baseDir . '/lib/private/Preview/Office.php', 'OC\\Preview\\OpenDocument' => $baseDir . '/lib/private/Preview/OpenDocument.php', 'OC\\Preview\\PDF' => $baseDir . '/lib/private/Preview/PDF.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 5caaf52478f18..64383667dfdcf 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -2213,6 +2213,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\Preview\\Db\\Preview' => __DIR__ . '/../../..' . '/lib/private/Preview/Db/Preview.php', 'OC\\Preview\\Db\\PreviewMapper' => __DIR__ . '/../../..' . '/lib/private/Preview/Db/PreviewMapper.php', 'OC\\Preview\\EMF' => __DIR__ . '/../../..' . '/lib/private/Preview/EMF.php', + 'OC\\Preview\\FFmpegOverIpClient' => __DIR__ . '/../../..' . '/lib/private/Preview/FFmpegOverIpClient.php', 'OC\\Preview\\Font' => __DIR__ . '/../../..' . '/lib/private/Preview/Font.php', 'OC\\Preview\\GIF' => __DIR__ . '/../../..' . '/lib/private/Preview/GIF.php', 'OC\\Preview\\Generator' => __DIR__ . '/../../..' . '/lib/private/Preview/Generator.php', @@ -2234,6 +2235,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\Preview\\MarkDown' => __DIR__ . '/../../..' . '/lib/private/Preview/MarkDown.php', 'OC\\Preview\\MimeIconProvider' => __DIR__ . '/../../..' . '/lib/private/Preview/MimeIconProvider.php', 'OC\\Preview\\Movie' => __DIR__ . '/../../..' . '/lib/private/Preview/Movie.php', + 'OC\\Preview\\MovieRemote' => __DIR__ . '/../../..' . '/lib/private/Preview/MovieRemote.php', 'OC\\Preview\\Office' => __DIR__ . '/../../..' . '/lib/private/Preview/Office.php', 'OC\\Preview\\OpenDocument' => __DIR__ . '/../../..' . '/lib/private/Preview/OpenDocument.php', 'OC\\Preview\\PDF' => __DIR__ . '/../../..' . '/lib/private/Preview/PDF.php', diff --git a/lib/private/Preview/FFmpegOverIpClient.php b/lib/private/Preview/FFmpegOverIpClient.php new file mode 100644 index 0000000000000..50b032bfb13d2 --- /dev/null +++ b/lib/private/Preview/FFmpegOverIpClient.php @@ -0,0 +1,340 @@ + */ + private array $files = []; + /** @var array */ + private array $openFiles = []; + private float $deadline = 0; + + /** + * @param string $address ``host:port``, ``tcp://host:port`` or ``unix:/path/to/socket`` + * @param int $timeout seconds a single run() may take + */ + public function __construct( + private string $address, + private string $secret, + private int $timeout = 120, + private int $connectTimeout = 5, + ) { + } + + /** + * @param list $args + * @param array $files seekable streams by the path they are opened with + * @return array{exitCode: int, stdout: string, stderr: string} + * @throws \RuntimeException if the server is unreachable, rejects the command or violates the protocol + */ + public function run(int $program, array $args, array $files): array { + $socket = @stream_socket_client($this->getRemoteSocket(), $errno, $errstr, $this->connectTimeout); + if ($socket === false) { + throw new \RuntimeException('Could not connect to ffmpeg-over-ip server ' . $this->address . ': ' . $errstr); + } + + try { + return $this->communicate($socket, $program, $args, $files); + } finally { + fclose($socket); + } + } + + /** + * Runs the command on an already established connection + * + * @param resource $socket + * @param list $args + * @param array $files + * @return array{exitCode: int, stdout: string, stderr: string} + * @throws \RuntimeException + */ + public function communicate($socket, int $program, array $args, array $files): array { + $this->files = $files; + $this->openFiles = []; + $this->deadline = microtime(true) + $this->timeout; + $stdout = ''; + $stderr = ''; + + $this->writeMessage($socket, self::MSG_COMMAND, $this->encodeCommand($program, $args)); + $this->writeMessage($socket, self::MSG_STDIN_CLOSE, ''); + + while (true) { + [$type, $payload] = $this->readMessage($socket); + switch ($type) { + case self::MSG_EXIT_CODE: + $exitCode = $this->unpack('N', $payload, 0, 4); + return [ + // The exit code is sent as uint32, a negative code wraps around + 'exitCode' => $exitCode > 0x7FFFFFFF ? $exitCode - 0x100000000 : $exitCode, + 'stdout' => $stdout, + 'stderr' => $stderr, + ]; + case self::MSG_ERROR: + throw new \RuntimeException('ffmpeg-over-ip server error: ' . $payload); + case self::MSG_STDOUT: + if (strlen($stdout) + strlen($payload) > self::MAX_STDOUT_SIZE) { + throw new \RuntimeException('ffmpeg-over-ip output exceeds ' . self::MAX_STDOUT_SIZE . ' bytes'); + } + $stdout .= $payload; + break; + case self::MSG_STDERR: + $stderr .= substr($payload, 0, max(0, self::MAX_STDERR_SIZE - strlen($stderr))); + break; + case self::MSG_PING: + $this->writeMessage($socket, self::MSG_PONG, $payload); + break; + case self::MSG_PONG: + break; + default: + if ($type >= 0x20 && $type <= 0x29) { + [$responseType, $response] = $this->handleFileRequest($type, $payload); + $this->writeMessage($socket, $responseType, $response); + } + } + } + } + + private function getRemoteSocket(): string { + $address = rtrim($this->address, '/'); + if (str_starts_with($address, 'unix:')) { + return 'unix://' . preg_replace('/^unix:(\/\/)?/', '', $address); + } + if (str_starts_with($address, 'tcp://')) { + return $address; + } + return 'tcp://' . $address; + } + + private function encodeCommand(int $program, array $args): string { + $encodedArgs = pack('n', count($args)); + foreach ($args as $arg) { + if (strlen($arg) > 0xFFFF) { + throw new \RuntimeException('ffmpeg-over-ip argument is too long'); + } + $encodedArgs .= pack('n', strlen($arg)) . $arg; + } + + $nonce = random_bytes(16); + $signature = hash_hmac('sha256', chr(self::PROTOCOL_VERSION) . $nonce . chr($program) . $encodedArgs, $this->secret, true); + return chr(self::PROTOCOL_VERSION) . $nonce . $signature . chr($program) . $encodedArgs; + } + + /** + * @return array{0: int, 1: string} response type and payload + */ + private function handleFileRequest(int $type, string $payload): array { + $requestId = $this->unpack('n', $payload, 0, 2); + switch ($type) { + case self::MSG_OPEN: + $fileId = $this->unpack('n', $payload, 2, 10); + $flags = $this->unpack('N', $payload, 4, 10); + $path = substr($payload, 10); + if (!isset($this->files[$path])) { + return $this->ioError($requestId, self::FIO_ENOENT); + } + if (($flags & (self::FIO_O_ACCMODE | self::FIO_O_CREAT | self::FIO_O_TRUNC)) !== 0) { + return $this->ioError($requestId, self::FIO_EROFS); + } + if (isset($this->openFiles[$fileId])) { + return $this->ioError($requestId, self::FIO_EINVAL); + } + $this->openFiles[$fileId] = ['path' => $path, 'offset' => 0]; + return [self::MSG_OPEN_OK, pack('nJ', $requestId, $this->files[$path]['size'])]; + + case self::MSG_READ: + $fileId = $this->unpack('n', $payload, 2, 8); + $length = min($this->unpack('N', $payload, 4, 8), self::MAX_READ_SIZE); + if (!isset($this->openFiles[$fileId])) { + return $this->ioError($requestId, self::FIO_EINVAL); + } + $file = &$this->openFiles[$fileId]; + $stream = $this->files[$file['path']]['stream']; + if (ftell($stream) !== $file['offset'] && fseek($stream, $file['offset']) !== 0) { + return $this->ioError($requestId, self::FIO_EIO); + } + // A short read is treated as end of file, so the full length must be read + $data = $length > 0 ? stream_get_contents($stream, $length) : ''; + if ($data === false) { + return $this->ioError($requestId, self::FIO_EIO); + } + $file['offset'] += strlen($data); + return [self::MSG_READ_OK, pack('n', $requestId) . $data]; + + case self::MSG_SEEK: + $fileId = $this->unpack('n', $payload, 2, 13); + $offset = $this->unpack('J', $payload, 4, 13); + $whence = ord($payload[12]); + if (!isset($this->openFiles[$fileId])) { + return $this->ioError($requestId, self::FIO_EINVAL); + } + $file = &$this->openFiles[$fileId]; + $base = match ($whence) { + self::FIO_SEEK_SET => 0, + self::FIO_SEEK_CUR => $file['offset'], + self::FIO_SEEK_END => $this->files[$file['path']]['size'], + default => null, + }; + if ($base === null || $base + $offset < 0) { + return $this->ioError($requestId, self::FIO_EINVAL); + } + $file['offset'] = $base + $offset; + return [self::MSG_SEEK_OK, pack('nJ', $requestId, $file['offset'])]; + + case self::MSG_FSTAT: + $fileId = $this->unpack('n', $payload, 2, 4); + if (!isset($this->openFiles[$fileId])) { + return $this->ioError($requestId, self::FIO_EINVAL); + } + return [self::MSG_FSTAT_OK, pack('nJN', $requestId, $this->files[$this->openFiles[$fileId]['path']]['size'], self::FILE_MODE)]; + + case self::MSG_CLOSE: + $fileId = $this->unpack('n', $payload, 2, 4); + if (!isset($this->openFiles[$fileId])) { + return $this->ioError($requestId, self::FIO_EINVAL); + } + unset($this->openFiles[$fileId]); + return [self::MSG_CLOSE_OK, pack('n', $requestId)]; + + default: + // Writing, truncating, deleting, renaming and creating directories + return $this->ioError($requestId, self::FIO_EROFS); + } + } + + /** + * @return array{0: int, 1: string} + */ + private function ioError(int $requestId, int $errno): array { + return [self::MSG_IO_ERROR, pack('nN', $requestId, $errno)]; + } + + /** + * Unpacks a single value from $payload, which must be at least $minLength bytes long + */ + private function unpack(string $format, string $payload, int $offset, int $minLength): int { + if (strlen($payload) < $minLength) { + throw new \RuntimeException('Invalid message from ffmpeg-over-ip server'); + } + return unpack($format, $payload, $offset)[1]; + } + + /** + * @param resource $socket + * @return array{0: int, 1: string} type and payload + */ + private function readMessage($socket): array { + $header = unpack('Ctype/Nlength', $this->read($socket, 5)); + if ($header['length'] > self::MAX_MESSAGE_SIZE) { + throw new \RuntimeException('Message from ffmpeg-over-ip server is too large'); + } + return [$header['type'], $this->read($socket, $header['length'])]; + } + + /** + * @param resource $socket + */ + private function read($socket, int $length): string { + $data = ''; + while (strlen($data) < $length) { + $this->applyTimeout($socket); + $chunk = fread($socket, $length - strlen($data)); + if ($chunk === false || $chunk === '') { + if (stream_get_meta_data($socket)['timed_out']) { + throw new \RuntimeException('ffmpeg-over-ip server did not finish within ' . $this->timeout . ' seconds'); + } + if (feof($socket)) { + throw new \RuntimeException('ffmpeg-over-ip server closed the connection'); + } + continue; + } + $data .= $chunk; + } + return $data; + } + + /** + * @param resource $socket + */ + private function writeMessage($socket, int $type, string $payload): void { + $data = pack('CN', $type, strlen($payload)) . $payload; + while ($data !== '') { + $this->applyTimeout($socket); + $written = fwrite($socket, $data); + if ($written === false || $written === 0) { + throw new \RuntimeException('Could not write to ffmpeg-over-ip server'); + } + $data = substr($data, $written); + } + } + + /** + * @param resource $socket + */ + private function applyTimeout($socket): void { + $remaining = $this->deadline - microtime(true); + if ($remaining <= 0) { + throw new \RuntimeException('ffmpeg-over-ip server did not finish within ' . $this->timeout . ' seconds'); + } + stream_set_timeout($socket, (int)$remaining, (int)(($remaining - (int)$remaining) * 1000000)); + } +} diff --git a/lib/private/Preview/Movie.php b/lib/private/Preview/Movie.php index 3c4e75bf21b30..c60b926d7998d 100644 --- a/lib/private/Preview/Movie.php +++ b/lib/private/Preview/Movie.php @@ -18,6 +18,15 @@ use Psr\Log\LoggerInterface; class Movie extends ProviderV2 { + /** ffprobe arguments (without the input) that print the color transfer of the first video stream */ + public const HDR_PROBE_ARGS = ['-select_streams', 'v:0', '-show_entries', 'stream=color_transfer', '-of', 'default=noprint_wrappers=1:nokey=1']; + + /** + * Tonemaps HDR to SDR. Forces colorspace to '2020_ncl' because some videos are + * tagged incorrectly as 'reserved' resulting in fail if not forced. + */ + public const HDR_FILTER = 'zscale=min=2020_ncl:t=linear:npl=100,format=gbrpf32le,zscale=p=bt709,tonemap=tonemap=hable:desat=0,zscale=t=bt709:m=bt709:r=tv,format=yuv420p'; + private IConfig $config; private ?string $binary = null; @@ -281,10 +290,7 @@ private function useHdr(string $absPath): bool { // load ffprobe path from configuration, otherwise generate binary path using ffmpeg binary path $ffprobe_binary = $this->config->getSystemValue('preview_ffprobe_path', null) ?? (pathinfo($this->binary, PATHINFO_DIRNAME) . '/ffprobe'); // run ffprobe on the video file to get value of "color_transfer" - $test_hdr_cmd = [$ffprobe_binary,'-select_streams', 'v:0', - '-show_entries', 'stream=color_transfer', - '-of', 'default=noprint_wrappers=1:nokey=1', - $absPath]; + $test_hdr_cmd = [$ffprobe_binary, ...self::HDR_PROBE_ARGS, $absPath]; $test_hdr_proc = proc_open($test_hdr_cmd, [1 => ['pipe', 'w'], 2 => ['pipe', 'w']], $test_hdr_pipes); if ($test_hdr_proc === false) { return false; @@ -294,15 +300,19 @@ private function useHdr(string $absPath): bool { $test_hdr_stderr = trim(stream_get_contents($test_hdr_pipes[2])); $test_hdr_stdout = trim(stream_get_contents($test_hdr_pipes[1])); proc_close($test_hdr_proc); + return self::isHdr($test_hdr_stdout, $test_hdr_stderr); + } + + /** + * Whether the output of ffprobe with HDR_PROBE_ARGS describes an HDR video that can be tonemapped + */ + public static function isHdr(string $ffprobeStdout, string $ffprobeStderr): bool { // search build options for libzimg (provides zscale filter) - $ffmpeg_libzimg_installed = strpos($test_hdr_stderr, '--enable-libzimg'); + $ffmpeg_libzimg_installed = strpos($ffprobeStderr, '--enable-libzimg'); // Only values of "smpte2084" and "arib-std-b67" indicate an HDR video. // Only return true if video is detected as HDR and libzimg is installed. - if (($test_hdr_stdout === 'smpte2084' || $test_hdr_stdout === 'arib-std-b67') && $ffmpeg_libzimg_installed !== false) { - return true; - } else { - return false; - } + $colorTransfer = trim($ffprobeStdout); + return ($colorTransfer === 'smpte2084' || $colorTransfer === 'arib-std-b67') && $ffmpeg_libzimg_installed !== false; } private function generateThumbNail(int $maxX, int $maxY, string $absPath, int $second): ?IImage { @@ -319,12 +329,10 @@ private function generateThumbNail(int $maxX, int $maxY, string $absPath, int $s if ($binaryType === 'ffmpeg') { if ($this->useHdr($absPath)) { - // Force colorspace to '2020_ncl' because some videos are - // tagged incorrectly as 'reserved' resulting in fail if not forced. $cmd = [$this->binary, '-y', '-ss', (string)$second, '-i', $absPath, '-f', 'mjpeg', '-vframes', '1', - '-vf', 'zscale=min=2020_ncl:t=linear:npl=100,format=gbrpf32le,zscale=p=bt709,tonemap=tonemap=hable:desat=0,zscale=t=bt709:m=bt709:r=tv,format=yuv420p', + '-vf', self::HDR_FILTER, $tmpPath]; } else { // always default to generating preview using non-HDR command diff --git a/lib/private/Preview/MovieRemote.php b/lib/private/Preview/MovieRemote.php new file mode 100644 index 0000000000000..ed6656d7abe35 --- /dev/null +++ b/lib/private/Preview/MovieRemote.php @@ -0,0 +1,132 @@ +config = Server::get(IConfig::class); + $this->logger = Server::get(LoggerInterface::class); + } + + #[\Override] + public function getMimeType(): string { + return '/video\/.*/'; + } + + #[\Override] + public function getThumbnail(File $file, int $maxX, int $maxY): ?IImage { + $address = $this->config->getSystemValueString('preview_ffmpeg_url', ''); + if ($address === '') { + $this->logger->error('MovieRemote preview provider is enabled, but no url is configured. Please provide the address of your ffmpeg-over-ip server to the \'preview_ffmpeg_url\' config variable.'); + return null; + } + $client = $this->getClient($address, $this->config->getSystemValueString('preview_ffmpeg_key', '')); + + $stream = $this->getSeekableStream($file); + if ($stream === null) { + return null; + } + // The extension helps ffmpeg to detect the container format + $extension = preg_replace('/[^a-z0-9]/', '', strtolower($file->getExtension())); + $input = '/input' . ($extension !== '' ? '.' . $extension : ''); + $files = [$input => ['stream' => $stream, 'size' => (int)$file->getSize()]]; + + try { + $probe = $client->run(FFmpegOverIpClient::PROGRAM_FFPROBE, [...Movie::HDR_PROBE_ARGS, $input], $files); + $filter = Movie::isHdr($probe['stdout'], $probe['stderr']) ? ['-vf', Movie::HDR_FILTER] : []; + + foreach (self::TIME_ATTEMPTS as $second) { + $result = $client->run(FFmpegOverIpClient::PROGRAM_FFMPEG, [ + '-ss', (string)$second, + '-i', $input, + '-f', 'mjpeg', '-vframes', '1', + ...$filter, + 'pipe:1', + ], $files); + + if ($result['exitCode'] === 0 && $result['stdout'] !== '') { + $image = new Image(); + $image->loadFromData($result['stdout']); + if ($image->valid()) { + $image->scaleDownToFit($maxX, $maxY); + return $image; + } + } + $this->logger->debug('MovieRemote preview generation attempt failed, file=' . $file->getPath() . ', time=' . $second, [ + 'exitCode' => $result['exitCode'], + 'output' => $result['stderr'], + ]); + } + } catch (\RuntimeException $e) { + $this->logger->warning('MovieRemote preview generation failed: ' . $e->getMessage(), [ + 'exception' => $e, + ]); + return null; + } finally { + fclose($stream); + } + + $this->logger->error('MovieRemote preview generation process failed, file=' . $file->getPath()); + return null; + } + + protected function getClient(string $address, string $secret): FFmpegOverIpClient { + return new FFmpegOverIpClient($address, $secret); + } + + /** + * Opens the file, copying it to a temporary stream if the storage does not support seeking + * + * @return resource|null + */ + private function getSeekableStream(File $file) { + $stream = $file->fopen('r'); + if (!is_resource($stream)) { + return null; + } + // Stream wrappers always claim to be seekable, so seeking is tried instead + try { + if (stream_get_meta_data($stream)['seekable'] && fseek($stream, 1) === 0 && fseek($stream, 0) === 0) { + return $stream; + } + } catch (\Throwable) { + } + fclose($stream); + + $stream = $file->fopen('r'); + if (!is_resource($stream)) { + return null; + } + $copy = fopen('php://temp', 'r+'); + stream_copy_to_stream($stream, $copy); + fclose($stream); + rewind($copy); + return $copy; + } +} diff --git a/lib/private/PreviewManager.php b/lib/private/PreviewManager.php index 67268b1013a50..ca8b700abdc6d 100644 --- a/lib/private/PreviewManager.php +++ b/lib/private/PreviewManager.php @@ -31,6 +31,7 @@ use OC\Preview\Krita; use OC\Preview\MarkDown; use OC\Preview\Movie; +use OC\Preview\MovieRemote; use OC\Preview\MP3; use OC\Preview\MSOffice2003; use OC\Preview\MSOffice2007; @@ -375,6 +376,7 @@ protected function registerCoreProviders(): void { $this->registerCoreProvider(Movie::class, '/video\/.*/', ['movieBinary' => $movieBinary]); } } + $this->registerCoreProvider(MovieRemote::class, '/video\/.*/'); } private function registerCoreProvidersOffice(): void { diff --git a/lib/private/SystemConfig.php b/lib/private/SystemConfig.php index 312dfdeca2a93..331c300ec542a 100644 --- a/lib/private/SystemConfig.php +++ b/lib/private/SystemConfig.php @@ -42,6 +42,8 @@ class SystemConfig { 'trusted_proxies' => true, 'preview_imaginary_url' => true, 'preview_imaginary_key' => true, + 'preview_ffmpeg_url' => true, + 'preview_ffmpeg_key' => true, 'proxyuserpwd' => true, 'sentry.dsn' => true, 'sentry.public-dsn' => true, diff --git a/tests/lib/Preview/FFmpegOverIpClientTest.php b/tests/lib/Preview/FFmpegOverIpClientTest.php new file mode 100644 index 0000000000000..262de988235ea --- /dev/null +++ b/tests/lib/Preview/FFmpegOverIpClientTest.php @@ -0,0 +1,198 @@ +client, $this->server] = stream_socket_pair(STREAM_PF_UNIX, STREAM_SOCK_STREAM, STREAM_IPPROTO_IP); + } + + #[\Override] + protected function tearDown(): void { + foreach ([$this->client, $this->server] as $socket) { + if (is_resource($socket)) { + fclose($socket); + } + } + parent::tearDown(); + } + + private function serverSends(int $type, string $payload = ''): void { + fwrite($this->server, pack('CN', $type, strlen($payload)) . $payload); + } + + /** + * @return list + */ + private function clientMessages(): array { + stream_set_blocking($this->server, false); + $data = stream_get_contents($this->server); + $messages = []; + while ($data !== '') { + $header = unpack('Ctype/Nlength', $data); + $messages[] = [$header['type'], substr($data, 5, $header['length'])]; + $data = substr($data, 5 + $header['length']); + } + return $messages; + } + + private function communicate(array $files = [], int $timeout = 10): array { + $client = new FFmpegOverIpClient('ffmpeg-over-ip:5050', 'secret', $timeout); + return $client->communicate($this->client, FFmpegOverIpClient::PROGRAM_FFPROBE, ['-i', '/input.mp4'], $files); + } + + private function getFiles(string $content): array { + $stream = fopen('php://memory', 'r+'); + fwrite($stream, $content); + return ['/input.mp4' => ['stream' => $stream, 'size' => strlen($content)]]; + } + + public function testSendsSignedCommand(): void { + $this->serverSends(0x03, pack('N', 0)); + + $this->assertSame(['exitCode' => 0, 'stdout' => '', 'stderr' => ''], $this->communicate()); + + $messages = $this->clientMessages(); + $this->assertCount(2, $messages); + [$type, $payload] = $messages[0]; + $this->assertSame(0x01, $type); + $this->assertSame(0x06, ord($payload[0])); + $nonce = substr($payload, 1, 16); + $signature = substr($payload, 17, 32); + $programAndArgs = substr($payload, 49); + $this->assertSame("\x02" . pack('n', 2) . pack('n', 2) . '-i' . pack('n', 10) . '/input.mp4', $programAndArgs); + $expected = hash_hmac('sha256', "\x06" . $nonce . $programAndArgs, 'secret', true); + $this->assertSame(bin2hex($expected), bin2hex($signature)); + $this->assertSame([0x11, ''], $messages[1]); + } + + public function testServesRegisteredFile(): void { + $this->serverSends(0x20, pack('nnNn', 1, 3, 0, 0) . '/input.mp4'); + $this->serverSends(0x23, pack('nnJC', 2, 3, 6, 0)); + $this->serverSends(0x21, pack('nnN', 3, 3, 100)); + $this->serverSends(0x25, pack('nn', 4, 3)); + $this->serverSends(0x23, pack('nnJC', 5, 3, -5, 2)); + $this->serverSends(0x21, pack('nnN', 6, 3, 2)); + $this->serverSends(0x23, pack('nnJC', 7, 3, 1, 1)); + $this->serverSends(0x21, pack('nnN', 8, 3, 1)); + $this->serverSends(0x24, pack('nn', 9, 3)); + $this->serverSends(0x12, 'out'); + $this->serverSends(0x13, 'err'); + $this->serverSends(0x03, pack('N', 0)); + + $result = $this->communicate($this->getFiles('Hello world')); + + $this->assertSame(['exitCode' => 0, 'stdout' => 'out', 'stderr' => 'err'], $result); + $this->assertSame([ + [0x40, pack('nJ', 1, 11)], + [0x43, pack('nJ', 2, 6)], + [0x41, pack('n', 3) . 'world'], + [0x45, pack('nJN', 4, 11, 0o100644)], + [0x43, pack('nJ', 5, 6)], + [0x41, pack('n', 6) . 'wo'], + [0x43, pack('nJ', 7, 9)], + [0x41, pack('n', 8) . 'l'], + [0x44, pack('n', 9)], + ], array_slice($this->clientMessages(), 2)); + } + + public function testRejectsOtherFileAccess(): void { + $this->serverSends(0x20, pack('nnNn', 1, 1, 0, 0) . '/etc/passwd'); + $this->serverSends(0x20, pack('nnNn', 2, 1, 0x0001, 0) . '/input.mp4'); + $this->serverSends(0x20, pack('nnNn', 3, 1, 0x0040, 0644) . '/input.mp4'); + $this->serverSends(0x21, pack('nnN', 4, 1, 100)); + $this->serverSends(0x22, pack('nn', 5, 1) . 'data'); + $this->serverSends(0x27, pack('n', 6) . '/input.mp4'); + $this->serverSends(0x29, pack('nn', 7, 0755) . '/tmp'); + $this->serverSends(0x20, pack('nnNn', 8, 1, 0, 0) . '/input.mp4'); + $this->serverSends(0x20, pack('nnNn', 9, 1, 0, 0) . '/input.mp4'); + $this->serverSends(0x03, pack('N', 1)); + + $this->assertSame(1, $this->communicate($this->getFiles('video'))['exitCode']); + $this->assertSame([ + [0x4F, pack('nN', 1, 2)], + [0x4F, pack('nN', 2, 30)], + [0x4F, pack('nN', 3, 30)], + [0x4F, pack('nN', 4, 22)], + [0x4F, pack('nN', 5, 30)], + [0x4F, pack('nN', 6, 30)], + [0x4F, pack('nN', 7, 30)], + [0x40, pack('nJ', 8, 5)], + [0x4F, pack('nN', 9, 22)], + ], array_slice($this->clientMessages(), 2)); + } + + public function testAnswersPing(): void { + $this->serverSends(0x05, 'ping'); + $this->serverSends(0x03, pack('N', 0)); + + $this->communicate(); + $this->assertSame([0x06, 'ping'], $this->clientMessages()[2]); + } + + public function testNegativeExitCode(): void { + $this->serverSends(0x03, pack('N', 0xFFFFFFFF)); + + $this->assertSame(-1, $this->communicate()['exitCode']); + } + + public function testServerError(): void { + $this->serverSends(0x04, 'authentication failed'); + + $this->expectException(\RuntimeException::class); + $this->expectExceptionMessage('ffmpeg-over-ip server error: authentication failed'); + $this->communicate(); + } + + public function testConnectionClosed(): void { + $this->serverSends(0x12, 'partial'); + stream_socket_shutdown($this->server, STREAM_SHUT_WR); + + $this->expectException(\RuntimeException::class); + $this->expectExceptionMessage('ffmpeg-over-ip server closed the connection'); + $this->communicate(); + } + + public function testMessageTooLarge(): void { + fwrite($this->server, pack('CN', 0x12, 2 * 1024 * 1024)); + + $this->expectException(\RuntimeException::class); + $this->expectExceptionMessage('Message from ffmpeg-over-ip server is too large'); + $this->communicate(); + } + + public function testTimeout(): void { + $this->expectException(\RuntimeException::class); + $this->expectExceptionMessage('ffmpeg-over-ip server did not finish within 1 seconds'); + $this->communicate([], 1); + } + + public function testUnreachable(): void { + $client = new FFmpegOverIpClient('unix:/nonexistent/ffmpeg-over-ip.sock', 'secret'); + + $this->expectException(\RuntimeException::class); + $this->expectExceptionMessage('Could not connect to ffmpeg-over-ip server unix:/nonexistent/ffmpeg-over-ip.sock'); + $client->run(FFmpegOverIpClient::PROGRAM_FFMPEG, ['-version'], []); + } +} diff --git a/tests/lib/Preview/MovieRemoteTest.php b/tests/lib/Preview/MovieRemoteTest.php new file mode 100644 index 0000000000000..bd557848526f8 --- /dev/null +++ b/tests/lib/Preview/MovieRemoteTest.php @@ -0,0 +1,191 @@ +, 2: array}> */ + private array $calls = []; + /** @var list */ + private array $results = []; + /** @var ?callable called with the files passed to each run */ + private $onRun = null; + + #[\Override] + protected function setUp(): void { + parent::setUp(); + + $this->overwriteSystemConfig('preview_ffmpeg_url', 'ffmpeg-over-ip:5050'); + $this->overwriteSystemConfig('preview_ffmpeg_key', 'secret'); + $this->imgPath = $this->prepareTestFile($this->fileName, \OC::$SERVERROOT . '/tests/data/' . $this->fileName); + + $client = $this->createMock(FFmpegOverIpClient::class); + $client->method('run')->willReturnCallback(function (int $program, array $args, array $files): array { + $this->calls[] = [$program, $args, $files]; + if ($this->onRun !== null) { + ($this->onRun)($files); + } + return array_shift($this->results) ?? $this->getResult($program === FFmpegOverIpClient::PROGRAM_FFPROBE ? "bt709\n" : $this->getJpeg()); + }); + + $this->provider = new class($client) extends MovieRemote { + public array $credentials = []; + + public function __construct( + private FFmpegOverIpClient $client, + ) { + parent::__construct(); + } + + #[\Override] + protected function getClient(string $address, string $secret): FFmpegOverIpClient { + $this->credentials = [$address, $secret]; + return $this->client; + } + }; + } + + private function getResult(string $stdout, int $exitCode = 0, string $stderr = ''): array { + return ['exitCode' => $exitCode, 'stdout' => $stdout, 'stderr' => $stderr]; + } + + private function getJpeg(): string { + return file_get_contents(\OC::$SERVERROOT . '/tests/data/testimage.jpg'); + } + + private function getThumbnail(?File $file = null): ?\OCP\IImage { + $file ??= new FileNode(Server::get(IRootFolder::class), $this->rootView, $this->imgPath); + return $this->provider->getThumbnail($file, 256, 256); + } + + public function testRunsFfprobeAndFfmpeg(): void { + $preview = $this->getThumbnail(); + + $this->assertNotNull($preview); + $this->assertSame(256, $preview->width()); + $this->assertSame(160, $preview->height()); + $this->assertSame(['ffmpeg-over-ip:5050', 'secret'], $this->provider->credentials); + + $this->assertCount(2, $this->calls); + $this->assertSame(FFmpegOverIpClient::PROGRAM_FFPROBE, $this->calls[0][0]); + $this->assertSame([...Movie::HDR_PROBE_ARGS, '/input.mp4'], $this->calls[0][1]); + $this->assertSame(FFmpegOverIpClient::PROGRAM_FFMPEG, $this->calls[1][0]); + $this->assertSame(['-ss', '5', '-i', '/input.mp4', '-f', 'mjpeg', '-vframes', '1', 'pipe:1'], $this->calls[1][1]); + $this->assertSame(['/input.mp4'], array_keys($this->calls[1][2])); + $this->assertSame(filesize(\OC::$SERVERROOT . '/tests/data/' . $this->fileName), $this->calls[1][2]['/input.mp4']['size']); + } + + public function testHdr(): void { + $this->results[] = $this->getResult("smpte2084\n", 0, 'configuration: --enable-libzimg'); + + $this->assertNotNull($this->getThumbnail()); + $this->assertSame(['-ss', '5', '-i', '/input.mp4', '-f', 'mjpeg', '-vframes', '1', '-vf', Movie::HDR_FILTER, 'pipe:1'], $this->calls[1][1]); + } + + public function testHdrWithoutZscale(): void { + $this->results[] = $this->getResult("smpte2084\n"); + + $this->assertNotNull($this->getThumbnail()); + $this->assertNotContains('-vf', $this->calls[1][1]); + } + + public function testTriesNextTimestamp(): void { + $this->results = [$this->getResult(''), $this->getResult('', 1), $this->getResult('not an image')]; + + $this->assertNotNull($this->getThumbnail()); + $this->assertSame(['5', '1', '0'], array_map(fn (array $call): string => $call[1][1], array_slice($this->calls, 1))); + } + + public function testAllAttemptsFail(): void { + $this->results = [$this->getResult(''), $this->getResult('', 1), $this->getResult('', 1), $this->getResult('', 1)]; + + $this->assertNull($this->getThumbnail()); + $this->assertCount(4, $this->calls); + } + + public function testServerFails(): void { + $this->onRun = function (): void { + throw new \RuntimeException('Could not connect'); + }; + + $this->assertNull($this->getThumbnail()); + $this->assertCount(1, $this->calls); + } + + public function testNoUrlConfigured(): void { + $this->overwriteSystemConfig('preview_ffmpeg_url', ''); + + $this->assertNull($this->getThumbnail()); + $this->assertSame([], $this->calls); + } + + public static function streamProvider(): array { + return [ + 'seekable stream wrapper (S3, SMB, SFTP, encryption)' => [ + fn (string $content) => CallbackWrapper::wrap(Utils::tryFopen('data://text/plain,' . $content, 'r'), fn () => null), + false, + ], + 'stream wrapper that cannot seek (WebDAV)' => [ + fn (string $content) => StreamWrapper::getResource(new NoSeekStream(Utils::streamFor($content))), + true, + ], + 'native stream that cannot seek (FTP)' => [ + function (string $content) { + [$read, $write] = stream_socket_pair(STREAM_PF_UNIX, STREAM_SOCK_STREAM, STREAM_IPPROTO_IP); + fwrite($write, $content); + fclose($write); + return $read; + }, + true, + ], + ]; + } + + #[\PHPUnit\Framework\Attributes\DataProvider('streamProvider')] + public function testStorageStreams(callable $fopen, bool $copied): void { + $content = 'video content'; + $opened = []; + $file = $this->createMock(File::class); + $file->method('fopen')->willReturnCallback(function () use ($fopen, $content, &$opened) { + return $opened[] = $fopen($content); + }); + $file->method('getSize')->willReturn(strlen($content)); + $file->method('getExtension')->willReturn('MP4'); + $file->method('getPath')->willReturn('/user/files/video.mp4'); + + $this->onRun = function (array $files) use ($content, $copied, &$opened): void { + $stream = $files['/input.mp4']['stream']; + $this->assertSame($copied, !in_array($stream, $opened, true)); + fseek($stream, 6); + $this->assertSame(substr($content, 6), stream_get_contents($stream)); + fseek($stream, 0); + $this->assertSame($content, stream_get_contents($stream)); + }; + + $this->assertNotNull($this->getThumbnail($file)); + $this->assertCount(2, $this->calls); + } +} From b0c6b2a51d7972044eac606c83de3e488abe78b3 Mon Sep 17 00:00:00 2001 From: "Simon L." Date: Wed, 4 Nov 2026 14:51:06 +0100 Subject: [PATCH 2/4] fix(preview): address review feedback for the MovieRemote provider - Only download the first 10 MB of videos on storages that cannot seek, using ProviderV2::getLocalFile() like the Movie provider, and send the real size of that file - Allow tls:// addresses with peer verification and reject http(s) urls with a clear error - Apply one configurable deadline (preview_ffmpeg_timeout) to all runs of a preview instead of 120 seconds per run - Fall back to non-HDR previews if ffprobe fails - Reject seeks that would overflow the file offset - Report the provider as unavailable without preview_ffmpeg_address - Rename preview_ffmpeg_url to preview_ffmpeg_address, as it is no url - Share the timestamps and ffmpeg arguments with the Movie provider Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 Signed-off-by: Simon L. --- config/config.sample.php | 30 +++++-- lib/private/Preview/FFmpegOverIpClient.php | 44 +++++++--- lib/private/Preview/Movie.php | 35 ++++---- lib/private/Preview/MovieRemote.php | 85 +++++++++++--------- lib/private/SystemConfig.php | 2 +- tests/lib/Preview/FFmpegOverIpClientTest.php | 56 +++++++++++-- tests/lib/Preview/MovieRemoteTest.php | 71 ++++++++++++---- 7 files changed, 232 insertions(+), 91 deletions(-) diff --git a/config/config.sample.php b/config/config.sample.php index 2246ef38bb06b..3a694990fd701 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -1634,24 +1634,40 @@ 'preview_imaginary_key' => 'secret', /** - * Set the address (``host:port``) of an ffmpeg-over-ip server to generate - * video previews on. Also requires the ``OC\Preview\MovieRemote`` provider - * to be enabled in the ``enabledPreviewProviders`` array. Neither ffmpeg nor - * the ffmpeg-over-ip client is needed on the Nextcloud server in that case. + * Set the address of an ffmpeg-over-ip server to generate video previews on, + * as ``host:port``, ``tls://host:port`` or ``unix:/path/to/socket``. This is + * not an HTTP URL. Also requires the ``OC\Preview\MovieRemote`` provider to + * be enabled in the ``enabledPreviewProviders`` array. Neither ffmpeg nor the + * ffmpeg-over-ip client is needed on the Nextcloud server in that case. * * The server reads the videos from Nextcloud over the same connection, so - * it needs no access to the data directory. The connection is not - * encrypted, so only use it within a trusted network. + * it needs no access to the data directory. ffmpeg-over-ip itself does not + * encrypt the connection and does not authenticate the server, so anyone + * answering on this address can read the videos. Either only use it within + * a trusted network, or put a TLS terminating proxy (e.g. stunnel) in front + * of the server and use a ``tls://`` address, whose certificate is verified + * against the system and imported certificates. + * + * Videos on storages that cannot seek (e.g. WebDAV or FTP external storage) + * are only sent partially, like with ``OC\Preview\Movie``. * * See https://github.com/steelbrain/ffmpeg-over-ip */ - 'preview_ffmpeg_url' => 'ffmpeg-over-ip:5050', + 'preview_ffmpeg_address' => 'ffmpeg-over-ip:5050', /** * The ``authSecret`` configured on the ffmpeg-over-ip server. */ 'preview_ffmpeg_key' => 'secret', + /** + * Seconds generating a single video preview on the ffmpeg-over-ip server + * may take in total. + * + * Defaults to ``120`` + */ + 'preview_ffmpeg_timeout' => 120, + /** * Only register providers that have been explicitly enabled * diff --git a/lib/private/Preview/FFmpegOverIpClient.php b/lib/private/Preview/FFmpegOverIpClient.php index 50b032bfb13d2..bf919faaa0ba2 100644 --- a/lib/private/Preview/FFmpegOverIpClient.php +++ b/lib/private/Preview/FFmpegOverIpClient.php @@ -66,18 +66,21 @@ class FFmpegOverIpClient { private array $files = []; /** @var array */ private array $openFiles = []; - private float $deadline = 0; + private float $deadline; /** - * @param string $address ``host:port``, ``tcp://host:port`` or ``unix:/path/to/socket`` - * @param int $timeout seconds a single run() may take + * @param string $address ``host:port``, ``tcp://host:port``, ``tls://host:port`` or ``unix:/path/to/socket`` + * @param int $timeout seconds all runs of this client may take together + * @param ?string $caFile certificates to verify the server with for ``tls://`` addresses */ public function __construct( private string $address, private string $secret, private int $timeout = 120, + private ?string $caFile = null, private int $connectTimeout = 5, ) { + $this->deadline = microtime(true) + $timeout; } /** @@ -87,7 +90,12 @@ public function __construct( * @throws \RuntimeException if the server is unreachable, rejects the command or violates the protocol */ public function run(int $program, array $args, array $files): array { - $socket = @stream_socket_client($this->getRemoteSocket(), $errno, $errstr, $this->connectTimeout); + $remoteSocket = $this->getRemoteSocket(); + $ssl = ['verify_peer' => true, 'verify_peer_name' => true]; + if ($this->caFile !== null) { + $ssl['cafile'] = $this->caFile; + } + $socket = @stream_socket_client($remoteSocket, $errno, $errstr, min($this->connectTimeout, $this->getRemainingTime()), STREAM_CLIENT_CONNECT, stream_context_create(['ssl' => $ssl])); if ($socket === false) { throw new \RuntimeException('Could not connect to ffmpeg-over-ip server ' . $this->address . ': ' . $errstr); } @@ -111,7 +119,6 @@ public function run(int $program, array $args, array $files): array { public function communicate($socket, int $program, array $args, array $files): array { $this->files = $files; $this->openFiles = []; - $this->deadline = microtime(true) + $this->timeout; $stdout = ''; $stderr = ''; @@ -159,10 +166,24 @@ private function getRemoteSocket(): string { if (str_starts_with($address, 'unix:')) { return 'unix://' . preg_replace('/^unix:(\/\/)?/', '', $address); } - if (str_starts_with($address, 'tcp://')) { - return $address; + if (preg_match('/^([a-z][a-z0-9+.-]*):\/\//i', $address, $matches) !== 1) { + return 'tcp://' . $address; + } + if (!in_array(strtolower($matches[1]), ['tcp', 'tls', 'ssl'], true)) { + throw new \RuntimeException('Invalid ffmpeg-over-ip address "' . $this->address . '", expected host:port, tls://host:port or unix:/path/to/socket'); + } + return $address; + } + + /** + * @throws \RuntimeException if the timeout is exceeded + */ + private function getRemainingTime(): float { + $remaining = $this->deadline - microtime(true); + if ($remaining <= 0) { + throw new \RuntimeException('ffmpeg-over-ip server did not finish within ' . $this->timeout . ' seconds'); } - return 'tcp://' . $address; + return $remaining; } private function encodeCommand(int $program, array $args): string { @@ -234,7 +255,7 @@ private function handleFileRequest(int $type, string $payload): array { self::FIO_SEEK_END => $this->files[$file['path']]['size'], default => null, }; - if ($base === null || $base + $offset < 0) { + if ($base === null || $offset > PHP_INT_MAX - $base || $base + $offset < 0) { return $this->ioError($requestId, self::FIO_EINVAL); } $file['offset'] = $base + $offset; @@ -331,10 +352,7 @@ private function writeMessage($socket, int $type, string $payload): void { * @param resource $socket */ private function applyTimeout($socket): void { - $remaining = $this->deadline - microtime(true); - if ($remaining <= 0) { - throw new \RuntimeException('ffmpeg-over-ip server did not finish within ' . $this->timeout . ' seconds'); - } + $remaining = $this->getRemainingTime(); stream_set_timeout($socket, (int)$remaining, (int)(($remaining - (int)$remaining) * 1000000)); } } diff --git a/lib/private/Preview/Movie.php b/lib/private/Preview/Movie.php index c60b926d7998d..f695080f099b4 100644 --- a/lib/private/Preview/Movie.php +++ b/lib/private/Preview/Movie.php @@ -18,6 +18,9 @@ use Psr\Log\LoggerInterface; class Movie extends ProviderV2 { + /** Timestamps (in seconds) to make attempts to generate a still */ + public const TIME_ATTEMPTS = [5, 1, 0]; + /** ffprobe arguments (without the input) that print the color transfer of the first video stream */ public const HDR_PROBE_ARGS = ['-select_streams', 'v:0', '-show_entries', 'stream=color_transfer', '-of', 'default=noprint_wrappers=1:nokey=1']; @@ -86,8 +89,7 @@ public function getThumbnail(File $file, int $maxX, int $maxY): ?IImage { $connectDirect = $this->connectDirect($file); - // Timestamps to make attempts to generate a still - $timeAttempts = [5, 1, 0]; + $timeAttempts = self::TIME_ATTEMPTS; // If HTTP/HTTPS direct connect is not available or if the file is encrypted, // process normally @@ -315,6 +317,21 @@ public static function isHdr(string $ffprobeStdout, string $ffprobeStderr): bool return ($colorTransfer === 'smpte2084' || $colorTransfer === 'arib-std-b67') && $ffmpeg_libzimg_installed !== false; } + /** + * ffmpeg arguments that write a single still of $input at $second as JPEG to $output + * + * @return list + */ + public static function getStillArgs(string $input, int $second, bool $hdr, string $output): array { + return [ + '-y', '-ss', (string)$second, + '-i', $input, + '-f', 'mjpeg', '-vframes', '1', + ...($hdr ? ['-vf', self::HDR_FILTER] : []), + $output, + ]; + } + private function generateThumbNail(int $maxX, int $maxY, string $absPath, int $second): ?IImage { $tmpPath = Server::get(ITempManager::class)->getTemporaryFile(); if ($tmpPath === false) { @@ -328,19 +345,7 @@ private function generateThumbNail(int $maxX, int $maxY, string $absPath, int $s $binaryType = substr(strrchr($this->binary, '/'), 1); if ($binaryType === 'ffmpeg') { - if ($this->useHdr($absPath)) { - $cmd = [$this->binary, '-y', '-ss', (string)$second, - '-i', $absPath, - '-f', 'mjpeg', '-vframes', '1', - '-vf', self::HDR_FILTER, - $tmpPath]; - } else { - // always default to generating preview using non-HDR command - $cmd = [$this->binary, '-y', '-ss', (string)$second, - '-i', $absPath, - '-f', 'mjpeg', '-vframes', '1', - $tmpPath]; - } + $cmd = [$this->binary, ...self::getStillArgs($absPath, $second, $this->useHdr($absPath), $tmpPath)]; } else { // Not supported unlink($tmpPath); diff --git a/lib/private/Preview/MovieRemote.php b/lib/private/Preview/MovieRemote.php index ed6656d7abe35..ab561a9f61421 100644 --- a/lib/private/Preview/MovieRemote.php +++ b/lib/private/Preview/MovieRemote.php @@ -10,6 +10,8 @@ namespace OC\Preview; use OCP\Files\File; +use OCP\Files\FileInfo; +use OCP\ICertificateManager; use OCP\IConfig; use OCP\IImage; use OCP\Image; @@ -22,8 +24,8 @@ * @see https://github.com/steelbrain/ffmpeg-over-ip */ class MovieRemote extends ProviderV2 { - /** Timestamps (in seconds) to make attempts to generate a still */ - private const TIME_ATTEMPTS = [5, 1, 0]; + /** Bytes downloaded of videos on storages that cannot seek, matching the first attempt of Movie */ + private const NON_SEEKABLE_SIZE = 10 * 1024 * 1024; private IConfig $config; private LoggerInterface $logger; @@ -39,36 +41,35 @@ public function getMimeType(): string { return '/video\/.*/'; } + #[\Override] + public function isAvailable(FileInfo $file): bool { + return $this->config->getSystemValueString('preview_ffmpeg_address', '') !== ''; + } + #[\Override] public function getThumbnail(File $file, int $maxX, int $maxY): ?IImage { - $address = $this->config->getSystemValueString('preview_ffmpeg_url', ''); - if ($address === '') { - $this->logger->error('MovieRemote preview provider is enabled, but no url is configured. Please provide the address of your ffmpeg-over-ip server to the \'preview_ffmpeg_url\' config variable.'); + if (!$this->isAvailable($file)) { return null; } - $client = $this->getClient($address, $this->config->getSystemValueString('preview_ffmpeg_key', '')); - $stream = $this->getSeekableStream($file); - if ($stream === null) { + $input = $this->getInput($file); + if ($input === null) { return null; } - // The extension helps ffmpeg to detect the container format $extension = preg_replace('/[^a-z0-9]/', '', strtolower($file->getExtension())); - $input = '/input' . ($extension !== '' ? '.' . $extension : ''); - $files = [$input => ['stream' => $stream, 'size' => (int)$file->getSize()]]; + $path = '/input' . ($extension !== '' ? '.' . $extension : ''); + $files = [$path => $input]; try { - $probe = $client->run(FFmpegOverIpClient::PROGRAM_FFPROBE, [...Movie::HDR_PROBE_ARGS, $input], $files); - $filter = Movie::isHdr($probe['stdout'], $probe['stderr']) ? ['-vf', Movie::HDR_FILTER] : []; - - foreach (self::TIME_ATTEMPTS as $second) { - $result = $client->run(FFmpegOverIpClient::PROGRAM_FFMPEG, [ - '-ss', (string)$second, - '-i', $input, - '-f', 'mjpeg', '-vframes', '1', - ...$filter, - 'pipe:1', - ], $files); + $client = $this->getClient( + $this->config->getSystemValueString('preview_ffmpeg_address'), + $this->config->getSystemValueString('preview_ffmpeg_key', ''), + $this->config->getSystemValueInt('preview_ffmpeg_timeout', 120), + ); + $hdr = $this->useHdr($client, $path, $files); + + foreach (Movie::TIME_ATTEMPTS as $second) { + $result = $client->run(FFmpegOverIpClient::PROGRAM_FFMPEG, Movie::getStillArgs($path, $second, $hdr, 'pipe:1'), $files); if ($result['exitCode'] === 0 && $result['stdout'] !== '') { $image = new Image(); @@ -89,23 +90,37 @@ public function getThumbnail(File $file, int $maxX, int $maxY): ?IImage { ]); return null; } finally { - fclose($stream); + fclose($input['stream']); + $this->cleanTmpFiles(); } $this->logger->error('MovieRemote preview generation process failed, file=' . $file->getPath()); return null; } - protected function getClient(string $address, string $secret): FFmpegOverIpClient { - return new FFmpegOverIpClient($address, $secret); + protected function getClient(string $address, string $secret, int $timeout): FFmpegOverIpClient { + return new FFmpegOverIpClient($address, $secret, $timeout, Server::get(ICertificateManager::class)->getAbsoluteBundlePath()); + } + + /** + * @param array $files + */ + private function useHdr(FFmpegOverIpClient $client, string $path, array $files): bool { + try { + $probe = $client->run(FFmpegOverIpClient::PROGRAM_FFPROBE, [...Movie::HDR_PROBE_ARGS, $path], $files); + } catch (\RuntimeException $e) { + $this->logger->debug('MovieRemote HDR detection failed: ' . $e->getMessage(), ['exception' => $e]); + return false; + } + return Movie::isHdr($probe['stdout'], $probe['stderr']); } /** - * Opens the file, copying it to a temporary stream if the storage does not support seeking + * Opens the video, downloading only its beginning to a temporary file if the storage cannot seek * - * @return resource|null + * @return array{stream: resource, size: int}|null */ - private function getSeekableStream(File $file) { + private function getInput(File $file): ?array { $stream = $file->fopen('r'); if (!is_resource($stream)) { return null; @@ -113,20 +128,18 @@ private function getSeekableStream(File $file) { // Stream wrappers always claim to be seekable, so seeking is tried instead try { if (stream_get_meta_data($stream)['seekable'] && fseek($stream, 1) === 0 && fseek($stream, 0) === 0) { - return $stream; + return ['stream' => $stream, 'size' => (int)$file->getSize()]; } } catch (\Throwable) { } fclose($stream); - $stream = $file->fopen('r'); - if (!is_resource($stream)) { + $localPath = $this->getLocalFile($file, self::NON_SEEKABLE_SIZE); + $stream = $localPath !== false ? fopen($localPath, 'r') : false; + if ($stream === false) { + $this->cleanTmpFiles(); return null; } - $copy = fopen('php://temp', 'r+'); - stream_copy_to_stream($stream, $copy); - fclose($stream); - rewind($copy); - return $copy; + return ['stream' => $stream, 'size' => (int)filesize($localPath)]; } } diff --git a/lib/private/SystemConfig.php b/lib/private/SystemConfig.php index 331c300ec542a..01f5cd195d8b6 100644 --- a/lib/private/SystemConfig.php +++ b/lib/private/SystemConfig.php @@ -42,7 +42,7 @@ class SystemConfig { 'trusted_proxies' => true, 'preview_imaginary_url' => true, 'preview_imaginary_key' => true, - 'preview_ffmpeg_url' => true, + 'preview_ffmpeg_address' => true, 'preview_ffmpeg_key' => true, 'proxyuserpwd' => true, 'sentry.dsn' => true, diff --git a/tests/lib/Preview/FFmpegOverIpClientTest.php b/tests/lib/Preview/FFmpegOverIpClientTest.php index 262de988235ea..1b235bac008dc 100644 --- a/tests/lib/Preview/FFmpegOverIpClientTest.php +++ b/tests/lib/Preview/FFmpegOverIpClientTest.php @@ -57,8 +57,8 @@ private function clientMessages(): array { return $messages; } - private function communicate(array $files = [], int $timeout = 10): array { - $client = new FFmpegOverIpClient('ffmpeg-over-ip:5050', 'secret', $timeout); + private function communicate(array $files = [], int $timeout = 10, ?FFmpegOverIpClient $client = null): array { + $client ??= new FFmpegOverIpClient('ffmpeg-over-ip:5050', 'secret', $timeout); return $client->communicate($this->client, FFmpegOverIpClient::PROGRAM_FFPROBE, ['-i', '/input.mp4'], $files); } @@ -143,6 +143,22 @@ public function testRejectsOtherFileAccess(): void { ], array_slice($this->clientMessages(), 2)); } + public function testRejectsSeekOverflow(): void { + $this->serverSends(0x20, pack('nnNn', 1, 1, 0, 0) . '/input.mp4'); + $this->serverSends(0x23, pack('nnJC', 2, 1, PHP_INT_MAX, 2)); + $this->serverSends(0x23, pack('nnJC', 3, 1, -6, 2)); + $this->serverSends(0x21, pack('nnN', 4, 1, 100)); + $this->serverSends(0x03, pack('N', 0)); + + $this->communicate($this->getFiles('video')); + $this->assertSame([ + [0x40, pack('nJ', 1, 5)], + [0x4F, pack('nN', 2, 22)], + [0x4F, pack('nN', 3, 22)], + [0x41, pack('n', 4) . 'video'], + ], array_slice($this->clientMessages(), 2)); + } + public function testAnswersPing(): void { $this->serverSends(0x05, 'ping'); $this->serverSends(0x03, pack('N', 0)); @@ -188,11 +204,41 @@ public function testTimeout(): void { $this->communicate([], 1); } - public function testUnreachable(): void { - $client = new FFmpegOverIpClient('unix:/nonexistent/ffmpeg-over-ip.sock', 'secret'); + public function testTimeoutCoversAllRuns(): void { + $client = new FFmpegOverIpClient('ffmpeg-over-ip:5050', 'secret', 1); + $this->serverSends(0x03, pack('N', 0)); + $this->communicate([], 1, $client); + usleep(1100000); + $this->serverSends(0x03, pack('N', 0)); + + $this->expectException(\RuntimeException::class); + $this->expectExceptionMessage('ffmpeg-over-ip server did not finish within 1 seconds'); + $this->communicate([], 1, $client); + } + + public static function invalidAddressProvider(): array { + return [['http://ffmpeg-over-ip:5050'], ['https://ffmpeg-over-ip:5050/']]; + } + + #[\PHPUnit\Framework\Attributes\DataProvider('invalidAddressProvider')] + public function testInvalidAddress(string $address): void { + $client = new FFmpegOverIpClient($address, 'secret'); + + $this->expectException(\RuntimeException::class); + $this->expectExceptionMessage('Invalid ffmpeg-over-ip address "' . $address . '", expected host:port, tls://host:port or unix:/path/to/socket'); + $client->run(FFmpegOverIpClient::PROGRAM_FFMPEG, ['-version'], []); + } + + public static function unreachableAddressProvider(): array { + return [['127.0.0.1:1'], ['tcp://127.0.0.1:1'], ['tls://127.0.0.1:1'], ['unix:/nonexistent/ffmpeg-over-ip.sock']]; + } + + #[\PHPUnit\Framework\Attributes\DataProvider('unreachableAddressProvider')] + public function testUnreachable(string $address): void { + $client = new FFmpegOverIpClient($address, 'secret'); $this->expectException(\RuntimeException::class); - $this->expectExceptionMessage('Could not connect to ffmpeg-over-ip server unix:/nonexistent/ffmpeg-over-ip.sock'); + $this->expectExceptionMessage('Could not connect to ffmpeg-over-ip server ' . $address); $client->run(FFmpegOverIpClient::PROGRAM_FFMPEG, ['-version'], []); } } diff --git a/tests/lib/Preview/MovieRemoteTest.php b/tests/lib/Preview/MovieRemoteTest.php index bd557848526f8..92bd6783a65cd 100644 --- a/tests/lib/Preview/MovieRemoteTest.php +++ b/tests/lib/Preview/MovieRemoteTest.php @@ -38,7 +38,7 @@ class MovieRemoteTest extends Provider { protected function setUp(): void { parent::setUp(); - $this->overwriteSystemConfig('preview_ffmpeg_url', 'ffmpeg-over-ip:5050'); + $this->overwriteSystemConfig('preview_ffmpeg_address', 'ffmpeg-over-ip:5050'); $this->overwriteSystemConfig('preview_ffmpeg_key', 'secret'); $this->imgPath = $this->prepareTestFile($this->fileName, \OC::$SERVERROOT . '/tests/data/' . $this->fileName); @@ -61,8 +61,8 @@ public function __construct( } #[\Override] - protected function getClient(string $address, string $secret): FFmpegOverIpClient { - $this->credentials = [$address, $secret]; + protected function getClient(string $address, string $secret, int $timeout): FFmpegOverIpClient { + $this->credentials = [$address, $secret, $timeout]; return $this->client; } }; @@ -87,13 +87,13 @@ public function testRunsFfprobeAndFfmpeg(): void { $this->assertNotNull($preview); $this->assertSame(256, $preview->width()); $this->assertSame(160, $preview->height()); - $this->assertSame(['ffmpeg-over-ip:5050', 'secret'], $this->provider->credentials); + $this->assertSame(['ffmpeg-over-ip:5050', 'secret', 120], $this->provider->credentials); $this->assertCount(2, $this->calls); $this->assertSame(FFmpegOverIpClient::PROGRAM_FFPROBE, $this->calls[0][0]); $this->assertSame([...Movie::HDR_PROBE_ARGS, '/input.mp4'], $this->calls[0][1]); $this->assertSame(FFmpegOverIpClient::PROGRAM_FFMPEG, $this->calls[1][0]); - $this->assertSame(['-ss', '5', '-i', '/input.mp4', '-f', 'mjpeg', '-vframes', '1', 'pipe:1'], $this->calls[1][1]); + $this->assertSame(['-y', '-ss', '5', '-i', '/input.mp4', '-f', 'mjpeg', '-vframes', '1', 'pipe:1'], $this->calls[1][1]); $this->assertSame(['/input.mp4'], array_keys($this->calls[1][2])); $this->assertSame(filesize(\OC::$SERVERROOT . '/tests/data/' . $this->fileName), $this->calls[1][2]['/input.mp4']['size']); } @@ -102,7 +102,7 @@ public function testHdr(): void { $this->results[] = $this->getResult("smpte2084\n", 0, 'configuration: --enable-libzimg'); $this->assertNotNull($this->getThumbnail()); - $this->assertSame(['-ss', '5', '-i', '/input.mp4', '-f', 'mjpeg', '-vframes', '1', '-vf', Movie::HDR_FILTER, 'pipe:1'], $this->calls[1][1]); + $this->assertSame(['-y', '-ss', '5', '-i', '/input.mp4', '-f', 'mjpeg', '-vframes', '1', '-vf', Movie::HDR_FILTER, 'pipe:1'], $this->calls[1][1]); } public function testHdrWithoutZscale(): void { @@ -112,11 +112,23 @@ public function testHdrWithoutZscale(): void { $this->assertNotContains('-vf', $this->calls[1][1]); } + public function testProbeFails(): void { + $this->onRun = function (): void { + if (count($this->calls) === 1) { + throw new \RuntimeException('ffprobe not found'); + } + }; + + $this->assertNotNull($this->getThumbnail()); + $this->assertCount(2, $this->calls); + $this->assertNotContains('-vf', $this->calls[1][1]); + } + public function testTriesNextTimestamp(): void { $this->results = [$this->getResult(''), $this->getResult('', 1), $this->getResult('not an image')]; $this->assertNotNull($this->getThumbnail()); - $this->assertSame(['5', '1', '0'], array_map(fn (array $call): string => $call[1][1], array_slice($this->calls, 1))); + $this->assertSame(['5', '1', '0'], array_map(fn (array $call): string => $call[1][2], array_slice($this->calls, 1))); } public function testAllAttemptsFail(): void { @@ -132,12 +144,20 @@ public function testServerFails(): void { }; $this->assertNull($this->getThumbnail()); - $this->assertCount(1, $this->calls); + $this->assertCount(2, $this->calls); + } + + public function testTimeoutConfig(): void { + $this->overwriteSystemConfig('preview_ffmpeg_timeout', 30); + + $this->assertNotNull($this->getThumbnail()); + $this->assertSame(30, $this->provider->credentials[2]); } public function testNoUrlConfigured(): void { - $this->overwriteSystemConfig('preview_ffmpeg_url', ''); + $this->overwriteSystemConfig('preview_ffmpeg_address', ''); + $this->assertFalse($this->provider->isAvailable($this->createMock(File::class))); $this->assertNull($this->getThumbnail()); $this->assertSame([], $this->calls); } @@ -164,21 +184,31 @@ function (string $content) { ]; } - #[\PHPUnit\Framework\Attributes\DataProvider('streamProvider')] - public function testStorageStreams(callable $fopen, bool $copied): void { - $content = 'video content'; - $opened = []; + private function getStorageFile(callable $fopen, string $content, int $cachedSize, array &$opened): File { + $storage = $this->createMock(\OCP\Files\Storage\IStorage::class); + $storage->method('isLocal')->willReturn(false); $file = $this->createMock(File::class); $file->method('fopen')->willReturnCallback(function () use ($fopen, $content, &$opened) { return $opened[] = $fopen($content); }); - $file->method('getSize')->willReturn(strlen($content)); + $file->method('getSize')->willReturn($cachedSize); + $file->method('getStorage')->willReturn($storage); $file->method('getExtension')->willReturn('MP4'); $file->method('getPath')->willReturn('/user/files/video.mp4'); + return $file; + } + + #[\PHPUnit\Framework\Attributes\DataProvider('streamProvider')] + public function testStorageStreams(callable $fopen, bool $copied): void { + $content = 'video content'; + $opened = []; + // A stale cached size is only used if the stream is passed on directly + $file = $this->getStorageFile($fopen, $content, 999, $opened); $this->onRun = function (array $files) use ($content, $copied, &$opened): void { $stream = $files['/input.mp4']['stream']; $this->assertSame($copied, !in_array($stream, $opened, true)); + $this->assertSame($copied ? strlen($content) : 999, $files['/input.mp4']['size']); fseek($stream, 6); $this->assertSame(substr($content, 6), stream_get_contents($stream)); fseek($stream, 0); @@ -188,4 +218,17 @@ public function testStorageStreams(callable $fopen, bool $copied): void { $this->assertNotNull($this->getThumbnail($file)); $this->assertCount(2, $this->calls); } + + public function testNonSeekableStreamIsDownloadedPartially(): void { + $content = str_repeat('a', 11 * 1024 * 1024); + $opened = []; + $file = $this->getStorageFile(fn (string $content) => StreamWrapper::getResource(new NoSeekStream(Utils::streamFor($content))), $content, strlen($content), $opened); + + $this->onRun = function (array $files): void { + $this->assertSame(10 * 1024 * 1024, $files['/input.mp4']['size']); + $this->assertSame(10 * 1024 * 1024, strlen(stream_get_contents($files['/input.mp4']['stream'], -1, 0))); + }; + + $this->assertNotNull($this->getThumbnail($file)); + } } From 1b4593c9ad428c47f4ca99eb3a1609fbb41bc927 Mon Sep 17 00:00:00 2001 From: "Simon L." Date: Wed, 4 Nov 2026 16:09:20 +0100 Subject: [PATCH 3/4] fix(preview): fix psalm and coding style issues in FFmpegOverIpClient Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 Signed-off-by: Simon L. --- lib/private/Preview/FFmpegOverIpClient.php | 15 +++++---------- tests/lib/Preview/MovieRemoteTest.php | 3 ++- 2 files changed, 7 insertions(+), 11 deletions(-) diff --git a/lib/private/Preview/FFmpegOverIpClient.php b/lib/private/Preview/FFmpegOverIpClient.php index bf919faaa0ba2..1410f875eeb7f 100644 --- a/lib/private/Preview/FFmpegOverIpClient.php +++ b/lib/private/Preview/FFmpegOverIpClient.php @@ -221,14 +221,13 @@ private function handleFileRequest(int $type, string $payload): array { } $this->openFiles[$fileId] = ['path' => $path, 'offset' => 0]; return [self::MSG_OPEN_OK, pack('nJ', $requestId, $this->files[$path]['size'])]; - case self::MSG_READ: $fileId = $this->unpack('n', $payload, 2, 8); $length = min($this->unpack('N', $payload, 4, 8), self::MAX_READ_SIZE); if (!isset($this->openFiles[$fileId])) { return $this->ioError($requestId, self::FIO_EINVAL); } - $file = &$this->openFiles[$fileId]; + $file = $this->openFiles[$fileId]; $stream = $this->files[$file['path']]['stream']; if (ftell($stream) !== $file['offset'] && fseek($stream, $file['offset']) !== 0) { return $this->ioError($requestId, self::FIO_EIO); @@ -238,9 +237,8 @@ private function handleFileRequest(int $type, string $payload): array { if ($data === false) { return $this->ioError($requestId, self::FIO_EIO); } - $file['offset'] += strlen($data); + $this->openFiles[$fileId]['offset'] += strlen($data); return [self::MSG_READ_OK, pack('n', $requestId) . $data]; - case self::MSG_SEEK: $fileId = $this->unpack('n', $payload, 2, 13); $offset = $this->unpack('J', $payload, 4, 13); @@ -248,7 +246,7 @@ private function handleFileRequest(int $type, string $payload): array { if (!isset($this->openFiles[$fileId])) { return $this->ioError($requestId, self::FIO_EINVAL); } - $file = &$this->openFiles[$fileId]; + $file = $this->openFiles[$fileId]; $base = match ($whence) { self::FIO_SEEK_SET => 0, self::FIO_SEEK_CUR => $file['offset'], @@ -258,16 +256,14 @@ private function handleFileRequest(int $type, string $payload): array { if ($base === null || $offset > PHP_INT_MAX - $base || $base + $offset < 0) { return $this->ioError($requestId, self::FIO_EINVAL); } - $file['offset'] = $base + $offset; - return [self::MSG_SEEK_OK, pack('nJ', $requestId, $file['offset'])]; - + $this->openFiles[$fileId]['offset'] = $base + $offset; + return [self::MSG_SEEK_OK, pack('nJ', $requestId, $base + $offset)]; case self::MSG_FSTAT: $fileId = $this->unpack('n', $payload, 2, 4); if (!isset($this->openFiles[$fileId])) { return $this->ioError($requestId, self::FIO_EINVAL); } return [self::MSG_FSTAT_OK, pack('nJN', $requestId, $this->files[$this->openFiles[$fileId]['path']]['size'], self::FILE_MODE)]; - case self::MSG_CLOSE: $fileId = $this->unpack('n', $payload, 2, 4); if (!isset($this->openFiles[$fileId])) { @@ -275,7 +271,6 @@ private function handleFileRequest(int $type, string $payload): array { } unset($this->openFiles[$fileId]); return [self::MSG_CLOSE_OK, pack('n', $requestId)]; - default: // Writing, truncating, deleting, renaming and creating directories return $this->ioError($requestId, self::FIO_EROFS); diff --git a/tests/lib/Preview/MovieRemoteTest.php b/tests/lib/Preview/MovieRemoteTest.php index 92bd6783a65cd..1b779ebe1252e 100644 --- a/tests/lib/Preview/MovieRemoteTest.php +++ b/tests/lib/Preview/MovieRemoteTest.php @@ -19,6 +19,7 @@ use OC\Preview\MovieRemote; use OCP\Files\File; use OCP\Files\IRootFolder; +use OCP\Files\Storage\IStorage; use OCP\Server; #[\PHPUnit\Framework\Attributes\Group('DB')] @@ -185,7 +186,7 @@ function (string $content) { } private function getStorageFile(callable $fopen, string $content, int $cachedSize, array &$opened): File { - $storage = $this->createMock(\OCP\Files\Storage\IStorage::class); + $storage = $this->createMock(IStorage::class); $storage->method('isLocal')->willReturn(false); $file = $this->createMock(File::class); $file->method('fopen')->willReturnCallback(function () use ($fopen, $content, &$opened) { From d18936a8a4f9f894ffda51f58429989a99870427 Mon Sep 17 00:00:00 2001 From: "Simon L." Date: Thu, 5 Nov 2026 09:29:01 +0100 Subject: [PATCH 4/4] fix(preview): harden the MovieRemote provider and its ffmpeg-over-ip client - Open the video on the storage, so it is not locked while the server works - Take the size from the opened stream instead of the filecache - Limit the data the server may read of videos on non-local storages to 20 MB, like the Movie provider - Track the stream position after failed seeks and report short reads as I/O errors instead of end of file - Send a cancel message when a run is aborted - Enforce the deadline while writing with non-blocking writes - Catch all errors and give up after the first failed connection - Report the reason of TLS errors, verify IPv6 literals and document the certificate bundle that is used - Register MovieRemote before Movie and log if no address is configured - Use the default timeout for values below 1 Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 Signed-off-by: Simon L. --- config/config.sample.php | 17 ++- lib/private/Preview/FFmpegOverIpClient.php | 128 ++++++++++++++++--- lib/private/Preview/MovieRemote.php | 56 +++++--- lib/private/PreviewManager.php | 10 +- tests/lib/Preview/FFmpegOverIpClientTest.php | 88 +++++++++++++ tests/lib/Preview/MovieRemoteTest.php | 54 ++++++-- tests/lib/PreviewManagerTest.php | 88 +++++++++++++ 7 files changed, 387 insertions(+), 54 deletions(-) create mode 100644 tests/lib/PreviewManagerTest.php diff --git a/config/config.sample.php b/config/config.sample.php index 3a694990fd701..6198e6ecb61d1 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -1645,11 +1645,18 @@ * encrypt the connection and does not authenticate the server, so anyone * answering on this address can read the videos. Either only use it within * a trusted network, or put a TLS terminating proxy (e.g. stunnel) in front - * of the server and use a ``tls://`` address, whose certificate is verified - * against the system and imported certificates. + * of the server and use a ``tls://`` address. Like for other outgoing + * connections of Nextcloud, its certificate is verified against Nextcloud's + * certificate bundle, so a certificate of an internal CA has to be imported + * with ``occ security:certificates:import``. * - * Videos on storages that cannot seek (e.g. WebDAV or FTP external storage) - * are only sent partially, like with ``OC\Preview\Movie``. + * Like with ``OC\Preview\Movie``, the server can only read up to 20 MB of + * videos on storages that are not local (e.g. S3 or external storage), and + * only the first 10 MB of videos on storages that cannot seek (e.g. WebDAV + * or FTP external storage). + * + * If ``OC\Preview\Movie`` is enabled as well, the server is tried first and + * the local ffmpeg is only used if the server could not generate a preview. * * See https://github.com/steelbrain/ffmpeg-over-ip */ @@ -1662,7 +1669,7 @@ /** * Seconds generating a single video preview on the ffmpeg-over-ip server - * may take in total. + * may take in total. Values below 1 use the default. * * Defaults to ``120`` */ diff --git a/lib/private/Preview/FFmpegOverIpClient.php b/lib/private/Preview/FFmpegOverIpClient.php index 1410f875eeb7f..3f0bfb8666b73 100644 --- a/lib/private/Preview/FFmpegOverIpClient.php +++ b/lib/private/Preview/FFmpegOverIpClient.php @@ -22,9 +22,13 @@ class FFmpegOverIpClient { public const PROGRAM_FFMPEG = 0x01; public const PROGRAM_FFPROBE = 0x02; + /** Code of the exception thrown if the server cannot be reached */ + public const ERROR_CONNECT = 1; + private const PROTOCOL_VERSION = 0x06; private const MSG_COMMAND = 0x01; + private const MSG_CANCEL = 0x02; private const MSG_EXIT_CODE = 0x03; private const MSG_ERROR = 0x04; private const MSG_PING = 0x05; @@ -61,23 +65,30 @@ class FFmpegOverIpClient { private const MAX_READ_SIZE = 16 * 1024 * 1024; private const MAX_STDOUT_SIZE = 64 * 1024 * 1024; private const MAX_STDERR_SIZE = 64 * 1024; + private const WRITE_CHUNK_SIZE = 64 * 1024; /** @var array */ private array $files = []; /** @var array */ private array $openFiles = []; + /** @var array position of each stream after the last successful read */ + private array $positions = []; private float $deadline; + private int $bytesRead = 0; + private bool $writing = false; /** * @param string $address ``host:port``, ``tcp://host:port``, ``tls://host:port`` or ``unix:/path/to/socket`` * @param int $timeout seconds all runs of this client may take together * @param ?string $caFile certificates to verify the server with for ``tls://`` addresses + * @param ?int $readLimit bytes the server may read in all runs of this client together */ public function __construct( private string $address, private string $secret, private int $timeout = 120, private ?string $caFile = null, + private ?int $readLimit = null, private int $connectTimeout = 5, ) { $this->deadline = microtime(true) + $timeout; @@ -87,7 +98,7 @@ public function __construct( * @param list $args * @param array $files seekable streams by the path they are opened with * @return array{exitCode: int, stdout: string, stderr: string} - * @throws \RuntimeException if the server is unreachable, rejects the command or violates the protocol + * @throws \RuntimeException if the server is unreachable (with code ERROR_CONNECT), rejects the command or violates the protocol */ public function run(int $program, array $args, array $files): array { $remoteSocket = $this->getRemoteSocket(); @@ -95,9 +106,25 @@ public function run(int $program, array $args, array $files): array { if ($this->caFile !== null) { $ssl['cafile'] = $this->caFile; } - $socket = @stream_socket_client($remoteSocket, $errno, $errstr, min($this->connectTimeout, $this->getRemainingTime()), STREAM_CLIENT_CONNECT, stream_context_create(['ssl' => $ssl])); + // PHP derives no peer name from IPv6 literals + if (preg_match('/^(?:tls|ssl):\/\/\[([^\]]+)\]/i', $remoteSocket, $matches) === 1) { + $ssl['peer_name'] = $matches[1]; + } + + // TLS errors are only reported as warnings + $errors = []; + set_error_handler(function (int $errno, string $message) use (&$errors): bool { + $errors[] = $message; + return true; + }); + try { + $socket = stream_socket_client($remoteSocket, $errno, $errstr, min($this->connectTimeout, $this->getRemainingTime()), STREAM_CLIENT_CONNECT, stream_context_create(['ssl' => $ssl])); + } finally { + restore_error_handler(); + } if ($socket === false) { - throw new \RuntimeException('Could not connect to ffmpeg-over-ip server ' . $this->address . ': ' . $errstr); + $reason = implode('; ', array_filter([$errstr, ...$errors])); + throw new \RuntimeException('Could not connect to ffmpeg-over-ip server ' . $this->address . ': ' . $reason, self::ERROR_CONNECT); } try { @@ -119,6 +146,23 @@ public function run(int $program, array $args, array $files): array { public function communicate($socket, int $program, array $args, array $files): array { $this->files = $files; $this->openFiles = []; + $this->positions = []; + $this->writing = false; + + try { + return $this->exchange($socket, $program, $args); + } catch (\Throwable $e) { + $this->cancel($socket); + throw $e; + } + } + + /** + * @param resource $socket + * @param list $args + * @return array{exitCode: int, stdout: string, stderr: string} + */ + private function exchange($socket, int $program, array $args): array { $stdout = ''; $stderr = ''; @@ -161,6 +205,19 @@ public function communicate($socket, int $program, array $args, array $files): a } } + /** + * Asks the server to stop the process, which it otherwise keeps running until its keepalive times out + * + * @param resource $socket + */ + private function cancel($socket): void { + if ($this->writing || !is_resource($socket)) { + return; + } + stream_set_blocking($socket, false); + @fwrite($socket, pack('CN', self::MSG_CANCEL, 0)); + } + private function getRemoteSocket(): string { $address = rtrim($this->address, '/'); if (str_starts_with($address, 'unix:')) { @@ -228,16 +285,17 @@ private function handleFileRequest(int $type, string $payload): array { return $this->ioError($requestId, self::FIO_EINVAL); } $file = $this->openFiles[$fileId]; - $stream = $this->files[$file['path']]['stream']; - if (ftell($stream) !== $file['offset'] && fseek($stream, $file['offset']) !== 0) { + // A short read is treated as end of file, so anything but the full length is an error + $expected = max(0, min($length, $this->files[$file['path']]['size'] - $file['offset'])); + if ($this->readLimit !== null && $this->bytesRead + $expected > $this->readLimit) { return $this->ioError($requestId, self::FIO_EIO); } - // A short read is treated as end of file, so the full length must be read - $data = $length > 0 ? stream_get_contents($stream, $length) : ''; - if ($data === false) { + $data = $this->readFile($file['path'], $file['offset'], $expected); + if ($data === null) { return $this->ioError($requestId, self::FIO_EIO); } - $this->openFiles[$fileId]['offset'] += strlen($data); + $this->bytesRead += $expected; + $this->openFiles[$fileId]['offset'] += $expected; return [self::MSG_READ_OK, pack('n', $requestId) . $data]; case self::MSG_SEEK: $fileId = $this->unpack('n', $payload, 2, 13); @@ -277,6 +335,28 @@ private function handleFileRequest(int $type, string $payload): array { } } + /** + * Reads exactly $length bytes at $offset of the file registered as $path + */ + private function readFile(string $path, int $offset, int $length): ?string { + if ($length === 0) { + return ''; + } + $stream = $this->files[$path]['stream']; + // ftell() is not reliable after a failed seek, so the position is tracked here + if (($this->positions[$path] ?? null) !== $offset && fseek($stream, $offset) !== 0) { + unset($this->positions[$path]); + return null; + } + unset($this->positions[$path]); + $data = stream_get_contents($stream, $length); + if ($data === false || strlen($data) !== $length) { + return null; + } + $this->positions[$path] = $offset + $length; + return $data; + } + /** * @return array{0: int, 1: string} */ @@ -333,14 +413,32 @@ private function read($socket, int $length): string { */ private function writeMessage($socket, int $type, string $payload): void { $data = pack('CN', $type, strlen($payload)) . $payload; - while ($data !== '') { - $this->applyTimeout($socket); - $written = fwrite($socket, $data); - if ($written === false || $written === 0) { - throw new \RuntimeException('Could not write to ffmpeg-over-ip server'); + $offset = 0; + $this->writing = true; + // A blocking write restarts its timeout after every partial write, so the deadline is enforced with select + stream_set_blocking($socket, false); + try { + while ($offset < strlen($data)) { + $remaining = $this->getRemainingTime(); + $read = $except = null; + $write = [$socket]; + $ready = stream_select($read, $write, $except, (int)$remaining, (int)(($remaining - (int)$remaining) * 1000000)); + if ($ready === false) { + throw new \RuntimeException('Could not write to ffmpeg-over-ip server'); + } + if ($ready === 0) { + continue; + } + $written = fwrite($socket, substr($data, $offset, self::WRITE_CHUNK_SIZE)); + if ($written === false) { + throw new \RuntimeException('Could not write to ffmpeg-over-ip server'); + } + $offset += $written; } - $data = substr($data, $written); + } finally { + stream_set_blocking($socket, true); } + $this->writing = false; } /** diff --git a/lib/private/Preview/MovieRemote.php b/lib/private/Preview/MovieRemote.php index ab561a9f61421..1a3c33acbbb00 100644 --- a/lib/private/Preview/MovieRemote.php +++ b/lib/private/Preview/MovieRemote.php @@ -26,6 +26,9 @@ class MovieRemote extends ProviderV2 { /** Bytes downloaded of videos on storages that cannot seek, matching the first attempt of Movie */ private const NON_SEEKABLE_SIZE = 10 * 1024 * 1024; + /** Bytes the server may read of videos on non-local storages, like Movie: the beginning and a moov atom of up to the same size */ + private const NON_LOCAL_READ_LIMIT = 2 * self::NON_SEEKABLE_SIZE; + private const DEFAULT_TIMEOUT = 120; private IConfig $config; private LoggerInterface $logger; @@ -52,19 +55,22 @@ public function getThumbnail(File $file, int $maxX, int $maxY): ?IImage { return null; } - $input = $this->getInput($file); - if ($input === null) { - return null; - } - $extension = preg_replace('/[^a-z0-9]/', '', strtolower($file->getExtension())); - $path = '/input' . ($extension !== '' ? '.' . $extension : ''); - $files = [$path => $input]; - + $input = null; try { + $input = $this->getInput($file); + if ($input === null) { + return null; + } + $extension = preg_replace('/[^a-z0-9]/', '', strtolower($file->getExtension())); + $path = '/input' . ($extension !== '' ? '.' . $extension : ''); + $files = [$path => $input]; + + $timeout = $this->config->getSystemValueInt('preview_ffmpeg_timeout', self::DEFAULT_TIMEOUT); $client = $this->getClient( $this->config->getSystemValueString('preview_ffmpeg_address'), $this->config->getSystemValueString('preview_ffmpeg_key', ''), - $this->config->getSystemValueInt('preview_ffmpeg_timeout', 120), + $timeout > 0 ? $timeout : self::DEFAULT_TIMEOUT, + $file->getStorage()->isLocal() ? null : self::NON_LOCAL_READ_LIMIT, ); $hdr = $this->useHdr($client, $path, $files); @@ -84,13 +90,15 @@ public function getThumbnail(File $file, int $maxX, int $maxY): ?IImage { 'output' => $result['stderr'], ]); } - } catch (\RuntimeException $e) { + } catch (\Throwable $e) { $this->logger->warning('MovieRemote preview generation failed: ' . $e->getMessage(), [ 'exception' => $e, ]); return null; } finally { - fclose($input['stream']); + if ($input !== null) { + fclose($input['stream']); + } $this->cleanTmpFiles(); } @@ -98,17 +106,21 @@ public function getThumbnail(File $file, int $maxX, int $maxY): ?IImage { return null; } - protected function getClient(string $address, string $secret, int $timeout): FFmpegOverIpClient { - return new FFmpegOverIpClient($address, $secret, $timeout, Server::get(ICertificateManager::class)->getAbsoluteBundlePath()); + protected function getClient(string $address, string $secret, int $timeout, ?int $readLimit): FFmpegOverIpClient { + return new FFmpegOverIpClient($address, $secret, $timeout, Server::get(ICertificateManager::class)->getAbsoluteBundlePath(), $readLimit); } /** * @param array $files + * @throws \RuntimeException if the server cannot be reached */ private function useHdr(FFmpegOverIpClient $client, string $path, array $files): bool { try { $probe = $client->run(FFmpegOverIpClient::PROGRAM_FFPROBE, [...Movie::HDR_PROBE_ARGS, $path], $files); - } catch (\RuntimeException $e) { + } catch (\Throwable $e) { + if ($e instanceof \RuntimeException && $e->getCode() === FFmpegOverIpClient::ERROR_CONNECT) { + throw $e; + } $this->logger->debug('MovieRemote HDR detection failed: ' . $e->getMessage(), ['exception' => $e]); return false; } @@ -118,17 +130,24 @@ private function useHdr(FFmpegOverIpClient $client, string $path, array $files): /** * Opens the video, downloading only its beginning to a temporary file if the storage cannot seek * + * The video is opened on the storage instead of the view, so that it is not + * locked while the server works on it. + * * @return array{stream: resource, size: int}|null */ private function getInput(File $file): ?array { - $stream = $file->fopen('r'); + $stream = $file->getStorage()->fopen($file->getInternalPath(), 'r'); if (!is_resource($stream)) { return null; } - // Stream wrappers always claim to be seekable, so seeking is tried instead + // Stream wrappers always claim to be seekable, so seeking is tried instead. The + // size is taken from the stream, as the cached size may be outdated. try { - if (stream_get_meta_data($stream)['seekable'] && fseek($stream, 1) === 0 && fseek($stream, 0) === 0) { - return ['stream' => $stream, 'size' => (int)$file->getSize()]; + if (stream_get_meta_data($stream)['seekable'] && fseek($stream, 0, SEEK_END) === 0) { + $size = ftell($stream); + if (fseek($stream, 0) === 0) { + return ['stream' => $stream, 'size' => $size !== false ? $size : (int)$file->getSize()]; + } } } catch (\Throwable) { } @@ -137,7 +156,6 @@ private function getInput(File $file): ?array { $localPath = $this->getLocalFile($file, self::NON_SEEKABLE_SIZE); $stream = $localPath !== false ? fopen($localPath, 'r') : false; if ($stream === false) { - $this->cleanTmpFiles(); return null; } return ['stream' => $stream, 'size' => (int)filesize($localPath)]; diff --git a/lib/private/PreviewManager.php b/lib/private/PreviewManager.php index ca8b700abdc6d..f73c7d4380262 100644 --- a/lib/private/PreviewManager.php +++ b/lib/private/PreviewManager.php @@ -365,7 +365,14 @@ protected function registerCoreProviders(): void { $this->registerCoreProvidersOffice(); - // Video requires ffmpeg + // Video requires an ffmpeg-over-ip server or ffmpeg, the server is tried first + if (in_array(MovieRemote::class, $this->getEnabledDefaultProvider(), true)) { + if ($this->config->getSystemValueString('preview_ffmpeg_address', '') === '') { + $this->container->get(LoggerInterface::class)->error('MovieRemote preview provider is enabled, but no address is configured. Please provide the address of your ffmpeg-over-ip server to the \'preview_ffmpeg_address\' config variable.'); + } else { + $this->registerCoreProvider(MovieRemote::class, '/video\/.*/'); + } + } if (in_array(Movie::class, $this->getEnabledDefaultProvider(), true)) { $movieBinary = $this->config->getSystemValue('preview_ffmpeg_path', null); if (!is_string($movieBinary)) { @@ -376,7 +383,6 @@ protected function registerCoreProviders(): void { $this->registerCoreProvider(Movie::class, '/video\/.*/', ['movieBinary' => $movieBinary]); } } - $this->registerCoreProvider(MovieRemote::class, '/video\/.*/'); } private function registerCoreProvidersOffice(): void { diff --git a/tests/lib/Preview/FFmpegOverIpClientTest.php b/tests/lib/Preview/FFmpegOverIpClientTest.php index 1b235bac008dc..10b15516312d4 100644 --- a/tests/lib/Preview/FFmpegOverIpClientTest.php +++ b/tests/lib/Preview/FFmpegOverIpClientTest.php @@ -159,6 +159,50 @@ public function testRejectsSeekOverflow(): void { ], array_slice($this->clientMessages(), 2)); } + public function testReadLimit(): void { + $client = new FFmpegOverIpClient('ffmpeg-over-ip:5050', 'secret', 10, null, 8); + $this->serverSends(0x20, pack('nnNn', 1, 1, 0, 0) . '/input.mp4'); + $this->serverSends(0x21, pack('nnN', 2, 1, 5)); + $this->serverSends(0x21, pack('nnN', 3, 1, 100)); + $this->serverSends(0x03, pack('N', 1)); + + $this->communicate($this->getFiles('Hello world'), 10, $client); + $this->assertSame([ + [0x40, pack('nJ', 1, 11)], + [0x41, pack('n', 2) . 'Hello'], + [0x4F, pack('nN', 3, 5)], + ], array_slice($this->clientMessages(), 2)); + } + + public function testFailedSeekDoesNotCorruptLaterReads(): void { + $content = ''; + for ($i = 0; strlen($content) < 20000; $i++) { + $content .= sprintf('%08d', $i); + } + $stream = fopen('php://memory', 'r+'); + fwrite($stream, $content); + // The stream is shorter than the size, so seeking to the end fails + $files = ['/input.mp4' => ['stream' => $stream, 'size' => 30000]]; + + $this->serverSends(0x20, pack('nnNn', 1, 1, 0, 0) . '/input.mp4'); + $this->serverSends(0x21, pack('nnN', 2, 1, 9000)); + $this->serverSends(0x23, pack('nnJC', 3, 1, 25000, 0)); + $this->serverSends(0x21, pack('nnN', 4, 1, 100)); + $this->serverSends(0x23, pack('nnJC', 5, 1, 9000, 0)); + $this->serverSends(0x21, pack('nnN', 6, 1, 100)); + $this->serverSends(0x23, pack('nnJC', 7, 1, 19990, 0)); + $this->serverSends(0x21, pack('nnN', 8, 1, 100)); + $this->serverSends(0x03, pack('N', 0)); + + $this->communicate($files); + $messages = array_slice($this->clientMessages(), 2); + $this->assertSame([0x41, pack('n', 2) . substr($content, 0, 9000)], $messages[1]); + $this->assertSame([0x4F, pack('nN', 4, 5)], $messages[3]); + $this->assertSame([0x41, pack('n', 6) . substr($content, 9000, 100)], $messages[5]); + // A short read would be taken as end of file + $this->assertSame([0x4F, pack('nN', 8, 5)], $messages[7]); + } + public function testAnswersPing(): void { $this->serverSends(0x05, 'ping'); $this->serverSends(0x03, pack('N', 0)); @@ -198,6 +242,32 @@ public function testMessageTooLarge(): void { $this->communicate(); } + public function testCancelsOnAbort(): void { + fwrite($this->server, pack('CN', 0x12, 2 * 1024 * 1024)); + + try { + $this->communicate(); + $this->fail('Expected an exception'); + } catch (\RuntimeException) { + } + $this->assertSame([0x02, ''], $this->clientMessages()[2]); + } + + public function testWriteDeadline(): void { + // The server never reads the response, so the write blocks once the socket buffer is full + $this->serverSends(0x20, pack('nnNn', 1, 1, 0, 0) . '/input.mp4'); + $this->serverSends(0x21, pack('nnN', 2, 1, 4 * 1024 * 1024)); + $start = microtime(true); + + try { + $this->communicate($this->getFiles(str_repeat('a', 4 * 1024 * 1024)), 1); + $this->fail('Expected an exception'); + } catch (\RuntimeException $e) { + $this->assertSame('ffmpeg-over-ip server did not finish within 1 seconds', $e->getMessage()); + } + $this->assertLessThan(3, microtime(true) - $start); + } + public function testTimeout(): void { $this->expectException(\RuntimeException::class); $this->expectExceptionMessage('ffmpeg-over-ip server did not finish within 1 seconds'); @@ -238,7 +308,25 @@ public function testUnreachable(string $address): void { $client = new FFmpegOverIpClient($address, 'secret'); $this->expectException(\RuntimeException::class); + $this->expectExceptionCode(FFmpegOverIpClient::ERROR_CONNECT); $this->expectExceptionMessage('Could not connect to ffmpeg-over-ip server ' . $address); $client->run(FFmpegOverIpClient::PROGRAM_FFMPEG, ['-version'], []); } + + public function testTlsErrorIsReported(): void { + // A plain server never answers the TLS handshake + $server = stream_socket_server('tcp://127.0.0.1:0'); + $address = 'tls://' . stream_socket_get_name($server, false); + $client = new FFmpegOverIpClient($address, 'secret', 1); + + try { + $client->run(FFmpegOverIpClient::PROGRAM_FFMPEG, ['-version'], []); + $this->fail('Expected an exception'); + } catch (\RuntimeException $e) { + $this->assertSame(FFmpegOverIpClient::ERROR_CONNECT, $e->getCode()); + $this->assertMatchesRegularExpression('/^Could not connect to ffmpeg-over-ip server ' . preg_quote($address, '/') . ': \S/', $e->getMessage()); + } finally { + fclose($server); + } + } } diff --git a/tests/lib/Preview/MovieRemoteTest.php b/tests/lib/Preview/MovieRemoteTest.php index 1b779ebe1252e..4696d563f6a46 100644 --- a/tests/lib/Preview/MovieRemoteTest.php +++ b/tests/lib/Preview/MovieRemoteTest.php @@ -20,6 +20,7 @@ use OCP\Files\File; use OCP\Files\IRootFolder; use OCP\Files\Storage\IStorage; +use OCP\Lock\ILockingProvider; use OCP\Server; #[\PHPUnit\Framework\Attributes\Group('DB')] @@ -62,8 +63,8 @@ public function __construct( } #[\Override] - protected function getClient(string $address, string $secret, int $timeout): FFmpegOverIpClient { - $this->credentials = [$address, $secret, $timeout]; + protected function getClient(string $address, string $secret, int $timeout, ?int $readLimit): FFmpegOverIpClient { + $this->credentials = [$address, $secret, $timeout, $readLimit]; return $this->client; } }; @@ -88,7 +89,7 @@ public function testRunsFfprobeAndFfmpeg(): void { $this->assertNotNull($preview); $this->assertSame(256, $preview->width()); $this->assertSame(160, $preview->height()); - $this->assertSame(['ffmpeg-over-ip:5050', 'secret', 120], $this->provider->credentials); + $this->assertSame(['ffmpeg-over-ip:5050', 'secret', 120, null], $this->provider->credentials); $this->assertCount(2, $this->calls); $this->assertSame(FFmpegOverIpClient::PROGRAM_FFPROBE, $this->calls[0][0]); @@ -139,20 +140,43 @@ public function testAllAttemptsFail(): void { $this->assertCount(4, $this->calls); } - public function testServerFails(): void { + public function testServerUnreachable(): void { $this->onRun = function (): void { - throw new \RuntimeException('Could not connect'); + throw new \RuntimeException('Could not connect', FFmpegOverIpClient::ERROR_CONNECT); + }; + + $this->assertNull($this->getThumbnail()); + $this->assertCount(1, $this->calls); + } + + public function testStorageError(): void { + $this->onRun = function (): void { + throw new \Exception('Connection reset'); }; $this->assertNull($this->getThumbnail()); $this->assertCount(2, $this->calls); } - public function testTimeoutConfig(): void { - $this->overwriteSystemConfig('preview_ffmpeg_timeout', 30); + public function testFileIsNotLocked(): void { + $this->onRun = function (): void { + $this->rootView->lockFile($this->imgPath, ILockingProvider::LOCK_EXCLUSIVE); + $this->rootView->unlockFile($this->imgPath, ILockingProvider::LOCK_EXCLUSIVE); + }; + + $this->assertNotNull($this->getThumbnail()); + } + + public static function timeoutProvider(): array { + return [[30, 30], [0, 120], [-5, 120]]; + } + + #[\PHPUnit\Framework\Attributes\DataProvider('timeoutProvider')] + public function testTimeoutConfig(int $config, int $timeout): void { + $this->overwriteSystemConfig('preview_ffmpeg_timeout', $config); $this->assertNotNull($this->getThumbnail()); - $this->assertSame(30, $this->provider->credentials[2]); + $this->assertSame($timeout, $this->provider->credentials[2]); } public function testNoUrlConfigured(): void { @@ -186,12 +210,15 @@ function (string $content) { } private function getStorageFile(callable $fopen, string $content, int $cachedSize, array &$opened): File { + $open = function () use ($fopen, $content, &$opened) { + return $opened[] = $fopen($content); + }; $storage = $this->createMock(IStorage::class); $storage->method('isLocal')->willReturn(false); + $storage->method('fopen')->with('files/video.mp4', 'r')->willReturnCallback($open); $file = $this->createMock(File::class); - $file->method('fopen')->willReturnCallback(function () use ($fopen, $content, &$opened) { - return $opened[] = $fopen($content); - }); + $file->method('fopen')->willReturnCallback($open); + $file->method('getInternalPath')->willReturn('files/video.mp4'); $file->method('getSize')->willReturn($cachedSize); $file->method('getStorage')->willReturn($storage); $file->method('getExtension')->willReturn('MP4'); @@ -203,13 +230,13 @@ private function getStorageFile(callable $fopen, string $content, int $cachedSiz public function testStorageStreams(callable $fopen, bool $copied): void { $content = 'video content'; $opened = []; - // A stale cached size is only used if the stream is passed on directly + // The cached size is outdated $file = $this->getStorageFile($fopen, $content, 999, $opened); $this->onRun = function (array $files) use ($content, $copied, &$opened): void { $stream = $files['/input.mp4']['stream']; $this->assertSame($copied, !in_array($stream, $opened, true)); - $this->assertSame($copied ? strlen($content) : 999, $files['/input.mp4']['size']); + $this->assertSame(strlen($content), $files['/input.mp4']['size']); fseek($stream, 6); $this->assertSame(substr($content, 6), stream_get_contents($stream)); fseek($stream, 0); @@ -218,6 +245,7 @@ public function testStorageStreams(callable $fopen, bool $copied): void { $this->assertNotNull($this->getThumbnail($file)); $this->assertCount(2, $this->calls); + $this->assertSame(20 * 1024 * 1024, $this->provider->credentials[3]); } public function testNonSeekableStreamIsDownloadedPartially(): void { diff --git a/tests/lib/PreviewManagerTest.php b/tests/lib/PreviewManagerTest.php new file mode 100644 index 0000000000000..17562c0a68189 --- /dev/null +++ b/tests/lib/PreviewManagerTest.php @@ -0,0 +1,88 @@ +config = $this->createMock(IConfig::class); + $this->config->method('getSystemValueBool')->willReturn(true); + $this->config->method('getSystemValue')->willReturnCallback(fn (string $key, mixed $default) => $this->systemConfig[$key] ?? $default); + $this->config->method('getSystemValueString')->willReturnCallback(fn (string $key, string $default) => $this->systemConfig[$key] ?? $default); + + $this->logger = $this->createMock(LoggerInterface::class); + $container = $this->createMock(ContainerInterface::class); + $container->method('get')->with(LoggerInterface::class)->willReturn($this->logger); + $binaryFinder = $this->createMock(IBinaryFinder::class); + $binaryFinder->method('findBinaryPath')->willReturn(false); + + $this->previewManager = new PreviewManager( + $this->config, + $this->createMock(IRootFolder::class), + $this->createMock(IEventDispatcher::class), + $this->createMock(GeneratorHelper::class), + null, + $this->createMock(Coordinator::class), + $container, + $binaryFinder, + $this->createMock(IMagickSupport::class), + ); + } + + /** + * @return list + */ + private function getVideoProviders(): array { + return array_map(fn (\Closure $provider): string => $provider()::class, $this->previewManager->getProviders()['/video\/.*/'] ?? []); + } + + public function testMovieRemoteIsTriedBeforeMovie(): void { + $this->systemConfig = [ + 'enabledPreviewProviders' => [Movie::class, MovieRemote::class], + 'preview_ffmpeg_path' => '/usr/bin/ffmpeg', + 'preview_ffmpeg_address' => 'ffmpeg-over-ip:5050', + ]; + + $this->assertSame([MovieRemote::class, Movie::class], $this->getVideoProviders()); + } + + public function testMovieRemoteWithoutAddress(): void { + $this->systemConfig = [ + 'enabledPreviewProviders' => [MovieRemote::class], + ]; + $this->logger->expects($this->once()) + ->method('error') + ->with($this->stringContains('preview_ffmpeg_address')); + + $this->assertSame([], $this->getVideoProviders()); + $this->assertFalse($this->previewManager->isMimeSupported('video/mp4')); + } +}