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

Some images generated with imaginary are incorrect #323

Closed
janusn opened this issue Nov 9, 2022 · 28 comments · Fixed by nextcloud/server#35105
Closed

Some images generated with imaginary are incorrect #323

janusn opened this issue Nov 9, 2022 · 28 comments · Fixed by nextcloud/server#35105
Assignees

Comments

@janusn
Copy link

janusn commented Nov 9, 2022

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 docker: linuxserver/nextcloud:latest
  • Nextcloud version 24.0.6
  • previewgenerator version 5.1.0
  • imaginary docker: h2non/imaginary:latest
  • Imaginary version 1.2.4
  • host OS: DSM 7.1.1-42962 Update 1
  • hardware: Synology DS-918+

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
Copy link
Member

st3iny commented Nov 10, 2022

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
Copy link

szaimen commented Nov 10, 2022

@janusn which Imaginary version are you running?

@szaimen
Copy link

szaimen commented Nov 10, 2022

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

@janusn
Copy link
Author

janusn commented Nov 10, 2022

@szaimen The version of imaginary is 1.2.4.

@szaimen
Copy link

szaimen commented Nov 10, 2022

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
Copy link
Author

janusn commented Nov 11, 2022

@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

  • /[User]/files/Photos/Birdie.jpg
  • /[User]/files/Photos/Frog.jpg
  • /[User]/files/Photos/Gorilla.jpg
  • /[User]/files/Photos/Toucan.jpg

but the previews of the following photos are correct:

  • /[User]/files/Photos/Library.jpg
  • /[User]/files/Photos/Nextcloud community.jpg
  • /[User]/files/Photos/Vineyard.jpg

The version of the aio-imaginary:

$ imaginary --version
dev

@szaimen
Copy link

szaimen commented Nov 11, 2022

Right. Then maybe @CarlSchwan has an idea?

@st3iny
Copy link
Member

st3iny commented Nov 11, 2022

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
Copy link

szaimen commented Nov 11, 2022

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: h2non/imaginary@234d1fe
https://github.com/nextcloud/server/search?q=X-Image-Width

@st3iny
Copy link
Member

st3iny commented Nov 11, 2022

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
Copy link
Member

st3iny commented Nov 11, 2022

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
Copy link

szaimen commented Nov 11, 2022

This should be fixable by parsing both headers

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

@szaimen
Copy link

szaimen commented Nov 11, 2022

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

@st3iny
Copy link
Member

st3iny commented Nov 11, 2022

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
Copy link

szaimen commented Nov 11, 2022

Sounds good! :)

@st3iny
Copy link
Member

st3iny commented Nov 11, 2022

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

@szaimen
Copy link

szaimen commented Nov 11, 2022

Fine by me 👍

@szaimen
Copy link

szaimen commented Nov 11, 2022

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

@st3iny
Copy link
Member

st3iny commented Nov 11, 2022

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

Great idea!

@szaimen
Copy link

szaimen commented Nov 11, 2022

Let me have a look at this then :)

@szaimen
Copy link

szaimen commented Nov 11, 2022

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

@janusn
Copy link
Author

janusn commented Nov 11, 2022

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

@szaimen
Copy link

szaimen commented Nov 11, 2022

Yes, will do. Thanks for bringing this up!

@janusn
Copy link
Author

janusn commented Nov 11, 2022

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

szaimen commented Nov 11, 2022

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

@st3iny
Copy link
Member

st3iny commented Nov 15, 2022

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

I'm on it.

st3iny added a commit to nextcloud/documentation that referenced this issue Nov 15, 2022
Ref nextcloud/previewgenerator#323
Ref nextcloud/server#35105

Signed-off-by: Richard Steinmetz <richard@steinmetz.cloud>
@szaimen
Copy link

szaimen commented Nov 15, 2022

Thanks! :)

st3iny added a commit to nextcloud/documentation that referenced this issue Nov 15, 2022
Ref nextcloud/previewgenerator#323
Ref nextcloud/server#35105

Co-authored-by: Simon L. <szaimen@e.mail.de>
Signed-off-by: Richard Steinmetz <richard@steinmetz.cloud>
backportbot-nextcloud bot pushed a commit to nextcloud/documentation that referenced this issue Nov 30, 2022
Ref nextcloud/previewgenerator#323
Ref nextcloud/server#35105

Co-authored-by: Simon L. <szaimen@e.mail.de>
Signed-off-by: Richard Steinmetz <richard@steinmetz.cloud>
backportbot-nextcloud bot pushed a commit to nextcloud/documentation that referenced this issue Nov 30, 2022
Ref nextcloud/previewgenerator#323
Ref nextcloud/server#35105

Co-authored-by: Simon L. <szaimen@e.mail.de>
Signed-off-by: Richard Steinmetz <richard@steinmetz.cloud>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants