knossos-project / knossos

KNOSSOS is a software tool for the visualization and annotation of 3D image data and was developed for the rapid reconstruction of neural morphology and connectivity.
https://knossos.app
GNU General Public License v2.0
74 stars 19 forks source link

Avoid redundant data slicing (CPU slicer) #102

Closed jmrk84 closed 9 years ago

jmrk84 commented 9 years ago

Slicing the data continuously leads to high CPU load without need. Fix requires:

orenshatz commented 9 years ago

The fix is incomplete/incomprehensive.

First, the "dc_changed" variables refer not only to datacubes (as suggested by their name, and implied by loader completion notification and by COMPARE_COORDINATE(newPosition_dc, lastPosition_dc) check in userMove) but also to single voxel moves (as implied by the x/y/z check in userMove). So it's not clear whether this is an implemenation of a voxel-driven or a dc-driven slice. Even if a voxel-driven one takes into account a complete dc change (as expected), at least the name of the "changed" variable should reflect this rather then a dc-driven slice.

More importantly, a false valued "dc_changed" variable eliminates slicing in the viewport, although at least part of the viewport is no longer synched to the current voxel coordinate. For example, if one drilled into xy, then yz would not be updated, instead of shifting to the right. This "bug" is on Joergen's behalf in the actual requirement above, but nevertheless is still a bug in knossos functionality. The correct requirement for ortho mode should be to completely copy-paste the current texture one voxel to the right (in this example) and only slice the left-most column. This is true regardlessly of whether a datacube was crossed or not.

jmrk84 commented 9 years ago

I agree, the variables are badly named. However, there is no bug. xy drilling (and all drilling etc) works as expected, at least based on the tests we've performed here. In fact, the changes appear to be extremely beneficial, as predicted: possible movements per second increased by a factor of 2-3 (or CPU load decreased by the same amount). Arbitrary mode is not affected by these changes and not faster (still has high idle CPU load as well)

orenshatz commented 9 years ago

This is impossible, given the code I've seen. I will debug it later using print-outs.

orenshatz commented 9 years ago

Ah just one thing - currently we reslice upon each and every finished loaded cubed, even if irrelavent to the currently sliced planes. Consider reopenning the bug to fix this.

jmrk84 commented 9 years ago

Your last comment is correct. Reslicing should only be triggered for new cubes that actually intersect with the current slicing plane. This would further increase performance, as fewer redundant reslices are triggered.

orenshatz commented 9 years ago

This is especially the case for slow streaming, where reslicing would (given the current, pre-fixed implementation) be repeatedly triggered, while for local datasets a burst of triggers is only momentarily felt by the user.

jmrk84 commented 9 years ago

Well, it is not so bad as you think, a single redundant reslice is pretty cheap.

orenshatz commented 9 years ago

Why single? For M=5 it's at least several tens of datacubes, hence reslices.

orenshatz commented 9 years ago

Though to be accurate, when crossing a datacube boundary only 25 should be reloaded. But all of them would necessarily be invisible, hence redudantly trigger reslices.

Optiligence commented 9 years ago

10 of them are visible in 2 of the viewports

jmrk84 commented 9 years ago

Yes, it is noticeable, esp. for M>5. The signal should only be emitted if the loaded cube intersects with the slice plane, then we are fine.

orenshatz commented 9 years ago

Correct. That's still 15 redundant reslices. Plus for big screens M would be rather 9ish.

orenshatz commented 9 years ago

Alright, then the bug remains open for now.

orenshatz commented 9 years ago

Additional requirement: Current implementation emits a reslice every time segmenation is changed. This should not be a complete reslice, since the texture data is unchanged. All that needs to be done is redraw the overlay.

Optiligence commented 9 years ago

I implemented the dc/oc split, the visibility thing and tried to find all places which require reslices. The only thing mentioned here which is missing, is the arbitrary mode which doesn’t even have working segmentation right now. I’m closing this. If there’re cases where it’s still broken, open a new issue.