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

Support blitting in webagg backend #19059

Merged
merged 2 commits into from
Dec 15, 2020
Merged

Support blitting in webagg backend #19059

merged 2 commits into from
Dec 15, 2020

Conversation

ianhi
Copy link
Contributor

@ianhi ianhi commented Dec 3, 2020

PR Summary

Closes: #4288
Closes: matplotlib/ipympl#228
Supersedes: #9240

I replaced the second Agg renderer by storing the previous buffer in a private attribute. Removing the second renderer as suggested by @tacaswell eliminates the the flickering issues noted in #9240 (comment)

I also tried out a modified version of the test case by @mbewley in #9240 (comment) and found that performance was improved considerably when blitting was enabled.

from imp import reload

import matplotlib
reload(matplotlib)

matplotlib.use('nbagg')

import matplotlib.backends.backend_nbagg
reload(matplotlib.backends.backend_nbagg)
import matplotlib.backends.backend_webagg_core
reload(matplotlib.backends.backend_webagg_core)


import numpy as np
import matplotlib.pyplot as plt
import matplotlib.patches as mpatches
IMSIZE = 7000
RED = (1,0,0,1)
GREEN = (0,1,0,1)

plt.ion()
class ExampleToggle:
    
    def __init__(self):
        self.fig = plt.figure()
        self.ax = self.fig.add_subplot(111)
        self.canvas = self.ax.figure.canvas

        self.im = np.random.randint(0, 255, [IMSIZE,IMSIZE, 3]).astype('uint8')
        self.tmp_colour = RED
        self.rect = mpatches.Rectangle([0, 0], 200, 200, edgecolor='yellow', facecolor=self.tmp_colour, linewidth=0.5, animated=True)
        self.ax.add_patch(self.rect)
        
        self.canvas.mpl_connect('button_press_event', self.clicked)
        self.ax.imshow(self.im)
        self.canvas.draw()
        self._been_clicked = False

    def clicked(self, event):
        if not self._been_clicked:
            self.background = self.canvas.copy_from_bbox(self.ax.bbox)    
            self._been_clicked = True

        if self.tmp_colour == RED:
            self.tmp_colour = GREEN
        elif self.tmp_colour == GREEN:
            self.tmp_colour = RED
        self.rect.set_facecolor(self.tmp_colour)
        self.canvas.restore_region(self.background)
        self.ax.draw_artist(self.rect)
        self.canvas.blit(self.ax.bbox)
et = ExampleToggle()

(the modification was to use the self._been_clicked as I was finding that otherwise the figure was resizing and the bbox was incorrect)

blit

Unlike the previous PR this does not need to implement copy_from_bbox as that is inherited from backend_agg.FigureCanvasAgg.

I didn't add any new tests but I did try the UAT notebook and found that the flickering mentioned is is resolved by this approach. Should we consider adding the above blit test for perforamance to the UAT notebook?

One issue is that at times the animation can get tripped up and fail to properly blit. Though I believe that this is an issue that was revealed by, rather than caused by, this PR. My theory is that maybe new messages reach the frontend before it finishes a redraw and then the frontend trips over itself? This is something I've noticed on occasion with ipympl as well, even without blitting. The result looks like this, though if I increase the interval of the animation then this pathology goes away:

with interval=32
borked-blit
with interval=100
slow-interval

PR Checklist

  • [UAT] Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • [N/A (i think?)] New features are documented, with examples if plot related.
  • [N/A] Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

would this get an API note or a new feature? or neither?

Replaced the second Agg renderer by storing the previous buffer in a private attribute. Removing the second renderer eliminates the the flickering issues noted in: matplotlib#9240 (comment)

Also, did not need to implement copy_from_bbox as that is inherited from backend_agg.FigureCanvasAgg
@ianhi
Copy link
Contributor Author

ianhi commented Dec 3, 2020

I didn't realize there was such a nice UAT added in the previous PR. I've extracted that and added it here:

import itertools

cnt = itertools.count()
bg = None

