Plugin screenshots exhaust memory #6778

Closed
hypeJunction opened this Issue Apr 23, 2014 · 7 comments

Comments

Projects
None yet
5 participants
@hypeJunction
Contributor

hypeJunction commented Apr 23, 2014

I haven't investigated which screenshot exactly causes this, but it seems unreasonable to run this on each page load. Perhaps we should implement some cache, or copy these to filestore and get them from there?

[23-Apr-2014 12:05:52 UTC] PHP Fatal error:  Allowed memory size of 67108864 bytes exhausted (tried to allocate 23901680 bytes) in \engine\lib\filestore.php on line 141

[23-Apr-2014 12:05:52 UTC] PHP Stack trace:

[23-Apr-2014 12:05:52 UTC] PHP   1. {main}() \engine\handlers\page_handler.php:0

[23-Apr-2014 12:05:52 UTC] PHP   2. page_handler() \engine\handlers\page_handler.php:46

[23-Apr-2014 12:05:52 UTC] PHP   3. call_user_func() \engine\lib\pagehandler.php:53

[23-Apr-2014 12:05:52 UTC] PHP   4. admin_plugin_screenshot_page_handler() \engine\lib\pagehandler.php:53

[23-Apr-2014 12:05:52 UTC] PHP   5. get_resized_image_from_existing_file() \engine\lib\admin.php:545

[23-Apr-2014 12:05:52 UTC] PHP   6. imagecreatefrompng() \engine\lib\filestore.php:141
@Srokap

This comment has been minimized.

Show comment
Hide comment
@Srokap

Srokap Apr 23, 2014

Contributor

I think we should introduce general limit on X*Y value where X and Y are image dimensions. This should be main memory filler when scaling/rotating with GD lib, so it makes sense to limit amount of data instead of bounding box. We also can check this limit before loading bitmap to memory, so that's good.

Not scaling image on every load is obviously another thing to address...

Contributor

Srokap commented Apr 23, 2014

I think we should introduce general limit on X*Y value where X and Y are image dimensions. This should be main memory filler when scaling/rotating with GD lib, so it makes sense to limit amount of data instead of bounding box. We also can check this limit before loading bitmap to memory, so that's good.

Not scaling image on every load is obviously another thing to address...

@Srokap Srokap added this to the Elgg 1.9.x milestone Apr 23, 2014

@iionly

This comment has been minimized.

Show comment
Hide comment
@iionly

iionly Apr 23, 2014

Contributor

Is there really a need to resize the image files of plugin screenshots? Who is going to see them? Only the admins. How often will the admins take a look at the screenshots? My guess is: not very often. Why not simply display them with fixed width and height that suits the page layout?

Contributor

iionly commented Apr 23, 2014

Is there really a need to resize the image files of plugin screenshots? Who is going to see them? Only the admins. How often will the admins take a look at the screenshots? My guess is: not very often. Why not simply display them with fixed width and height that suits the page layout?

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Apr 23, 2014

Contributor

I tend to agree with @iionly

Contributor

hypeJunction commented Apr 23, 2014

I tend to agree with @iionly

@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow Apr 23, 2014

Member

This is only being suggested because resizing images is hard to do right.
We should make it less hard, then the becomes a non issue.
On Apr 23, 2014 7:35 AM, "Ismayil Khayredinov" notifications@github.com
wrote:

I tend to agree with @iionly https://github.com/iionly


Reply to this email directly or view it on GitHubhttps://github.com/Elgg/Elgg/issues/6778#issuecomment-41168107
.

Member

ewinslow commented Apr 23, 2014

This is only being suggested because resizing images is hard to do right.
We should make it less hard, then the becomes a non issue.
On Apr 23, 2014 7:35 AM, "Ismayil Khayredinov" notifications@github.com
wrote:

I tend to agree with @iionly https://github.com/iionly


Reply to this email directly or view it on GitHubhttps://github.com/Elgg/Elgg/issues/6778#issuecomment-41168107
.

@iionly

This comment has been minimized.

Show comment
Hide comment
@iionly

iionly Apr 23, 2014

Contributor

The question of "to do it right" is not that easy to answer. The problem is that image resizing with the GD library can result in memory issues due to the image resolution (not image file size). My guess is that the origin of the image resizing being included is that it comes from the community plugin repository plugin. It makes sense in this plugin to resize the preview images because every user gets to see them (on the community site or when this plugin is installed on any other site). With the plugin images in the admin backend the number of users is much more limited.

One possibility "to get it right" - avoiding memory issues - would be to predict memory requirement necessary for the image resizing. But this would not solve the problem in all cases as you would still need to fall back on scaling the images if the memory limit of the site is too strict.

It might make sense to postphone the image resizing until Elgg 1.10 when a 3rd party library is used for image resizing instead of falling back on the GD library and simply scale the images in Elgg 1.9 with setting width and height.

Contributor

iionly commented Apr 23, 2014

The question of "to do it right" is not that easy to answer. The problem is that image resizing with the GD library can result in memory issues due to the image resolution (not image file size). My guess is that the origin of the image resizing being included is that it comes from the community plugin repository plugin. It makes sense in this plugin to resize the preview images because every user gets to see them (on the community site or when this plugin is installed on any other site). With the plugin images in the admin backend the number of users is much more limited.

One possibility "to get it right" - avoiding memory issues - would be to predict memory requirement necessary for the image resizing. But this would not solve the problem in all cases as you would still need to fall back on scaling the images if the memory limit of the site is too strict.

It might make sense to postphone the image resizing until Elgg 1.10 when a 3rd party library is used for image resizing instead of falling back on the GD library and simply scale the images in Elgg 1.9 with setting width and height.

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Jul 10, 2014

Contributor

Can we just drop the support for screenshots? I don't think they are of any use to anyone. They are hidden under more info, and people would have seen the screenshots on the community/provider site before downloading the plugin. I am removing my screenshots from all manifests, as they keep polluting the logs.

Contributor

hypeJunction commented Jul 10, 2014

Can we just drop the support for screenshots? I don't think they are of any use to anyone. They are hidden under more info, and people would have seen the screenshots on the community/provider site before downloading the plugin. I am removing my screenshots from all manifests, as they keep polluting the logs.

@jdalsem jdalsem self-assigned this Oct 20, 2014

@jdalsem

This comment has been minimized.

Show comment
Hide comment
Member

jdalsem commented Oct 30, 2014

ref #4301

@jdalsem jdalsem referenced this issue Jun 19, 2015

Closed

feature(plugins): improved plugin listing #8528

0 of 3 tasks complete

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Jun 25, 2015

feature(plugins): Improved plugin listing
Fixes #8412
Fixes #4158
Fixes #4301
Fixes #6778

BREAKING CHANGE:
The plugin views are redesigned to display in a lightbox. This completely
removes the views forms/admin/plugins/filter and forms/admin/plugins/sort.

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Jun 29, 2015

feature(plugins): Improved plugin listing
Fixes #8412
Fixes #4158
Fixes #4301
Fixes #6778

BREAKING CHANGE:
The plugin views are redesigned to display in a lightbox. This completely
removes the views forms/admin/plugins/filter and forms/admin/plugins/sort.

@mrclay mrclay closed this in #8588 Jun 29, 2015

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