Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Modernize previews #37564

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 72 additions & 4 deletions lib/private/Preview/Generator.php
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ class Generator {
public const SEMAPHORE_ID_ALL = 0x0a11;
public const SEMAPHORE_ID_NEW = 0x07ea;

private \OCP\ILogger $logger;
Copy link
Author

Choose a reason for hiding this comment

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

I don't actually write PHP. Can this leading slash be removed?


/** @var IPreview */
private $previewManager;
/** @var IConfig */
Expand All @@ -64,6 +66,14 @@ class Generator {
/** @var IEventDispatcher */
private $eventDispatcher;

/**
* GD image type for the preview format setting
*
* `null` if the preview format setting is not set (=> use default handling
* instead).
*/
private null|int $previewImageType;

public function __construct(
IConfig $config,
IPreview $previewManager,
Expand All @@ -72,12 +82,49 @@ public function __construct(
EventDispatcherInterface $legacyEventDispatcher,
IEventDispatcher $eventDispatcher
) {
$this->logger = \OC::$server->getLogger();
Copy link
Author

Choose a reason for hiding this comment

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

Can this leading slash be removed?


$this->config = $config;
$this->previewManager = $previewManager;
$this->appData = $appData;
$this->helper = $helper;
$this->legacyEventDispatcher = $legacyEventDispatcher;
$this->eventDispatcher = $eventDispatcher;

$this->setPreviewImageType();
}

private function setPreviewImageType(): void {
// Only allow previews to be common image formats that there is a clear
// need for.
// See https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/MIME_types#image_types
$ALLOWED_FORMATS = ['AVIF', 'GIF', 'JPEG', 'PNG', 'WEBP'];
$FALLBACK_MESSAGE = 'Falling back to the default preview format.';

$this->previewImageType = null;

$previewFormat = strtoupper(
$this->config->getAppValue('preview', 'format')
);

if (!$previewFormat) return;
if (!in_array($previewFormat, $ALLOWED_FORMATS)) {
$this->logger->error(
'Preview format must be one of [' .
implode(', ', $ALLOWED_FORMATS) .
"]; got $previewFormat. $FALLBACK_MESSAGE"
);
return;
}
if (!(imagetypes() & constant("IMG_$previewFormat"))) {
$this->logger->error(
'The installation does not support preview format ' .
"$previewFormat. $FALLBACK_MESSAGE"
);
return;
}

$this->previewImageType = constant("IMAGETYPE_$previewFormat");
}

/**
Expand Down Expand Up @@ -132,8 +179,14 @@ public function generatePreviews(File $file, array $specifications, $mimeType =
throw new NotFoundException('Cannot read file');
}

if ($mimeType === null) {
$mimeType = $file->getMimeType();
// `$mimeType` argument overrides preview format setting overrides
// default to the same MIME type as the file it is a preview for
if (!$mimeType) {
if (!is_null($this->previewImageType)) {
$mimeType = image_type_to_mime_type($this->previewImageType);
} else {
$mimeType = $file->getMimeType();
}
}

$previewFolder = $this->getPreviewFolder($file);
Expand Down Expand Up @@ -271,7 +324,7 @@ private function getSmallImagePreview(ISimpleFolder $previewFolder, File $file,
continue;
}

$preview = $this->helper->getThumbnail($provider, $file, 256, 256, $crop);
$preview = $this->helper->getThumbnail($provider, $file, 256, 256, $crop, $mimeType);

if (!($preview instanceof IImage)) {
continue;
Expand Down Expand Up @@ -437,7 +490,17 @@ private function getMaxPreview(ISimpleFolder $previewFolder, File $file, $mimeTy
$previewConcurrency = $this->getNumConcurrentPreviews('preview_concurrency_new');
$sem = self::guardWithSemaphore(self::SEMAPHORE_ID_NEW, $previewConcurrency);
try {
$preview = $this->helper->getThumbnail($provider, $file, $maxWidth, $maxHeight);
// Although we know the provider here, and it *should* know
// its own MIME type, note that it actually doesn't - so we
// have to pass it.
//
// *`IProviderV2->getMimeType`*
//
// > Regex with the mimetypes that are supported by this
// provider
//
// This is also the case for `$this->getSmallImagePreview`.
$preview = $this->helper->getThumbnail($provider, $file, $maxWidth, $maxHeight, false, $mimeType);
Comment on lines +493 to +503
Copy link
Author

@aentwist aentwist Apr 6, 2023

Choose a reason for hiding this comment

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

High value target for another, potentially big breaking change. Should be a separate issue.. probably. I've already done too much refactoring here.

} finally {
self::unguardWithSemaphore($sem);
}
Expand Down Expand Up @@ -692,6 +755,9 @@ private function getPreviewFolder(File $file) {
* @param string $mimeType
* @return null|string
* @throws \InvalidArgumentException
*
* @deprecated this is goofy
* @see https://www.php.net/manual/en/function.image-type-to-extension.php
*/
private function getExtention($mimeType) {
switch ($mimeType) {
Comment on lines +758 to 763
Copy link
Author

Choose a reason for hiding this comment

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

Also consider targeting this... separate issue

Expand All @@ -701,6 +767,8 @@ private function getExtention($mimeType) {
return 'jpg';
case 'image/gif':
return 'gif';
case 'image/webp':
return 'webp';
default:
throw new \InvalidArgumentException('Not a valid mimetype: "' . $mimeType . '"');
}
Expand Down
4 changes: 2 additions & 2 deletions lib/private/Preview/GeneratorHelper.php
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,11 @@ public function __construct(IRootFolder $rootFolder, IConfig $config) {
*
* @return bool|IImage
*/
public function getThumbnail(IProviderV2 $provider, File $file, $maxWidth, $maxHeight, bool $crop = false) {
public function getThumbnail(IProviderV2 $provider, File $file, $maxWidth, $maxHeight, bool $crop = false, string $mimeType = null) {
if ($provider instanceof Imaginary) {
return $provider->getCroppedThumbnail($file, $maxWidth, $maxHeight, $crop) ?? false;
}
return $provider->getThumbnail($file, $maxWidth, $maxHeight) ?? false;
return $provider->getThumbnail($file, $maxWidth, $maxHeight, $mimeType) ?? false;

Check failure

Code scanning / Psalm

TooManyArguments

Too many arguments for method OCP\Preview\IProviderV2::getthumbnail - saw 4
}

/**
Expand Down
3 changes: 2 additions & 1 deletion lib/private/Preview/Image.php
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ abstract class Image extends ProviderV2 {
/**
* {@inheritDoc}
*/
public function getThumbnail(File $file, int $maxX, int $maxY): ?IImage {
public function getThumbnail(File $file, int $maxX, int $maxY, string $mimeType = null): ?IImage {
$maxSizeForImages = \OC::$server->getConfig()->getSystemValueInt('preview_max_filesize_image', 50);
$size = $file->getSize();

Expand All @@ -55,6 +55,7 @@ public function getThumbnail(File $file, int $maxX, int $maxY): ?IImage {

if ($image->valid()) {
$image->scaleDownToFit($maxX, $maxY);
if ($mimeType) $image->setMimeType($mimeType);

return $image;
}
Expand Down
8 changes: 5 additions & 3 deletions lib/private/PreviewManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -243,13 +243,15 @@ public function isMimeSupported($mimeType = '*') {
/**
* Check if a preview can be generated for a file
*/
public function isAvailable(\OCP\Files\FileInfo $file): bool {
public function isAvailable(\OCP\Files\FileInfo $file, ?string $previewMimeType = null): bool {
if (!$previewMimeType) $previewMimeType = $file->getMimeType();

if (!$this->config->getSystemValue('enable_previews', true)) {
return false;
}

$this->registerCoreProviders();
if (!$this->isMimeSupported($file->getMimetype())) {
if (!$this->isMimeSupported($previewMimeType)) {
return false;
}

Expand All @@ -259,7 +261,7 @@ public function isAvailable(\OCP\Files\FileInfo $file): bool {
}

foreach ($this->providers as $supportedMimeType => $providers) {
if (preg_match($supportedMimeType, $file->getMimetype())) {
if (preg_match($supportedMimeType, $previewMimeType)) {
foreach ($providers as $providerClosure) {
$provider = $this->helper->getProvider($providerClosure);
if (!($provider instanceof IProviderV2)) {
Expand Down
75 changes: 55 additions & 20 deletions lib/private/legacy/OC_Image.php
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,12 @@ class OC_Image implements \OCP\IImage {
// Default memory limit for images to load (256 MBytes).
protected const DEFAULT_MEMORY_LIMIT = 256;

// Default quality for jpeg images
/**
* Default quality for jpeg images
*
* @deprecated this choice should be left to the image library
* @see https://www.php.net/manual/en/function.imagejpeg.php#refsect1-function.imagejpeg-parameters
*/
protected const DEFAULT_JPEG_QUALITY = 80;

Comment on lines +55 to 62
Copy link
Author

Choose a reason for hiding this comment

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

remove

/** @var false|resource|\GdImage */
Expand Down Expand Up @@ -123,6 +128,19 @@ public function mimeType(): ?string {
return $this->valid() ? $this->mimeType : null;
}

public function setMimeType(string $mimeType): void {
$this->mimeType = $mimeType;
}

/**
* Goofy alias for `mimeType`
*
* @deprecated in favor of `mimeType`
*/
public function dataMimeType(): ?string {
return $this->mimeType();
}

/**
* Returns the width of the image or -1 if no image is loaded.
*
Expand Down Expand Up @@ -352,23 +370,6 @@ public function resource() {
return $this->resource;
}

/**
* @return string Returns the mimetype of the data. Returns null if the data is not valid.
*/
public function dataMimeType(): ?string {
if (!$this->valid()) {
return null;
}

switch ($this->mimeType) {
case 'image/png':
case 'image/jpeg':
case 'image/gif':
return $this->mimeType;
default:
return 'image/png';
}
}

/**
* @return null|string Returns the raw image data.
Expand All @@ -377,6 +378,9 @@ public function data(): ?string {
if (!$this->valid()) {
return null;
}

$quality = $this->getQuality();

ob_start();
switch ($this->mimeType) {
case "image/png":
Expand All @@ -385,12 +389,14 @@ public function data(): ?string {
case "image/jpeg":
/** @psalm-suppress InvalidScalarArgument */
imageinterlace($this->resource, (PHP_VERSION_ID >= 80000 ? true : 1));
$quality = $this->getJpegQuality();
$res = imagejpeg($this->resource, null, $quality);
$res = imagejpeg($this->resource, null, $this->getJpegQuality());
Copy link
Author

@aentwist aentwist Apr 6, 2023

Choose a reason for hiding this comment

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

use $quality (backed by format quality setting from 0-100 with default -1 instead of format jpeg_quality from 10-100 with default 80)

-1 -> ~75 (breaking change)

break;
case "image/gif":
$res = imagegif($this->resource);
break;
case "image/webp":
$res = imagewebp($this->resource, null, $quality);
break;
default:
$res = imagepng($this->resource);
$this->logger->info('OC_Image->data. Could not guess mime-type, defaulting to png', ['app' => 'core']);
Expand All @@ -402,6 +408,33 @@ public function data(): ?string {
return ob_get_clean();
}

/**
* Gets the image quality with which to write lossy image previews
*
* @return int the image quality on a scale of 0-100, or -1 for the default
*/
private function getQuality(): int {
$quality = $this->config->getAppValue('preview', 'quality');

if ($quality && (!intval($quality) || $quality < 0 || $quality > 100)) {
$this->logger->error(
'Preview quality must be an integer from 0 to 100; got ' .
"$quality. Falling back to the default preview quality."
);
Copy link
Author

Choose a reason for hiding this comment

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

Need to set the quality to the default here

}

// If quality is not set by the user or is invalid, then set it to the
// GD default value to entrust choosing a well-optimized default
// quality to the image library (which should make a much more educated
// choice than we would make). Hopefully GD doesn't mangle this idea...
// but it appears they do - WebP default should be 75 yet is 80.
//
// See https://developers.google.com/speed/webp/docs/cwebp#options
if (!$quality) $quality = -1;
Comment on lines +429 to +436
Copy link
Author

Choose a reason for hiding this comment

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

This will be very hard to communicate effectively to users. We don't know what the defaults are, and defaults (when using -1) should be tuned to / vary by format. Maybe we should set a default? Also I have very little faith in PHP to handle this idea correctly even though it should..


return (int) $quality;
}

/**
* @return string - base64 encoded, which is suitable for embedding in a VCard.
*/
Expand All @@ -411,6 +444,8 @@ public function __toString() {

/**
* @return int
*
* @deprecated
*/
protected function getJpegQuality(): int {
Copy link
Author

Choose a reason for hiding this comment

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

remove

$quality = $this->config->getAppValue('preview', 'jpeg_quality', (string) self::DEFAULT_JPEG_QUALITY);
Expand Down
3 changes: 2 additions & 1 deletion lib/public/IPreview.php
Original file line number Diff line number Diff line change
Expand Up @@ -108,10 +108,11 @@ public function isMimeSupported($mimeType = '*');
* Check if a preview can be generated for a file
*
* @param \OCP\Files\FileInfo $file
* @param ?string $previewMimeType MIME type of the preview to be generated

Check failure

Code scanning / Psalm

MismatchingDocblockParamType

Parameter $previewMimeType has wrong type 'null|string', should be 'string'
* @return bool
* @since 8.0.0
*/
public function isAvailable(\OCP\Files\FileInfo $file);
public function isAvailable(\OCP\Files\FileInfo $file, string $previewMimeType);

/**
* Generates previews of a file
Expand Down