def onclick_handle(event):
    """Should draw elevating green line on each mouse click"""
    global bg
    if bg is None:
        bg = ax.figure.canvas.copy_from_bbox(ax.bbox)        
    ax.figure.canvas.restore_region(bg)

    cur_y = (next(cnt) % 10) * 0.1
    ln.set_ydata([cur_y, cur_y])
    ax.draw_artist(ln)
    ax.figure.canvas.blit(ax.bbox)


fig, ax = plt.subplots()
ax.plot([0, 1], [0, 1], 'r')
ln, = ax.plot([0, 1], [0, 0], 'g', animated=True)
plt.show()
ax.figure.canvas.draw()

ax.figure.canvas.mpl_connect('button_press_event', onclick_handle)

Here as well if I click to fast I can get smearing:
smearing

@tacaswell
Copy link
Member

Thanks for working on this @ianhi :)

@danielballan has also noticed this form of glitching in ipympl and is interested in fixing it. I agree the issue is that via some mechanism one or more of the diff frames are being lost or processed out of order. We either need to make the front ends smart enough to detect this (by tacking a frame number on to everything and verifying that that they come in order) or on the kernel side force a non-diff every N frame (aka key-frames analogous to the way that mpeg works internally).

@tacaswell tacaswell added this to the v3.4.0 milestone Dec 3, 2020
@tacaswell
Copy link
Member

I also think than we can defer dealing with the key frames / generic skipping to a later PR.

Copy link

@thomasaarholt thomasaarholt left a comment

Choose a reason for hiding this comment

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

Very excited to try this!

dtype=np.uint32)
.reshape((renderer.height, renderer.width)))
diff = buff != last_buffer
diff = buff != self._last_buff
output = np.where(diff, buff, 0)

buf = BytesIO()
Copy link

Choose a reason for hiding this comment

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

