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

Size limit for download as jpg #4172

Merged
merged 10 commits into from
Sep 24, 2015

Conversation

will-moore
Copy link
Member

See https://trac.openmicroscopy.org/ome/ticket/12982

This limits the download as jpeg / png / tiff to images of 12k * 12k or smaller.
This cut-off was determined by trying to download images of various sizes.
Above 2147483648 pixels (46k * 46k) we get

java.lang.NegativeArraySizeException: null
    at omeis.providers.re.RGBIntBuffer.<init>(RGBIntBuffer.java:62) ~[common.jar:na]

For 15k images we get

java.lang.OutOfMemoryError: Java heap space
    at omeis.providers.re.RGBIntBuffer.<init>(RGBIntBuffer.java:62) ~[common.jar:na]
    at omeis.providers.re.RenderingStrategy.getIntBuffer(RenderingStrategy.java:186) ~[rendering.jar:na]
    at omeis.providers.re.HSBStrategy.renderAsPackedInt(HSBStrategy.java:275) ~[rendering.jar:na]

but 12k images download OK, but quite slowly.

To test:

  • Select an image smaller than 12k * 12k or equivalent, and try to download as jpg/png/tiff (should work for 'big' images as well as regular images.
  • Select an image bigger than 12k * 12k and check that the download as jpg/png/tiff options are disabled with a suitable tooltip.
  • Select multiple images. If any single image is too big, the download as jpg/png/tiff options should be disabled. Otherwise, download as a zip should work OK.

--rebased-to #4223

@mtbc
Copy link
Member

mtbc commented Sep 16, 2015

(Regarding that first puzzling exception message see c9612623)

Limit set by OOM error in omeis.providers.re.RGBIntBuffer
"""
can = True
k = 12100 * 12100
Copy link
Member

Choose a reason for hiding this comment

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

this could go to settings.py

@pwalczysko
Copy link
Member

The functionality works as described in the PR header.
Interesting edge case (sizewise, not usage-wise) is the 5.0 data/big-svs/UMD001_ORO.svs which is on the trout db in every group in Project called 5.0 data, Dataset big-svs.
The size is 17317 x 11041. It did download on trout latest, but very slowly.
I understand there is some discussion still ongoing about where exactly to put the cutoff
@will-moore @aleksandra-tarkowska

@will-moore
Copy link
Member Author

As discussed with @aleksandra-tarkowska, since the limit depends on OOM error, this may vary with different set-ups so it's best to make the limit configurable (see last commit)

@will-moore
Copy link
Member Author

You can now set cut-off like this:

$ bin/omero config set omero.client.download_as_jpg.max_size 1000000
$ bin/omero admin restart

Then log out of web & log in again....

@jburel
Copy link
Member

jburel commented Sep 18, 2015

the same limit will hold for export as png so I will strip the jpg

@will-moore
Copy link
Member Author

@jburel but then you are left with 'download_as.max_size' which doesn't have so much meaning. I could go for download_as_jpg_tiff_png.max_size but that seems like overkill.


:return: Integer
"""
size = self.getConfigService().getConfigValue(
Copy link
Member

Choose a reason for hiding this comment

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

The same here, it is breaking change for 5.1 series. It should go in try catch block to guarantee compatibility with < 5.1.4 server. see #4191

@will-moore will-moore closed this Sep 21, 2015
@will-moore will-moore reopened this Sep 21, 2015
@@ -628,6 +628,12 @@ def leave_none_unset_int(s):
" 'webtest/webclient_plugins/center_plugin.overlay.js.html',"
" 'channel_overlay_panel']``. "
"The javascript loads data into ``$('#div_id')``.")],
"omero.web.download_jpg.max_size":
Copy link
Member

Choose a reason for hiding this comment

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

that can be removed now

@sbesson
Copy link
Member

sbesson commented Sep 21, 2015

Closing/reopening to include #4194 in the merge commit for Travis.

