IGLU: Fix issues with texture accessor

Summary: This diff fixes an issue with IGLU texture_accessor: when using the async path, the code is not handling the case where the data is available in the first call to getRequestStatus. In the current code, if this happens, the data is never actually read from the PBO. This diff adds a new flag to track whether the data has been read or not, and ensure that the data is always read before returning from getBytes. Reviewed By: rameshviswanathan Differential Revision: D49943435 fbshipit-source-id: 10b123f214550bc930ac673a8fcfeeb26999c352
facebook · Oct 25, 2023 · abedd0f · abedd0f
1 parent 24b2108
commit abedd0f
Show file tree

Hide file tree

Showing 5 changed files with 86 additions and 16 deletions.
diff --git a/IGLU/texture_accessor/OpenGLTextureAccessor.cpp b/IGLU/texture_accessor/OpenGLTextureAccessor.cpp
@@ -19,8 +19,7 @@
 #include <secure_lib/secure_string.h>
 #endif
 
-namespace iglu {
-namespace textureaccessor {
+namespace iglu::textureaccessor {
 
 OpenGLTextureAccessor::OpenGLTextureAccessor(std::shared_ptr<igl::ITexture> texture,
                                              igl::IDevice& device) :
@@ -49,27 +48,39 @@ OpenGLTextureAccessor::OpenGLTextureAccessor(std::shared_ptr<igl::ITexture> text
     // Create PBO and allocate size
     context.genBuffers((GLsizei)1, &pboId_);
     context.bindBuffer(GL_PIXEL_PACK_BUFFER, pboId_);
-    context.bufferData(GL_PIXEL_PACK_BUFFER, textureBytesPerImage_, nullptr, GL_STREAM_READ);
+    context.bufferData(GL_PIXEL_PACK_BUFFER, textureBytesPerImage_, nullptr, GL_DYNAMIC_READ);
     context.bindBuffer(GL_PIXEL_PACK_BUFFER, 0);
   }
 };
 
 void OpenGLTextureAccessor::requestBytes(igl::ICommandQueue& commandQueue,
                                          std::shared_ptr<igl::ITexture> texture) {
+  dataCopied_ = false;
   if (texture) {
     IGL_ASSERT(textureWidth_ == texture->getDimensions().width &&
                textureHeight_ == texture->getDimensions().height);
     texture_ = std::move(texture);
     frameBuffer_->updateDrawable(texture_);
+    textureAttached_ = false;
   }
 
   if (asyncReadbackSupported_) {
     auto& glTexture = static_cast<igl::opengl::Texture&>(*texture_);
     auto& context = glTexture.getContext();
 
-    auto oglFrameBuffer = static_cast<igl::opengl::Framebuffer*>(&(*frameBuffer_));
-    oglFrameBuffer->bindBufferForRead();
+    auto oglFrameBuffer = static_cast<igl::opengl::Framebuffer*>(frameBuffer_.get());
 
+    oglFrameBuffer->bindBufferForRead();
+    if (!textureAttached_) {
+      igl::opengl::Texture::AttachmentParams params;
+      params.read = true;
+      params.face = 0;
+      params.layer = 0;
+      params.mipLevel = 0;
+      params.stereo = false;
+      glTexture.attachAsColor(0u, params);
+      textureAttached_ = true;
+    }
     const auto& properties = glTexture.getProperties();
     context.pixelStorei(GL_PACK_ALIGNMENT,
                         glTexture.getAlignment(properties.getBytesPerRow(textureWidth_), 0));
@@ -89,6 +100,7 @@ void OpenGLTextureAccessor::requestBytes(igl::ICommandQueue& commandQueue,
   const auto range = igl::TextureRangeDesc::new2D(0, 0, textureWidth_, textureHeight_);
   frameBuffer_->copyBytesColorAttachment(commandQueue, 0, latestBytesRead_.data(), range);
 
+  dataCopied_ = true;
   status_ = RequestStatus::Ready;
 }
 
@@ -111,20 +123,23 @@ RequestStatus OpenGLTextureAccessor::getRequestStatus() {
 };
 
 std::vector<unsigned char>& OpenGLTextureAccessor::getBytes() {
-  if (asyncReadbackSupported_ && status_ == RequestStatus::InProgress) {
+  if (asyncReadbackSupported_ && status_ != RequestStatus::NotInitialized && !dataCopied_) {
     auto& texture = static_cast<igl::opengl::Texture&>(*texture_);
     auto& context = texture.getContext();
     context.bindBuffer(GL_PIXEL_PACK_BUFFER, pboId_);
     auto bytes =
         context.mapBufferRange(GL_PIXEL_PACK_BUFFER, 0, textureBytesPerImage_, GL_MAP_READ_BIT);
-    checked_memcpy_robust(latestBytesRead_.data(),
-                          latestBytesRead_.size(),
-                          bytes,
-                          textureBytesPerImage_,
-                          textureBytesPerImage_);
+    if (IGL_VERIFY(bytes)) {
+      checked_memcpy_robust(latestBytesRead_.data(),
+                            latestBytesRead_.size(),
+                            bytes,
+                            textureBytesPerImage_,
+                            textureBytesPerImage_);
+    }
     context.unmapBuffer(GL_PIXEL_PACK_BUFFER);
     context.bindBuffer(GL_PIXEL_PACK_BUFFER, 0);
     status_ = RequestStatus::Ready;
+    dataCopied_ = true;
     if (sync_ != nullptr) {
       context.deleteSync(sync_);
       sync_ = nullptr;
@@ -133,5 +148,4 @@ std::vector<unsigned char>& OpenGLTextureAccessor::getBytes() {
   return latestBytesRead_;
 }
 
-} // namespace textureaccessor
-} // namespace iglu
+} // namespace iglu::textureaccessor
diff --git a/IGLU/texture_accessor/OpenGLTextureAccessor.h b/IGLU/texture_accessor/OpenGLTextureAccessor.h
@@ -36,7 +36,9 @@ class OpenGLTextureAccessor : public ITextureAccessor {
 
   GLuint pboId_ = 0;
   GLsync sync_ = nullptr;
+  bool dataCopied_ = false;
   bool asyncReadbackSupported_ = false;
+  bool textureAttached_ = false;
 };
 
 } // namespace textureaccessor

diff --git a/src/igl/opengl/DeviceFeatureSet.cpp b/src/igl/opengl/DeviceFeatureSet.cpp
@@ -452,8 +452,8 @@ bool DeviceFeatureSet::isInternalFeatureSupported(InternalFeatures feature) cons
            hasESExtension(*this, "GL_EXT_shadow_samplers");
 
   case InternalFeatures::UnmapBuffer:
-    return hasDesktopVersion(*this, GLVersion::v2_0) || hasExtension(Extensions::MapBuffer) ||
-           hasExtension(Extensions::MapBufferRange);
+    return hasDesktopOrESVersion(*this, GLVersion::v2_0, GLVersion::v3_0_ES) ||
+           hasExtension(Extensions::MapBuffer) || hasExtension(Extensions::MapBufferRange);
 
   case InternalFeatures::UnpackRowLength:
     return hasDesktopOrESVersion(*this, GLVersion::v2_0, GLVersion::v3_0_ES) ||

diff --git a/src/igl/opengl/IContext.cpp b/src/igl/opengl/IContext.cpp
@@ -2084,7 +2084,13 @@ void IContext::getSynciv(GLsync sync,
     IGL_ASSERT_MSG(getSyncivProc_, "No supported function for glGetSynciv\n");
   }
   GLCALL_PROC(getSyncivProc_, sync, pname, bufSize, length, values);
-  APILOG("glGetSynciv(%p, %s, %u, %p, %p)\n", sync, pname, bufSize, length, values);
+  APILOG("glGetSynciv(%p, %s, %u, %p, %p) = %d\n",
+         sync,
+         GL_ENUM_TO_STRING(pname),
+         bufSize,
+         length,
+         values,
+         values == nullptr ? 0 : *values);
   GLCHECK_ERRORS();
 }
 

diff --git a/src/igl/tests/iglu/TextureAccessor.cpp b/src/igl/tests/iglu/TextureAccessor.cpp
@@ -103,5 +103,53 @@ TEST_F(TextureAccessorTest, testRequestAndGetBytesSync) {
   }
 }
 
+TEST_F(TextureAccessorTest, reuseTextureAccessor) {
+  ASSERT_NO_THROW(textureAccessor_ =
+                      iglu::textureaccessor::TextureAccessorFactory::createTextureAccessor(
+                          iglDev_->getBackendType(), texture_, *iglDev_));
+  ASSERT_TRUE(textureAccessor_ != nullptr);
+
+  // Verify requestStatus before
+  ASSERT_EQ(textureAccessor_->getRequestStatus(),
+            iglu::textureaccessor::RequestStatus::NotInitialized);
+
+  // First Upload
+  {
+    // Update texture data
+    const auto rangeDesc = TextureRangeDesc::new2D(0, 0, OFFSCREEN_TEX_WIDTH, OFFSCREEN_TEX_HEIGHT);
+    texture_->upload(rangeDesc, data::texture::TEX_RGBA_2x2);
+
+    auto bytes = textureAccessor_->requestAndGetBytesSync(*cmdQueue_);
+    // Verify requestStatus after
+    ASSERT_EQ(textureAccessor_->getRequestStatus(), iglu::textureaccessor::RequestStatus::Ready);
+
+    // 2x2 texture * 4 bytes per pixel
+    ASSERT_EQ(bytes.size(), 16);
+    // Verify data
+    auto* pixels = reinterpret_cast<uint32_t*>(bytes.data());
+    for (int i = 0; (i < textureSizeInBytes_ / 4); i++) {
+      ASSERT_EQ(pixels[i], data::texture::TEX_RGBA_2x2[i]);
+    }
+  }
+
+  // Second Upload
+  {
+    // Update texture data
+    const auto rangeDesc = TextureRangeDesc::new2D(0, 0, OFFSCREEN_TEX_WIDTH, OFFSCREEN_TEX_HEIGHT);
+    texture_->upload(rangeDesc, data::texture::TEX_RGBA_GRAY_2x2);
+
+    auto bytes = textureAccessor_->requestAndGetBytesSync(*cmdQueue_);
+    // Verify requestStatus after
+    ASSERT_EQ(textureAccessor_->getRequestStatus(), iglu::textureaccessor::RequestStatus::Ready);
+
+    // 2x2 texture * 4 bytes per pixel
+    ASSERT_EQ(bytes.size(), 16);
+    // Verify data
+    auto* pixels = reinterpret_cast<uint32_t*>(bytes.data());
+    for (int i = 0; (i < textureSizeInBytes_ / 4); i++) {
+      ASSERT_EQ(pixels[i], data::texture::TEX_RGBA_GRAY_2x2[i]);
+    }
+  }
+}
 } // namespace tests
 } // namespace igl