'Re: Review Request: Refactoring for KisPrescaledProjection plus'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       koffice-devel
Subject:    Re: Review Request: Refactoring for KisPrescaledProjection plus
From:       "Dmitry Kazakov" <dimula73 () gmail ! com>
Date:       2009-08-05 12:35:51
Message-ID: 20090805123551.21962.67832 () localhost
[Download RAW message or body]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/1156/
-----------------------------------------------------------

(Updated 2009-08-05 12:35:51.021256)

Review request for KOffice.

Changes
-------

Here is the second version of the patch with refactoring for KisPrescaledProjection. \
I've spent almost a week to understand fully this "subpixels" drawing. Here was a \
couple of bugs in previous version.

Most significant changes:
1) KisProjectionCache ang KisImagePyramid are now derived from a base class \
KisProjectionBackend 2) Drawing itself is moved out from KisPrescaledProjection. Now \
all (almost all) drawing is done through a special stucture named a "patch". Why is \
it needed? In previos versions of KisPrescaledProjection we were requesting small \
QImage tiles of a projection to draw them on a canvas. We took them from a backend, \
scaled them down (with Blitz or QPainter - no difference) and then drew them onto \
m_prescaledQImage. It was a not good idea really. What about border effect in \
billinear scaling? That's why we got artefacts on small scales (~30%)[1] and that's \
why we couldn't use smoothScale on huge scales (~1600%) (btw, we still can't use \
smooth scale on level 1600% exactly due to some Qt's optimization (see below), but we \
can use it on 1601% level and others =) )  This problem is solved with "patches" \
system. We request some rect (a "patch") from the backend, but it returns us a bit \
more pixels, than we asked. Those pixels will help us to eliminate border effect. 3) \
Usage of the mipmapping is controlled by "useMipmapping" entry of a config file. Btw, \
i think we should change configuration of zooming a bit. I think it should be like:

zoomBackend: cache | mipmapping
zoomAlgo0to100: nearestNeighbour | bilinear
zoomAlgo100to200: nearestNeighbour | bilinear
zoomAlgoAfter200: nearestNeighbour | bilinear

What do you think?

4) And of course mipmapping added =)

===
Known bugs and fixmes:
1) Sometimes canvas is bounded by one-pixel white lines at right and bottom edges. I \
know it's due to some rounding in KisProjectionCache but i don't know how to fix it \
better. We convert image<->canvas pixels twice during one update. This a legacy \
procedure. I think i'll change it soon. 2) I don't know whether we should prescale a \
patch with Blitz. Does it really gives us time benefit? 3) Patch borders don't work \
with large images due to indirect painting stuff. We can't request KisImage for \
QImages when we want.

Mipmapping:
4) Still no threading
5) Should use SSE for downsampling
6) It's a bit more slowly than projectionCache due to the use of tiled paint device. \
It needs some profilng.

Summary (updated)
-------

Makes prescaled projection code simplier and clearer. It also removes weird [1], \
simply unused [2] and previously unused code with bugs [3].

Most significant changes:
1) imageRectFromViewPortPixels and viewRectFromImagePixels now just convert rects' \
coords, without respect to canvas size (they still do respect image size).  2) They \
both made _private_ so this change won't brake anything outside 3) Broke the link \
from KisCanvas2 to KisPrescaledProjection::viewRectFromImagePixels. It was quite bad \
idea to use internal conversion function from outside. Reasons:  a) they are internal
    b) vRect can be changed (expanded) by prescaled projection during update (mipmap \
case)  c) viewRectFromImagePixels was called twice: from KisCanvas2 first and then \
from KisPrescaledProjection::updateCanvasProjection again 4) now it's easier to \
maintain

[1] kis_pescaled_projection.cc:412-431
[2] kis_pescaled_projection.cc:396-403
[3] kis_pescaled_projection.cc:532

Diffs (updated)
-----

  trunk/koffice/krita/ui/CMakeLists.txt 1000698 
  trunk/koffice/krita/ui/canvas/kis_canvas2.cpp 1000698 
  trunk/koffice/krita/ui/canvas/kis_image_pyramid.h PRE-CREATION 
  trunk/koffice/krita/ui/canvas/kis_image_pyramid.cpp PRE-CREATION 
  trunk/koffice/krita/ui/canvas/kis_prescaled_projection.h 1000698 
  trunk/koffice/krita/ui/canvas/kis_prescaled_projection.cpp 1000698 
  trunk/koffice/krita/ui/canvas/kis_projection_backend.h PRE-CREATION 
  trunk/koffice/krita/ui/canvas/kis_projection_backend.cpp PRE-CREATION 
  trunk/koffice/krita/ui/canvas/kis_projection_cache.h 1000698 
  trunk/koffice/krita/ui/canvas/kis_projection_cache.cpp 1000698 
  trunk/koffice/krita/ui/kis_config.h 1000698 
  trunk/koffice/krita/ui/kis_config.cc 1000698 
  trunk/koffice/krita/ui/tests/kis_prescaled_projection_test.cpp 1000698 

Diff: http://reviewboard.kde.org/r/1156/diff

Testing
-------

Thanks,

Dmitry

_______________________________________________
koffice-devel mailing list
koffice-devel@kde.org
https://mail.kde.org/mailman/listinfo/koffice-devel

[prev in list] [next in list] [prev in thread] [next in thread] 