mypaint / libmypaint

libmypaint, a.k.a. "brushlib", is a library for making brushstrokes which is used by MyPaint and other projects.
http://mypaint.org
Other
310 stars 86 forks source link

Spectrum mixing speed up by direct mixing colours that are close enough #182

Open ChengduLittleA opened 3 years ago

ChengduLittleA commented 3 years ago

Here I have a patch:

diff --git a/brushmodes.c b/brushmodes.c
index f0c68b6..eede454 100644
--- a/brushmodes.c
+++ b/brushmodes.c
@@ -70,6 +70,12 @@ void draw_dab_pixels_BlendMode_Normal (uint16_t * mask,
   }
 };

+inline int color_direct_mixable(uint16_t r, uint16_t g, uint16_t b, uint16_t* rgba){
+  return ((rgba[0]==r || r/abs(rgba[0]-r)>50)&&
+          (rgba[1]==g || g/abs(rgba[1]-g)>50)&&
+          (rgba[2]==b || b/abs(rgba[2]-b)>50));
+}
+
 void draw_dab_pixels_BlendMode_Normal_Paint (uint16_t * mask,
                                        uint16_t * rgba,
                                        uint16_t color_r,
@@ -91,7 +97,7 @@ void draw_dab_pixels_BlendMode_Normal_Paint (uint16_t * mask,
       uint32_t opa_b = (1<<15)-opa_a; // bottomAlpha
       // optimization- if background has 0 alpha we can just do normal additive
       // blending since there is nothing to mix with.
-      if (rgba[3] <= 0) {
+      if (rgba[3] <= 0 || opacity>30000 || color_direct_mixable(color_r, color_g, color_b, rgba)) {
         rgba[3] = opa_a + opa_b * rgba[3] / (1<<15);
         rgba[0] = (opa_a*color_r + opa_b*rgba[0])/(1<<15);
         rgba[1] = (opa_a*color_g + opa_b*rgba[1])/(1<<15);
@@ -351,6 +357,16 @@ void draw_dab_pixels_BlendMode_Normal_and_Eraser_Paint (uint16_t * mask,
     for (; mask[0]; mask++, rgba+=4) {
       const uint32_t opa_a = mask[0]*(uint32_t)opacity/(1<<15); // topAlpha
       const uint32_t opa_b = (1<<15)-opa_a; // bottomAlpha
+
+    if (rgba[3] <= 0 || opacity>30000 || color_direct_mixable(color_r, color_g, color_b, rgba)) {
+      const uint32_t opa_a1 = opa_a * color_a / (1<<15);
+      rgba[3] = opa_a1 + opa_b * rgba[3] / (1<<15);
+      rgba[0] = (opa_a1*color_r + opa_b*rgba[0])/(1<<15);
+      rgba[1] = (opa_a1*color_g + opa_b*rgba[1])/(1<<15);
+      rgba[2] = (opa_a1*color_b + opa_b*rgba[2])/(1<<15);
+      continue;
+    }
+
       const uint32_t opa_a2 = opa_a * color_a / (1<<15); // erase-adjusted alpha
       const uint32_t opa_out = opa_a2 + opa_b * rgba[3] / (1<<15);

@@ -458,7 +474,7 @@ void draw_dab_pixels_BlendMode_LockAlpha_Paint (uint16_t * mask,
       uint32_t opa_b = (1<<15)-opa_a; // bottomAlpha
       opa_a *= rgba[3];
       opa_a /= (1<<15);
-      if (rgba[3] <= 0) {
+      if (rgba[3] <= 0 || opacity>30000 || color_direct_mixable(color_r, color_g, color_b, rgba)) {
         rgba[0] = (opa_a*color_r + opa_b*rgba[0])/(1<<15);
         rgba[1] = (opa_a*color_g + opa_b*rgba[1])/(1<<15);
         rgba[2] = (opa_a*color_b + opa_b*rgba[2])/(1<<15);

Because most brushes are dabbed repeatedly around the same spot, so there are a lot overlapping, so if colours are close enough, we don't really need to do spectrum conversion, which greatly speeds up large brushes.

Results are still visually correct:

图片


My original description which I thought it was a bug: (wrong, don't look at it)

Because spectrum mixing is ridiculously slow even with fully opaque brushes, I am looking into the code and found the spectrum switch seems to be controlled by "alpha underneath" instead of "input alpha"? Is this a typo error or am I not understanding the algorithm correctly?

brushmodes.c line 375:

      if (spectral_factor && rgba[3] != 0) { // apparently it should be "color_a!=65535" here?
        // Convert straightened tile pixel color to a spectral
        float spectral_b[10] = {0};
        rgb_to_spectral(
          (float)rgba[0] / rgba[3],
          (float)rgba[1] / rgba[3],
          (float)rgba[2] / rgba[3],
          spectral_b
          );

I'm not sure I understood the code correctly, but I think for opaque brushes there should not be any spectrum mixing? Or if it's a different mindset I'd appreciate a more in-depth explanation.

Thanks and have a great day!

ChengduLittleA commented 3 years ago

Also: I noticed in the forum that we might have a floating point back end that is supposed to be a bit faster? when is that gonna come?

ChengduLittleA commented 3 years ago

Hi I was mistaken in the original issue description... so I updated a patch to speed up the spectral mixing by directly mixing colous that are close enough.

odysseywestra commented 3 years ago

@briend if you are still around, this would be up your alley to review.

briend commented 3 years ago

I think it probably makes more sense to just flip pigment mode off if a brush is slow :-)

Maybe just make it exact and optimize when rgba[3] == 0 || opa_a == 1<<15. There's enough weird stuff in here that I don't really understand; I think it might be matching a lot more than you think it should and giving you the false impression that it's a lot faster (but it's acting a lot like just flipping pigment mode off :-). Using opacity instead of the applied dab mask opa_a is probably not good, and I don't know what this is doing, seems like the unsigned ints would wrap around when subtracting, and you have a div by zero potential there too:

r / abs( rgba[0] - r ) > 50
ChengduLittleA commented 3 years ago

I think it might be matching a lot more than you think it should

Yeah that's how I expect it to be, in this case if the differential of colour and target rgb is within 1/50 of original colour, then it's off. It has to match a lot because a lot of brushes are just dabbling in the same places, like that acrylic brush, a lot of times things are just overlapping, mostly only dabs outside "dabbed" region would have meaningful pigment mixing. (If you apply it very lightly, the "inside" would also mix in pigment mode because the colour haven't been close enough). We can have a dynamic threshold as a setting as well.

and you have a div by zero potential there too

That's why I have that equal condition first.

briend commented 3 years ago

Oh I didn’t know that the uint subtraction gets promoted to signed Int, so negatives are ok huh. I still wonder about comparing the alpha unassociated color (‘straight”) (r) with the associated alpha color (“premultiplied”) (rgba). Does that make sense? I also think || opacity>30000 is bound to cause issues. Imagine a soft airbrush that is fully opaque; it wont use spectrum mixing at all even though many dabs are < 30000 opacity as you move from the center and the dab mask is applied. But if you instead use the applied dab mask (opa_a > 30000) you’ll have a discontinuity when it jumps from additive to pigment somewhere in the middle of the gradient.

ChengduLittleA commented 3 years ago

Ah I see... that could be an issue, but I don't think I fully understand the logic here... I probably should not be comparing with the premult value. What would you suggest is the best thing to do? After all the goal is to eliminate "a ton of" spectrum mixing for very opaque brushes.

I do have some colour differences when I apply airbrush pretty lightly and over a bit pressure, maybe that could be a case, but that could also be caused by the "very low alpha cut off" in the code, didn't check that carefully, however the current code works in my paintings pretty well, so I guess it's not that far off. Maybe just need some tweaks to those conditions.