Made calculation of table in BrightnessContrastEffect lazy, and extracted the algorithm

[Lehonti]

The algorithm part was moved to its own class so that other effects are able to use it without creating the effect itself.

I am thinking that later on we could also use an LRU cache with a key of type (int Brightness, int Contrast), and by doing that we could get rid of the PropertyChanged handler.

I'm aware that after the refactorings done in PencilSketchEffect, and SoftenPortraitEffect, the table is calculated for every row (however, I think the efficiency in GlowEffect is the same). These commits could be discarded and just kept as reference, but they show that this is one further step towards pixel-by-pixel calculation, similar to what many effects already do, and hopefully the benefits of a thread-safe LRU cache would become clearer.

I thought of using ConcurrentDictionary.GetOrAdd, but that is very expensive in memory terms, because there is no tracking of what key was used last, and no deleting of old tables from memory.

[cameronwhite]

moved to its own class so that other effects are able to use it without creating the effect itself

I'd like it to be easy to compose effects from other effects without having to factor out the code somewhere else, so I think we should instead be trying to fix any issues / shortcomings in the effect API that make this hard to do

Re: LRU cache I agree with the concerns you mentioned. I'd really rather not add any sort of cache for this lookup table, since I think the problem should be avoidable entirely:

• An effect that's composed of multiple effects really shouldn't be allocating a new instance of the sub-effect (and any caches it might have) for every tile, or whatever granularity the effect's Render method is applied at. That's the easiest way to do things currently of course, but I'd rather fix that than add more code to work around it

• I'm also not entirely sure how big of a performance impact the lookup table here actually has, but that would need benchmarking to verify

[Lehonti]

@cameronwhite I share your vision of making effects composable and more tidy. And I'm trying to get closer to that goal with the refactorings to AsyncEffectRenderer (and similar) as well as the effects. I think there is still some way to go in that regard. For example, to be able to change the API we should probably decouple LivePreviewManager.Start from EffectData.PropertyChanged, which is itself triggered through several layers of indirection, for example through SimpleEffectDialog, and that's just one step of the refactoring...

Why not both, though? Having the algorithms in their own classes (which is more feasible in the short term) and making the effects composable.

[cameronwhite]

Why not both, though?

I don't think it's inherently wrong to have an algorithm in its own class, but I don't want to have to do this for every single effect (especially to then have it not be really necessary later if the BaseEffect API has been improved)

I guess I'm also not fully understanding why this specific change is important. The original code was already working fine by creating the brightness effect and applying it to the current tile?

[Lehonti]

@cameronwhite just to clarify, my suggestion is not extracting the algorithm from every single effect, only those that are currently being created inside the Render method of other effects, which I don't see as a clean way to compose them.

But if I understand correctly, your point is that it's not that bad, and when the API is improved and made composable, it won't be hard to migrate all of these compositions at once?

// From something like this (currently)
effect1.Render(src, dst, rois);
effect2.Render(src, dst, rois);
effect3.Render(src, dst, rois);

// To something (in rough terms) like this
return Render.Chain(
    [effect1, effect2, effect3],
    src, dst, rois);

[cameronwhite]

Correct, I don't think the current approach is that bad (aside from the effects being re-created each time Render() is called for a tile, which isn't good for performance).
Calling effect1.Render(), effect2.Render() seems like a pretty straightforward sequence to write and understand?

Although, the second call is a bit bug-prone since it needs to be effect2.Render(dst, dst, rois) (rather than reading from the original src)

[Lehonti]

@cameronwhite I propose a different, slightly more conservative approach:

• After thinking a little bit, I realized that the logic from BrightnessContrastEffect that I am trying to extract is essentially a parameterizable, unary pixel op, so the extracted class (BrightnessContrastPixelOp) inherits from UnaryPixelOp and the effect just uses that op
• I considered the point you mentioned about the cache. The effects (PencilSketchEffect and SoftenPortraitEffect) that change the parameters have been left as they currently are in the master branch (that is, they are using the effect, not the op), so that the cache is not re-created for every tile.
• The only effect (other than BrightnessContrastEffect) that was modified to use the pixel op was already re-creating the effect every time anyway. But again, if this change is not desired, it can be discarded by reverting commit c96630d

[cameronwhite]

Thanks! I'm good with this set of changes 👍