sf::Image::copy() - Fixed the incorrect alpha value being calculated for semi-transparent pixels on fully transparent pixels

[pmachapman]

Description

This PR fixes a bug where the alpha value for a semi-transparent pixel is messed up when placed on top of a transparent pixel. This bug resulting in darker pixels appearing where the transparency should be lighter.

Tasks

• Tested on Linux
• Tested on Windows
• Tested on macOS
• Tested on iOS
• Tested on Android

How to test this PR?

Run the code below, which is self-contained and has the following graphic encoded as a string literal:

Without the fix, it will look like (zoomed in 5x):

After the fix, it will look like (zoomed in 5x):

#include <SFML/Graphics.hpp>

int main()
{
    sf::RenderWindow window(sf::VideoMode(100, 100), "Minimal, complete and verifiable example");
    window.setFramerateLimit(60);

    // Setup the image data
    char tree_png[] = "\x89\x50\x4E\x47\xD\xA\x1A\xA\x0\x0\x0\xD\x49\x48\x44\x52\x0\x0\x0\xB\x0\x0\x0\x27\x8\x6\x0\x0\x0\xD9\xC2\xB2\x6B\x0\x0\x0\x1\x73\x52\x47\x42\x0\xAE\xCE\x1C\xE9\x0\x0\x0\x4\x67\x41\x4D\x41\x0\x0\xB1\x8F\xB\xFC\x61\x5\x0\x0\x0\x9\x70\x48\x59\x73\x0\x0\xE\xBF\x0\x0\xE\xBF\x1\x38\x5\x53\x24\x0\x0\x0\xE7\x49\x44\x41\x54\x38\x4F\xED\x95\x3B\xA\xC2\x40\x10\x86\x67\x17\xF1\x85\xA2\x47\xF0\x28\x29\xB4\xD7\x42\x11\x54\xF0\x6\xA\x9E\x43\x8C\x37\x10\xB4\x10\x6D\xD2\xC7\x43\x78\x4\x8F\x10\x4B\x2D\x1C\xFF\xD9\x6C\x24\x82\x92\x28\x8\x16\xF9\x60\xB2\xF3\xF8\x92\x62\x8B\x9\x7D\x82\x92\x87\xBF\xEB\xD6\xD4\xAD\x30\x46\xDA\x96\x9A\x88\x1B\x18\x9D\xC2\x9C\x3C\xD6\x97\x55\xAB\xB7\x3F\xE7\xA4\x82\xB8\xC2\x61\x45\xC1\x7C\x3\x2F\x18\x1C\xCC\x1D\x9C\x1D\xE5\x6F\x7\xB\x45\x6A\x1A\xF6\xDF\xC3\xC4\xAE\x4E\x23\xA\xF0\xC6\xDA\xE6\x69\xA8\x7F\x22\x53\x26\xC7\xC9\xE4\x38\x99\x1C\xE7\xB7\x72\x10\xA6\x89\x4\x90\xD9\xB3\x45\x2\xEC\x69\xD6\xD7\x29\xB6\xCD\xD1\x76\x5E\x22\x73\xF1\xCC\x52\x13\xE\xDB\xE1\x4\x6D\x6C\x27\x15\xED\x38\xC0\x58\x8E\xCA\x6D\xF6\x37\x4B\xA9\x1E\x72\x84\x3F\x1F\xB1\x4D\xA9\x35\x5B\x3F\xCD\xFF\xE8\x9E\x53\xF3\x95\x9C\x47\x54\x6D\xC4\x91\xBA\x82\x30\xBF\x13\x91\xCB\x88\x82\x14\x6F\x90\xEB\x2B\x11\x51\xF1\xE\xB6\x21\x31\x7A\x40\x47\x5F\x43\x0\x0\x0\x0\x49\x45\x4E\x44\xAE\x42\x60\x82";

    // Load the layer to place on top of the transparent background
    sf::Image layer;
    if (!layer.loadFromMemory(&tree_png, sizeof(tree_png)))
    {
        window.close();
        return 1;
    }

    // Create a transparent image
    sf::Image image;
    sf::Vector2u size = layer.getSize();
    image.create(size.x, size.y, sf::Color::Transparent);

    // Copy the layer on top of the image
    image.copy(layer, 0, 0, sf::IntRect(), true);

    // Load the resulting image into a texutre, and draw the sprite
    sf::Texture texture;
    if (texture.loadFromImage(image))
    {
        sf::Sprite sprite;
        sprite.setTexture(texture, true);

        while (window.isOpen())
        {
            sf::Event event;
            while (window.pollEvent(event))
            {
                if (event.type == sf::Event::Closed)
                    window.close();
            }

            window.clear(sf::Color::White);
            window.draw(sprite);
            window.display();
        }
    }
}

[Bromeon]

Thanks for your contribution!

I'm not sure if adding a special case is the right way to do this -- if the equation is incorrect, it is likely incorrect for other values, too. Also, this would introduce a non-continuity in the equation.

Uint8 alpha = src[3];
dst[0] = static_cast<Uint8>((src[0] * alpha + dst[0] * (255 - alpha)) / 255);
dst[1] = static_cast<Uint8>((src[1] * alpha + dst[1] * (255 - alpha)) / 255);
dst[2] = static_cast<Uint8>((src[2] * alpha + dst[2] * (255 - alpha)) / 255);

This is the alpha blending equation dst = alpha * src + (1 - alpha) * dst, where alpha is the source alpha channel.
Seems correct so far.

Uint8 alpha = src[3];
dst[3] = static_cast<Uint8>(alpha + dst[3] * (255 - alpha) / 255);

For the resulting alpha, it seems the formula is dst = alpha + dst * (1 - alpha).
See here, where it's written as RA = SA + DA × (1 − SA).

In the case of dst == 0 (destination alpha), we would have dst = alpha, so it would indeed amount to a copy from source -- but only for the alpha channel. The resulting RGB channels are not dependent on the destination alpha, only source alpha.

It seems to me that SFML is following the correct alpha blending formula, but maybe I'm missing something.

[pmachapman]

Yes, you are right - I scratched my head for a while and couldn't fault the formula myself. There could be some subtle order of operations bug which I am not seeing however (I do not write a lot of C++ code).

In the tests I ran, it only occurred when the destination alpha was zero (a destination alpha of 1 worked perfectly), so my testing only matched the edge case, hence the unsightly if clause.

And, yes the non-continuity is far from ideal. I don't mind if this PR isn't merged (I have my own subclass of sf::Image that works around the bug), but I thought I might share this, in case it is an issue that is bugging others.

Apologies if a PR was the wrong way to do it, I thought that having a working example was a better option than me try and explain over many paragraphs :-)

[kimci86]

It seems the expected formula is the "over" operator as described here.
I had a try implementing it. I wonder how the formula is supposed to make sense when the resulting alpha is zero. I needed an if statement for this case. It looks as expected on the provided example.

Uint8 src_alpha = src[3];
Uint8 dst_alpha = dst[3];
Uint8 out_alpha = static_cast<Uint8>(src_alpha + dst_alpha * (255 - src_alpha) / 255);

if(out_alpha)
{
    dst[0] = static_cast<Uint8>((src[0] * src_alpha + dst[0] * dst_alpha * (255 - src_alpha) / 255) / out_alpha);
    dst[1] = static_cast<Uint8>((src[1] * src_alpha + dst[1] * dst_alpha * (255 - src_alpha) / 255) / out_alpha);
    dst[2] = static_cast<Uint8>((src[2] * src_alpha + dst[2] * dst_alpha * (255 - src_alpha) / 255) / out_alpha);
}
dst[3] = out_alpha;

[Bromeon]

@pmachapman No worries, glad to discuss this 🙂

If you want the semantics that you describe (copy RGBA if dest A is zero), standard Alpha Blending might not be the right blending equation for you. Maybe instead of operating on sf::Image, you could render sf::Sprite to sf::RenderTexture? There you have many more blend modes and equations, done in hardware.

---

@kimci86

I wonder how the formula is supposed to make sense when the resulting alpha is zero.

With this:

That happens when both source and dest alpha are zero. Which is a valid case, and results in (0 + 0) / 0, which will explode.
I don't think this is the correct formula -- at least not with the special case you introduced.

[Bromeon]

As mentioned above, I think the current implementation is correct, in the sense that it just applies standard alpha blending. More sophisticated blending methods should probably go via dedicated sf::Blend and SFML's rendering, not on the sf::Image level.

Other opinions? Should we keep this open?

[pmachapman]

Thank you all for your time and effort looking into this.

I agree that there are better blending tools in SFML that should be used instead of this method.

My only real argument for opening this PR and suggesting this fix is that the visual error does occur from valid input to the method, and that the fix itself is not a major change. A simple algorithm, such as is currently in the main branch, is always preferred, but it appears the algorithm cannot handle this valid edge case.

I of course would love it merged for those reasons, but if this issue happens to others, I am content that it is documented here, and that they can use the SF::Blend, as per the comments above.

[kimci86]

My opinion is that standard alpha blending is not working well when the background is not opaque.

I think either proper blending should be implemented or the documentation should tell that using Image::copy on a transparent background looks a little weird.

---

I looked into GIMP source code and, if I did not completely get lost in the huge code base, the "over" operator (a.k.a. normal blending) seems to be implemented with the following code.

Unfold gimp-2.10.30/app/operations/layer-modes/gimpoperationnormal.c:107-144

while (samples--)
{
  gfloat layer_alpha;

  layer_alpha = layer[ALPHA] * opacity;
  if (has_mask)
    layer_alpha *= *mask;

  out[ALPHA] = layer_alpha + in[ALPHA] - layer_alpha * in[ALPHA];

  if (out[ALPHA])
    {
      gfloat layer_weight = layer_alpha / out[ALPHA];
      gfloat in_weight    = 1.0f - layer_weight;
      gint   b;

      for (b = RED; b < ALPHA; b++)
        {
          out[b] = layer[b] * layer_weight + in[b] * in_weight;
        }
    }
  else
    {
      gint b;

      for (b = RED; b < ALPHA; b++)
        {
          out[b] = in[b];
        }
    }

  in    += 4;
  layer += 4;
  out   += 4;

  if (has_mask)
    mask++;
}

Ignoring the layer opacity and mask, this could be adapted to sf::Image like so (untested):

Uint8 src_alpha = src[3];
Uint8 dst_alpha = dst[3];
Uint8 out_alpha = static_cast<Uint8>(src_alpha + dst_alpha - src_alpha * dst_alpha / 255);

dst[3] = out_alpha;

if (out_alpha)
    for (int i = 0; i < 3; i++)
        dst[i] = static_cast<Uint8>((src[i] * src_alpha + dst[i] * (out_alpha - src_alpha)) / out_alpha);
else
    for (int i = 0; i < 3; i++)
        dst[i] = src[i];

[Bromeon]

That sounds like a plan! Maybe the use of the "over" operator should also be mentioned in the Image::copy() documentation.

[pmachapman]

Thank you @kimci86 for taking the time to investigate and find that all out.

I have implemented the "over" operator, and it works brilliantly.

[codecov]

Codecov Report

Merging #1993 (3dbaf6e) into master (abe4208) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

❗ Current head 3dbaf6e differs from pull request most recent head 21af7d5. Consider uploading reports for the commit 21af7d5 to get more accurate results

@@            Coverage Diff            @@
##           master   #1993      +/-   ##
=========================================
- Coverage    6.58%   6.55%   -0.03%     
=========================================
  Files         184     182       -2     
  Lines       15788   15753      -35     
  Branches     4162    4080      -82     
=========================================
- Hits         1040    1033       -7     
+ Misses      14616   14591      -25     
+ Partials      132     129       -3     

Impacted Files	Coverage Δ
include/SFML/Graphics/Image.hpp	0.00% <ø> (ø)
src/SFML/Graphics/Image.cpp	0.00% <0.00%> (ø)
src/SFML/Window/Unix/VulkanImplX11.cpp	8.97% <0.00%> (-3.38%)	⬇️
src/SFML/Window/Win32/VulkanImplWin32.cpp	8.33% <0.00%> (-3.17%)	⬇️
src/SFML/System/FileInputStream.cpp	60.86% <0.00%> (-0.84%)	⬇️
src/SFML/System/Err.cpp	0.00% <0.00%> (ø)
src/SFML/Audio/Music.cpp	0.00% <0.00%> (ø)
src/SFML/Network/Ftp.cpp	0.00% <0.00%> (ø)
src/SFML/Network/Http.cpp	0.00% <0.00%> (ø)
src/SFML/Audio/ALCheck.cpp	0.00% <0.00%> (ø)
... and 70 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2837b36...21af7d5. Read the comment docs.

[Bromeon]

Great to hear! 🙂

@pmachapman Could you document the new semantics in the Image::copy() method docs? Maybe even with a link to Wikipedia.

Also, @kimci86 do you think this is something worth adding to tests?

[kimci86]

Also, @kimci86 do you think this is something worth adding to tests?

I do not feel it needs to be added right now, especially as there are no unit tests yet for other sf::Image methods. Of course that would be great to have eventually.

[pmachapman]

@Bromeon I have updated the documentation as requested, in the header file. Please let me know if there is anywhere else I need to update, or if I have made an error.

[eXpl0it3r]

Can you squash the two commits? 🙂

[eXpl0it3r]

Might actually be a good idea to fix this already on for 2.6.
Would you mind rebasing your commit onto the 2.6.x branch and changing the PR target to 2.6.x.?
If you're unsure, then I can also do it for you. 🙂

[pmachapman]

I have rebased this PR to the 2.6.x branch.

[eXpl0it3r]

Thanks for this fix! 🥳