KisPainter::paintPolygon used in paintops

Hi,

I'm using paintPolygon in dynabrush and in spray.
For paintops it has one disadvantage. It is really slow.
I made callgrind log [1] where I use it in modified spray [2].

To speed it up about of 4%, line 728 in kis_painter:
KisPaintDeviceSP polygon = new KisPaintDevice(d->device->colorSpace(),
"polygon");
Why is string used? When I call paintPolygon in paintop, it is called e.g. 600
times in a stroke. Creating QString is quit slow. Is that string really
needed? What is its purpose? Could we use e.g. int for id, if it is id?

That method create a lot of objects like KisSelection,QImage, QPainter, QColor

What would be the solution for paintops?
Made own class for paintPolygon? Or optimalize paintPolygon somehow in
KisPainter (I don't have idea how to cache created objects in KisPainter :( )?

I use paintPolygon for painting shapes like filled aliased circle,
polygon,etc..

Ideas?

[1] http://lukast.mediablog.sk/tmp/callgrind-filling-painter.zip
[2] http://lukast.mediablog.sk/tmp/play-spray.patch
_______________________________________________
kimageshop mailing list
kimageshop@...
https://mail.kde.org/mailman/listinfo/kimageshop

On Fri, 13 Mar 2009, LukasT.dev@... wrote:

> Hi,
>
> I'm using paintPolygon in dynabrush and in spray.
> For paintops it has one disadvantage. It is really slow.
> I made callgrind log [1] where I use it in modified spray [2].
>
> To speed it up about of 4%, line 728 in kis_painter:
> KisPaintDeviceSP polygon = new KisPaintDevice(d->device->colorSpace(),
> "polygon");
> Why is string used? When I call paintPolygon in paintop, it is called e.g. 600

Ouch! That's bad -- the string is only used to give the paint device a name, which
is handy for debugging. Remove it! I'll check the rest of krita for places where
this really shouldn't be done.

> times in a stroke. Creating QString is quit slow. Is that string really
> needed? What is its purpose? Could we use e.g. int for id, if it is id?
>
> That method create a lot of objects like KisSelection,QImage, QPainter, QColor
>
> What would be the solution for paintops?
> Made own class for paintPolygon? Or optimalize paintPolygon somehow in
> KisPainter (I don't have idea how to cache created objects in KisPainter :( )?

It is possible -- as long as you keep using the same kispainter object, and use
that object in only one thread. Otherwise, you're sunk.

> I use paintPolygon for painting shapes like filled aliased circle,

I'd use paintEllipse for that -- but that calles paintPolygon in the end, too.

> polygon,etc..

I'm sure there are plenty of opportunities to tune this. I'll try to find some
time next week to look at the callgrind trace -- this weekend is going to be
horribly busy.

Boudewijn

_______________________________________________
kimageshop mailing list
kimageshop@...
https://mail.kde.org/mailman/listinfo/kimageshop

[Cyrille Berger]

Some parts of this message have been removed. Learn more about Nabble's security policy.

On Friday 13 March 2009, LukasT.dev@... wrote:
> What would be the solution for paintops?
> Made own class for paintPolygon? Or optimalize paintPolygon somehow in
> KisPainter (I don't have idea how to cache created objects in KisPainter :(
> )?
Optimize paintPolygon.

I don't share your analysis of the problem :/ In the callgrind you give, the creation of selection and objects isn't visible at all. The main cost I see is the computation of exactBounds (and worse here, we pay it twice). One might note, that those exactBounds aren't needed in this case, since we allready know the extent of the function.

An other area of improvement could be to not use the QPainter, and to directly fill the mask, there is most likely going to be a huge speed up as well, since QPainter::fillRect is to general for our purpose.

--
Cyrille Berger

_______________________________________________
kimageshop mailing list
kimageshop@...
https://mail.kde.org/mailman/listinfo/kimageshop

Some parts of this message have been removed. Learn more about Nabble's security policy.

I still want to speed up the paintPolygon...

On Friday 13 March 2009 14:44:04 Cyrille Berger wrote:

> On Friday 13 March 2009, LukasT.dev@... wrote:
> > What would be the solution for paintops?
> > Made own class for paintPolygon? Or optimalize paintPolygon somehow in
> > KisPainter (I don't have idea how to cache created objects in KisPainter
> > :( )?
>
> Optimize paintPolygon.
>
> I don't share your analysis of the problem :/ In the callgrind you give,
> the creation of selection and objects isn't visible at all. The main cost I
> see is the computation of exactBounds (and worse here, we pay it twice).
> One might note, that those exactBounds aren't needed in this case, since we
> allready know the extent of the function.

Which function? Can you describe it more? I would implement it, but I don't see what you see..You are standing on the shoulders of giants?
Can you take me there? :)

>
> An other area of improvement could be to not use the QPainter, and to
> directly fill the mask, there is most likely going to be a huge speed up as
> well, since QPainter::fillRect is to general for our purpose.

How would you fill it then? Using what class?

_______________________________________________
kimageshop mailing list
kimageshop@...
https://mail.kde.org/mailman/listinfo/kimageshop

[Cyrille Berger]

Some parts of this message have been removed. Learn more about Nabble's security policy.

On Friday 10 April 2009, LukasT.dev@... wrote:
> > I don't share your analysis of the problem :/ In the callgrind you give,
> > the creation of selection and objects isn't visible at all. The main cost
> > I see is the computation of exactBounds (and worse here, we pay it
> > twice). One might note, that those exactBounds aren't needed in this
> > case, since we allready know the extent of the function.
>
> Which function? Can you describe it more? I would implement it, but I don't
> see what you see..You are standing on the shoulders of giants?
> Can you take me there? :)

If you look at fillPainterPath, you can see the cost is divided in two
functions:
KisPaintDevice::applySelectionMask
QPainter::fillRect

The first function represent 56% of the total cost, and most of what is done
inside of that function can be avoided in our case. We don't need the call to
exactBounds (we allready know it, 38%), and we don't need the crop (the paint
device allready have the correct dimension, 17.80%). So basically we would
just have to transfer the selection mask from "polygonMaskImage" to "polygon".

