Fix gray alpha png

[j-jorge]

This is mostly what is described by @rh101 in #2230 with the addition of the following:

• A fix small typo in the CMakeLists.txt for the detection of GTK3.
• A change in convertRG8ToRGBA8 to copy the pixels as RRRA instead of RARA (as it was introduced in Remove deprecated pixel formats L8, A8, LA8 #1839).

I could not reproduce the output shown by @rh101 in its comment without the point above. From my understanding the PNG is loaded into an Image with the correct pixel format (RG8), then this image is passed to a Texture2D alongside a render pixel format set to RGBA. The Texture2D subsequently calls updatWithMipmaps which may convert the pixels into the render format. By the time the Texture2D reaches the Sprite the pixel format of the texture is RGBA.

Tested with cpp-tests' Texture2D/5:PNG Test (Linux) and in my app (Linux and Android).

I hereby invoke @smilediver to double check the loading of two-channels images, and @halx99 for the convertRG8ToRGBA8 part :)

[aismann]

Is there a tester on cpp-tests?

[rh101]

I could not reproduce the output shown by @rh101 in its comment without the point above. From my understanding the PNG is loaded into an Image with the correct pixel format (RG8), then this image is passed to a Texture2D alongside a render pixel format set to RGBA. The Texture2D subsequently calls updatWithMipmaps which may convert the pixels into the render format. By the time the Texture2D reaches the Sprite the pixel format of the texture is RGBA.

I don't quite understand how you are not able to reproduce the result, because no other changes were done when I did the test, and definitely no change to convertRG8ToRGBA8. Are you referring to a different test altogether, or some code that you are using which is not covered by an existing test? If so, then you need to provide code to show how you're using it.

Also, please always add screenshots of actual and expected output.

Aside from that, while I am not sure if the existing code in convertRG8ToRGBA8 is based on some convention, it is still correct, because how the shader interprets the data is what is important:

static void convertRG8ToRGBA8(const unsigned char* data, size_t dataLen, unsigned char* outData)
{
    for (ssize_t i = 0, l = dataLen - 1; i < l; i += 2)
    {
        *outData++ = data[i];      // R
        *outData++ = data[i + 1];      // G
        *outData++ = data[i];      // B
        *outData++ = data[i + 1];  // A
    }
}

The only way you would still be seeing a problem is if you forgot to pass the correct format of PixelFormat::RG8 to the Sprite::create() call, which may be the case since I can't see it in the list of current changes in this PR:

Sprite::create("Images/test_image_ai88.png", PixelFormat::RG8);

Without that, then it would still be treated as RGBA, in which case the modification you made to convertRG8ToRGBA8 would result in "fixing" the issue, but it's not actually a fix, because you're simply forcing the all 3 color channels to contain the same value, which is effectively what the positionTextureGrayAlpha.frag shader does:

static void convertRG8ToRGBA8(const unsigned char* data, size_t dataLen, unsigned char* outData)
{
    for (ssize_t i = 0, l = dataLen - 1; i < l; i += 2)
    {
        *outData++ = data[i];      // R
        *outData++ = data[i];      // G
        *outData++ = data[i];      // B
        *outData++ = data[i + 1];  // A
    }
}

All this does is allow the existing positionTextureColor fragment shader to show the correct gray-scale output:

void main()
{
    FragColor = v_color * texture(u_tex0, v_texCoord);
}

This is because in Sprite::setTexture(), it never actually uses the POSITION_TEXTURE_GRAY_ALPHA shader:

if (needsUpdatePS)
{
    const PixelFormat pixelFormat = _texture->getPixelFormat();

    if (pixelFormat == PixelFormat::RG8)  ///<== this would be false
        setProgramState(backend::ProgramType::POSITION_TEXTURE_GRAY_ALPHA);
    else
        setProgramState(backend::ProgramType::POSITION_TEXTURE_COLOR); // <== this shader is being used
}

You should confirm this in your own test with a breakpoint on the above section of code.

So, please double-check your code. Re-test after reverting the change to convertRG8ToRGBA8, making sure that the correct format of PixelFormat::RG8 is being passed to the Sprite::Create method.

I think that the change to convertRG8ToRGBA8 is unnecessary.

[smilediver]

I think this custom gray+alpha shader solution is probably the best one.

Btw, I think the reason why convertRG8ToRGBA8() expands RG into this "weird" RGRG format is for backwards compatibility with GLES2. Because in GLES2 and old OpenGL versions there was only GL_LUMINANCE_ALPHA​ pixel format for two channels textures, that loaded RGBA channels with LLLA. So usually in shaders you would read ra channels. But this format was removed in favor of RG8, and now you read rg. So expanding RG into RGRG is backwards compatible with GLES2 and current graphic API formats. Though, not very efficient. :)

[smilediver]

This will fail on GLES2, because for two channel textures it uses GL_LUMINANCE_ALPHA pixel format which loads data into shader's rgba channels as LLLA. So for GLES2 you need to use (c.r, c.r, c.r, c.a). So you either need an #if defined(GLES2), or use compatibility macro RG8_CHANNEL defined in base.glsl. Check videoTextureNV12.frag shader for example.

[rh101]

Would (c.r, c.r, c.r, c.a) work for both cases? For RG8, it's being loaded as RGRG into the RGBA channels, so instead of using R and G, why not just use R and A, since A has the same value as G?

[smilediver]

I treat loading RG8 as RGRG a bug, so I would go with an #if defined(GLES2). GLES2 will die at some point (In few years? Currently just 4.1% of Android market share), so this R and A channels is a deprecated legacy, and I wouldn't build a new feature based on outdated stuff.

[smilediver]

Use lowp precision as default for both float and int. That's enough for simple color processing.

[rh101]

This shader was copied from positionTextureColor.frag, with the only section changed being what is in the main() function. The existing shaders use the same settings for precision etc..

[smilediver]

I'm aware that all shaders for some reason were marked with highp, and I've actually benchmarked that it has a noticeable performance impact on mobile devices.

[rh101]

I didn't realise there was such a performance hit on mobile devices, so if there is no need to have them marked as highp, then at some point we should adjust the rest of the shaders as well.

[smilediver]

I've tried to find the notes from the tests I've did: so I was rendering ~1 million vertices: 300 triangles and 1000 batches, with the standard Axmol's position texture color shader, and the difference was 51ms (lowp) vs 54ms (highp), so about 5%. But this will depend on your game's fillrate: the more pixels you draw, the bigger impact will be.

Btw, the device I was testing didn't really support lowp, so it was falling back to mediump. On device with true lowp the difference would be larger. But I think most devices only support mediump and highp.

[smilediver]

Use highp precision for texture coordinates.

[j-jorge]

I've tracked the initialization of the Sprite in 5a192ef, with the pixel format passed to Sprite::create:

1. we start at Sprite::create(…/test_image_ai88.png, RG8)
2. arguments are forwarded to Sprite::initWithFile
3. this function calls TextureCache::addImage(…/test_image_ai88.png, RG8)
4. a new texture is created, we call Image::initWithImageFile(…/test_image_ai88.png)
5. this forwards to Image::initWithImageData, then Image::initWithPngData
6. at the end of initWithPngData the image's pixel format is RG8, as expected.
7. back to TextureCache, we call Texture2D::initWithImage(image, RG8 /* as passed to addImage() */)
8. this forwards to Texture2D::updateWithImage. Here the requested pixel format is changed to RGBA.
9. then we go to Texture2D::updateWithData which forwards to Texture2D::updateWithMipmaps
10. then we call PixelFormatUtils::convertDataToFormat which converts the pixels to RGBA (actually RGRG).
11. back to Sprite, we call Sprite::setTexture, and the texture's pixel format is indeed RGBA.

Since the format change in Texture2D::updateWithData is conditioned by AX_USE_METAL || !AX_GLES_PROFILE I wonder if you (@rh101) end up outside this condition.

I don't see the point of the conversion from RG8 to RGRG because the pixel format, in the end, still claims it's RGBA! It means that no code can rely on the actual data when pixelFormat == RGBA because it's either RGRG or RGBA. Can't we just keep the RG8 pixel format for the non-GLES2 case?

[rh101]

Since the format change in Texture2D::updateWithData is conditioned by AX_USE_METAL || !AX_GLES_PROFILE I wonder if you (@rh101) end up outside this condition.

I don't see the point of the conversion from RG8 to RGRG because the pixel format, in the end, still claims it's RGBA! It means that no code can rely on the actual data when pixelFormat == RGBA because it's either RGRG or RGBA. Can't we just keep the RG8 pixel format for the non-GLES2 case?

Sorry, I'm not clear on the purpose of your post, or what it is you're actually asking. I am also not overly familiar with that section of code, or why the conversions happen, so perhaps the person who can answer it for you is @halx99.

[smilediver]

I think it's a bug if you request RG texture and actual image is RG and it still gets converted to RGBA (loading RGRG). But as I've said, it might have been done for backwards compatibility. But maybe it's ok to break it in this case, since it behaves like a bug, because if you request RG and supply RG, then you should get RG.

Now, if you request RGBA and the image is RG, then it's another question if it should be loaded as RGRG or RRRG. Maybe it could be a separate feature, where you could request some channel swizzles before loading data into the requested format.

[j-jorge]

Hello, just a quick note about the problems we have here. I tried to remove the RG to RGBA conversion but I still have cases where a conversion happens. Digging a bit, it seems that most of the API does not want to use the pixel format of the image resources :(

• TextureCache uses Texture2D::getDefaultAlphaPixelFormat() by default, which is RGBA8. Some functions accept an explicit pixel format, some don't (e.g. addImageAsync).
• Texture2D has obviously the same default to RGBA8
• Sprite uses the same default.

This does not follow the principle that the textures are just x-channels buffers and that the semantics is given by the shader.

I think the member Texture2D::g_defaultAlphaPixelFormat should not exist. Texture2D should use the pixel format of the source image as-is, and Sprite should pick a shader that works well with the texture's pixel format by default.

Now this is a lot of work for the few hours I can give to Axmol, and I'm very afraid of the side effects in many places I have no knowledge of. I doubt I would be very helpful here.

[smilediver]

I think the member Texture2D::g_defaultAlphaPixelFormat should not exist.

Agreed, it has been a big source of issues and frustration for us. Instead of this, there should be a way to convert the texture being loaded either by asking for another pixel format, or specifying some kind of channel swizzles. But this should be done as a parameter for loading functions, not a global variable.

Now this is a lot of work for the few hours I can give to Axmol, and I'm very afraid of the side effects in many places I have no knowledge of. I doubt I would be very helpful here.

Yeah, from your summary of the issues it seems there's a need to rethink and refactor how textures are loaded (and reloaded on GL context loss). But that would touch a lot of places and would include a lot of breaking changes. Not sure what is the best way forward in this case and if such changes would be welcome.

[rh101]

Perhaps the larger refactoring should be part of Axmol v3, since breaking changes would be expected in that major update.