Bug 436731

Summary:	Huge brush tip images unnecessarily slow down Krita on every start of the stroke
Product:	[Applications] krita	Reporter:	Tiar <tamtamy.tymona>
Component:	Brush engines	Assignee:	Dmitry Kazakov <dimula73>
Status:	RESOLVED FIXED
Severity:	normal	CC:	dimula73, halla
Priority:	NOR
Version:	git master (please specify the git hash!)
Target Milestone:	---
Platform:	Mint (Ubuntu based)
OS:	Linux
Latest Commit:	https://invent.kde.org/graphics/krita/commit/323805d23cb950d5f7d6bb67fadf6c4b8e2194b4	Version Fixed In:
Sentry Crash Report:

Description Tiar 2021-05-07 13:03:18 UTC

SUMMARY
If you have the same brush (in Pixel Engine) with the same brush tips options and the same size, and you only change whether it's a huge brush tip image - for example 3.000x3.000, or a small brush tip image - 100x100, you'll see a big difference in speed of starting the line. 

Since the result size of the brush is the same in both cases, it shouldn't cause much slowdown - maybe only a bit to scale down the brush tip, but it shouldn't happen before every stroke.

I marked it as a bug since I can see there is some KisSharedQImagePyramid used for brush tips so I believe it must not be intentional that Krita scales it down every time the stroke starts.

Note that "every start of the stroke" does include strokes in the brush editor, which makes the brush editor very laggy if you try to edit a brush preset like this.


STEPS TO REPRODUCE
1. Have a Pixel Engine brush, preferably without Size dynamics.
2. Have two identical brush tips, just one should be 3.000x3.000, and the other one scaled down to 100x100.
3. Try to paint with one brush tip, and then the other.

OBSERVED RESULT
Slowdown in the brush editor and at the beginning of every stroke on the canvas.


EXPECTED RESULT
Slowdown, if necessary, only on the first stroke.


SOFTWARE/OS VERSIONS
Krita 3241355287

Krita

 Version: 5.0.0-prealpha (git 3241355)
 Languages: pl, pl_PL, pl
 Hidpi: true

Qt

  Version (compiled): 5.12.8
  Version (loaded): 5.12.8

OS Information

Comment 1 Dmitry Kazakov 2021-05-07 13:06:12 UTC

In the beginning of the stroke we do <n-cpu-cores> copies of the brush. I guess KisBrush::clone() does not preserve image pyramid. To allow pyramid preservation we need to implement proper copy-on-write for it with QSharedDataPointer.

Comment 2 Halla Rempt 2021-05-14 10:19:10 UTC

Weird -- here I get two copies of the image pyramid, but I have an 8 core computer.

Comment 3 Tiar 2021-06-01 22:47:41 UTC

Sister bug report about patterns: bug 437950.

Comment 4 Tiar 2021-06-01 22:51:31 UTC

Confirmed by other people, for example here: https://krita-artists.org/t/story-experiment-verdict-after-brush-recreation-in-krita-4-4-3/22797/17 (the thread is a bit fragmented but the outcome was this bug report).

Comment 5 Dmitry Kazakov 2021-09-28 11:42:18 UTC

Git commit 016cca213259e0be610c3bed2b91337dd2697ab2 by Dmitry Kazakov.
Committed on 28/09/2021 at 11:41.
Pushed by dkazakov into branch 'master'.

Fix too many copies of KisQImagePyramid on every start of the stroke

We should generate the pyramid only once, before making <numcpu>
clones. Otherwise, it can delay the stroke quite a bit on bigger
brushes.

M  +4    -0    libs/brush/kis_auto_brush.cpp
M  +2    -1    libs/brush/kis_auto_brush.h
M  +7    -0    libs/brush/kis_brush.cpp
M  +8    -0    libs/brush/kis_brush.h
M  +6    -0    libs/brush/kis_brushes_pipe.h
M  +5    -0    libs/brush/kis_imagepipe_brush.cpp
M  +1    -0    libs/brush/kis_imagepipe_brush.h
M  +2    -0    plugins/paintops/defaultpaintops/brush/kis_brushop.cpp
M  +10   -3    plugins/paintops/libpaintop/kis_brush_based_paintop.cpp

https://invent.kde.org/graphics/krita/commit/016cca213259e0be610c3bed2b91337dd2697ab2

Comment 6 Dmitry Kazakov 2021-09-29 10:56:46 UTC

Git commit e7d22031dabb8785f3dbeb37a8c3e14f551db9dc by Dmitry Kazakov.
Committed on 29/09/2021 at 10:56.
Pushed by dkazakov into branch 'master'.

Fix delays on the stroke start when the brush is too big

This patch implements two things:

1) Makes sure that KisBrushBasedPaintOpSettings::m_savedBrush is
   actually passed by into the stroke (with cloning of course).
   Cloning of brushes is cheap due to lazy copying mechanism of
   QImage.

