Processing framework and color space conversion

[Dmitry Kazakov]

> Hello everyone, yesterday i began trying to port the color space
> conversion action to the processing framework using shear, crop and other
> actions as a reference.
> Though i soon hit a wall because what they share in common in the end is
> just the processing framework (applicator, visitor, commands), but the
> implementation differs quite a bit (not only because they do different
> things).
>


First, a short historical introduction. In the beginning we have a simple
visitors framework based on KisNodeVisitor. It guaranteed nothing. The
iteration through nodes was handled manually by adding recursion to the
methods of every visitor. See [0]. This older framework also couldn't
handle undo in centralized way. You should have started a macro in
KisUndoAdapter manually, then call the visitor.

Then we got a processing framework, which has a special class
KisProcessingApplicator, which does almost everything for you. It iterates
through layers, creates an undo macro, emits signals and setDirty()
requests. The applicator can execute two kinds of entities: processings and
commands. Processing is just a visitor, that accepts a node and an undo
adapter, where it should save the work it has done. Please note that this
undoAdapter might not be the one stored in the image. It'll be a special
one. Processing should not think about signals, macros locking and etc. It
just should do its work with the node and save its undo information to the
suggested undo adapter. A command is more general entity. It is just a
KUndo2Commnad, forked copy of QUndoCommand. You can have a look into Qt's
documentation for it. There is no difference in how they work.

[0] - KisColorSpaceConvertVisitor::visit(KisGroupLayer *)



> 1) I see KisTransaction used "sometimes" but sometimes not, other times a
> KisCommand is used without a transaction around. What's the difference
> between the two?
>

KisTransaction is a tool for saving the modifications done on a paint
device. If you want to modify a paint device and want to make it undoable,
you should create a KisTransaction class and then pass it to the undo
adapter [1]. Internally, KisTransaction creates a KisTransactionData
object, which is a subclass of KUndo2Command and works like a usual
command. The analogy between KisTransaction and KisTransactionData can be
described as QMutexLocker and QMutex. The former one is just a wrapper for
the latter.

[1] - via KisTransaction::commit(KisUndoAdapter*)



> 2) Color space conversion already had a visitor class, though inheriting
> from KisNodeVisitor.
> I then changed the inheritance to KisProcessingVisitor and changed all the
> function declarations while keeping the internal implementation the same
> (rerouting command stuff to KisUndoAdapter).
> So a part from the fact that the implementation might not be thread safe,
> is there something else to change?
>

Yes, the new implementation allows simple multithreading. The older one
didn't have it at all. And you needn't think about thread safety inside
KisProcessingVisitor.



> 3) This is kind of connected with 2, comparing
> KisTransformProcessingVisitor and KisColorSpaceConvertVisitor private
> functions, the first one when has to update the layers sometimes calls
> transformClones internal function that loops on the registered clones of a
> layer and does stuff with it.. while the color conversion does a different
> thing, adding commands for original, paintDevice, projection... why?
>
I understand that there's a reason but i don't get why it has to be that
> different (i mean they should both work on the entire image and all the
> layers).
>

Hm... you probably already know that, just in case, here is the link to the
description of what "Visitor" design pattern is supposed to do [2].
KisTransformProcessingVisitor doesn't work with entire image.
KisProcessingApplicator wolks through layers and call an appropriate method
of the visitor for every node.

And transformClones() is a special handling for layer offsets (they are use
by both transformation and clones, so the conflicts should be resolved
manually). You don't need to think about clones much in the color
conversion visitor.


[2] - https://en.wikipedia.org/wiki/Visitor_pattern



> 4) Pretending that the first three point are not a problem and i know what
> happens there's another "issue": if i get it right the processing framework
> just multithreads the access to layers. Though for an operation like color
> conversion if you have a single layer or the like, the performance will be
> the same as if it was sequential.
> Here the point should be that the layer should be split in N rects,
> depending on the number of threads and layer size, then these rects given
> to the threads to do color conversion.
> Though given the job framework we have and the fact that the processing
> framework already enqueued jobs for visiting nodes, i don't know how to do
> this (because enqueueing other jobs wouldn't work... and just spawning
> other threads from the processing threads doesn't seems that "clean").
>

I don't think splitting the layer into rects and doing additional
multithreading for a single-layered case is worth the effort. Converting a
single layer takes a couple of seconds only and it is not so regular
operation for painters, so they would not notice if we make it even twice
faster :)

And for multilayered case we already have multithreading provided by the
processings framework. Adding more threads may easily saturate cores and
memory bus, so the benefit might drop to zero. Some time ago I was trying
to make bitBlt() calls multithreaded (in addition to what we have already)
and it gave only 5-20% better performance for rendering. Obviously it
wasn't worth the effort to continue those experiments.



> 5) When everything from the processing framework is done it should emits
> some signals (the ones i've asked to emit).
> In the case of the color conversion i emit ColorSpaceChangedSignal and
> ModifiedSignal.
> The first is not managed by KisCanvas2 so i just created all the necessary
> functions, taking example from Shear (which calls startResizeImage).
> Though, looking at the Shear implementation, i don't get why it needs to
> emit an event there to get finishResizeImage called.
> If startResizeImage is called by Qt GUI event loop, since the signal for
> finishResizeImage uses AutoConnection, the call to the slot will be done in
> the GUI thread (and it will be a direct call).
> If instead we are in another thread, the event will be queued and then
> called when startResizeImage is finished and this call will be done one the
> same thread.
> Why couldn't they be substituted with a direct call since in the end they
> end up doing a direct call, just in different order?
> What i mean is that in the first case calling finishResizeImage() and then
> startUpdateInPatches() directly seems to me the same.
> And for the second case the opposite, startUpdateInPatches() and then
> finishResizeImage(), is again the same for that case.
>

Short answer: for your case just emit ColorSpaceChangedSignal and
ModifiedSignal and don't think about the rest :)

Long answer:

startResizeImage() is called from the image worker's thread. So all the
signals it emits get queued. It forms the queue in a special way: first
sigContinueResizeImage() to resize the openGL textures and then
startUpdateInPatches() to initiated their re-fetching data from the image.
This fetching must happen form image worker threads only. After the data is
fetched, it gets queued to be pushed further to openGL.

In your case of colorspace change, all pixel data updates are handled by
KisProcessingApplicator. You should just emit ColorSpaceChangedSignal,
which is just "a metadata", it doesn't start any updates.



> 6) talking about the implementation of those function calls... what
> exactly createImageTextureTiles and updateCache do? Why they have to be
> called both?
> If in startResizeImage we are in a different thread,
> createImageTextureTiles will be called after updateCache (and it doesn't
> make sense to me, also because internally has a call to
> updateTextureFormat, which is needed before calling updateCache, since it
> does a color conversion).
>

Reconsider these facts with the knowledge that all the signals from
startResizeImage() are queued and executed in the main GUI thread.
updateCache() is called directly and it fetches data from the image in
multithreaded way.


> 7) Shouldn't be updateCache a good candidate for multithreading too?
> (since another color conversion may happen there on tiles that should be
> independent from one another).
> How could i proceed doing it? (Should i use strokes?).
>

updateCache() is already called in multithreaded way due to
Qt::DirectConnection :)


I sound quite clueless about all this, in truth i think i know something
> (or i've guessed something) but instead of listing my guesses i thought of
> just asking things as if i new nothing ^^'.
>

That is perfectly ok :)




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