Differential D3321

Work around low framerate of stabilizer
ClosedPublic
Actions

Authored by alvinhochun on Nov 9 2016, 5:08 PM.

Details

Reviewers
dkazakov
rempt
Maniphest Tasks
T4182: Fix low framerate of stabilizer on windows
Commits
R37:11f0acc2ce29: Work around low framerate of stabilizer
R37:37b716206829: Work around low framerate of stabilizer
R37:7d9d92a9bbcd: Work around low framerate of stabilizer
Summary

Work around low framerate of stabilizer by delaying and painting the
stroke progressively.

The KisStabilizerDelayedPaintHelper class collects the sampled events
and distributes them evenly with a timestamp attached, then a timer firing
at a closer interval would paint the line bit by bit in order to give a
smoother user feedback.

The config option stabilizerDelayedPaintInterval with default value 20
controls the paint interval. The delayed painting is disabled if this
value is higher than stabilizerSampleSize, which means it is disabled by
default on non-Windows system.

Tested it a bit locally on Windows on the 3.1 branch and it should work pretty fine.

I sometimes get a bent final segment though it might not be related to this patch.

Refs: T4182

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.
alvinhochun updated this revision to Diff 8043.Nov 9 2016, 5:08 PM
alvinhochun retitled this revision from to Work around low framerate of stabilizer.
alvinhochun updated this object.
alvinhochun edited the test plan for this revision. (Show Details)
alvinhochun set the repository for this revision to R37 Krita.
alvinhochun added a project: Krita: Stable bug fixes.
alvinhochun added a reviewer: Krita.
alvinhochun added a subscriber: dkazakov.
dkazakov added a comment.Nov 10 2016, 10:06 AM

Hi, @alvinhochun!

I just tested your patch. It basically works fine, except of one problem: the brush outline now detaches from the stroke itself and continues to just with 50ms framerate :) The effect is best seen when using Delay feature of the stabilizer. I guess it can be fixed by moving m_d->previousPaintInformation = newInfo; into the delayed painting as well. As far as I remember the outline painting code routines use this position for painting outlines.

I sometimes get a bent final segment? Maybe I did something wrong with that.
Does not work with "Finish line" off (will fix that tomorrow, pretty obvious from the code)

I can check the final segment problem when you finish the patch codewise :) This problem usually arises when the stabilizer's deque gets populated with the same events and therefore, the strokes sticks to a single position. Please check commit 54098bcd976ee40eb8ea0dc9c2d234c86b30d1ad, where I fixed the problem once :)

The delay is actually a bit noticeably annoying. Try testing with stabilizerSampleSize=100 or higher

The stabilizer is expected to create a delay in the stroke, so it is not a problem. But the cursor outline jumps, yes, are annoying.

Paint interval (stabilizerDelayedPaintInterval) is now hard-coded to be 20
Now the workaround is enable regardless of stabilizerSampleSize. Maybe it should only be used when stabilizerSampleSize is above 25?
But technically one can use a stabilizerDelayedPaintInterval higher than stabilizerSampleSize if stabilizerSampleSize is very low, to reduce update rate.

Well, I would limit the rules to the following:

  1. stabilizerDelayedPaintInterval and stabilizerSampleSize are configurable via the config file
  2. If (stabilizerDelayedPaintInterval >= stabilizerSampleSize) the delayed painting feature is completely disabled (timers are also a bit expensive performance-wise)
  3. Default value for stabilizerDelayedPaintInterval is 20ms, which means the feature is disabled on Linux in a natural way.

Codewise, I would really love it if the feature would be encapsulated into something like KisStabilizedEventsSampler. Then we will not have any problems with enabling/disabling it. And probably, some kind on unittesting will be possible as well. Although I don't think I have a nice and ready proposal on hot to achieve it :)

alvinhochun added a comment.Nov 10 2016, 3:57 PM
In D3321#61956, @dkazakov wrote:

Hi, @alvinhochun!

I just tested your patch. It basically works fine, except of one problem: the brush outline now detaches from the stroke itself and continues to just with 50ms framerate :) The effect is best seen when using Delay feature of the stabilizer. I guess it can be fixed by moving m_d->previousPaintInformation = newInfo; into the delayed painting as well. As far as I remember the outline painting code routines use this position for painting outlines.

m_d->previousPaintInformation seems to be used in quite a few places so I'm not sure if it is safe to just move the assignment somewhere else. It could introduce some timing-related bugs. In fact I think the current patch has already subtly broke airbrushing (though I haven't actually tested it yet.)

Do you think I might have to add a field just for that?

