nextcloud / previewgenerator

Nextcloud app to do preview generation in the background.
https://apps.nextcloud.com/apps/previewgenerator
GNU Affero General Public License v3.0
450 stars 56 forks source link

Some images generated with imaginary are incorrect #323

Closed janusn closed 1 year ago

janusn commented 1 year ago

Some square preview images generated with command $ occ preview:generate-all -vvv are a crop of the original images and the aspect ratio are not preserved. On the other hand, the square preview images generated by web interface of Nextcloud are correctly. They respect the aspect ratios and letterboxed correctly.

Environment:

  • Nextcloud version 24.0.6
  • previewgenerator version 5.1.0
  • Imaginary version 1.2.4

The content of the config.php:

<?php
$CONFIG = array (
  'memcache.local' => '\\OC\\Memcache\\APCu',
  'memcache.distributed' => '\\OC\\Memcache\\Redis',
  'memcache.locking' => '\\OC\\Memcache\\Redis',
  'redis' => 
  array (
    'host' => 'redis',
    'port' => 6379,
    'timeout' => 1.5,
  ),
  'filelocking.enabled' => true,
  'datadirectory' => '/data',
  'instanceid' => '<omitted for security>',
  'passwordsalt' => '<omitted for security>',
  'secret' => '<omitted for security>',
  'trusted_domains' => 
  array (
    0 => '<omitted> for security>',
  ),
  'dbtype' => 'mysql',
  'version' => '24.0.6.1',
  'trusted_proxies' => ['swag'],
  'overwrite.cli.url' => '<omitted for security>',
  'dbname' => 'nextcloud',
  'dbhost' => '10.27.0.40:3306',
  'dbport' => '',
  'dbtableprefix' => 'oc_',
  'mysql.utf8mb4' => true,
  'dbuser' => '<omitted for security>',
  'dbpassword' => '<omitted for security>',
  'installed' => true,
  'app_install_overwrite' => 
  array (
    0 => 'files_excludedirs',
  ),
  'default_phone_region' => 'GB',
  'maintenance' => false,
  'mail_smtpmode' => 'smtp',
  'mail_smtpsecure' => 'ssl',
  'mail_sendmailmode' => 'smtp',
  'mail_smtpauth' => 1,
  'mail_from_address' => 'No.Reply',
  'mail_domain' => '<omitted for security>',
  'mail_smtpauthtype' => 'LOGIN',
  'mail_smtphost' => '<omitted for security>',
  'mail_smtpport' => '465',
  'mail_smtpname' => '<omitted for security>',
  'mail_smtppassword' => '<omitted for security>',
  'preview_max_x' => null,
  'preview_max_y' => null,
  'jpeg_quality' => '60',
  'preview_max_scale_factor' => 1,
  'enabledPreviewProviders' => [
    'OC\Preview\MP3',
    'OC\Preview\TXT',
    'OC\Preview\MarkDown',
    'OC\Preview\OpenDocument',
    'OC\Preview\Krita',
    'OC\Preview\Imaginary',
  ],
  'preview_imaginary_url' => 'http://imaginary:9000',
);

The previewgenerator settings:

$ php /config/www/nextcloud/occ config:app:get previewgenerator squareSizes
64 256 1024
$ php /config/www/nextcloud/occ config:app:get previewgenerator widthSizes
64 256 1024
$ php /config/www/nextcloud/occ config:app:get previewgenerator heightSizes
64 256 1024

A sample of an original image: 200042733-9284683a-bf54-4180-b0e4-63faba51b630

The preview generated for both grid view and detailed view on the Files pane. 200042792-55a96c2c-3ce2-4390-b3f3-6d4aa512268d

Another example of an original image: 200041760-34ad1f18-4f18-42bc-8549-486b11ad6d82

The preview generated for both grid view and detailed view on the Files pane. 200042005-39e74c9b-b6ce-4833-a9f8-e82b23ada8c3

st3iny commented 1 year ago

I see, that you are persistent.

I'll try to reproduce it. However, I suspect this being either a bug in the server or the imaginary upstream.

szaimen commented 1 year ago

@janusn which Imaginary version are you running?

szaimen commented 1 year ago

Can you for a test check if it works with the nextcloud/aio-imaginary:latest image corrrectly?

janusn commented 1 year ago

@szaimen The version of imaginary is 1.2.4.

szaimen commented 1 year ago

