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
Changes from 1 commit
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
45 changes: 42 additions & 3 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 @@ -373,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 @@ -381,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 @@ -398,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 @@ -407,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