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

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

Open
corranwebster opened this issue Sep 25, 2020 · 6 comments
Open
Labels
ETS Backlog Good issue for ETS team members to look at type: enhancement type: refactor

Comments

@corranwebster
Copy link
Contributor

corranwebster commented Sep 25, 2020

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 corranwebster added type: enhancement type: refactor ETS Backlog Good issue for ETS team members to look at labels Sep 25, 2020
@jwiggins
Copy link
Member

jwiggins commented Dec 1, 2020

#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:

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

@rkern
Copy link
Member

rkern commented Dec 1, 2020

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
Copy link
Member

rkern commented Dec 1, 2020

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.

@jwiggins
Copy link
Member

jwiggins commented Dec 2, 2020

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

@rkern
Copy link
Member

rkern commented Dec 2, 2020

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.

@jwiggins
Copy link
Member

jwiggins commented Mar 3, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ETS Backlog Good issue for ETS team members to look at type: enhancement type: refactor
Projects
None yet
Development

No branches or pull requests

3 participants