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

Dynamic generation of the LUTs png #581

Merged
merged 20 commits into from
Nov 20, 2024
Merged

Dynamic generation of the LUTs png #581

merged 20 commits into from
Nov 20, 2024

Conversation

Tom-TBT
Copy link
Contributor

@Tom-TBT Tom-TBT commented Sep 17, 2024

Hello,
here's the change I propose to generate the LUT png dynamically within a view of webgateway.

I opened the PR so we can discuss it from here, but I haven't yet solved the version compatibility problems between OMERO.web and the plugins.

What I have done:

  • change views.listLuts_json to return index for all luts on the server (before, only the ones in LUTS_IN_PNG had index > -1)
  • Added a views.luts_png that generates a png, with a custom-made reader for different LUT encoding (binary, text, tsv)
  • Changed the URL of the loaded png from url('../img/luts_10.png') to url('/webgateway/luts_png/')

Several other points:

  • I generate the gradient used for the shading of color bars. How did you make it originally? The original gradient isn't exactly linear and stops at about 2/3rd of the array:
    image
    I guess that the gradient stopping at half is for display. Here I have a slight change with numpy.linspace(255, 0, 180, dtype="uint8") but display is very similar
  • The generation of the LUT png takes about 2 seconds on my dev server, but after the first time, it's cached with Django. However, this will be slow on servers that don't have the caching setup.
  • The caching uses a hash of the LUT names present on the server. If a LUT is added or removed, the hash changes and it generates a new image. I don't think invalidating the older cache matters here, as reverting a change in the LUTs would still reuse the "old cache" (anyway the old cache should eventually timeout)

@will-moore
Copy link
Member

@Tom-TBT I created it by importing a single-channel Image into OMERO that is a gradient from black to white, like the "fake" images (or I created it via the API conn.createImageFromNumpySeq() - can't remember which), then I applied each LUT to it in turn and had the rendering engine render it. Then stitched each of those together.
So, it is simpler logic because it lets the rendering engine do the work, and it ensures that you get the same LUT that will be used for rendering actual images.

@Tom-TBT
Copy link
Contributor Author

Tom-TBT commented Sep 17, 2024

then I applied each LUT to it in turn and had the rendering engine render it. Then stitched each of those together.

Sorry, I only meant for the thing called "gradient.png" in LUTS_IN_PNG. Overall I'm okay with the generation of the png (I would nonetheless double check my LUT reader implementation, but I'm getting the raw RGB values out of it).

@will-moore
Copy link
Member

Ah - understood. I don't remember about that I'm afraid. But if you have something that looks similar then that's fine.

@will-moore
Copy link
Member

The original "gradient.png" (pre-dating LUTs) was probably created manually in photoshop, then copied into the LUTs image later.

@@ -2076,23 +2077,97 @@ def listLuts_json(request, conn=None, **kwargs):
We include 'png_index' which is the index of each LUT within the
static/webgateway/img/luts_10.png or -1 if LUT is not found.
Copy link
Member

Choose a reason for hiding this comment

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

Need to update this docstring and mention version etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

docstring updated with mention to v5.28.0

@will-moore
Copy link
Member

Everything seems to be working, but I didn't manage to test adding a new LUT. I duplicated a LUT and renamed it to create a new one in the luts directory on my Docker server, but after restarting I'm not seeing the new one in the lists (nothing to do with this PR).

As I understand it, to use the dynamic LUTs list, clients need to use /webgateway/luts_png/ for the image and /webgateway/luts/?version=2 for the JSON listing?

So it should be possible for a client/app e.g. figure, to try reverse("webgateway_luts_png") and if the URL isn't found then stick with the old behaviour, but if it is then use the dynamic png and listing.

@Tom-TBT
Copy link
Contributor Author

Tom-TBT commented Sep 30, 2024

As I understand it, to use the dynamic LUTs list, clients need to use /webgateway/luts_png/ for the image and /webgateway/luts/?version=2 for the JSON listing?

Yes, that way omero-web is retrocompatible with the plugins.

So it should be possible for a client/app e.g. figure, to try reverse("webgateway_luts_png") and if the URL isn't found then stick with the old behaviour, but if it is then use the dynamic png and listing.

Thanks for the tip, I tried something like this, to then return the appropriate URL from a view of the plugin.
With that, the plugins are also compatible with earlier versions of omero-web. Figure and iViewer handle differently the LUT previews, so they will be patched with different implementations too.

@will-moore
Copy link
Member

I guess e.g. omero-figure could load the LUTS json with /webgateway/luts/?version=2 and check whether it gets back version:1 or version:2. If it gets version 2 then use /webgateway/luts_png/ for the image etc.

@knabar knabar added this to the 5.28.0 milestone Oct 4, 2024
@knabar
Copy link
Member

knabar commented Oct 7, 2024

I like this approach of dynamically generating the LUTS preview PNG.

Having the listLuts_json view handle a version argument seems to introduce a lot of conditionals both in that view and on the client side; could the view just return the old JSON structure with the new information added, e.g.

{
  luts: [{
    png_index: -1,
    png_index_new: 25,
    ...
  }],
  png_luts: ...,
  png_luts_new: ...,
}

This way old clients would still work, and new clients would also still use the same old URL and only need to check for the existence of png_luts_new - if it's there, they can use the new PNG URL.

@Tom-TBT
Copy link
Contributor Author

Tom-TBT commented Oct 8, 2024

Could the view just return the old JSON structure with the new information added

I changed the implementation. Now the new index and list are returned next to the old one. Implementing it here doesn't require any checks since changes on the client and server are packaged together (it won't be the case for figure and iviewer).

I have a question about the caching of the png. A new PNG is generated each time a LUT is renamed, added, or removed because there won't be a matching key in the cache.

But what should be the timeout of the cache? The generation of the PNG takes a few seconds. Setting it to None would prevent the cache from expiring and there would be a "slowdown" only the first time after a change in the LUT list. (It seems to me that the cache is invalidated when restarting omero-web). Would you be ok having that cache timeout set to None?

@knabar
Copy link
Member

knabar commented Oct 22, 2024

This looks good to me.

Since the cached value depends on the available LUTs, I don't see a problem with caching indefinitely.

@knabar knabar self-requested a review October 22, 2024 09:04
@Tom-TBT
Copy link
Contributor Author

Tom-TBT commented Oct 22, 2024

Thank you Andreas, I thus changed the timeout value to None.

@knabar
Copy link
Member

knabar commented Nov 1, 2024

@will-moore do you mind taking a final look at this PR before I merge? Thank you!

@jburel
Copy link
Member

jburel commented Nov 1, 2024

@will-moore could you please validate the following:

  • server with luts (Perceptibly uniform luts openmicroscopy#6398) and this PR: 8 new LUTs in the menu
  • server without the LUTS and this PR: same as before but the menu should be dynamically generated
  • server with luts and without this PR: menu without the new 8 LUTs

@will-moore
Copy link
Member

will-moore commented Nov 15, 2024

  1. Looks good with new LUTs displayed in webclient Preview panel (also works in webgateway viewer):

Screenshot 2024-11-18 at 09 57 55

  1. LUT generated dynamically, without server PR (no new LUTs). Menu looks fine too:

Screenshot 2024-11-18 at 09 23 55

@will-moore will-moore mentioned this pull request Nov 20, 2024
@knabar knabar merged commit 4f05b5e into ome:master Nov 20, 2024
10 checks passed
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.

4 participants