Add ImageVisual gamma and smarter in-shader contrast limits

[tlambert03]

This is a companion to #1842. It puts (re)calculation of contrast limits (and adds gamma) to the ImageVisual shader. If clims are changed after the ImageVisual has been created, then the texture will only be rebuilt (_need_texture_upload == True) if the new clims are outside of the bounds of the previous clims. (note: unlike VolumeVisual in #1842, ImageVisual holds a copy of the data, so rescaling is easier).

possible embellishments include: allowing gamma and/or contrast limits to be set per channel (for an rgb image).

[djhoese]

Nice. You'll have to add docstrings for gamma and stuff. Does it make sense to do gamma on an RGB image at all?

[tlambert03]

oh right, will do.

Does it make sense to do gamma on an RGB image at all?

I see no reason why not! :) i just see it as 3 regular monochromatic channels. that said, I can't immediately think of an application. In any case it would be trivial to allow the gamma setter to accept either a float or a 3-tuple, and I don't think it would get in anyone's way.

[kmuehlbauer]

@tlambert03 and @djhoese Can this be realized via filters? At least gamma looks like it can be done with filters.

[tlambert03]

@tlambert03 and @djhoese Can this be realized via filters? At least gamma looks like it can be done with filters.

not sure I follow. what do you mean by filters in this case? accessing a particular sample from a texture? If so, I can't see my way through that one... if not, let me know what i'm missing.

[kmuehlbauer]

@tlambert03 Sorry for being short with explanation.

Filters can be used to hook shader/code into existing visuals. There are some filters already implemented (see here). The good thing is, that all additional code goes into the filter. This prevents from crowding the Visual with additional kwargs/code, which are only needed for specific cases. The filter can be attached to and removed from the visual.

So here there might be a gamma-filter which can be attached to an ImageVisual.

[tlambert03]

ah! ok thanks, will read up.

[kmuehlbauer]

