Review Request 115923: Render SvgItem natively rather than going through QQuickPaintedItem

[David Edmundson]

> On Feb. 20, 2014, 8:13 p.m., Martin Gräßlin wrote:
> > src/declarativeimports/core/svgitem.cpp, line 45
> > <https://git.reviewboard.kde.org/r/115923/diff/1/?file=245131#file245131line45>
> >
> >     are you sure that you can delete the texture here? Is the object being destroyed from the rendering thread?
> 
> David Edmundson wrote:
>     It's a good question, but I think it's OK. 
>     
>     I would have thought they'd have some very strong guards against deleting a QQuickItem whilst they're still trying to use it. That would cause people other major problems. 
>     
>     The texture inherits from QObject so I could call deleteLater() instead? Alternately I can subclass the node and delete the texture in the destructor of the node, which would be quite tidy.
>     
>     Hopefully in a week or so I'll be able to give you a far more concrete answer when I actually understand how this area works.
> 
> Martin Gräßlin wrote:
>     > The texture inherits from QObject so I could call deleteLater() instead?
>     
>     I'm not sure whether that would help. The gl destruction code needs to be called from the rendering thread, which is not necessarily the same thread as which created the texture.
>     
>     I just checked how I solved this problem in the WindowThumbnailItem and it looks like I created my own QSGSimpleTextureNode which cleans up. Of course that assumes that the node gets cleaned up in the right way.
>     
>     /me is a little bit unhappy with the Qt documentation around it. Especially who has ownership over what.

In this case it's always true, as we create in updatePaintNode which is always in the render thread.
I'll subclass anyway, will save me having deletes in two places.


> On Feb. 20, 2014, 8:13 p.m., Martin Gräßlin wrote:
> > src/declarativeimports/core/svgitem.cpp, line 127
> > <https://git.reviewboard.kde.org/r/115923/diff/1/?file=245131#file245131line127>
> >
> >     nitpick: looking at the surrounding coding style the * should be moved from type to name
> 
> David Edmundson wrote:
>     I'm really not sure which Martin is worse...
>     /me grumbles and fixes it.
> 
> Martin Klapetek wrote:
>     There is a coding style defined and it's there so the resulting code, shared with the whole world and worked on by many people, does not look like it went through set of earthquakes :P

If I ever learn how to add astyle to my pre-commit hooks, you are going to be out of a job.


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115923/#review50402
-----------------------------------------------------------


On Feb. 20, 2014, 7:22 p.m., David Edmundson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115923/
> -----------------------------------------------------------
> 
> (Updated Feb. 20, 2014, 7:22 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> -------
> 
> The rationale behind this patch is on the mailing list in the thread "Minutes Monday" 
> 
> This doesn't boost performance or save memory much, but it paves the way for texture sharing, faster resizing, and plenty of other things.
> 
> Based on Frederick's comment I have reverted my changes to use QImage everywhere, otherwise we lose out on the local QPixmap cache in KImageCache.
> Changes to plasmacore are minimal.
> 
> I'm currently porting FrameSVG which is where we should see more gains, but I thought I should get this reviewed/merged in parallel.
> 
> I have only seen one regression which is in the analog clock.
> Some odd code in the analog clock (by me apparently!) means the width is dependent on the current width, which due to some changes in this patch ends up in a constant spiral getting to infinitely sized and explode.  
> 
> 
> Changelog (in reverse order):
> Remove manual isDirty tracking in SvgItem
> Always resize the node geometry on resizes
> Update to paint to fill the size of the object, not the size of texture
> Fix leaking texture
> Add convenient QImage image() getter in SVG
> Avoid repainting if node is not changed
> Render SvgItem natively rather than going through QQuickPaintedItem
> 
> 
> Diffs
> -----
> 
>   src/declarativeimports/core/svgitem.h c8be7cc 
>   src/declarativeimports/core/svgitem.cpp e90751a 
>   src/plasma/svg.h 01d98f8 
>   src/plasma/svg.cpp 9ec2aa5 
> 
> Diff: https://git.reviewboard.kde.org/r/115923/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20140220/cb23819d/attachment-0001.html>