Add a new renderer for LOD images

[xiaoyu-wu]

A new pair of image source and renderer are added to visualize an image larger than memory size and has multiple level of details (LODs). Images are rendered with increasing resolution so that the UI remain responsive during scrolling.

[corranwebster]

I'd like to think a bit about how to generalize this and possibly combine with the (largely stubbed out) downsample methods. The core ideas are good.

[corranwebster]

I think that using Traits Futures is probably preferable: https://github.com/enthought/traits-futures . The TraitsExecutor in that library is probably a drop-in replacement (or close to it).

[corranwebster]

Playing around with the example, I'm noticing a few issues with the UX which would be nice to fix.

The first is that it is very jittery when moving, as it renders the first few LoD before getting the bounds change event - it would be nice to have some mechanism to estimate the initial LoD based on the render time we have (rough guideline is that you have 50-100ms for the first render) - since we anticipate that getting the data is the slow operation, we might be able to step through the first few LoD without rendering if we get the data fast enough.

The second is that there may be an issue with bounds estimation, in that there is a jittery white area in the direction that is being scrolled. Either we're not estimating the slices correctly and so underpainting, or the area we are painting on is getting out of sync with the movement.

Thirdly, if I replace the scrollbars with the more usual pan and zoom tools I get distortion of the image while panning. This would also point to the slicing not being done correctly.

As much as possible with this we'd like the UX for LoD to be as unobtrusive as possible.

[corranwebster]

I think that this is the wrong way of going about this. The current ImagePlot is heavily optimized for the use case of a (possibly large) in-memory image and so trying to hook into this is likely to cause issues since it is effectively a specialized "leaf" subclass (getting CMapImagePlot to play nicely as a subclass was fairly awful). Subclassing a "leaf" subclass is almost always problematic (@mdickinson would probably say "never do this").

So what is the "right" way to do it? In terms of data flow, I'd use a different approach:

