enable icon indicating copy to clipboard operation
enable copied to clipboard

Retire SWIG-based Agg backend in favour of Cython-based Celiagg

Open corranwebster opened this issue 5 years ago • 6 comments

The SWIG-based Agg wrapper is showing its age: it is using an old version of Agg; it doesn't work with modern GCC or SWIG versions, the build process is idiosyncratic, and it is overall hard to maintain and modify. Celiagg is a Cython-based alternative wrapper of the Agg backend with better overall architecture and, importantly, is a stand-along package which is independent of Kiva. There is already a Celiagg backend for Kiva.

We should switch the default image backend to use Celiagg, and deprecate and retire the SWIG-based Agg wrapper.

Most of the pieces are in place for this, but it will likely be a multi-phase process:

  1. Audit Celiagg for feature compatibility (does it do everything right) and performance (we can take some performance degradation in exchange for reducing maintenance overhead, but 10x slower is too much).
  2. If there are discrepancies, fix them.
  3. Expose the current Agg backend as a separate-but-equivalent Kiva backend to the image backend ('oldagg' or 'swigagg' or something, not just 'agg').
  4. Switch the image backend to use Celiagg (while keeping the current celiagg backend)
  5. Deprecate the agg backend
  6. Remove the agg backend

corranwebster avatar Sep 25 '20 14:09 corranwebster

#251 is connected here.

I believe the root issue is that GraphicsContext.draw_image is expecting a GraphicsContext instance as a possible argument. Unfortunately this is done in such a way which causes a hard dependency on the current agg backend in kiva.

Here's all the serious occurrences of context objects being blitted directly, as far as I can tell:

  • https://github.com/enthought/enable/blob/4.8.1/kiva/agg/src/graphics_context.i#L719-L737
  • https://github.com/enthought/enable/blob/4.8.1/kiva/quartz/ABCGI.pyx#L736-L737
  • https://github.com/enthought/enable/blob/4.8.1/enable/component.py#L738-L775 (self._backbuffer is a context instance)
  • Scrollbar has lots of draw_image calls, but appears to be otherwise broken since the image_for method which can create those images is nowhere to be found.

Some refactoring (and probably deprecations) will need to be done to make changes here possible.

jwiggins avatar Dec 01 '20 17:12 jwiggins

Can you clarify why you think there is a "hard dependency" on the current agg backend?

  • The first link just goes away if we blow away the current agg backend, so that's no problem.

  • The second is just a branch that handles an agg GraphicsContext if you happen to pass one in; you are never required or encouraged to. Now, I did have to do that at one point because there were places where someone explicitly used the kiva.agg.GraphicsContextArray to draw something off-screen that they wanted blitted in. I think it would be more fruitful to hunt down those usages and determine whether they really care about kiva.agg; they can probably all just use kiva.celiagg as a drop-in.

  • The third creates a GraphicsContext of the same type as the passed-in GraphicsContext, so if we're drawing into a kiva.celiagg.GraphicsContext, the backbuffer would be a kiva.celiagg.GraphicsContext, which is a case that seems to be well-handled in the draw_image() implementation.

  • Scrollbar is indeed broken, but I'm pretty sure that the return value of image_for() was an array, not a GraphicsContext.

rkern avatar Dec 01 '20 21:12 rkern

One of those places where kiva.agg.GraphicsContextArray is explicitly used is in the Chaco ImagePlot:

https://github.com/enthought/chaco/blob/4a6683521cce012b5df4830d64f41518798fc817/chaco/image_plot.py#L73-L74

There's no real reason that that must explicitly depend on kiva.agg. Instead, _compute_cached_image() could/should take the destination gc as an argument and do the same type dance as Component.

rkern avatar Dec 01 '20 21:12 rkern

Perhaps the situation has improved in four years. The chaco code you linked to is somewhat concerning though. It makes any refactoring somewhat difficult.

jwiggins avatar Dec 02 '20 07:12 jwiggins

I suspect the main thing to make the celiagg backend a drop-in replacement for these explicit uses of GraphicsContextArray is to expose self.gc.array as self.bmp_array. We do use GraphicsContextArray elsewhere (for example, in tests) for the specific purpose of accessing this array. This should be supported in the kiva.celiagg implementation, and we might as well use the kiva.agg interface for this.

rkern avatar Dec 02 '20 21:12 rkern

Step 1 is now covered by the benchmark suite introduced in #647. Step 2 will follow shortly. Step 3 is covered by #669.

Steps 4-6 come later. I'm not sure how we should schedule those.

jwiggins avatar Mar 03 '21 11:03 jwiggins