alvinhochun updated this revision to Diff 8077.Nov 10 2016, 4:02 PM
alvinhochun updated this object.

This should fix the issue with the "finish line" option.

alvinhochun added a comment.Nov 10 2016, 4:08 PM

Minor nitpick: I think with the new patch the way it handles timing is a bit inaccurate. The painting of a line segment should checkthe timestamp of the second point, but this code now checks the timestamp of the first point, therefore segments might be painted a tiny bit earlier. But this shouldn't affect the functionality at all.

alvinhochun updated this revision to Diff 8092.Nov 11 2016, 1:08 PM
alvinhochun updated this object.

Fixed the cursor

alvinhochun updated this revision to Diff 8101.Nov 11 2016, 5:43 PM

I extracted the code into its class KisStabilizerDelayedPaintHelper. @dkazakov how do you think of it?

Currently all the function implementations are inlined, but they can be moved to the cpp file if you want. I just put it there so that the compiler can optimize it a bit more without link-time optimization, but this is not something significant. Perhaps you would also want the class to be exported?

Also cleaned up some unused stuff in KisStabilizedEventsSampler.

I'll add the interval options check in the next update.

alvinhochun added a comment.Nov 11 2016, 7:02 PM

Hm, I tested it a bit more and I seem to be feeling a bit of stuttering. Maybe it's due to the refresh rate of my display being 60Hz?

alvinhochun updated this revision to Diff 8109.Nov 12 2016, 10:07 AM
alvinhochun updated this object.
alvinhochun edited reviewers, added: dkazakov, rempt; removed: Krita.

This one should be mostly complete, please review.

gladhorn added a subscriber: gladhorn.Nov 14 2016, 8:47 PM
gladhorn added inline comments.
libs/ui/tool/KisStabilizerDelayedPaintHelper.h
38

I don't think there is any sensible use for QPair (in general). I'd suggest using a simple struct.

struct TimedPaintInfo {
    int some_sensible_name;
    KisPaintInformatino another_name;
};

Is actually nicer to work with and just as efficient.

40

m_running seems somewhat redundant - is the bool really needed or could it be done through the timer states? Maybe add a function:

inline bool isRunning() const {
  return m_paintTimer.isActive();
}

actually I see you even have a running() function already.

alvinhochun added a comment.Nov 15 2016, 7:53 AM

@gladhorn: Replied to the inline comments.

I wouldn't be able to make the changes before 1pm UTC. @rempt would you mind waiting for this before tagging for beta 4? Or would it be better to wait for RC1?

libs/ui/tool/KisStabilizerDelayedPaintHelper.h
38

True, I whipped this up initially just to test how it works, but then didn't want to bother changing it. (Why fix it if it isn't broken)

40

I didn't want to rely on the timer being active because it relies on the implementation of the timer. Also, the m_running flag check is all over the place probably because I was coding too defensively (actually at one point I wanted to put in asserts but eventually didn't.)

I also somehow thought QTimer::isActive() would give false when elapsed() is being fired, maybe I was just drunk (even though I never drink.) Also, checking that inside stabilizerDelayedPaint isn't really needed anyway.

You're right that it could just use the timer state, and the timer implementation probably wouldn't change in the future. I will make the change and may as well remove all the unnecessary m_running checks...

rempt edited edge metadata.Nov 15 2016, 8:10 AM

I don't mind waiting, no problem at all.

alvinhochun updated this revision to Diff 8161.Nov 15 2016, 1:58 PM
alvinhochun edited edge metadata.
alvinhochun marked 4 inline comments as done.
dkazakov accepted this revision.Nov 15 2016, 3:27 PM
dkazakov edited edge metadata.

Hi, @alvinhochun!

I have tested your patch and it seems like it works perfectly! Please push your changes!

I have only one "nitpicking" comment: I would wrap the callbacks' types into a typedefs, just for the cuteness sake :)

This revision is now accepted and ready to land.Nov 15 2016, 3:27 PM
Closed by commit R37:37b716206829: Work around low framerate of stabilizer (authored by alvinhochun). · Explain WhyNov 15 2016, 4:10 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 8170

View Options

libs/ui/CMakeLists.txt

Loading...
View Options

libs/ui/kis_config.h

Loading...
View Options

libs/ui/kis_config.cc

Loading...
View Options

libs/ui/tool/KisStabilizerDelayedPaintHelper.h

Loading...
View Options

libs/ui/tool/KisStabilizerDelayedPaintHelper.cpp

Loading...
View Options

libs/ui/tool/kis_tool_freehand_helper.cpp

Loading...