Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 2163e410 authored by Nolan Scobie's avatar Nolan Scobie
Browse files

Fix erroneous self deletion on SkImage creation failure 

TL;DR: Skia should always call releaseProc, and maybe sooner than we thought.

There are multiple scenarios where SkImage:MakeFromTexture will fail,
returning a nullptr and calling releaseProc due to a RefCntedCallback
falling out of scope. Previously this could cause mUsageCount to fall to
0, resulting in the AutoBackendTextureRelease deleting itself even
though DeferredLayerUpdater owned a ref and expected it to still exist.

Also added logging for some reasons that could cause the later call to
MakeFromTexture to fail.

Bug: b/246831853
Test: hwui_unit_tests
Change-Id: I7fd2566b9a85fe286f72b0fc42eba5450cac69b0
parent 38b3e558

libs/hwui/Android.bp

+1 −0
Original line number Diff line number Diff line
@@ -678,6 +678,7 @@ cc_test { 
    srcs: [

        "tests/unit/main.cpp",

        "tests/unit/ABitmapTests.cpp",

        "tests/unit/AutoBackendTextureReleaseTests.cpp",

        "tests/unit/CacheManagerTests.cpp",

        "tests/unit/CanvasContextTests.cpp",

        "tests/unit/CanvasOpTests.cpp",

libs/hwui/AutoBackendTextureRelease.cpp

+12 −4
Original line number Diff line number Diff line
@@ -32,9 +32,17 @@ AutoBackendTextureRelease::AutoBackendTextureRelease(GrDirectContext* context, 
    bool createProtectedImage = 0 != (desc.usage & AHARDWAREBUFFER_USAGE_PROTECTED_CONTENT);

    GrBackendFormat backendFormat =

            GrAHardwareBufferUtils::GetBackendFormat(context, buffer, desc.format, false);

    LOG_ALWAYS_FATAL_IF(!backendFormat.isValid(),

                        __FILE__ " Invalid GrBackendFormat. GrBackendApi==%" PRIu32

                                 ", AHardwareBuffer_Format==%" PRIu32 ".",

                        static_cast<int>(context->backend()), desc.format);

    mBackendTexture = GrAHardwareBufferUtils::MakeBackendTexture(

            context, buffer, desc.width, desc.height, &mDeleteProc, &mUpdateProc, &mImageCtx,

            createProtectedImage, backendFormat, false);

    LOG_ALWAYS_FATAL_IF(!mBackendTexture.isValid(),

                        __FILE__ " Invalid GrBackendTexture. Width==%" PRIu32 ", height==%" PRIu32

                                 ", protected==%d",

                        desc.width, desc.height, createProtectedImage);

}



void AutoBackendTextureRelease::unref(bool releaseImage) {
@@ -74,13 +82,13 @@ void AutoBackendTextureRelease::makeImage(AHardwareBuffer* buffer, 
    AHardwareBuffer_Desc desc;

    AHardwareBuffer_describe(buffer, &desc);

    SkColorType colorType = GrAHardwareBufferUtils::GetSkColorTypeFromBufferFormat(desc.format);

    // The following ref will be counteracted by Skia calling releaseProc, either during

    // MakeFromTexture if there is a failure, or later when SkImage is discarded. It must

    // be called before MakeFromTexture, otherwise Skia may remove HWUI's ref on failure.

    ref();

    mImage = SkImage::MakeFromTexture(

            context, mBackendTexture, kTopLeft_GrSurfaceOrigin, colorType, kPremul_SkAlphaType,

            uirenderer::DataSpaceToColorSpace(dataspace), releaseProc, this);

    if (mImage.get()) {

        // The following ref will be counteracted by releaseProc, when SkImage is discarded.

        ref();

    }

}



void AutoBackendTextureRelease::newBufferContent(GrDirectContext* context) {

libs/hwui/AutoBackendTextureRelease.h

+6 −0
Original line number Diff line number Diff line
@@ -25,6 +25,9 @@ 
namespace android {

namespace uirenderer {



// Friend TestUtils serves as a proxy for any test cases that require access to private members.

class TestUtils;



/**

 * AutoBackendTextureRelease manages EglImage/VkImage lifetime. It is a ref-counted object

 * that keeps GPU resources alive until the last SkImage object using them is destroyed.
@@ -66,6 +69,9 @@ private: 


    // mImage is the SkImage created from mBackendTexture.

    sk_sp<SkImage> mImage;



    // Friend TestUtils serves as a proxy for any test cases that require access to private members.

    friend class TestUtils;

};



} /* namespace uirenderer */

libs/hwui/tests/common/TestUtils.h

+6 −0
Original line number Diff line number Diff line
@@ -16,6 +16,7 @@ 


#pragma once



#include <AutoBackendTextureRelease.h>

#include <DisplayList.h>

#include <Matrix.h>

#include <Properties.h>
@@ -293,6 +294,11 @@ public: 
    static SkRect getClipBounds(const SkCanvas* canvas);

    static SkRect getLocalClipBounds(const SkCanvas* canvas);



    static int getUsageCount(const AutoBackendTextureRelease* textureRelease) {

        EXPECT_NE(nullptr, textureRelease);

        return textureRelease->mUsageCount;

    }



    struct CallCounts {

        int sync = 0;

        int contextDestroyed = 0;

libs/hwui/tests/unit/AutoBackendTextureReleaseTests.cpp

0 → 100644
+73 −0
Original line number Diff line number Diff line 
/*

 * Copyright (C) 2022 The Android Open Source Project

 *

 * Licensed under the Apache License, Version 2.0 (the "License");

 * you may not use this file except in compliance with the License.

 * You may obtain a copy of the License at

 *

 *      http://www.apache.org/licenses/LICENSE-2.0

 *

 * Unless required by applicable law or agreed to in writing, software

 * distributed under the License is distributed on an "AS IS" BASIS,

 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.

 * See the License for the specific language governing permissions and

 * limitations under the License.

 */



#include <gtest/gtest.h>



#include "AutoBackendTextureRelease.h"

#include "tests/common/TestUtils.h"



using namespace android;

using namespace android::uirenderer;



AHardwareBuffer* allocHardwareBuffer() {

    AHardwareBuffer* buffer;

    AHardwareBuffer_Desc desc = {

            .width = 16,

            .height = 16,

            .layers = 1,

            .format = AHARDWAREBUFFER_FORMAT_R8G8B8A8_UNORM,

            .usage = AHARDWAREBUFFER_USAGE_CPU_READ_RARELY | AHARDWAREBUFFER_USAGE_CPU_WRITE_RARELY,

    };

    constexpr int kSucceeded = 0;

    int status = AHardwareBuffer_allocate(&desc, &buffer);

    EXPECT_EQ(kSucceeded, status);

    return buffer;

}



// Expands to AutoBackendTextureRelease_makeImage_invalid_RenderThreadTest,

// set as friend in AutoBackendTextureRelease.h

RENDERTHREAD_TEST(AutoBackendTextureRelease, makeImage_invalid) {

    AHardwareBuffer* buffer = allocHardwareBuffer();

    AutoBackendTextureRelease* textureRelease =

            new AutoBackendTextureRelease(renderThread.getGrContext(), buffer);



    EXPECT_EQ(1, TestUtils::getUsageCount(textureRelease));



    // SkImage::MakeFromTexture should fail if given null GrDirectContext.

    textureRelease->makeImage(buffer, HAL_DATASPACE_UNKNOWN, /*context = */ nullptr);



    EXPECT_EQ(1, TestUtils::getUsageCount(textureRelease));



    textureRelease->unref(true);

    AHardwareBuffer_release(buffer);

}



// Expands to AutoBackendTextureRelease_makeImage_valid_RenderThreadTest,

// set as friend in AutoBackendTextureRelease.h

RENDERTHREAD_TEST(AutoBackendTextureRelease, makeImage_valid) {

    AHardwareBuffer* buffer = allocHardwareBuffer();

    AutoBackendTextureRelease* textureRelease =

            new AutoBackendTextureRelease(renderThread.getGrContext(), buffer);



    EXPECT_EQ(1, TestUtils::getUsageCount(textureRelease));



    textureRelease->makeImage(buffer, HAL_DATASPACE_UNKNOWN, renderThread.getGrContext());



    EXPECT_EQ(2, TestUtils::getUsageCount(textureRelease));



    textureRelease->unref(true);

    AHardwareBuffer_release(buffer);

}