From b10ec8ef8bdf5f500c77e02f3446ee1e432d6b8b Mon Sep 17 00:00:00 2001 From: Andrey Golubev Date: Wed, 24 Jul 2019 23:36:18 +0300 Subject: [PATCH] Merge pull request #14985 from andrey-golubev:gapi_fix_ocl_umat * G-API: fix GOCLExecutable issue with UMat lifetime Add tests on initialized/uninitialized outputs for all backends * Use proper clean-up procedure for magazine * Rename InitOut test and reduce tested sizes * Enable output allocation test --- modules/gapi/src/api/gbackend.cpp | 2 +- modules/gapi/src/backends/ocl/goclbackend.cpp | 20 +++++++++++ modules/gapi/test/common/gapi_core_tests.hpp | 1 + .../gapi/test/common/gapi_core_tests_inl.hpp | 36 +++++++++++++++++-- modules/gapi/test/cpu/gapi_core_tests_cpu.cpp | 8 +++++ .../gapi/test/cpu/gapi_core_tests_fluid.cpp | 8 +++++ modules/gapi/test/gpu/gapi_core_tests_gpu.cpp | 8 +++++ 7 files changed, 80 insertions(+), 3 deletions(-) diff --git a/modules/gapi/src/api/gbackend.cpp b/modules/gapi/src/api/gbackend.cpp index 6d4af8f6ad..43227bf590 100644 --- a/modules/gapi/src/api/gbackend.cpp +++ b/modules/gapi/src/api/gbackend.cpp @@ -103,7 +103,7 @@ void bindInArg(Mag& mag, const RcDesc &rc, const GRunArg &arg, bool is_umat) auto& mag_umat = mag.template slot()[rc.id]; mag_umat = to_ocv(util::get(arg)).getUMat(ACCESS_READ); #else - util::throw_error(std::logic_error("UMat is not supported in stadnalone build")); + util::throw_error(std::logic_error("UMat is not supported in standalone build")); #endif // !defined(GAPI_STANDALONE) } else diff --git a/modules/gapi/src/backends/ocl/goclbackend.cpp b/modules/gapi/src/backends/ocl/goclbackend.cpp index e5a6493ede..ab2c420564 100644 --- a/modules/gapi/src/backends/ocl/goclbackend.cpp +++ b/modules/gapi/src/backends/ocl/goclbackend.cpp @@ -150,6 +150,26 @@ void cv::gimpl::GOCLExecutable::run(std::vector &&input_objs, // has received from user (or from another Island, or mix...) // FIXME: Check input/output objects against GIsland protocol + // NB: We must clean-up m_res before this function returns because internally (bindInArg, + // bindOutArg) we work with cv::UMats, not cv::Mats that were originally placed into the + // input/output objects. If this is not done and cv::UMat "leaves" the local function scope, + // certain problems may occur. + // + // For example, if the original output (cv::Mat) is re-initialized by the user but we still + // hold cv::UMat -> we get cv::UMat that has a parent that was already destroyed. Also, + // since we don't own the data (the user does), there's no point holding it after we're done + const auto clean_up = [&input_objs, &output_objs] (cv::gimpl::Mag* p) + { + // Only clean-up UMat entries from current scope, we know that inputs and outputs are stored + // as UMats from the context below, so the following procedure is safe + auto& umats = p->slot(); + // NB: avoid clearing the whole magazine, there's also pre-allocated internal data + for (auto& it : input_objs) umats.erase(it.first.id); + for (auto& it : output_objs) umats.erase(it.first.id); + }; + // RAII wrapper to clean-up m_res + std::unique_ptr cleaner(&m_res, clean_up); + for (auto& it : input_objs) magazine::bindInArg (m_res, it.first, it.second, true); for (auto& it : output_objs) magazine::bindOutArg(m_res, it.first, it.second, true); diff --git a/modules/gapi/test/common/gapi_core_tests.hpp b/modules/gapi/test/common/gapi_core_tests.hpp index b249a4c944..5644c19e3d 100644 --- a/modules/gapi/test/common/gapi_core_tests.hpp +++ b/modules/gapi/test/common/gapi_core_tests.hpp @@ -123,6 +123,7 @@ struct BackendOutputAllocationTest : TestWithParamBase<> }; // FIXME: move all tests from this fixture to the base class once all issues are resolved struct BackendOutputAllocationLargeSizeWithCorrectSubmatrixTest : BackendOutputAllocationTest {}; +GAPI_TEST_FIXTURE(ReInitOutTest, initNothing, , 1, out_sz) } // opencv_test #endif //OPENCV_GAPI_CORE_TESTS_HPP diff --git a/modules/gapi/test/common/gapi_core_tests_inl.hpp b/modules/gapi/test/common/gapi_core_tests_inl.hpp index e0408ddf77..5b22ef98b5 100644 --- a/modules/gapi/test/common/gapi_core_tests_inl.hpp +++ b/modules/gapi/test/common/gapi_core_tests_inl.hpp @@ -1310,8 +1310,7 @@ TEST_P(BackendOutputAllocationTest, CorrectlyPreallocatedOutput) EXPECT_EQ(out_mat_gapi_ref.data, out_mat_gapi.data); } -// FIXME: known issue with OCL backend - PR #14985 -TEST_P(BackendOutputAllocationTest, DISABLED_IncorrectOutputMeta) +TEST_P(BackendOutputAllocationTest, IncorrectOutputMeta) { // G-API code ////////////////////////////////////////////////////////////// cv::GMat in1, in2, out; @@ -1479,6 +1478,39 @@ TEST_P(BackendOutputAllocationTest, LargerPreallocatedSizeWithSmallSubmatrix) EXPECT_NE(out_mat_gapi.data, out_mat_gapi_submat.datastart); } +TEST_P(ReInitOutTest, TestWithAdd) +{ + in_mat1 = cv::Mat(sz, type); + in_mat2 = cv::Mat(sz, type); + cv::randu(in_mat1, cv::Scalar::all(0), cv::Scalar::all(100)); + cv::randu(in_mat2, cv::Scalar::all(0), cv::Scalar::all(100)); + + // G-API code ////////////////////////////////////////////////////////////// + cv::GMat in1, in2, out; + out = cv::gapi::add(in1, in2, dtype); + cv::GComputation c(cv::GIn(in1, in2), cv::GOut(out)); + + const auto run_and_compare = [&c, this] () + { + // G-API code ////////////////////////////////////////////////////////////// + c.apply(cv::gin(in_mat1, in_mat2), cv::gout(out_mat_gapi), getCompileArgs()); + + // OpenCV code ///////////////////////////////////////////////////////////// + cv::add(in_mat1, in_mat2, out_mat_ocv, cv::noArray()); + + // Comparison ////////////////////////////////////////////////////////////// + EXPECT_EQ(0, cv::countNonZero(out_mat_gapi != out_mat_ocv)); + EXPECT_EQ(out_mat_gapi.size(), sz); + }; + + // run for uninitialized output + run_and_compare(); + + // run for initialized output (can be initialized with a different size) + initOutMats(out_sz, type); + run_and_compare(); +} + } // opencv_test #endif //OPENCV_GAPI_CORE_TESTS_INL_HPP diff --git a/modules/gapi/test/cpu/gapi_core_tests_cpu.cpp b/modules/gapi/test/cpu/gapi_core_tests_cpu.cpp index 458372f440..665d525bc1 100644 --- a/modules/gapi/test/cpu/gapi_core_tests_cpu.cpp +++ b/modules/gapi/test/cpu/gapi_core_tests_cpu.cpp @@ -448,4 +448,12 @@ INSTANTIATE_TEST_CASE_P(BackendOutputAllocationLargeSizeWithCorrectSubmatrixTest Values(cv::Size(50, 50)), Values(-1), Values(CORE_CPU))); + +INSTANTIATE_TEST_CASE_P(ReInitOutTestCPU, ReInitOutTest, + Combine(Values(CV_8UC3, CV_16SC4, CV_32FC1), + Values(cv::Size(640, 480)), + Values(-1), + Values(CORE_CPU), + Values(cv::Size(640, 400), + cv::Size(10, 480)))); } diff --git a/modules/gapi/test/cpu/gapi_core_tests_fluid.cpp b/modules/gapi/test/cpu/gapi_core_tests_fluid.cpp index 960136efe0..be158d029f 100644 --- a/modules/gapi/test/cpu/gapi_core_tests_fluid.cpp +++ b/modules/gapi/test/cpu/gapi_core_tests_fluid.cpp @@ -267,6 +267,14 @@ INSTANTIATE_TEST_CASE_P(BackendOutputAllocationLargeSizeWithCorrectSubmatrixTest Values(-1), Values(CORE_FLUID))); +INSTANTIATE_TEST_CASE_P(ReInitOutTestFluid, ReInitOutTest, + Combine(Values(CV_8UC3, CV_16SC4, CV_32FC1), + Values(cv::Size(640, 480)), + Values(-1), + Values(CORE_FLUID), + Values(cv::Size(640, 400), + cv::Size(10, 480)))); + //---------------------------------------------------------------------- // FIXME: Clean-up test configurations which are enabled already #if 0 diff --git a/modules/gapi/test/gpu/gapi_core_tests_gpu.cpp b/modules/gapi/test/gpu/gapi_core_tests_gpu.cpp index 8c68a935e8..0f67688f83 100644 --- a/modules/gapi/test/gpu/gapi_core_tests_gpu.cpp +++ b/modules/gapi/test/gpu/gapi_core_tests_gpu.cpp @@ -393,6 +393,14 @@ INSTANTIATE_TEST_CASE_P(DISABLED_BackendOutputAllocationLargeSizeWithCorrectSubm Values(-1), Values(CORE_GPU))); +INSTANTIATE_TEST_CASE_P(ReInitOutTestGPU, ReInitOutTest, + Combine(Values(CV_8UC3, CV_16SC4, CV_32FC1), + Values(cv::Size(640, 480)), + Values(-1), + Values(CORE_GPU), + Values(cv::Size(640, 400), + cv::Size(10, 480)))); + //TODO: fix this backend to allow ConcatVertVec ConcatHorVec #if 0 INSTANTIATE_TEST_CASE_P(ConcatVertVecTestGPU, ConcatVertVecTest,