-
Notifications
You must be signed in to change notification settings - Fork 102
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
Updates to ThumbnailLoader and ThumbnailService #5827
Conversation
@@ -21,13 +21,14 @@ | |||
|
|||
package org.openmicroscopy.shoola.env.data.model; | |||
|
|||
import java.awt.Graphics2D; | |||
import java.awt.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no wildcard imports
4250654
to
e7ede6b
Compare
ThumbnailLoader calls new API, checks if image has pyramids and can determine what state a thumbnail is in if it is missing ThumbnailBean has additional simple load method that only returns a thumbnail if is available, otherwise returns an empty array
e7ede6b
to
78727d9
Compare
@@ -1423,4 +1488,4 @@ public void setDiskSpaceChecking(boolean diskSpaceChecking) { | |||
} | |||
} | |||
|
|||
} | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can just leave this EOL here.
Add empty implementation to Destroyable
Testing with a local build. Can't get the thumbnails in Insight:
Will do some investigation, maybe I can find out why... |
if (maxWidth <= 0) | ||
public ThumbnailLoader(SecurityContext ctx, Collection<DataObject> imgs, | ||
int maxWidth, int maxHeight, Collection<Long> userIDs) { | ||
if (images == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the NPE... Typo, has to be imgs
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concur:
Abnormal termination due to an uncaught exception.
java.lang.NullPointerException: No images.
at org.openmicroscopy.shoola.env.data.views.calls.ThumbnailLoader.<init>(ThumbnailLoader.java:303)
at org.openmicroscopy.shoola.env.data.views.calls.ThumbnailLoader.<init>(ThumbnailLoader.java:330)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Along with the s/thumbnailMetadata/thumbMetaData/
I'd also suggest you force push away all the whitespace changes.
StopWatch s1 = new Slf4JStopWatch("omero._createThumbnail"); | ||
if (thumbnailMetadata == null) { | ||
if (thumbMetaData == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did this variable get changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's now a local variable for the method. A new method that passes in thumbnailMetadata as a parameter was added.
} | ||
|
||
byte[] value = retrieveThumbnail(thumbnailMetadata); | ||
// I don't really know why this is here, no iquery calls being that I can see... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has to do with the Hibernate/transaction handling of this service since it's stateful.
} | ||
} | ||
|
||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment wasn't Javadoc. If you want it to be then best to remove the "non-Javadoc" and fix the @see
and add a bit more.
…k still is returned in some cases
|
||
private ThumbnailStorePrx getThumbnailStore(PixelsData pxd) throws DSAccessException, | ||
DSOutOfServiceException, ServerError { | ||
// System.out.println(image.getId()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could just be removed.
Looks good. There's just a commented out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two javadoc fixes needed
* @return The result is a negative integer if str1 is _numerically_ less than str2. | ||
* The result is a positive integer if str1 is _numerically_ greater than str2. | ||
* The result is zero if the strings are _numerically_ equal. | ||
* @note It does not work if "1.10" is supposed to be equal to "1.10.0". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
store.setRenderingDefId(rndDefId); | ||
} | ||
|
||
if (VersionCompare.compare(context.getGateway().getServerVersion(), VERSION_THUMBNAIL_NO_DEFAULT) >= 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a big issue as the gateway holds the server version as string, it doesn't request it over and over again. But if you want to keep it in Insight directly, you could add a LookupNames.SERVER_VERSION
(or LookupNames.5_4_8_OR_LATER
or something like that) and 'bind' it to the Registry when Insight connects to the server somewhere around here: https://github.com/openmicroscopy/openmicroscopy/blob/ffa5419fb669871f959973f68b2c3fa0e4e2b576/components/insight/SRC/org/openmicroscopy/shoola/env/data/DataServicesFactory.java#L575 (like the 'SESSION_KEY' field is bound to the Registry 3 lines above).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's where Gateway obtains it's server version during connection/session creation https://github.com/openmicroscopy/openmicroscopy/blob/040837bf61ce493d0a742a3fe871acc60847407d/components/blitz/src/omero/gateway/Gateway.java#L1066
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave it to you guys to work out what's most "Insight-esque"; my point is mostly about whether or not the albeit slight overhead of a redudant check for each and every thumbnail is a concern.
f54c3b5
to
466e94f
Compare
…sion of server connected to. Cleaned up javadoc
466e94f
to
9c4b34e
Compare
Looks good: For >= 5.4.8 the "Loading" thumbnail is displayed, for < 5.4.8 it's the clock. I tried to move the check to the login/connect method and add 'LookupName' to do it more the "Insight-esque" way. Don't know if you want to include it or have a different idea, rgozim#3 . |
int maxWidth = Integer.parseInt(getConfigService() | ||
.getConfigValue("omero.pixeldata.max_plane_width")); | ||
int maxHeight = Integer.parseInt(getConfigService() | ||
.getConfigValue("omero.pixeldata.max_plane_height")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these two properties should go into the connect method as well and get stored as 'LookupName' so that they are only requested once.
Thanks, guys. Considering the significant greenness of https://ci.openmicroscopy.org/job/OMERO-DEV-merge-daily/1015/ I'm going to merge this for an initial build. A combination of @dominikl's PR as well as handling of |
@@ -1227,11 +1317,11 @@ protected void actOnOneGroup(Set<Long> pixelsIds) { | |||
thumbnailMetadata = local; | |||
} | |||
|
|||
BufferedImage image = createScaledImage(theZ, theT); | |||
BufferedImage image = inProgress? null : createScaledImage(theZ, theT); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rgozim : can you explain this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, now a couple days late for our build so I'm merging for rc1. I think this change still needs explanation since (if I'm reading the diff correctly) it's the only change in logic outside of your new methods. What change in behavior did you need that led to it? How does it behave for bulk and non-bulk methods? Etc.
@will-moore points out that iviewer makes use of the single thumbnail loading method in |
// System.out.println(image.getId()); | ||
ThumbnailStorePrx store = service.createThumbnailStore(ctx); | ||
if (!store.setPixelsId(pxd.getId())) { | ||
store.resetDefaults(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is no longer covered in a try/catch block as it was previously. (cF. https://github.com/openmicroscopy/openmicroscopy/pull/5827/files?utf8=%E2%9C%93&diff=unified&w=1#diff-cc769d8303d8c7e9b3f1a015e3d7abfdL133)
This leads to:
serverExceptionClass = "ome.conditions.ApiUsageException"
message = "Unable to reset rendering settings in a read-only group for Pixels set id:149552"
at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
at java.lang.Class.newInstance(Class.java:442)
at IceInternal.BasicStream.createUserException(BasicStream.java:2779)
at IceInternal.BasicStream.access$300(BasicStream.java:14)
at IceInternal.BasicStream$EncapsDecoder10.throwException(BasicStream.java:3298)
at IceInternal.BasicStream.throwException(BasicStream.java:2291)
at IceInternal.OutgoingAsync.throwUserException(OutgoingAsync.java:399)
at omero.api.ThumbnailStorePrxHelper.end_resetDefaults(ThumbnailStorePrxHelper.java:3614)
at omero.api.ThumbnailStorePrxHelper.resetDefaults(ThumbnailStorePrxHelper.java:3501)
at omero.api.ThumbnailStorePrxHelper.resetDefaults(ThumbnailStorePrxHelper.java:3488)
at org.openmicroscopy.shoola.env.data.views.calls.ThumbnailLoader.getThumbnailStore(ThumbnailLoader.java:301)
at org.openmicroscopy.shoola.env.data.views.calls.ThumbnailLoader.access$000(ThumbnailLoader.java:70)
at org.openmicroscopy.shoola.env.data.views.calls.ThumbnailLoader$1.doCall(ThumbnailLoader.java:223)
What this PR does
Updates thumbnail loading of the Insight client to try convey the state of an images import status to a user.
Code has been added to Insight to check if an image to be displayed has pyramids. If it does, a check is performed to determine if an image has completed it's import process. Please note, this PR assumes that the final, and slowest, part of an image import is the pyramid generation.
Server code has been updated to only return a thumbnail if a thumbnail is available. Otherwise, nothing is returned and it becomes the clients responsibility to display a loading symbol.
Testing this PR
Setup a local OMERO.server, and import some images greater than 2K width/height (squig/ome/data_repo/test_images_good/png)
To do: add testing steps
Related reading
Trello card
Changes
Adds an extra public method to the ThumbnailStore / ThumbnailBean API
Changes logic of ThumbnailLoader in Insight.