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

Confine projections to no more than 256MiB of raw data #115

Merged
merged 2 commits into from
Feb 14, 2020

Conversation

chris-allan
Copy link
Member

What this PR does

Projections from the OMERO.web default viewer are currently not confined. In 2019 the stack sizes across all active channels can easily be 1-2 GiB which will, on a reasonable machine, result in projection times > minute. The stack size can also be > Java maxint which will result in integer overflows and HTTP 500s.

This PR confines the maximum projection size as 256MiB of raw data, disabling the "Maximum Intensity" viewing option of the viewer if this condition is met.

Testing this PR

  1. Find an image where the projection size as defined as sizeX * sizeY * sizeZ * activeChannelCount * bytesPerPixel is greater than 256MiB as well as one that is not

  2. Open the viewer

  3. Ensure that the "Maximum Intensity" viewing option is disabled or enabled accordingly

@jburel
Copy link
Member

jburel commented Jan 28, 2020

The size limit 256 * 1024 * 1024 should probably be turned into a config value that other clients e.g. iviewer can access so that they can adjust the controls accordingly too

@chris-allan
Copy link
Member Author

@jburel: Agreed. I would have pursued that but it's 3+ PRs to do.

Shall we get this in and I can follow up with those changes and we can decide if this should actually be enforced at the rendering engine level as well? Since we're using the microservices predominantly it's less of an issue for us and adding a check there is very easy.

@jburel
Copy link
Member

jburel commented Feb 14, 2020

I will do some functional testing this am

@jburel
Copy link
Member

jburel commented Feb 14, 2020

Imported a fake image my-special-test-file&pixelType=uint8&sizeX=3000&sizeY=3000&sizeZ=300&sizeC=4.fake
Without this PR:
Screenshot 2020-02-14 at 10 29 28

With this PR:
I cannot click on the Maximum intensity radio button

From a "internal" viewer perspective, this works fine.

Do you want me to create an issue for the config parameter?

@chris-allan
Copy link
Member Author

Sure, I should be able to start the work on it next week and we can decide on when we want to get it in and how.

@jburel
Copy link
Member

jburel commented Feb 14, 2020

Discussed with @chris-allan,
open a PR in https://github.com/ome/omero-common property omero.pixeldata.max_projection_bytes
We can then consume that property when ready

@jburel
Copy link
Member

jburel commented Feb 14, 2020

merging
Adjustment to be made when we have the property in place

@will-moore
Copy link
Member

idr0077 (currently importing) has some images 1920 x 1920 x 285 or more and I can see users wanting to play a movie of the projection as at https://twitter.com/karlriha/status/1227154739198033920, but this PR won't be in that release I guess.

@will-moore
Copy link
Member

Apologies for not looking closer at the code before, but I noticed that the active channel counting only happens when you first launch the viewer (not when you turn channels on/off).
So it's possible to start off below the threshold, then go over the threshold by turning on multiple channels and projection doesn't get disabled.
I noticed this because of a similar issue porting this to iviewer.

To fix it properly, we'd need to check for disabling the projection when channels turn on/off.
Also, if we're already doing projection, to turn projection off when too many channels turn on. OR we disable channel buttons when projection is on, but that would probably be harder and more confusing.
Don't know if it's worth it in this viewer? or in iviewer?

@jburel
Copy link
Member

jburel commented Feb 17, 2020

The safest at this stage will be to check the number of channels regardless of the number of active channels
We will probably disable it deeper in the stack later on

@chris-allan
Copy link
Member Author

I can definitely open a follow up PR that switches the checks from the number of active channels to the total number of channels; that would err on the side of caution.

@chris-allan
Copy link
Member Author

Follow up is in #130.

/cc @jburel, @will-moore

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants