ax::Image converts gray + alpha PNGs to RGBA.

[j-jorge]

#2047 introduced support for gray + alpha PNGs by mapping the data directly to PixelFormat::RG8 textures. This did not work well as some textures, especially monochrome black + transparency, were displayed as green + transparency. This commit works around the problem by converting the image to RGBA instead, using libpng's API.

[smilediver]

This has to be reverted, it breaks loading of 2 channel images.

[j-jorge]

Could you develop a bit? I don't understand if two-channels images and gray+alpha PNGs can't be supported or if I broke something as a side effect of my patch.

[smilediver]

Your changes literally remove the part where gray+alpha is loaded as two channel RG format, and arbitrarily loads it as 4 channel texture. Not to mention it doubles the used memory.

Graphics APIs have moved away from assigning purposes to the image/texture channels. Now R means one channel texture, RG - 2, RGB - 3, RGBA - 4, and you choose what each channel represents by interpreting that in the shader or other code. There's no such thing as gray+alpha, it's just a two channel texture: for your project maybe it's gray alpha, for others maybe it's multi channel SDF, etc.

[j-jorge]

Thanks for the clarification :) I am well aware of the extra memory usage and about the shift in the interpretation of texture channels in game engine.

Now maybe I'm silly but when I edit a grayscale image in my editor, save it as a gray+alpha PNG, and see that libpng labels it as PNG_COLOR_TYPE_GRAY_ALPHA, maybe I can expect it to be displayed, by default, as a grayscale texture with transparency? IMHO other interpretations of the channels should be an opt-in.

I don't mind if this commit gets reverted, and I think that #2047 was broken too under your rationale since it would mix the two channels unconditionally when premultiplied alpha is enabled. If you could tell me how I can get a gray+alpha image displayed as a gray+alpha texture, it would be greatly appreciated :)

[j-jorge]

I guess the "main" breaking part here is the removal of these lines in the switch:

case PNG_COLOR_TYPE_GRAY_ALPHA:
    _pixelFormat = backend::PixelFormat::RG8;
    break;

[smilediver]

Well, I get what you're saying about what one may expect from "gray+alpha" PNG in normal circumstances, but this is a game engine, not a regular image viewer or editor. Here, you load data that can be interpreted differently by the engine in different circumstances. If you have RGB PNG image, it doesn't mean it contains an actual image. It may be a regular image, a 3D model texture, normal map, masks, terrain heights, or some other 2D data, that just fits this format and is useful to store it like that. So from the perspective of an engine, if I'm loading a two channel texture, I expect it to just load it as a two channel texture, and not convert it to some other format or somehow change the data.

From quick glance #2047 seems fine, cause it's an opt in feature and not a default behavior. Just to be clear, I'm not against having some method that could convert two channel RG (aka grey+alpha in this case) to RGBA format (maybe there already is?), but that should be an opt in, not a default.

Also, if it gets converted to RGBA anyway, why not just save that PNG as RGBA?

Btw, I'm just curious, what is your use case for grey+alpha images?

[j-jorge]

So from the perspective of an engine, if I'm loading a two channel texture, I expect it to just load it as a two channel texture, and not convert it to some other format or somehow change the data.

I totally agree that the conversion to RGBA is a mistake. I took a shortcut but it was a bad one.

As I am writing this I am compiling cpp-test to try to find a correct workaround. Maybe I had another bug because most gray+alpha textures work fine (the ones when I only have white+alpha actually) but one failed (the one where I only have black+alpha).

Btw, I'm just curious, what is your use case for grey+alpha images?

I use them to draw repeating patterns in the menus (example here, for the bombs/explosions on the background, and the stars in the button), with SamplerAddressMode::REPEAT. The black+alpha one is like a stencil I use as a peephole on the full screen to focus on an element, with SamplerAddressMode::CLAMP_TO_EDGE.

[j-jorge]

Some additional information about the original bug I tried to solve. Here is a capture of the test Texture2D/5:PNG Test, before #2047:

The top-left sprite is test_image_ai88.png, which is a gray+alpha PNG. As you can see it was already incorrectly displayed. I dumped some variables:

ax::Image::_hasPremultipliedAlpha=0
ax::Image::PNG_PREMULTIPLIED_ALPHA_ENABLED=1
AX_ENABLE_PREMULTIPLIED_ALPHA=1

Then the same capture but with #2047:

This time ax::Image::_hasPremultipliedAlpha has switched to 1. It's better but still incorrect.

Also, if it gets converted to RGBA anyway, why not just save that PNG as RGBA?

The texture goes into ImageMagick upon packaging and its save it as gray+alpha even if the source is RGBA. I could try to avoid this step but I liked the idea of having a small texture file (yet I used a larger memory requirement with this PR, I know… :)).

[rh101]

@halx99 As pointed out in the posts above, this PR breaks loading 2 channel images, so can we please revert this PR?

@j-jorge Can you please provide a sample black+alpha file and related code for displaying it in order to reproduce the issue you're seeing?

[j-jorge]

Can you please provide a sample black+alpha file and related code for displaying it in order to reproduce the issue you're seeing?

@rh101 Yes I can, it's right here in Axmol! See my comment above, the sample file is test_image_ai88.png and the code is at line 435 of cpp-tests/Source/Texture2dTest/Texture2dTest.cpp.

[rh101]

@rh101 Yes I can, it's right here in Axmol! See my comment above, the sample file is test_image_ai88.png and the code is at line 435 of cpp-tests/Source/Texture2dTest/Texture2dTest.cpp.

The simplest way to get the output you require is to use a PNG file in the RGBA format, or alternatively, use an appropriate shader that handles the RG data as gray/alpha.

Forcing the conversion from RG to RGBA in the engine code, as this PR has done, is not the way to solve this.

The following fragment shader works:

#version 310 es
precision highp float;
precision highp int;

layout(location = COLOR0) in vec4 v_color;
layout(location = TEXCOORD0) in vec2 v_texCoord;

layout(binding = 0) uniform sampler2D u_tex0;

layout(location = SV_Target0) out vec4 FragColor;

void main()
{
    vec4 c = texture(u_tex0, v_texCoord);
    FragColor = v_color * vec4(c.r, c.r, c.r, c.g);
}

Then setup the sprite so you explicitly pass the required pixel format as RG8:

// grayscale with alpha
auto ai88 = Sprite::create("Images/test_image_ai88.png", PixelFormat::RG8);
ai88->setPosition(s.width / 4.0f, s.height * 3.0f / 4.0f);
addChild(ai88);

The result is this, which is the correct output (using v2.2.1 of Axmol, without the changes you made in this PR):

You can add the custom shader to your own project, and assign it to the sprite via Sprite::setProgramState().

If you want to add this functionality to the engine code, then this is how I personally implemented it for the test output above:

Add file positionTextureGrayAlpha.frag with the above shader code to the /core/renderer/shaders/ folder.

Add the following to Shaders.cpp:
AX_DLL const std::string_view positionTextureGrayAlpha_frag = "positionTextureGrayAlpha_fs"sv;

Add the following to Shaders.h:
extern AX_DLL const std::string_view positionTexture_frag;

Add the following to Enums.h:

struct ProgramType
{
    enum : uint32_t
    {
...
        POSITION_TEXTURE_GRAY_ALPHA,          // positionTextureColor_vert,       positionTextureGreyAlpha_frag
...

Add the following to ProgramManager.cpp:

bool ProgramManager::init()
{
...
    registerProgram(ProgramType::POSITION_TEXTURE_GRAY_ALPHA, positionTextureColor_vert, positionTextureGrayAlpha_frag,
                    VertexLayoutType::Sprite);
...
}

Modify Sprite.cpp:

void Sprite::setTexture(Texture2D* texture)
{
...
    if (needsUpdatePS)
    {
        auto pixelFormat = _texture->getPixelFormat();
        if (pixelFormat == PixelFormat::RG8)
        {
            setProgramState(backend::ProgramType::POSITION_TEXTURE_GRAY_ALPHA);
        }
        else
        {
            setProgramState(backend::ProgramType::POSITION_TEXTURE_COLOR);
        }
    }
    else
    {
        updateProgramStateTexture(_texture);
    }
}

Also, from a review of the code, there seems to be a very minor mistake in the calculation for the pre-multiplied alpha that was added in #2047, method Image::premultiplyAlpha(), being:

twoBytes[i] = ((p[0] * p[1] + 1) >> 8) | (p[1] << 8);

should be

twoBytes[i] = ((p[0] * (p[1] + 1)) >> 8) | (p[1] << 8);

[halx99]

@halx99 As pointed out in the posts above, this PR breaks loading 2 channel images, so can we please revert this PR?

reverted, thanks