This improves performance especially with big brushes.
Details
Details
- Reviewers
abrahams rempt - Group Reviewers
Krita - Maniphest Tasks
- T1116: Replace rand with rand_r in airbrush noisy as well as total random color source
- Commits
- R37:9dfaa7a3bd21: use rand_r instead of rand in BrushMaskScalarApplicator and…
Diff Detail
Diff Detail
- Repository
- R37 Krita
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
this version of the diff uses the random number generator that's safe to use in concurrent situations to initialize a per-stack random state that then gets used in the inner loops.
it's still fast, but it doesn't concurrently access the same random state across threads.
Comment Actions
Hi tpaulssen: please add Boud and myself as reviewers to your new diffs. Nobody gets alerted that about the unreviewed patches if no reviewer has been specified.
Comment Actions
I think some network hiccup must've happened brcause I can recall you and boud being set as reviewers
Comment Actions
I think this is a good idea, but rand_r does not work cross-platform. Could you see if you can get the same speedup with C++ stdlib?
Comment Actions
There is also a class KisRandomSource which does some random number generation, you could put a more permanent solution there. It also makes Boost available.
Comment Actions
I'm not sure KisRandomGenerator is the right place: that is more concerned with predictable random for a given x,y location so filters that need random can be implemented as filter layers/masks.
Comment Actions
Dear @abrahams, with these new changes that use the c++11 random engines i still get the noticable speedup.
except the TotalRandomColorSource is very intensely bottlenecked by the kc.fromQColor call - especially the color space calculations. No clue what to do about that, though.
Comment Actions
You can do...
int channels = dev->colorspace()->channelCount()
quint data[channels];
for (i=0;i<channels;i++) {
data[i] = YOURRANDOMCODE;
}
data[channels-1]=255;//make sure the opacity is 100%
KoColor color(data, dev->colorspace());Mind you, this code will currently crash in CMYK because there's a bug in CMYK colorspace traits that is fixed in my branch, but I didn't expect it to need to be fixed in master yet... But it should get your the full value of the image CS.
Comment Actions
We've decided on IRC that KoColorspace should probably supply randomization functionality, so this patch is now, in my opinion, ready to land.
Comment Actions
OK, great! I do have one question though, do we want to create the rand_dev, rand_engine and rand_distr as class members instead of instantiating them at the time of the function call?
Comment Actions
@abrahams I've checked and the methods in question get called concurrently, so having the engines be shared would cause concurrent access to the seed again, which is what I had hoped to prevent with these patches in the first place. I don't know of a better approach than this, but if someone ever finds one, they are surely free to put it in :)