There is an other possible improvement, since we only create
"polygonMaskImage" to apply on the "polygon" paint device, we can also edit
directly the alpha channel of polygon. So instead of

KisRectIterator rectIt = polygonMask->createRectIterator(x, y, rectWidth, rectHeight);

while (!rectIt.isDone()) {
(*rectIt.rawData()) = qRed(polygonMaskImage.pixel(rectIt.x() - x, rectIt.y() - y));
++rectIt;
}

You can do:

KisRectIterator rectIt = polygon->createRectIterator(x, y, rectWidth, rectHeight);

while (!rectIt.isDone()) {
polygon->colorSpace()->applyAlphaU8Mask(pixelIt.rawData(), qRed(polygonMaskImage.pixel(rectIt.x() - x, rectIt.y() - y)), 1);
++rectIt;
}

And then remove all reference to polygonMask.

> > An other area of improvement could be to not use the QPainter, and to
> > directly fill the mask, there is most likely going to be a huge speed up
> > as well, since QPainter::fillRect is to general for our purpose.
>
> How would you fill it then? Using what class?
When I first wrote the mail (or at least as far as I remember...), I thought it would be tricky... since I first thought that it was the fillPath and not fillRect the problem... ok back to the point, this should be ultra easy. QPainter::fillRect is just used to clean th QImage (which really makes me wonder why it is that slow... they might have improve that in Qt4.5 or it might be worth investigation...), so probably a call to QImage::fill(qRgba(OPACITY_TRANSPARENT, OPACITY_TRANSPARENT, OPACITY_TRANSPARENT, 255)) should be enough, and quiet cheap.

Hope it's more clear now :)

--
Cyrille Berger

_______________________________________________
kimageshop mailing list
kimageshop@...
https://mail.kde.org/mailman/listinfo/kimageshop

On Sunday 12 April 2009 13:00:05 Cyrille Berger wrote:

> On Friday 10 April 2009, LukasT.dev@... wrote:
> > > I don't share your analysis of the problem :/ In the callgrind you
> > > give, the creation of selection and objects isn't visible at all. The
> > > main cost I see is the computation of exactBounds (and worse here, we
> > > pay it twice). One might note, that those exactBounds aren't needed in
> > > this case, since we allready know the extent of the function.
> >
> > Which function? Can you describe it more? I would implement it, but I
> > don't see what you see..You are standing on the shoulders of giants?
> > Can you take me there? :)
>
> If you look at fillPainterPath, you can see the cost is divided in two
> functions:
> KisPaintDevice::applySelectionMask
> QPainter::fillRect
>
> The first function represent 56% of the total cost, and most of what is
>  done inside of that function can be avoided in our case. We don't need the
>  call to exactBounds (we allready know it, 38%), and we don't need the crop
>  (the paint device allready have the correct dimension, 17.80%). So
>  basically we would just have to transfer the selection mask from
>  "polygonMaskImage" to "polygon".
>
> There is an other possible improvement, since we only create
> "polygonMaskImage" to apply on the "polygon" paint device, we can also edit
> directly the alpha channel of polygon.  So instead of
>
>             KisRectIterator rectIt = polygonMask->createRectIterator(x, y,
> rectWidth, rectHeight);
>
>             while (!rectIt.isDone()) {
>                 (*rectIt.rawData()) =
>  qRed(polygonMaskImage.pixel(rectIt.x() - x, rectIt.y() - y));
>                 ++rectIt;
>             }
>
> You can do:
>
>             KisRectIterator rectIt = polygon->createRectIterator(x, y,
> rectWidth, rectHeight);
>
>             while (!rectIt.isDone()) {
>            polygon->colorSpace()->applyAlphaU8Mask(pixelIt.rawData(),
> qRed(polygonMaskImage.pixel(rectIt.x() - x, rectIt.y() - y)), 1);
>                   ++rectIt;
>             }
>
> And then remove all reference to polygonMask.
>
> > > An other area of improvement could be to not use the QPainter, and to
> > > directly fill the mask, there is most likely going to be a huge speed
> > > up as well, since QPainter::fillRect is to general for our purpose.
> >
> > How would you fill it then? Using what class?
>
> When I first wrote the mail (or at least as far as I remember...), I
>  thought it would be tricky... since I first thought that it was the
>  fillPath and not fillRect the problem... ok back to the point, this should
>  be ultra easy. QPainter::fillRect is just used to clean th QImage (which
>  really makes me wonder why it is that slow... they might have improve that
>  in Qt4.5 or it might be worth investigation...), so probably a call to
> QImage::fill(qRgba(OPACITY_TRANSPARENT, OPACITY_TRANSPARENT,
> OPACITY_TRANSPARENT, 255)) should be enough, and quiet cheap.
>
> Hope it's more clear now :)
>

I'm back on this issue. Last time I did not put enough effort into this. Now
I'm more inspired.
if fillPainterPath there is

        fillPainter.fillRect(fillRect, paintColor(), OPACITY_OPAQUE);

if fillRect in kis_fill_painter there

KoColor kc2(kc); // get rid of const
kc2.convertTo(device()->colorSpace());

We create QImage polygonMaskImage
and two QColor in fillRect, fillPath

so for every particle is created so much objects...Remember the easy
optimizations? [1] Maybe the easy solution would be to cache colors as much as
possible..

[1] http://wiki.koffice.org/index.php?title=Krita/Optimization

CyrilleB: I hope to catch you on IRC and you could mentor me to implement
improvements you mention...






_______________________________________________
kimageshop mailing list
kimageshop@...
https://mail.kde.org/mailman/listinfo/kimageshop

On Tuesday 29 September 2009, LukasT.dev@... wrote:

...

I got a valgrind log for you now --
http://www.valdyas.org/~boud/splatter.callgrind. This shows that the biggests
issue is indeed exactBounds(), but you might be able to find other issues.

--
Boudewijn Rempt | http://www.valdyas.org
_______________________________________________
kimageshop mailing list
kimageshop@...
https://mail.kde.org/mailman/listinfo/kimageshop

On Tue, Sep 29, 2009 at 1:45 PM, Boudewijn Rempt <boud@...> wrote:

On Tuesday 29 September 2009, LukasT.dev@... wrote:

...

I got a valgrind log for you now --
http://www.valdyas.org/~boud/splatter.callgrind. This shows that the biggests
issue is indeed exactBounds(), but you might be able to find other issues.

Some other ideas:

I'm not sure if QImage::pixel suffers from the same slowness as setPixel, but it might be worth to try using QImage scanline instead. Same with QPainter::fillRect vs QImage::fill.

Adapt the size of the QImage to the path. It isn't needed to create a 256x256 QImage if you are just painting a 10 pixel circle.

Combine several paths into one QPainterPath. No idea, if that works with the spray tool but it would probably help a lot if you could group the circles etc. in in qpainterpath before using fillPainterPath.

_______________________________________________
kimageshop mailing list
kimageshop@...
https://mail.kde.org/mailman/listinfo/kimageshop

On Tuesday 29 September 2009, Sven Langkamp wrote:
>
> I'm not sure if QImage::pixel suffers from the same slowness as setPixel,
> but it might be worth to try using QImage scanline instead. Same with
> QPainter::fillRect vs QImage::fill.

When doing filters for Hyves, I found that using scanline made a huge
difference.
 
> Adapt the size of the QImage to the path. It isn't needed to create a
> 256x256 QImage if you are just painting a 10 pixel circle.
>
> Combine several paths into one QPainterPath. No idea, if that works with
>  the spray tool but it would probably help a lot if you could group the
>  circles etc. in in qpainterpath before using fillPainterPath.
>


--
Boudewijn Rempt | http://www.valdyas.org
_______________________________________________
kimageshop mailing list
kimageshop@...
https://mail.kde.org/mailman/listinfo/kimageshop