Yeah, so to make it work correctly, it is required to build imaginary from master IIRC since no new release was pushed since two years. That is what we are doing with the nextcloud/aio-imaginary:latest image. Hence my question if you could test with that.

janusn commented 1 year ago

@szaimen Thanks for the pointer. I have replaced the container from the image h2non/imaginary:latest with the image nextcloud/aio-imaginary:latest. Unfortunately, it still produces strange distorted previews.

In fact, you can see the previews of the build-in stock photos generated from aio-imaginary are distorted, such as

but the previews of the following photos are correct:

The version of the aio-imaginary:

$ imaginary --version
dev
szaimen commented 1 year ago

Right. Then maybe @CarlSchwan has an idea?

st3iny commented 1 year ago

On the other hand, the square preview images generated by web interface of Nextcloud are correctly. They respect the aspect ratios and letterboxed correctly.

I couldn't reproduce it. For me it's the exact other way around.

I only enabled the Imaginary provider to be able to isolate the issue. Both square previews generated by generate-all are fine. Square previews generated by the web interface are weird and look like your examples.

I'm currently investigating the server code to find a difference between both calls to the Preview API.

EDIT: I repeated the procedure multiple times and now all previews are broken. It seems that only the first previews ever generated are fine. After that, all previews are weird, even when I restart the Imaginary server.

szaimen commented 1 year ago

If I remember the discussion with Carl correctly, the maintainer of Imaginary submitted in the last moment a commit that broke something. Not sure if it is the same issue though. Probably it is then this: https://github.com/h2non/imaginary/pull/382/commits/234d1fecb7e0bc49f51f6f2152ba77c1d1388fa6 https://github.com/nextcloud/server/search?q=X-Image-Width

st3iny commented 1 year ago

I tested some more and noticed that the previews are only broken if the source image is smaller than the actual preview.

E.g. Supplying an Image with 8000x6000 produces correct results. Supplying an image with 640x480 produces weird results because Nextcloud asks for a preview of 1024x1024.

st3iny commented 1 year ago

Bingo!

@szaimen This is correct. I added debugging code to Imaginary which dumps each generated image to a file and they are correct. It seems like our server does not parse the width and height values correctly and thus distorts the image.

This should be fixable by parsing both headers.

szaimen commented 1 year ago

This should be fixable by parsing both headers

Indeed but only if the header was specified to be emitted, correct?

szaimen commented 1 year ago

See https://github.com/h2non/imaginary/pull/382/files#diff-77ed712c0c6f7b52686415504853bc522f5f0ee3e9e0399cfd60a48813aa6aaeR110

szaimen commented 1 year ago

So rather fail if one of these headers was not found instead of generating false previews?

st3iny commented 1 year ago

Yes, but I came up with a workaround. The image is now parsed when width and height are not specified. This has a minor performance impact but there is no way around it. We need to have the exact width and height values or previews will be distorted.

I'm also planning to amend the documentation with a warning to alert admins to use the -return-size flag. Ref https://docs.nextcloud.com/server/latest/admin_manual/installation/server_tuning.html#previews

szaimen commented 1 year ago

Sounds good! :)

st3iny commented 1 year ago

I'll also link to the aio docker image of Imaginary because the default one is outdated and doesn't include the flag ...

szaimen commented 1 year ago

Fine by me 👍

szaimen commented 1 year ago

However I am thjnking about tweaking the container so that -return-size is added by default. WDYT?

st3iny commented 1 year ago

However I am thjnking about tweaking the container so that -return-size is added by default. WDYT?

Great idea!

szaimen commented 1 year ago

Let me have a look at this then :)

szaimen commented 1 year ago

Should we improve the docs on this? Shall I do or will you do @st3iny ? :)

janusn commented 1 year ago

For the document, please mention the registry "nextcloud/aio-imaginary". Otherwise people like me will still get the wrong image. Thanks a ton!

szaimen commented 1 year ago

Yes, will do. Thanks for bringing this up!

janusn commented 1 year ago

@szaimen Thanks. I have forgotten to mention. It is better to give the imaginary a real version number to be returned by the following command as well. It is much easier to report any bug on it. $ imaginary --version

szaimen commented 1 year ago

I see, however since this is built staight from their github source code, I don't know how to apply that change...

st3iny commented 1 year ago

Should we improve the docs on this? Shall I do or will you do @st3iny ? :)

I'm on it.

szaimen commented 1 year ago

Thanks! :)