[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-panel-devel
Subject: Re: Review Request 115923: Render SvgItem natively rather than going through QQuickPaintedItem
From: Sebastian_Kügler <sebas () kde ! org>
Date: 2014-02-21 23:18:23
Message-ID: 20140221231823.2004.63621 () probe ! kde ! org
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115923/#review50494
-----------------------------------------------------------
I've run your native_render_frame branch for a bit, some observations on my system:
- It mostly works
- taskbar doesn't find some elements in the svg, leading to lots of messages like \
this: plasma_shell(11872)/default QSvgTinyDocument::draw: Couldn't find node \
normalbottomright. Skipping rendering.
- we seem to be hitting a few slow code pathes, which mean that I'm getting some \
smaller and longer freezes
- memory usage has gone up quite a bit, from 178MB to 328MB
- I'm seeing lots of threads of plasma-shell in ps
I like the idea, though, and I think those issues are something that can be sorted \
out. Looking at the code, it looks pretty clean so far, so nice work!
- Sebastian Kügler
On Feb. 21, 2014, 3:32 p.m., David Edmundson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115923/
> -----------------------------------------------------------
>
> (Updated Feb. 21, 2014, 3:32 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
>
>
[Attachment #5 (text/html)]
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 \
solid;"> <tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/115923/">https://git.reviewboard.kde.org/r/115923/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I've run your \
native_render_frame branch for a bit, some observations on my system:
- It mostly works
- taskbar doesn't find some elements in the svg, leading to lots of messages like \
this: plasma_shell(11872)/default QSvgTinyDocument::draw: Couldn't find node \
normalbottomright. Skipping rendering.
- we seem to be hitting a few slow code pathes, which mean that I'm getting some \
smaller and longer freezes
- memory usage has gone up quite a bit, from 178MB to 328MB
- I'm seeing lots of threads of plasma-shell in ps
I like the idea, though, and I think those issues are something that can be sorted \
out. Looking at the code, it looks pretty clean so far, so nice work! </pre> <br />
<p>- Sebastian Kügler</p>
<br />
<p>On February 21st, 2014, 3:32 p.m. UTC, David Edmundson wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for Plasma.</div>
<div>By David Edmundson.</div>
<p style="color: grey;"><i>Updated Feb. 21, 2014, 3:32 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
plasma-framework
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">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</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/declarativeimports/core/svgitem.h <span style="color: \
grey">(c8be7cc)</span></li>
<li>src/declarativeimports/core/svgitem.cpp <span style="color: \
grey">(e90751a)</span></li>
<li>src/plasma/svg.h <span style="color: grey">(01d98f8)</span></li>
<li>src/plasma/svg.cpp <span style="color: grey">(9ec2aa5)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/115923/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic