axmolengine / axmol

Axmol Engine – A Multi-platform Engine for Desktop, XBOX (UWP) and Mobile games. (A fork of Cocos2d-x-4.0)
https://axmol.dev
MIT License
922 stars 205 forks source link

RenderTexture recovery from volatile texture cache results in vertically flipped image and other issues #2163

Closed rh101 closed 1 month ago

rh101 commented 1 month ago
  1. Set preserve EGL context to false via this.mGLSurfaceView.setPreserveEGLContextOnPause(false); in AxmolActivity.java
  2. Add a RenderTexture as a child to the scene, render something to it and display it.
  3. Put the app in the background by switching to another application on the device
  4. Switch back to the test app with the render texture
  5. Notice that the render texture is now flipped
  6. Put the test app into the background again then switch back to it
  7. Notice that now the content of the render texture disappears complete

Two separate issues.

The first issue is that an image is created from the render texture in order for it to be saved in the cache, but the texture data read via glReadPixels results in the flipped image.

The second issue is that that if the texture disappears if the application is put into the background more than once, meaning the EGL context loss event happens more than once.

Example of original RenderTexture output: image

After first recovery from context loss: image

After another recovery, it's complete blank.

rh101 commented 1 month ago

In this code:

void RenderTexture::listenToBackground(EventCustom* /*event*/)
{
#if AX_ENABLE_CACHE_TEXTURE_DATA
    // to get the rendered texture data
    auto func = [&](RefPtr<Image> uiTextureImage) {
        if (uiTextureImage)
        {
            _UITextureImage = uiTextureImage;
            const Vec2& s = _texture2D->getContentSizeInPixels();
            VolatileTextureMgr::addDataTexture(_texture2D, uiTextureImage->getData(), s.width * s.height * 4,
                                               backend::PixelFormat::RGBA8, s);
        }
        else
        {
            AXLOGW("Cache rendertexture failed!");
        }
    };
    auto callback = std::bind(func, std::placeholders::_1);
    newImage(callback, false);
#endif
}

The call newImage(callback, false); creates the image, and the second parameter is named flipImage, but that parameter is not used in the newImage method at all. In the previous Cocos2d-x implementation, it was used, but it was always called with false anyway, and I'm not sure why.

halx99 commented 1 month ago

The readPixels was refactor 4 years ago, I forget why, but maybe we can add a parameter to specify whether flipImage, currently backend always return a flipImage data

rh101 commented 1 month ago

Yes I see, and given RenderTexture textures are always flipped by Y by default, we need a way to flip it back after reading it from the backend.

The cocos2d-x version of Texture2DGL::getBytes has that flipImage flag, but it doesn't seem like it was ever set to true by the calls from RenderTexture::newImage.

rh101 commented 1 month ago

Flipping the image contents may be a slow process, and which gets worse with larger image size, so would any flipping happening at the time the image is being saved to the cache cause issues with when the Android app is about to be put into the background? The reason is because of Activity.onPause(), and this specific section:

onPause() execution is very brief and does not necessarily offer enough time to perform save operations. For this reason, don't use onPause() to save application or user data, make network calls, or execute database transactions. Such work might not complete before the method completes.

Instead, perform heavy-load shutdown operations during [onStop()](https://developer.android.com/reference/android/app/Activity#onStop()). For more information about suitable operations to perform during onStop(), see the next section. For more information about saving data, see the section about saving and restoring state.

We use onPause to read the texture data and save it to the cache, so while it is working at the moment, if things get any slower it may become an issue. The fact that the GLSurfaceView lifecycle is used for the background and foreground events is probably more of the issue. I feel that it should be refactored, so that background and foreground events are sent from the Activity.onStop() and Activity.onResume(); the GLSurfaceView lifecycle would no longer be in control of those events. We can then control when the GLSurfaceView is paused and resumed, and prevent any issues arising from an EGL context loss on the GLSurfaceView.onPause(), since we only call this when we have read and processed all the data we need from the render target.

It would be like this:

  1. Application is moved to background
  2. Activity.onPause() is called by the OS. We can't do any intensive operations in here.
  3. Activity.onStop() is called by the OS. Event EVENT_COME_TO_BACKGROUND is broadcast, which triggers the backup of texture data into the volatile texture cache, along with allowing the app to save any game data (as recommended in the onStop() documentation).
  4. Once the above is complete, we finally call GLSurfaceView.onPause(), at which point the OS may or may not invalidate the EGL context (we can't control this).
  5. When the application is brought back to the foreground, Activity.onResume() is called by the OS.
  6. We call GLSurfaceView.onResume()
  7. Broadcast EVENT_COME_TO_FOREGROUND so that all user data and cached textures are reloaded

If this is done, then we can do heavy processing when reading the texture data, meaning flipping etc., without any issues when the application is being put into the background. If we don't want to change it to the above, then perhaps we should do the flipping on recovering images from the volatile texture cache, since there are no time restrictions at this point. We would have to store a flag in the texture cache entry to indicate whether the image needs to be flipped or not.

halx99 commented 1 month ago

The readPixels was refactor 4 years ago, I forget why, but maybe we can add a parameter to specify whether flipImage, currently backend always return a flipImage data

I have remember it, and I make a PR for fix above 2 issues: #2166