2) Implements a special manager class (KisPresetShadowUpdater) that
   regenerates brush'es heavy data structures (such as brush outline
   and image pyramid) in background (with a 1500 ms delay after the
   brush change).

It means that the brush on the server will always store the correct
data cached. When the user starts the stroke, this data is just cloned
and passed to the stroke without extra recalculation (which may take
up to 1000 ms for heavy brush tips).

** POSSIBLE REGRESSIONS **

1) It may happen that the brush outline will be outdated after some of
   the brush settings are changed.

2) It may happen that the changes in the brush settings will not be
   visible in the actual stroke right aftert that.

I haven't seen these regressions in real life, but, theoretically,
they can happen.

M  +0    -1    libs/brush/CMakeLists.txt
D  +0    -50   libs/brush/KisSharedQImagePyramid.cpp
D  +0    -49   libs/brush/KisSharedQImagePyramid.h
M  +14   -3    libs/brush/kis_auto_brush.cpp
M  +3    -0    libs/brush/kis_auto_brush.h
M  +53   -36   libs/brush/kis_brush.cpp
M  +8    -5    libs/brush/kis_brush.h
M  +1    -1    libs/brush/kis_gbr_brush.cpp
M  +29   -2    libs/brush/kis_imagepipe_brush.cpp
M  +4    -1    libs/brush/kis_imagepipe_brush.h
M  +1    -1    libs/brush/kis_text_brush.cpp
M  +4    -3    libs/brush/tests/kis_gbr_brush_test.cpp
M  +1    -1    libs/brush/tests/kis_imagepipe_brush_test.cpp
A  +85   -0    libs/global/KisLazySharedCacheStorage.h     [License: GPL(v2.0+)]
M  +12   -1    libs/image/brushengine/kis_paintop_preset.cpp
M  +17   -2    libs/image/brushengine/kis_paintop_preset.h
M  +10   -0    libs/image/brushengine/kis_paintop_settings.cpp
M  +5    -0    libs/image/brushengine/kis_paintop_settings.h
M  +1    -0    libs/ui/CMakeLists.txt
A  +121  -0    libs/ui/KisPresetShadowUpdater.cpp     [License: GPL(v2.0+)]
A  +32   -0    libs/ui/KisPresetShadowUpdater.h     [License: GPL(v2.0+)]
M  +6    -1    libs/ui/kis_canvas_resource_provider.cpp
M  +2    -0    libs/ui/kis_canvas_resource_provider.h
M  +22   -2    plugins/paintops/libpaintop/kis_brush_based_paintop_settings.cpp
M  +3    -0    plugins/paintops/libpaintop/kis_brush_based_paintop_settings.h

https://invent.kde.org/graphics/krita/commit/e7d22031dabb8785f3dbeb37a8c3e14f551db9dc

Comment 7 Dmitry Kazakov 2021-09-29 12:47:09 UTC

Okay, the brush is still valid when you create a masking brush with 3000px cache. This brush doesn't seem to be cached anywhere and constructing it in the beginnin g of the stroke is expensive :(

Comment 8 Dmitry Kazakov 2021-09-30 11:14:04 UTC

Git commit a094d19e7147db6204ea54eef4467c1592c3fb55 by Dmitry Kazakov.
Committed on 30/09/2021 at 11:12.
Pushed by dkazakov into branch 'master'.

Implement a cache for masked brushes by KisPresetShadowUpdater

Masked brushes are initialized in two steps:

1) KisPresetShadowUpdater calls KisPaintOpPreset::coldInitInForeground()
   to create the cache of the masking preset in the GUI thread (to be able
   to fetch resources needed by the masking preset)

2) KisPresetShadowUpdater calls KisPaintOpPreset::coldInitInBackground()
   to regenerate caches of the masking preset, like outline and pyramid.

M  +44   -7    libs/image/brushengine/kis_paintop_preset.cpp
M  +6    -0    libs/image/brushengine/kis_paintop_preset.h
M  +12   -8    libs/ui/KisPresetShadowUpdater.cpp

https://invent.kde.org/graphics/krita/commit/a094d19e7147db6204ea54eef4467c1592c3fb55

Comment 9 Dmitry Kazakov 2021-09-30 12:17:34 UTC

Git commit 6839165795970fa27afa77001b015cdd3fd4a43a by Dmitry Kazakov.
Committed on 30/09/2021 at 12:17.
Pushed by dkazakov into branch 'krita/5.0'.

Fix too many copies of KisQImagePyramid on every start of the stroke

We should generate the pyramid only once, before making <numcpu>
clones. Otherwise, it can delay the stroke quite a bit on bigger
brushes.

M  +4    -0    libs/brush/kis_auto_brush.cpp
M  +2    -1    libs/brush/kis_auto_brush.h
M  +7    -0    libs/brush/kis_brush.cpp
M  +8    -0    libs/brush/kis_brush.h
M  +6    -0    libs/brush/kis_brushes_pipe.h
M  +5    -0    libs/brush/kis_imagepipe_brush.cpp
M  +1    -0    libs/brush/kis_imagepipe_brush.h
M  +2    -0    plugins/paintops/defaultpaintops/brush/kis_brushop.cpp
M  +10   -3    plugins/paintops/libpaintop/kis_brush_based_paintop.cpp

