<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">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.<div>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).</div></div></blockquote><div><br><br></div><div>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.<br><br></div><div>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.<br></div><div><br>[0] - KisColorSpaceConvertVisitor::visit(KisGroupLayer *)<br></div><div><br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div></div><div>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?</div></div></blockquote><div><br></div><div>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.<br><br></div><div>[1] - via KisTransaction::commit(KisUndoAdapter*)<br></div><div><br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div></div><div>2) Color space conversion already had a visitor class, though inheriting from KisNodeVisitor.</div><div>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).</div><div>So a part from the fact that the implementation might not be thread safe, is there something else to change?</div></div></blockquote><div><br></div><div>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.<br></div><div><br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div></div><div>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?</div></div></blockquote><div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>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).</div></div></blockquote><div><br></div><div>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.<br><br></div><div>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.<br></div><div><br><br>[2] - <a href="https://en.wikipedia.org/wiki/Visitor_pattern">https://en.wikipedia.org/wiki/Visitor_pattern</a><br><br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div></div><div>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.</div><div>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.</div><div>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").</div></div></blockquote><div><br></div><div>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 :)<br><br></div><div>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.<br></div><div><br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div></div><div>5) When everything from the processing framework is done it should emits some signals (the ones i've asked to emit).</div><div>In the case of the color conversion i emit ColorSpaceChangedSignal and ModifiedSignal. </div><div>The first is not managed by KisCanvas2 so i just created all the necessary functions, taking example from Shear (which calls startResizeImage).</div><div>Though, looking at the Shear implementation, i don't get why it needs to emit an event there to get finishResizeImage called.</div><div>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). </div><div>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.</div><div>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?</div><div>What i mean is that in the first case calling finishResizeImage() and then startUpdateInPatches() directly seems to me the same.</div><div>And for the second case the opposite, startUpdateInPatches() and then finishResizeImage(), is again the same for that case.</div></div></blockquote><div><br></div><div>Short answer: for your case just emit ColorSpaceChangedSignal and ModifiedSignal and don't think about the rest :)<br></div><div><br></div><div>Long answer: <br><br>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.<br><br></div><div>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.<br></div><div><br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div></div><div>6) talking about the implementation of those function calls... what exactly createImageTextureTiles and updateCache do? Why they have to be called both?<br></div><div>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).</div></div></blockquote><div><br></div><div>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.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div></div><div>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).</div><div>How could i proceed doing it? (Should i use strokes?).</div></div></blockquote><div><br></div><div>updateCache() is already called in multithreaded way due to Qt::DirectConnection :)<br></div><div><br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div></div><div>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 ^^'.</div>

</div></blockquote></div><br>That is perfectly ok :)<br><br><br></div><div class="gmail_extra"><br clear="all"><br>-- <br><div class="gmail_signature">Dmitry Kazakov</div>
</div></div>