@sbesson sbesson closed this Sep 21, 2015
@sbesson sbesson reopened this Sep 21, 2015
@sbesson sbesson closed this Sep 21, 2015
@sbesson sbesson reopened this Sep 21, 2015
@snoopycrimecop snoopycrimecop mentioned this pull request Sep 22, 2015
@dominikl
Copy link
Member

@jburel Guess this new property has to be taken into account by Insight as well, right? I think at the moment jpg export is only allowed for non-tiled images.

@jburel
Copy link
Member

jburel commented Sep 22, 2015

@dominikl : it will have to, This is applicable in the viewer only we will also have to adjust the "export as" script used to export as jpeg/png/tiff (right-hand panel)

@jburel
Copy link
Member

jburel commented Sep 22, 2015

I still think we should modify the name to something like size_download_as.
We then have method like canExportAsJpg/downloadSsJpeg with description "can export as jpeg, png etc. This is even more confusing

@atarkowska
Copy link
Member

As this PR is not ready yet, could you please exclude it as is blocking #4191 (this is blocker for 5.1.4 and should be reviewed ASAP)

@jburel
Copy link
Member

jburel commented Sep 22, 2015

This PR will also require changes in insight to be in synch and also in the export script (i.e. 2 more PRs) This, in my opinion, will require further testing and I do not feel comfortable to have it included for the release.
We do not know the consequence, for example, if we run the script on several large images.
cc @pwalczysko @gusferguson, @dominikl

@will-moore
Copy link
Member Author

@jburel From the user's point of view in web, this PR fixes the 404 bug (by preventing the user trying to download as jpg when the image is too big) and this also prevents OOM errors on the server etc. So I think this should be merged as it is, without waiting for Insight. This is not a feature that was previously aligned between the clients, so we are not introducing divergence here. This is simply the first step of convergence!

@atarkowska
Copy link
Member

@will-moore I agree with you but I think priority is to get #4191 in as it is blocker, this PR could be merged just after that as it introduce new property

@will-moore will-moore closed this Sep 22, 2015
@jburel
Copy link
Member

jburel commented Sep 22, 2015

The first step will be to turn it off if the image is a big image with the actual limit i.e. the one used to build pyramid. That parameter is already in place
That is also the limit used in the script => no need for further PRs and extra tests

@will-moore will-moore reopened this Sep 22, 2015
…pg_12982

Conflicts:
	components/tools/OmeroWeb/omeroweb/decorators.py
@will-moore
Copy link
Member Author

Sorry - missed this last night - just fixing now...

@will-moore
Copy link
Member Author

@jburel @sbesson Final change is in now - revert to using 'Big Image' size as limit for download as.
You want to do a final build now?

@jburel
Copy link
Member

jburel commented Sep 23, 2015

The export as "JPEG, PNG, TIFF" options are greyed out for Big image
The OME-TIFF option should probably also be greyed out since it is not possible to perform it but it does not crash so this can be done in another PR
Tested with 8kx8k.jpeg

@jburel
Copy link
Member

jburel commented Sep 23, 2015

The options are greyed out but if you click on the option e.g. png, you can still download the image as png for example.

@will-moore
Copy link
Member Author

@jburel Should be fixed now!

@dominikl
Copy link
Member

Works like expected. Export as JPG/PNG/TIFF options are greyed out and disabled if a selected image is a 'big' image. Export as OME-TIFF still avaiable, fails on 'big' images but provides an explanation why it failed.

@jburel
Copy link
Member

jburel commented Sep 24, 2015

Added an item to https://trello.com/c/4mDNygGv/38-5-1-3-follow-up so we can work on the new limit in 5.2

sbesson added a commit that referenced this pull request Sep 24, 2015
@sbesson sbesson merged commit fdca377 into ome:dev_5_1 Sep 24, 2015
@sbesson sbesson added this to the 5.1.4 milestone Sep 25, 2015
@will-moore will-moore deleted the big_image_export_jpg_12982 branch February 18, 2019 04:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants