[calligra/krita-iterators2-rempt] krita/image/tiles3: Handle the situation where the ng iterators get an empty stretch

[Dmitry Kazakov]

Hi!

This patch introduces new preconditions which should be satisfied by the
caller code, that is it should check whether the rect is not empty. None of
Krita code (except of invert()) does it. It means all the rest code breaks
guarantees for the iterators. That is rather dangerous, isn't it?

I think we should fix it before we merge it to master and make it the main
and only api in Krita.

I see two ways how we can handle it without breaking any guarantees:

1) Just make isDone() inlined (probably, not a very good solution)
2) Do the iteration like Qt does it in Java-style iterators:

KisRectIteratorSP it = dev->createRectIteratorNG(0, 0, 128, 128);
while (it->nextPixel()) {
    // do something
}

This approach is free from both the issues: it has no additional function
call for isDone() (what Cyrille wanted to achieve), and it doesn't
introduce any preconditions for the iterators, which mean they can handle
empty and invalid rects as well.



On Wed, Apr 25, 2012 at 4:09 PM, Boudewijn Rempt <boud at valdyas.org> wrote:

> Git commit 3c5acc6686a6abb88448355d88e445370dc894eb by Boudewijn Rempt.
> Committed on 25/04/2012 at 14:07.
> Pushed by rempt into branch 'krita-iterators2-rempt'.
>
> Handle the situation where the ng iterators get an empty stretch
>
> Assert in developer mode so we know where to add the check for emptiness
> before creating the iterator; make the iterator check at least a pixel
> in release mode.
>
> We cannot re-instate the old isDone() api because that would impair
> performance too much:
>
> 13:26:45 < boud> CyrilleB: do you remember why the NG iterators don't have
> an isDone() check so the loop always checks at least once? (with do {} while
>                 (it-NextPixel())
> 13:45:34 < CyrilleB> boud: performance
> 13:46:13 < CyrilleB> boud: especially since in 99.99% of the case, one
> does have something to iterate over
> 13:50:34 < boud> aah
> 13:50:49 < boud> because it's the reason of the crash in
> KisSelection::invert
> 13:52:29 < boud> maybe if the rect is empty we should make it have a
> width/height of one so there's always one pixel read in all cases
> 13:53:04 < CyrilleB> or rather have an assert if you try to create an
> iterator with a null width/height
> 13:53:20 < CyrilleB> (or both, if you want to ensure good behaviour for
> users)
>                    it goes wrong
> 13:53:35 < CyrilleB> s/good behaviour/non crashing behaviour/
>
> M  +5    -1    krita/image/tiles3/kis_hline_iterator.cpp
> M  +7    -0    krita/image/tiles3/kis_rect_iterator.cpp
> M  +4    -0    krita/image/tiles3/kis_vline_iterator.cpp
>
> http://commits.kde.org/calligra/3c5acc6686a6abb88448355d88e445370dc894eb
>
> diff --git a/krita/image/tiles3/kis_hline_iterator.cpp
> b/krita/image/tiles3/kis_hline_iterator.cpp
> index fcc5bda..47b3b72 100644
> --- a/krita/image/tiles3/kis_hline_iterator.cpp
> +++ b/krita/image/tiles3/kis_hline_iterator.cpp
> @@ -19,12 +19,16 @@
>  #include "kis_hline_iterator.h"
>
>
> -KisHLineIterator2::KisHLineIterator2(KisDataManager *dataManager, qint32
> x, qint32 y, qint32 w, qint32 offsetX, qint32 offsetY, bool writable) :
> KisBaseIterator(dataManager, writable, offsetX, offsetY)
> +KisHLineIterator2::KisHLineIterator2(KisDataManager *dataManager, qint32
> x, qint32 y, qint32 w, qint32 offsetX, qint32 offsetY, bool writable)
> +    : KisBaseIterator(dataManager, writable, offsetX, offsetY)
>  {
>     x -= offsetX;
>     y -= offsetY;
>     Q_ASSERT(dataManager != 0);
>
> +    Q_ASSERT(w > 0); // for us, to warn us when abusing the iterators
> +    if (w < 1) w == 1;  // for release mode, to make sure there's always
> at least one pixel read.
> +
>     m_x = x;
>     m_y = y;
>
> diff --git a/krita/image/tiles3/kis_rect_iterator.cpp
> b/krita/image/tiles3/kis_rect_iterator.cpp
> index a2299e1..fae59dc 100644
> --- a/krita/image/tiles3/kis_rect_iterator.cpp
> +++ b/krita/image/tiles3/kis_rect_iterator.cpp
> @@ -30,6 +30,13 @@ KisRectIterator2::KisRectIterator2(KisTiledDataManager
> *dataManager,
>         m_width(width),
>         m_height(height)
>  {
> +    Q_ASSERT(width > 0); // for us, to warn us when abusing the iterators
> +    if (width < 1) width == 1;  // for release mode, to make sure there's
> always at least one pixel read.
> +
> +    Q_ASSERT(height > 0); // for us, to warn us when abusing the iterators
> +    if (height < 1) height == 1;  // for release mode, to make sure
> there's always at least one pixel read.
> +
> +
>     left -= offsetX;
>     top -= offsetY;
>     m_left = left;
> diff --git a/krita/image/tiles3/kis_vline_iterator.cpp
> b/krita/image/tiles3/kis_vline_iterator.cpp
> index 0855de2..8775c2c 100644
> --- a/krita/image/tiles3/kis_vline_iterator.cpp
> +++ b/krita/image/tiles3/kis_vline_iterator.cpp
> @@ -26,6 +26,10 @@ KisVLineIterator2::KisVLineIterator2(KisDataManager
> *dataManager, qint32 x, qint
>     x -= offsetX;
>     y -= offsetY;
>     Q_ASSERT(dataManager != 0);
> +
> +    Q_ASSERT(h > 0); // for us, to warn us when abusing the iterators
> +    if (h < 1) h == 1;  // for release mode, to make sure there's always
> at least one pixel read.
> +
>     m_lineStride = m_pixelSize * KisTileData::WIDTH;
>
>     m_x = x;
>
>


-- 
Dmitry Kazakov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kimageshop/attachments/20120425/e82a7814/attachment-0001.html>