@tlambert03 This is an example with the Isoline-Filter in action (https://github.com/vispy/vispy/blob/master/examples/basics/scene/contour.py).

I might be wrong, but the gamma-stuff looks like it can implemented likewise.

[djhoese]

The color transform happens after the gamma though, isn't that a problem?

[tlambert03]

I just finished working my way through filters and hooks and stuff (very cool, glad to know about it), and had the same question. Yeah, we need to apply the gamma prior to the $color_transform here.

@kmuehlbauer, any tricks on using a filter when you actually need to hook somewhere the middle of the shader? Could the shader itself be rewritten in a way that made it "hookable" prior to $color_transofrm?

[kmuehlbauer]

@tlambert03 I'll have a closer look the next day. IIRC you can specify some position in the shaders, but I have to check first. I'll report back.

[djhoese]

I think you're talking about pre/post which are kind of talked about here:

vispy/vispy/visuals/filters/base_filter.py

Lines 40 to 43 in cf244a2

	vhook : {'pre', 'post'}
	Hook name to attach the vertex shader to.
	vpos : int
	Position in the hook to attach the vertex shader.

So pre means before the shader, post is after the shader. I'm not sure how the pos works. That seems error prone.

[kmuehlbauer]

@djhoese correct, this is what I'd like to figure out.

[tlambert03]

added gamma to docstring. Will wait to hear from @kmuehlbauer as to whether this can be done with filters... but otherwise, I think this is ready to go (from my perspective)

[kmuehlbauer]

@tlambert03 Sorry, I did not manage to look at it yet. Please let it sit for another two days, before I finally surrender.

This is a very nice addition and should not be hold off too long.

[kmuehlbauer]

@tlambert03 I'm currently testing against your branch. But I get the same error as in Travis:

using the example in /examples/basic/scene/image.py

/home/k.muehlbauer/miniconda3/envs/vispy/bin/python /home/k.muehlbauer/projects/vispy/examples/basics/scene/image.py
WARNING: Traceback (most recent call last):
  File "/home/k.muehlbauer/projects/vispy/examples/basics/scene/image.py", line 62, in <module>
    app.run()
  File "/home/k.muehlbauer/miniconda3/envs/vispy/lib/python3.8/site-packages/vispy/app/_default_app.py", line 62, in run
    return default_app.run()
  File "/home/k.muehlbauer/miniconda3/envs/vispy/lib/python3.8/site-packages/vispy/app/application.py", line 152, in run
    return self._backend._vispy_run()
  File "/home/k.muehlbauer/miniconda3/envs/vispy/lib/python3.8/site-packages/vispy/app/backends/_egl.py", line 112, in _vispy_run
    self._vispy_process_events()
  File "/home/k.muehlbauer/miniconda3/envs/vispy/lib/python3.8/site-packages/vispy/app/backends/_egl.py", line 107, in _vispy_process_events
    win._on_draw()
  File "/home/k.muehlbauer/miniconda3/envs/vispy/lib/python3.8/site-packages/vispy/app/backends/_egl.py", line 246, in _on_draw
    self._vispy_canvas.events.draw(region=None)  # (0, 0, w, h))
  File "/home/k.muehlbauer/miniconda3/envs/vispy/lib/python3.8/site-packages/vispy/util/event.py", line 455, in __call__
    self._invoke_callback(cb, event)
  File "/home/k.muehlbauer/miniconda3/envs/vispy/lib/python3.8/site-packages/vispy/util/event.py", line 473, in _invoke_callback
    _handle_exception(self.ignore_callback_errors,
  << caught exception here: >>
  File "/home/k.muehlbauer/miniconda3/envs/vispy/lib/python3.8/site-packages/vispy/util/event.py", line 471, in _invoke_callback
    cb(event)
  File "/home/k.muehlbauer/miniconda3/envs/vispy/lib/python3.8/site-packages/vispy/scene/canvas.py", line 217, in on_draw
    self._draw_scene()
  File "/home/k.muehlbauer/miniconda3/envs/vispy/lib/python3.8/site-packages/vispy/scene/canvas.py", line 266, in _draw_scene
    self.draw_visual(self.scene)
  File "/home/k.muehlbauer/miniconda3/envs/vispy/lib/python3.8/site-packages/vispy/scene/canvas.py", line 304, in draw_visual
    node.draw()
  File "/home/k.muehlbauer/miniconda3/envs/vispy/lib/python3.8/site-packages/vispy/scene/visuals.py", line 99, in draw
    self._visual_superclass.draw(self)
  File "/home/k.muehlbauer/miniconda3/envs/vispy/lib/python3.8/site-packages/vispy/visuals/visual.py", line 435, in draw
    if self._prepare_draw(view=self) is False:
  File "/home/k.muehlbauer/miniconda3/envs/vispy/lib/python3.8/site-packages/vispy/visuals/image.py", line 490, in _prepare_draw
    self._build_texture()
  File "/home/k.muehlbauer/miniconda3/envs/vispy/lib/python3.8/site-packages/vispy/visuals/image.py", line 461, in _build_texture
    self.shared_program['clim'] = self.clim_normalized
  File "/home/k.muehlbauer/miniconda3/envs/vispy/lib/python3.8/site-packages/vispy/visuals/image.py", line 297, in clim_normalized
    range_min, range_max = self._texture_limits
TypeError: iteration over a 0-d array
ERROR: Invoking <bound method SceneCanvas.on_draw of <SceneCanvas (egl) at 0x7fcc4d29f160>> for DrawEvent

[kmuehlbauer]

Problem is if clim='auto'. Then this string gets propagated to self._texture_limits and this is expanded to range_min, range_max = self._texture_limits in clim_normalized.

[tlambert03]

Ah yep, sorry I missed that before you sat down to take a look. Will fix later today

…

On Mar 22, 2020, at 6:04 AM, Kai Mühlbauer ***@***.***> wrote:  Problem is if clim='auto'. Then this string gets propagated to self._texture_limits and this is expanded to range_min, range_max = self._texture_limits in clim_normalized. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[tlambert03]

ok, fixed... but let me know if you approve of my approach:
it seems that RGB images with clim = 'auto' don't ever actually look at the data min/max as described in the docstring... which I guess just means we assume that all RGBs have been delivered pre-normed to 0-1 right? I added an else clause for that case that just sets clim to (0,1) (overwriting auto as is also done for the non-RGB case)...
If you'd prefer that clim stay as the str "auto", I can add some different logic to keep the shader happy.

[djhoese]

Does it make sense to scale RGBs ever? I saw this napari issue (napari/napari#130) so I'm curious what @sofroniewn has to say too. Ugh as I'm typing this I realize I have an application that does exactly this but with float textures. For meteorological satellite imagery it is common to combine three (or more) image arrays in to a single RGB/A image and tweak the limits that go in to the RGB. If this wasn't a science visualization library I'd say we shouldn't, but I think we have to.

[kmuehlbauer]

@djhoese @tlambert Currently digging into the filter thing. Filters are used very widely (eg for picking). There two Filters are used (AlphaFilter and PickingFilter). Nevertheless I could not wrap my head around the hook-position stuff. This is quite hard to follow.

Anyway from looking at the code we could work the clim stuff and the gamma stuff the same way color_transform is done by using color_transform for this and just chaining the transformations in the proper way. Does this make sense?

[tlambert03]

If this wasn't a science visualization library I'd say we shouldn't, but I think we have to.

Yeah, I think this is exactly the point. I agree that scaling an RGB image (which is already usually pre-scaled) is unusual... but I don't think it's "nonsensical" per se. It's all about visualization afterall, and it's conceivable that someone might want to better visualize shadows/highlights in an RGB image. Here's an example of how napari could work if this PR merged:

[tlambert03]

ok, I changed it to put the logic in the color_transform... There was one slightly ugly thing: vispy.shader.Function didn't seem to allow for type overloads (or I couldn't figure it out), so the build_color_transform ended up a bit more verbose.

I'm not sure I feel that strongly one way or the other.

• Here is the comparison between master and the previous version, where the logic is in the main shader.
• Here is the comparison between master and the current version, where the logic is in the color_transform.

Let me know what you think

[kmuehlbauer]

@tlambert03 I'll have a look tomorrow morning (cet) and report back. Any other comments from @djhoese or others are much appreciated.

[djhoese]

I'm not sure it is any cleaner, but I guess it is more reusable. Could you add some tests for all of this?

[tlambert03]

hmm, I might need some guidance here. wasn't sure how you tested shaders, so I took a look at test_image.py and I see that it checks that the offscreen buffer matches some pre-generated image in https://github.com/vispy/test-data.

since contrast adjustment will cause clipping on the offscreen buffer, seems like the only way to actually test the shader here is to either add new images for both gamma and clim adjustment to the test-data repo, or somehow modify assert_image_approved to be able to modify the standard_file (which seems like a bad approach). suggestions?

[kmuehlbauer]

@tlambert03

Side note: After a bit of thinking I have to admit that it is more natural to have these changes within the ImageVisual since it's close connection to Images itself. Nevertheless good thing to get filters back into mind.

I've had not the time to do a thorough review. The version where the logic is in the color_transform indeed looks more reusable. It might also be better suited for later refactoring, if there will be the need to do so.

Regarding the tests, we would need to add new images AFAICS. In this PR was a short discussion on image comparison and further I mentioned some pitfalls I hit when adding the image to the test-repo. Also @djhoese and @larsoner might help out, if necessary.

@tlambert03 Thanks for doing this, this is a great addition to ImageVisual!

[kmuehlbauer]

@tlambert03 After skimming the referenced PR you might also just compare buffers as @djhoese suggested over there if they are small enough.

[tlambert03]

I added some tests. I struggled a bit with rounding error, particularly with gamma... still not quite clear to me why. anyway, I asserted that the post-rendered 8-bit values in the canvas buffer are within 1 of the expected value (atol=1), while also confirming that changing clim/gamma does significantly change the rendered buffer. (for example)

[kmuehlbauer]

@tlambert03 This is looking good to me.

Just one suggestion to change. The second thing is, how to handle the string quotes. There have been some changes in string quoting introduced. I'm not sure vispy has some guidelines, but looking at the code we are using kind of "single quotes for data, double quotes for human-readable strings". We should at least be consistent throughout the code. Thoughts @djhoese?

[djhoese]

I've never actually heard of that single/double quote for data/human-readable concept. I'm ok trying to have something like this but would like it defined in a contributor's guide in the docs. And/or we switch to some flake8/black/pep8 auto-checker and fixer.

[kmuehlbauer]

I'm OK with any solution. But my main concern is that it should be consistent.

[tlambert03]

sure, will change. I use black (which sets these all as double quotes) for almost all my other projects, so I have many times had to undo a muscle-memorized "format document" when working on these files :)

[tlambert03]

ok, made the requested changes. Once this is all set to go and approved, I'd love if we could also work
getting #1842 in (that one has a far bigger impact on performance in napari) ... and perhaps use a similar strategy with the mesh visual?

[kmuehlbauer]

@djhoese Sorry for my ignorance, but I'm not (yet) adapted to azure. Any idea why the linux build fails?

[djhoese]

It was some weird error about a missing wheel subpackage even though it was already building a wheel. I restarted the job and hopefully it will pass now.

[djhoese]

Still failing, but found the issue: pypa/manylinux#512

An update to the manylinux docker images is causing it. Not your fault.

[djhoese]

The manylinux image issue was fixed. I restarted the Azure jobs and now they pass. I took today off and am taking Monday off too. Unless I decide to work on VisPy stuff on those days I probably won't look at this and the other PR until later next week. The last time I looked at this code I thought it was good to go. I'll re-review when I come back to this, but I think we can consider this one done. Thanks again @tlambert03!