• have a data source which holds the lowest resolution LoD (either completely in-memory, or the lowest LoD that can load a slice in a few milliseconds from a disk/remote data store) and presents it as a regular ImageData
• when the bounds change, calculate the correct LoD for the zoom level and kick off a background job to get the data from the slow store and compute the high-resolution image chunk, and then schedule a re-draw
• if the re-draw happens first it will follow the standard path for rendering the low-resolution image
• when the background job has computed the high-resolution image it will replace the cached image with its own (assuming the bounds haven't changed since it started) and request another redraw to update. If it happens to finish before the re-draw then the high-resolution image will already be in the cache, so the generation of the low-resolution image will be skipped.

This data flow will likely give a better UX - smooth low-res scrolling, followed by a snap to the appropriate LoD. There should also be a flag to bypass all of this when rendering off-screen (the existing use_downsampling would work well).

Architecturally, I'd be tempted to introduce the LoD API as part of the basic abstract data source API (eg. adding an optional LoD argument to get_data) and integrating this into the base ImagePlot and ImageData classes (taking some care with the dependencies - we can probably assume Python 3 for this, but it may not hurt to use Traits Futures) and use the use_downsampling flag to toggle the LoD behaviour.

[corranwebster]

The function signature doesn't match that of the superclass.

[corranwebster]

Suggested change

	more pixes in both x and y than the screen area.
	more pixels in both x and y than the screen area.

[xiaoyu-wu]

@corranwebster Thanks for your thoughts. I have made changes to BaseDataSource, ImageData, ImagePlot based on suggested structure and data flow.

Support for LOD behavior is now toggled by ImageData.support_downsampling and ImagePlot.use_downsampling. Computation of cache for high resolution images is submitted to a TraitsExecutor upon change of index_mapper and bounds and redraws after the result is ready.

With such change, the original UX issues are now resolved naturally, which can be observed from the updated demo.

To pass CI tests, I added traits_futures to the dependencies.

[corranwebster]

Should update the docstring below to describe the new optional argument

[xiaoyu-wu]

Updated.

[corranwebster]

This shouldn't inherit from Handler. This creates a problem with the life-cycle of the executor - we may need Enable to send a new type of event to indicate that the plot is being closed.

[xiaoyu-wu]

How about overriding the cleanup method of Component and adding codes that stop the executor there?

[corranwebster]

I think the main remaining concern I have is around the life-cycle of the executor. I had some discussion with Mark Dickinson about this this morning and his feeling was that the executor should be controlled by the application and passed to the plot, which at a minimum means that the executor should be a public trait not a private one, but also probably means that we shouldn't try to clean it up unless we know that the plot created it as a default rather than user code passing it in (noting that on trait change handlers have a distressing tendency to create defaults for the old value only to immediately throw it away).

Which brings up the second part of the concern, which is that the cleanup method isn't guaranteed to be called: in particular if the plot is removed from its container before the window is closed, then the cleanup method never gets called. This is probably a bug in Enable - cleanup should probably be called as part of Component.remove as well. Fixing the Enable bug is outside the scope of the PR, but we should be aware of it.

Other than these concerns, the PR is coming together nicely.

[xiaoyu-wu]

I think the main remaining concern I have is around the life-cycle of the executor.

Thanks for your thoughts on this. I agree that the traits executor is better a public trait and passed in during instantiate. (In fact, the very original commit was using a passed-in job scheduler.)

Two possible solutions are:

1. Enforce that a trait executor should be passed in during instantiation of the ImagePlot object if use_downsampling is True. In this case, creating / destroying of the executor should be handled explicitly by the user.
2. Check for a passed-in executor but also create one if not provided. In this case, we need a flag to help remember if the executor is created by this plot and be responsible for its destruction, which I believe introduced too much complexity that is not strongly related to the core functions of this class.

I prefer the 1st solution. Would you agree? @corranwebster

[mdickinson]

I also prefer the 1st solution, requiring an executor. That always leaves open the possibility of making it optional later on (i.e., moving from solution 1 to solution 2) if there's demand for that. Going the other way (implementing solution 2, realising that it's problematic, and then removing functionality) would be harder.

[siddhantwahal]

An edge case that isn't handled here occurs when col_min is greater than array_width or col_max is less than 0. These only occur when the image disappears from the plot, e.g., panning the image to the right as much as possible and then zooming in leads to negative x_max and col_max.

While x_min and col_min are also negative in this example, col_min is explicitly clipped to 0, whereas col_max isn't, leading to col_max - col_min < 0.

When these bounds are used to compute the LOD in _calculate_necessary_lod:

chaco/chaco/image_plot.py

Lines 432 to 438 in e41cfd3

	for lod in range(len(self.value.lod_data_entry))[::-1]:
	index_bounds, screen_rect = self._calc_zoom_coords(virtual_rect, lod=lod)
	array_width = index_bounds[1] - index_bounds[0]
	array_height = index_bounds[3] - index_bounds[2]
	if (array_width >= screen_rect[2]) and (array_height >= screen_rect[3]):
	break
	return lod

no LOD will satisfy col_max - col_min >= screen_rect[2], and the necessary LOD is set to 0. This causes the highest resolution image to be unnecessarily cached, potentially crashing the program if the image is too large to fit in memory (actually, only the slice [row_min:row_max, :-abs(col_max)] is cached, but that can still be large).

The fix is simple:

-        col_min = max(col_min, 0)
-        col_max = min(col_max, array_width)
-        row_min = max(row_min, 0)
-        row_max = min(row_max, array_height)
+        col_min, col_max = np.clip([col_min, col_max], 0, array_width)
+        row_min, row_max = np.clip([row_min, row_max], 0, array_height)

[corranwebster]

FWIW - some other Enthought teams have reported success using traits-futures but just using it to directly update the plot data as LoD information comes in, without needing to integrate traits futures into the plotting code.