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

Conversation

aentwist
Copy link

@aentwist aentwist commented Apr 4, 2023

Closes #13552
Closes #30118

Summary

See commits and #13552

  • add preview format setting
  • add preview quality setting
  • add avif support

TODO

  • tests
  • documentation

Checklist

Fix preview providers generate thumbnails based on source file type
instead of their type. This worked because providers were always called
with a file of the same type. This is not an assumption that can be made
any longer.

Note that preview providers don't actually know their type - although
they should - so instead make them respect an optional MIME type
parameter to avoid potential breaking changes.

Signed-off-by: Anderson Entwistle <[email protected]>
Allow preview format to be specified to be a specific type for all
files. Previously, preview format was the same type as the original file.

Format can be jpeg, png, gif, webp, or avif.

Signed-off-by: Anderson Entwistle <[email protected]>
Signed-off-by: Anderson Entwistle <[email protected]>
$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

$res = imagewebp($this->resource, null, $quality);
break;
case "image/avif":
$res = imageavif($this->resource, null, $quality);

Check failure

Code scanning / Psalm

UndefinedFunction

Function imageavif does not exist
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing speed:
imageavif($this->resource, null, $quality,10);

@@ -727,6 +765,15 @@
$this->logger->debug('OC_Image->loadFromFile, webp images not supported: ' . $imagePath, ['app' => 'core']);
}
break;
case IMAGETYPE_AVIF:

Check failure

Code scanning / Psalm

UndefinedConstant

Const IMAGETYPE_AVIF is not defined
@@ -727,6 +765,15 @@
$this->logger->debug('OC_Image->loadFromFile, webp images not supported: ' . $imagePath, ['app' => 'core']);
}
break;
case IMAGETYPE_AVIF:
if (imagetypes() & IMG_AVIF) {

Check failure

Code scanning / Psalm

UndefinedConstant

Const IMG_AVIF is not defined
if (imagetypes() & IMG_AVIF) {
if (!$this->checkImageSize($imagePath)) return false;

$this->resource = imagecreatefromavif($imagePath);

Check failure

Code scanning / Psalm

UndefinedFunction

Function imagecreatefromavif does not exist
@@ -108,10 +108,11 @@
* 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'
}

public function isAvailable(FileInfo $file): bool {
return (bool) (imagetypes() & IMG_AVIF);

Check failure

Code scanning / Psalm

UndefinedConstant

Const IMG_AVIF is not defined
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
@szaimen szaimen added this to the Nextcloud 27 milestone Apr 4, 2023
@szaimen szaimen added the 2. developing Work in progress label Apr 4, 2023
@aentwist
Copy link
Author

aentwist commented Apr 6, 2023

I tried to avoid breaking changes because I was hoping to get this out quick as a feature release.

Now I see that Nextcloud does not follow semantic versioning? Also the release cadence for majors is much quicker than expected... about every 6 months?

If the target is next major then some stuff should be broken here. At minimum the default JPEG quality, which is currently preserved.

Comment on lines +55 to 62
/**
* 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;

Copy link
Author

Choose a reason for hiding this comment

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

remove

@@ -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)

@@ -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

@@ -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?

@@ -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?

Comment on lines +493 to +503
// 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);
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.

Comment on lines +758 to 763
*
* @deprecated this is goofy
* @see https://www.php.net/manual/en/function.image-type-to-extension.php
*/
private function getExtention($mimeType) {
switch ($mimeType) {
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

Comment on lines +429 to +436
// 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;
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..

This was referenced May 3, 2023
@skjnldsv skjnldsv modified the milestones: Nextcloud 27, Nextcloud 28 May 9, 2023
@aentwist aentwist mentioned this pull request May 29, 2023
9 tasks
@JanisPlayer JanisPlayer mentioned this pull request Sep 12, 2023
17 tasks
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
This was referenced Nov 6, 2023
@AndyScherzinger AndyScherzinger removed this from the Nextcloud 28 milestone Nov 10, 2023
@susnux susnux added this to the Nextcloud 30 milestone Mar 18, 2024
@susnux susnux requested review from a team, ArtificialOwl, Fenn-CS and sorbaugh and removed request for a team March 18, 2024 21:51
@susnux
Copy link
Contributor

susnux commented Mar 18, 2024

@aentwist are you still working on this? Because to be mergeable the PR needs the be rebased (resolve conflicts) and the code analysis errors need to be fixed.

Thank you :)

@aentwist
Copy link
Author

No, I am not

@susnux susnux removed this from the Nextcloud 30 milestone Mar 19, 2024
@susnux
Copy link
Contributor

susnux commented Mar 19, 2024

No, I am not

Sad, but thank you for the pull request! We can have this as inspiration to fix the issues. With PHP 8.1 providing AVIF support we should be able to mainline this with Nextcloud 30.

@aentwist
Copy link
Author

aentwist commented Aug 1, 2024

Closing due to Nextcloud's jank release process, where I am unfortunately tagged for every single release, hopefully only under the condition that this is open. This is spot on with the rest of my Nextcloud experience. I do not, and have not ever used Nextcloud, cannot recommend Nextcloud, and do not like getting 5 emails from being tagged in a random beta release. Please feel free to use this, or even reopen it, when there is a plan to actually move it forward. Good luck. AVIF is awesome.

@aentwist aentwist closed this Aug 1, 2024
@aentwist
Copy link
Author

aentwist commented Aug 8, 2024

Reopening in good faith in light of a newer alternative I have [to closing the PR] to not be tagged all the time #46946 (comment). Thank goodness, I really hope this can be useful.

we changed a bit the behaviour, you can use the Draft state to avoid mentions

@aentwist aentwist reopened this Aug 8, 2024
@aentwist aentwist marked this pull request as draft August 8, 2024 19:40
@skjnldsv skjnldsv modified the milestones: Nextcloud 30, Nextcloud 31 Aug 14, 2024
@skjnldsv skjnldsv closed this Aug 14, 2024
@skjnldsv skjnldsv removed this from the Nextcloud 31 milestone Aug 14, 2024
@susnux
Copy link
Contributor

susnux commented Aug 14, 2024

In case someone asks why this was closed:

Pull requests that get reassigned multiple major versions, thus not getting merged, are probably orphaned.
Meaning probably the creator lost their interest in getting it merged (so the PR is not in a merge-able state).
If you want to work on this again, feel free to reopen. Otherwise there is no good reason in having 🧟 PRs out here.

@skjnldsv
Copy link
Member

Closing due to Nextcloud's jank release process, where I am unfortunately tagged for every single release, hopefully only under the condition that this is open. This is spot on with the rest of my Nextcloud experience. I do not, and have not ever used Nextcloud, cannot recommend Nextcloud, and do not like getting 5 emails from being tagged in a random beta release. Please feel free to use this, or even reopen it, when there is a plan to actually move it forward. Good luck. AVIF is awesome.

And adding an extra reply, after reading this.
I 100% get it @aentwist. I'm sorry you're experiencing this.
It's very hard to find a release process that fits everyone. Many are ok with this, but there are PRs like yours that drags on time and unfortunately, as you can see, community or nc devs did not reviewed this PR in more than a year.

I really appreciate you taking the time to write this, especially since you mentioned you're not a php dev, that shows some good faith. If someone wants to re-open or create a separate PR to continue this work, please feel free to do so. Unless this gets scheduled from other Nextcloud devs, if a PR isn't all green and ready, it's hard for us to invest that much time into guiding you to a mergeable state.

We're always open for discussion, and the closure is not a definitive state, it's about cleaning staled PRs.
Have a nice day!
John

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Format for all previews AVIF Allow more preview formats
10 participants