From cea2dafa0f2e130b88ff05728e30b528deb3e2ef Mon Sep 17 00:00:00 2001 From: Andrey Pavlenko Date: Thu, 3 Sep 2015 17:18:59 +0300 Subject: [PATCH] man/unmap, preventing getMat/getUMat from temp object, fix thread-unsafe code in `UMat::getMat()` --- modules/core/include/opencv2/core/mat.hpp | 1 + modules/core/src/ocl.cpp | 58 ++++++++++++++--------- modules/core/src/umatrix.cpp | 34 ++++++++----- modules/core/test/test_umat.cpp | 28 ++++++++--- 4 files changed, 82 insertions(+), 39 deletions(-) diff --git a/modules/core/include/opencv2/core/mat.hpp b/modules/core/include/opencv2/core/mat.hpp index e85bf6fa5b..5c5284f8d5 100644 --- a/modules/core/include/opencv2/core/mat.hpp +++ b/modules/core/include/opencv2/core/mat.hpp @@ -496,6 +496,7 @@ struct CV_EXPORTS UMatData void* handle; void* userdata; int allocatorFlags_; + int mapcount; }; diff --git a/modules/core/src/ocl.cpp b/modules/core/src/ocl.cpp index 1074f9b011..f6e27302ff 100644 --- a/modules/core/src/ocl.cpp +++ b/modules/core/src/ocl.cpp @@ -4514,6 +4514,7 @@ public: CV_Assert(u->refcount >= 0); CV_Assert(u->handle != 0 && u->urefcount == 0); + CV_Assert(u->mapcount == 0); if(u->tempUMat()) { CV_Assert(u->origdata); @@ -4572,12 +4573,16 @@ public: else { cl_int retval = 0; - void* data = clEnqueueMapBuffer(q, (cl_mem)u->handle, CL_TRUE, - (CL_MAP_READ | CL_MAP_WRITE), - 0, u->size, 0, 0, 0, &retval); - CV_OclDbgAssert(retval == CL_SUCCESS); - CV_OclDbgAssert(clEnqueueUnmapMemObject(q, (cl_mem)u->handle, data, 0, 0, 0) == CL_SUCCESS); - CV_OclDbgAssert(clFinish(q) == CL_SUCCESS); + if (u->tempUMat()) + { + CV_Assert(u->mapcount == 0); + void* data = clEnqueueMapBuffer(q, (cl_mem)u->handle, CL_TRUE, + (CL_MAP_READ | CL_MAP_WRITE), + 0, u->size, 0, 0, 0, &retval); + CV_OclDbgAssert(retval == CL_SUCCESS); + CV_OclDbgAssert(clEnqueueUnmapMemObject(q, (cl_mem)u->handle, data, 0, 0, 0) == CL_SUCCESS); + CV_OclDbgAssert(clFinish(q) == CL_SUCCESS); + } } } u->markHostCopyObsolete(false); @@ -4715,11 +4720,16 @@ public: } #endif - cl_int retval = 0; - u->data = (uchar*)clEnqueueMapBuffer(q, (cl_mem)u->handle, CL_TRUE, - (CL_MAP_READ | CL_MAP_WRITE), - 0, u->size, 0, 0, 0, &retval); - if(u->data && retval == CL_SUCCESS) + cl_int retval = CL_SUCCESS; + if (!u->deviceMemMapped()) + { + CV_Assert(u->refcount == 1); + CV_Assert(u->mapcount++ == 0); + u->data = (uchar*)clEnqueueMapBuffer(q, (cl_mem)u->handle, CL_TRUE, + (CL_MAP_READ | CL_MAP_WRITE), + 0, u->size, 0, 0, 0, &retval); + } + if (u->data && retval == CL_SUCCESS) { u->markHostCopyObsolete(false); u->markDeviceMemMapped(true); @@ -4765,7 +4775,6 @@ public: if( !u->copyOnMap() && u->deviceMemMapped() ) { CV_Assert(u->data != NULL); - u->markDeviceMemMapped(false); #ifdef HAVE_OPENCL_SVM if ((u->allocatorFlags_ & svm::OPENCL_SVM_BUFFER_MASK) != 0) { @@ -4792,16 +4801,21 @@ public: return; } #endif - CV_Assert( (retval = clEnqueueUnmapMemObject(q, - (cl_mem)u->handle, u->data, 0, 0, 0)) == CL_SUCCESS ); - if (Device::getDefault().isAMD()) - { - // required for multithreaded applications (see stitching test) - CV_OclDbgAssert(clFinish(q) == CL_SUCCESS); - } - if (u->refcount == 0) + { + CV_Assert(u->mapcount-- == 1); + CV_Assert((retval = clEnqueueUnmapMemObject(q, + (cl_mem)u->handle, u->data, 0, 0, 0)) == CL_SUCCESS); + if (Device::getDefault().isAMD()) + { + // required for multithreaded applications (see stitching test) + CV_OclDbgAssert(clFinish(q) == CL_SUCCESS); + } + u->markDeviceMemMapped(false); u->data = 0; + u->markDeviceCopyObsolete(false); + u->markHostCopyObsolete(true); + } } else if( u->copyOnMap() && u->deviceCopyObsolete() ) { @@ -4811,9 +4825,9 @@ public: #endif CV_Assert( (retval = clEnqueueWriteBuffer(q, (cl_mem)u->handle, CL_TRUE, 0, u->size, alignedPtr.getAlignedPtr(), 0, 0, 0)) == CL_SUCCESS ); + u->markDeviceCopyObsolete(false); + u->markHostCopyObsolete(true); } - u->markDeviceCopyObsolete(false); - u->markHostCopyObsolete(true); } bool checkContinuous(int dims, const size_t sz[], diff --git a/modules/core/src/umatrix.cpp b/modules/core/src/umatrix.cpp index ad7efae16d..b7c60726ab 100644 --- a/modules/core/src/umatrix.cpp +++ b/modules/core/src/umatrix.cpp @@ -60,7 +60,7 @@ static Mutex umatLocks[UMAT_NLOCKS]; UMatData::UMatData(const MatAllocator* allocator) { prevAllocator = currAllocator = allocator; - urefcount = refcount = 0; + urefcount = refcount = mapcount = 0; data = origdata = 0; size = 0; flags = 0; @@ -73,6 +73,7 @@ UMatData::~UMatData() { prevAllocator = currAllocator = 0; urefcount = refcount = 0; + CV_Assert(mapcount == 0); data = origdata = 0; size = 0; flags = 0; @@ -221,6 +222,7 @@ UMat Mat::getUMat(int accessFlags, UMatUsageFlags usageFlags) const UMat hdr; if(!data) return hdr; + CV_Assert((!u || u->mapcount==0) && "Don't get UMat from temp-Mat!"); accessFlags |= ACCESS_RW; UMatData* temp_u = u; if(!temp_u) @@ -637,18 +639,28 @@ Mat UMat::getMat(int accessFlags) const { if(!u) return Mat(); + CV_Assert(!u->tempUMat() && "Don't get Mat from temp UMat! Use copyTo()."); // TODO Support ACCESS_READ (ACCESS_WRITE) without unnecessary data transfers accessFlags |= ACCESS_RW; - u->currAllocator->map(u, accessFlags); - CV_Assert(u->data != 0); - Mat hdr(dims, size.p, type(), u->data + offset, step.p); - hdr.flags = flags; - hdr.u = u; - hdr.datastart = u->data; - hdr.data = u->data + offset; - hdr.datalimit = hdr.dataend = u->data + u->size; - CV_XADD(&hdr.u->refcount, 1); - return hdr; + UMatDataAutoLock autolock(u); + if(CV_XADD(&u->refcount, 1) == 0) + u->currAllocator->map(u, accessFlags); + if (u->data != 0) + { + Mat hdr(dims, size.p, type(), u->data + offset, step.p); + hdr.flags = flags; + hdr.u = u; + hdr.datastart = u->data; + hdr.data = u->data + offset; + hdr.datalimit = hdr.dataend = u->data + u->size; + return hdr; + } + else + { + CV_XADD(&u->refcount, -1); + CV_Assert(u->data != 0 && "Error mapping of UMat to host memory."); + return Mat(); + } } void* UMat::handle(int accessFlags) const diff --git a/modules/core/test/test_umat.cpp b/modules/core/test/test_umat.cpp index 56a7a18172..7c1310dd03 100644 --- a/modules/core/test/test_umat.cpp +++ b/modules/core/test/test_umat.cpp @@ -243,9 +243,11 @@ TEST_P(UMatBasicTests, GetUMat) EXPECT_MAT_NEAR(ub, ua, 0); } { - Mat b; - b = a.getUMat(ACCESS_RW).getMat(ACCESS_RW); - EXPECT_MAT_NEAR(b, a, 0); + UMat u = a.getUMat(ACCESS_RW); + { + Mat b = u.getMat(ACCESS_RW); + EXPECT_MAT_NEAR(b, a, 0); + } } { Mat b; @@ -253,9 +255,11 @@ TEST_P(UMatBasicTests, GetUMat) EXPECT_MAT_NEAR(b, a, 0); } { - UMat ub; - ub = ua.getMat(ACCESS_RW).getUMat(ACCESS_RW); - EXPECT_MAT_NEAR(ub, ua, 0); + Mat m = ua.getMat(ACCESS_RW); + { + UMat ub = m.getUMat(ACCESS_RW); + EXPECT_MAT_NEAR(ub, ua, 0); + } } } @@ -1268,5 +1272,17 @@ TEST(UMat, DISABLED_Test_same_behaviour_write_and_write) ASSERT_TRUE(exceptionDetected); // data race } +TEST(UMat, mat_umat_sync) +{ + UMat u(10, 10, CV_8UC1, Scalar(1)); + { + Mat m = u.getMat(ACCESS_RW).reshape(1); + m.setTo(Scalar(255)); + } + + UMat uDiff; + compare(u, 255, uDiff, CMP_NE); + ASSERT_EQ(0, countNonZero(uDiff)); +} } } // namespace cvtest::ocl