Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Support blitting in webagg backend#19059
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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
I didn't realize there was such a nice UAT added in the previous PR. I've extracted that and added it here: importitertoolscnt=itertools.count()bg=Nonedefonclick_handle(event):"""Should draw elevating green line on each mouse click"""globalbgifbgisNone:bg=ax.figure.canvas.copy_from_bbox(ax.bbox)ax.figure.canvas.restore_region(bg)cur_y= (next(cnt)%10)*0.1ln.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: |
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). |
I also think than we can defer dealing with the key frames / generic skipping to a later PR. |
thomasaarholt left a comment
There was a problem hiding this 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() |
thomasaarholtDec 3, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
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 bothbuff andbuf 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 :)
There was a problem hiding this comment.
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 betweenbuff andbuf because we cannot directly returnbuff 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
See#19117.
thomasaarholt commentedDec 3, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@ianhi I modified your top example to have a toggle for blitting ( The speedup is very nice! Code herefromimpimportreloadimportmatplotlibreload(matplotlib)matplotlib.use('nbagg')importmatplotlib.backends.backend_nbaggreload(matplotlib.backends.backend_nbagg)importmatplotlib.backends.backend_webagg_corereload(matplotlib.backends.backend_webagg_core)importnumpyasnpimportmatplotlib.pyplotaspltimportmatplotlib.patchesasmpatchesfromipywidgetsimportOutputIMSIZE=7000RED= (1,0,0,1)GREEN= (0,1,0,1)o=Output()plt.ion()classExampleToggle:def__init__(self,blit=True):self.blit=blitself.fig=plt.figure()self.ax=self.fig.add_subplot(111)self.canvas=self.ax.figure.canvasself.im=np.random.randint(0,255, [IMSIZE,IMSIZE,3]).astype('uint8')self.tmp_colour=REDself.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=Falsedefclicked(self,event):witho:print('clicked')ifnotself._been_clicked:self.background=self.canvas.copy_from_bbox(self.ax.bbox)self._been_clicked=Trueifself.tmp_colour==RED:self.tmp_colour=GREENelifself.tmp_colour==GREEN:self.tmp_colour=REDself.rect.set_facecolor(self.tmp_colour)ifself.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) |
| def blit(self, bbox=None): | ||
| self._png_is_old = True | ||
| self.manager.refresh_all() |
There was a problem hiding this comment.
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 abbox argument but doesn't do anything with it. Is that a mistake?
There was a problem hiding this comment.
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 notNone. 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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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...
| def blit(self, bbox=None): | ||
| self._png_is_old = True | ||
| self.manager.refresh_all() |
There was a problem hiding this comment.
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 notNone. 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.
Uh oh!
There was an error while loading.Please reload this page.
…g figuretaking the example frommatplotlib#9240Co-Authored-By: Thomas A Caswell <tcaswell@gmail.com>
As noted inmatplotlib#19059, there is both a `buff` and `buf` in this function.The latter is a PNG file, so rename it to `png`.
meassinal commentedApr 25, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
But I still see that |
@meassinal see:#19762 which was a follow up to this |
meassinal commentedApr 26, 2021
@ianhi thanks a lot for this. Anyway, I'm looking for an improvement regarding of performance issue of |
@meassinal sorry I missed this. If this is still an issue could you please ask about that onhttps://discourse.matplotlib.org/ |
Uh oh!
There was an error while loading.Please reload this page.
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.
(the modification was to use the
self._been_clickedas I was finding that otherwise the figure was resizing and the bbox was incorrect)Unlike the previous PR this does not need to implement
copy_from_bboxas that is inherited frombackend_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
with interval=100
PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsand runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).would this get an API note or a new feature? or neither?