I notice that there are both buff and buf in this code (I realise you didn't introduce or touch the last one). Do you think you could attempt to clear up what the difference between them is, and perhaps make the variable names self explanatory?

Making it clearer now will make it easier for the next developer who looks at this to understand what's going on :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't left a comment because I'm not 100% sure this is the reason. But here's the idea of what I might leave:

We differentiate between buff and buf because we cannot directly return buff as that is a reference to a location in memory that may change later.

There may also be something happening with the save where the buffer gets converted from whatever the renderer uses into a png buffer.

Copy link
Member

Choose a reason for hiding this comment

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

See #19117.

@thomasaarholt
Copy link

thomasaarholt commented Dec 3, 2020

@ianhi I modified your top example to have a toggle for blitting (ExampleToggle(blit=False/True)), and added an output widget to track clicks on the figure (mostly for debugging). If the example is too slow for someone out there trying this, set IMSIZE = 1000 instead of 7000.

The speedup is very nice!

Code here
from imp import reload

import matplotlib
reload(matplotlib)

matplotlib.use('nbagg')

import matplotlib.backends.backend_nbagg
reload(matplotlib.backends.backend_nbagg)
import matplotlib.backends.backend_webagg_core
reload(matplotlib.backends.backend_webagg_core)

import numpy as np
import matplotlib.pyplot as plt
import matplotlib.patches as mpatches
from ipywidgets import Output
IMSIZE = 7000
RED = (1,0,0,1)
GREEN = (0,1,0,1)
o = Output()
plt.ion()
class ExampleToggle:
    
    def __init__(self, blit=True):
        self.blit = blit
        self.fig = plt.figure()
        self.ax = self.fig.add_subplot(111)
        self.canvas = self.ax.figure.canvas

        self.im = np.random.randint(0, 255, [IMSIZE,IMSIZE, 3]).astype('uint8')
        self.tmp_colour = RED
        self.rect = mpatches.Rectangle(
            [0, 0], 200, 200, 
            edgecolor='yellow', 
            facecolor=self.tmp_colour, 
            linewidth=0.5, 
            animated=self.blit)
        self.ax.add_patch(self.rect)
        
        self.canvas.mpl_connect('button_press_event', self.clicked)
        self.ax.imshow(self.im)
        self.canvas.draw()
        self._been_clicked = False

    def clicked(self, event):
        with o:
            print('clicked')
            if not self._been_clicked:
                self.background = self.canvas.copy_from_bbox(self.ax.bbox)    
                self._been_clicked = True

            if self.tmp_colour == RED:
                self.tmp_colour = GREEN
            elif self.tmp_colour == GREEN:
                self.tmp_colour = RED
            self.rect.set_facecolor(self.tmp_colour)
            if self.blit:
                self.canvas.restore_region(self.background)
                self.ax.draw_artist(self.rect)
                self.canvas.blit(self.ax.bbox)

et = ExampleToggle(blit=False)
display(o)

Comment on lines +156 to +158
def blit(self, bbox=None):
self._png_is_old = True
self.manager.refresh_all()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied this from #4290 so I assume it was done correctly by @tacaswell, but I find it odd that this accepts a bbox argument but doesn't do anything with it. Is that a mistake?

Copy link
Member

Choose a reason for hiding this comment

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

That's to match the super class; it should blit only the section in the bbox if it's not None. Just re-blitting everything is fine, but perhaps a bit under-optimized. I think you would have to add to the current wire protocol to be able to do bbox'd blitting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense. Should a docstring be added explaining something to that effect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I see how to implement more selective blitting for the ipympl frontend, but would/can the backend Agg renderer also needing blitting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other consideration is that if any part of the image is transparent it will be forced to be a full image. That could also be optimized a bit with frontend blitting. Discussed far in the past here: #5419 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

I'm sure it could be possible, but would require sending a different message probably. Doesn't have to be done in this PR though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed I'm happy with this PR as is. Better blitting seems like a good 2021 goal.

Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

Was that all that was needed? It seems so simple...

Comment on lines +156 to +158
def blit(self, bbox=None):
self._png_is_old = True
self.manager.refresh_all()
Copy link
Member

Choose a reason for hiding this comment

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

That's to match the super class; it should blit only the section in the bbox if it's not None. Just re-blitting everything is fine, but perhaps a bit under-optimized. I think you would have to add to the current wire protocol to be able to do bbox'd blitting.

lib/matplotlib/backends/web_backend/nbagg_uat.ipynb Outdated Show resolved Hide resolved
…g figure

taking the example from matplotlib#9240

Co-Authored-By: Thomas A Caswell <tcaswell@gmail.com>
@QuLogic QuLogic merged commit 7db82ea into matplotlib:master Dec 15, 2020
@ianhi ianhi deleted the blit branch December 15, 2020 04:53
QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Dec 15, 2020
As noted in matplotlib#19059, there is both a `buff` and `buf` in this function.
The latter is a PNG file, so rename it to `png`.
@QuLogic QuLogic mentioned this pull request Dec 15, 2020
3 tasks
@meassinal
Copy link

meassinal commented Apr 25, 2021

But I still see that supports_blit = False in the source code.

@ianhi
Copy link
Contributor Author

ianhi commented Apr 25, 2021

@meassinal see: #19762 which was a follow up to this

@meassinal
Copy link

@ianhi thanks a lot for this. Anyway, I'm looking for an improvement regarding of performance issue of ax.imshow(img, origin='upper', interpolation="nearest") using backend_webagg_core along with tornado.web (websocket). I'm wondering when having animated artists interacted on the image caused slow performance, either in the matplotlib window but i could solve it with tkinter while in the web browser I couldn't. One way around is to use blit for redrawing speed but blit is not supportable in the webagg_core. I appreciate if you could give some tips regarding this issue.

@ianhi
Copy link
Contributor Author

ianhi commented Jun 28, 2021

@meassinal sorry I missed this. If this is still an issue could you please ask about that on https://discourse.matplotlib.org/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

blitting doesn't work - would be useful to speed up graph draw for faster realtime-graphs nbagg blit support
5 participants