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

[RFC] Refactor image handling #28279

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

J0WI
Copy link
Contributor

@J0WI J0WI commented Aug 1, 2021

There are various tickets about poor image handling in Nextcloud.

Nextcloud uses the OC_Image wrapper which uses GD. But GD is slow and does not support formats like SVG. That's why some modules (avatar, preview and theming app) have implemented their own image handling.

This draft aims to replace the legacy OC_Image class with a generic Image class and a set of compatible backends using GD, Imagick, Gmagick or Vips. It turned out, that the IImage interface itself does not provide all methods and not all of them are meaningful for all backends (notably the obsolete GD resource). There's also no simple way to add additional file format support.

An alternative approach would be to pull a high level image library like intervention/image or imagine/imagine.

The goal is, to provide a generic image handling that uses the fastest backend and all common and modern file types available on the host. And last but not least, to replace direct Imagick usage in avatar, preview and theming app and finally deprecate the legacy OC_Image class.

@szaimen szaimen added the 2. developing Work in progress label Aug 8, 2021
@szaimen szaimen added this to the Nextcloud 23 milestone Aug 8, 2021
@skjnldsv skjnldsv mentioned this pull request Oct 13, 2021
@skjnldsv skjnldsv modified the milestones: Nextcloud 23, Nextcloud 24 Oct 21, 2021
@skjnldsv skjnldsv mentioned this pull request Mar 24, 2022
@blizzz blizzz mentioned this pull request Mar 31, 2022
This was referenced Apr 7, 2022
@blizzz blizzz modified the milestones: Nextcloud 24, Nextcloud 25 Apr 21, 2022
This was referenced Aug 12, 2022
@J0WI
Copy link
Contributor Author

J0WI commented Aug 18, 2022

@blizzz @szaimen I've seen you listed this on beta releases, but I need some feedback on this RFC before I continue here.

@CarlSchwan
Copy link
Member

Nice work, I'll review it tomorrow

@blizzz blizzz mentioned this pull request Aug 24, 2022
Copy link
Member

@icewind1991 icewind1991 left a comment

Choose a reason for hiding this comment

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

Overall a great step forward

lib/private/Image/Common.php Outdated Show resolved Hide resolved
lib/private/Image/Common.php Show resolved Hide resolved
lib/private/Image/Common.php Show resolved Hide resolved
lib/private/Image/Gd.php Outdated Show resolved Hide resolved
lib/private/Image/Vips.php Show resolved Hide resolved
tests/lib/Image/Vips.php Outdated Show resolved Hide resolved
@blizzz blizzz mentioned this pull request Aug 30, 2022
@J0WI J0WI removed this from the Nextcloud 25 milestone Aug 30, 2022
@szaimen szaimen added this to the Nextcloud 26 milestone Aug 30, 2022
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Found 100 potential problems in the proposed changes. Check the Files changed tab for more details.

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Found 100 potential problems in the proposed changes. Check the Files changed tab for more details.

@J0WI J0WI modified the milestones: Nextcloud 26, Nextcloud 27 Feb 23, 2023
@J0WI
Copy link
Contributor Author

J0WI commented Apr 2, 2023

ping @icewind1991 @skjnldsv

This was referenced May 3, 2023
@skjnldsv skjnldsv modified the milestones: Nextcloud 27, Nextcloud 28 May 9, 2023
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
This was referenced Nov 6, 2023
This was referenced Nov 14, 2023
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
This was referenced Mar 12, 2024
@Altahrim Altahrim mentioned this pull request Mar 20, 2024
@skjnldsv skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024
@skjnldsv skjnldsv modified the milestones: Nextcloud 30, Nextcloud 31 Aug 6, 2024
@skjnldsv skjnldsv closed this Aug 14, 2024
@skjnldsv skjnldsv removed this from the Nextcloud 31 milestone Aug 14, 2024
@sorbaugh
Copy link
Contributor

Hello @J0WI, it seems that some of the tickets you mentioned in the original comment have already been addressed so some the points have been addressed. I'm reopening this complex topic and would like to treat this as an "Overview" issue and maybe rescope it to the remaining open issues 🚀

@sorbaugh sorbaugh reopened this Aug 16, 2024
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Psalm found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

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.

7 participants