https://invent.kde.org/graphics/krita/commit/6839165795970fa27afa77001b015cdd3fd4a43a

Comment 10 Dmitry Kazakov 2021-09-30 12:17:42 UTC

Git commit ab3365ff76bdd5dfbc9e83e22b108531ae0041c2 by Dmitry Kazakov.
Committed on 30/09/2021 at 12:17.
Pushed by dkazakov into branch 'krita/5.0'.

Fix delays on the stroke start when the brush is too big

This patch implements two things:

1) Makes sure that KisBrushBasedPaintOpSettings::m_savedBrush is
   actually passed by into the stroke (with cloning of course).
   Cloning of brushes is cheap due to lazy copying mechanism of
   QImage.

2) Implements a special manager class (KisPresetShadowUpdater) that
   regenerates brush'es heavy data structures (such as brush outline
   and image pyramid) in background (with a 1500 ms delay after the
   brush change).

It means that the brush on the server will always store the correct
data cached. When the user starts the stroke, this data is just cloned
and passed to the stroke without extra recalculation (which may take
up to 1000 ms for heavy brush tips).

** POSSIBLE REGRESSIONS **

1) It may happen that the brush outline will be outdated after some of
   the brush settings are changed.

2) It may happen that the changes in the brush settings will not be
   visible in the actual stroke right aftert that.

I haven't seen these regressions in real life, but, theoretically,
they can happen.

M  +0    -1    libs/brush/CMakeLists.txt
D  +0    -50   libs/brush/KisSharedQImagePyramid.cpp
D  +0    -49   libs/brush/KisSharedQImagePyramid.h
M  +14   -3    libs/brush/kis_auto_brush.cpp
M  +3    -0    libs/brush/kis_auto_brush.h
M  +53   -36   libs/brush/kis_brush.cpp
M  +8    -5    libs/brush/kis_brush.h
M  +1    -1    libs/brush/kis_gbr_brush.cpp
M  +29   -2    libs/brush/kis_imagepipe_brush.cpp
M  +4    -1    libs/brush/kis_imagepipe_brush.h
M  +1    -1    libs/brush/kis_text_brush.cpp
M  +4    -3    libs/brush/tests/kis_gbr_brush_test.cpp
M  +1    -1    libs/brush/tests/kis_imagepipe_brush_test.cpp
A  +85   -0    libs/global/KisLazySharedCacheStorage.h     [License: GPL(v2.0+)]
M  +12   -1    libs/image/brushengine/kis_paintop_preset.cpp
M  +17   -2    libs/image/brushengine/kis_paintop_preset.h
M  +10   -0    libs/image/brushengine/kis_paintop_settings.cpp
M  +5    -0    libs/image/brushengine/kis_paintop_settings.h
M  +1    -0    libs/ui/CMakeLists.txt
A  +121  -0    libs/ui/KisPresetShadowUpdater.cpp     [License: GPL(v2.0+)]
A  +32   -0    libs/ui/KisPresetShadowUpdater.h     [License: GPL(v2.0+)]
M  +6    -1    libs/ui/kis_canvas_resource_provider.cpp
M  +2    -0    libs/ui/kis_canvas_resource_provider.h
M  +22   -2    plugins/paintops/libpaintop/kis_brush_based_paintop_settings.cpp
M  +3    -0    plugins/paintops/libpaintop/kis_brush_based_paintop_settings.h

https://invent.kde.org/graphics/krita/commit/ab3365ff76bdd5dfbc9e83e22b108531ae0041c2

Comment 11 Dmitry Kazakov 2021-09-30 12:17:51 UTC

Git commit 323805d23cb950d5f7d6bb67fadf6c4b8e2194b4 by Dmitry Kazakov.
Committed on 30/09/2021 at 12:17.
Pushed by dkazakov into branch 'krita/5.0'.

Implement a cache for masked brushes by KisPresetShadowUpdater

Masked brushes are initialized in two steps:

1) KisPresetShadowUpdater calls KisPaintOpPreset::coldInitInForeground()
   to create the cache of the masking preset in the GUI thread (to be able
   to fetch resources needed by the masking preset)

2) KisPresetShadowUpdater calls KisPaintOpPreset::coldInitInBackground()
   to regenerate caches of the masking preset, like outline and pyramid.

M  +44   -7    libs/image/brushengine/kis_paintop_preset.cpp
M  +6    -0    libs/image/brushengine/kis_paintop_preset.h
M  +12   -8    libs/ui/KisPresetShadowUpdater.cpp

https://invent.kde.org/graphics/krita/commit/323805d23cb950d5f7d6bb67fadf6c4b8e2194b4