Fix unpacking overlapping unpack buffer rows on NVIDIA GL
When unpack parameters are set so that rows being read overlap in
the unpack buffer stored in GPU memory, NVIDIA GL driver may not
upload the last pixels of the last one or more rows of a texture. The
driver may also crash when the amount of overlap is high.
This issue affects both TexImage* and TexSubImage* calls.
Work around the issue by uploading textures row by row when the rows
being read overlap in the unpack buffer. The workaround could possibly
be optimized by uploading several of the first rows with a single call
in some cases where the amount of overlap is low, but this is expected
to be a rarely used corner case, so the added complexity that the
optimization would create seems like a bad tradeoff.
The issue does not seem to be triggered when the layers (images) of a
3D texture overlap, as long as the rows inside the images don't.
The workaround has been ported from Chromium.
This patch adds setting dirty bits when unpack state is set in
StateManagerGL.
The included test case also reveals some issue in the D3D backend, but
this is left to be addressed later.
BUG=angleproject:1376
TEST=angle_end2end_tests
Change-Id: I7dbe73ebb70bbbc284fa92381546f4f2f832d333
Reviewed-on: https://chromium-review.googlesource.com/346430
Reviewed-by: Jamie Madill <jmadill@chromium.org>
Commit-Queue: Olli Etuaho <oetuaho@nvidia.com>
diff --git a/src/libANGLE/renderer/gl/StateManagerGL.cpp b/src/libANGLE/renderer/gl/StateManagerGL.cpp
index 9bd8b75..f76f785 100644
--- a/src/libANGLE/renderer/gl/StateManagerGL.cpp
+++ b/src/libANGLE/renderer/gl/StateManagerGL.cpp
@@ -432,7 +432,7 @@
mUnpackSkipRows = skipRows;
mFunctions->pixelStorei(GL_UNPACK_SKIP_ROWS, mUnpackSkipRows);
- // TODO: set dirty bit once one exists
+ mLocalDirtyBits.set(gl::State::DIRTY_BIT_UNPACK_SKIP_ROWS);
}
if (mUnpackSkipPixels != skipPixels)
@@ -440,7 +440,7 @@
mUnpackSkipPixels = skipPixels;
mFunctions->pixelStorei(GL_UNPACK_SKIP_PIXELS, mUnpackSkipPixels);
- // TODO: set dirty bit once one exists
+ mLocalDirtyBits.set(gl::State::DIRTY_BIT_UNPACK_SKIP_PIXELS);
}
if (mUnpackImageHeight != imageHeight)
@@ -448,7 +448,7 @@
mUnpackImageHeight = imageHeight;
mFunctions->pixelStorei(GL_UNPACK_IMAGE_HEIGHT, mUnpackImageHeight);
- // TODO: set dirty bit once one exists
+ mLocalDirtyBits.set(gl::State::DIRTY_BIT_UNPACK_IMAGE_HEIGHT);
}
if (mUnpackSkipImages != skipImages)
@@ -456,7 +456,7 @@
mUnpackSkipImages = skipImages;
mFunctions->pixelStorei(GL_UNPACK_SKIP_IMAGES, mUnpackSkipImages);
- // TODO: set dirty bit once one exists
+ mLocalDirtyBits.set(gl::State::DIRTY_BIT_UNPACK_SKIP_IMAGES);
}
bindBuffer(GL_PIXEL_UNPACK_BUFFER, unpackBuffer);
@@ -1616,4 +1616,9 @@
}
}
+GLuint StateManagerGL::getBoundBuffer(GLenum type)
+{
+ ASSERT(mBuffers.find(type) != mBuffers.end());
+ return mBuffers[type];
+}
}
diff --git a/src/libANGLE/renderer/gl/StateManagerGL.h b/src/libANGLE/renderer/gl/StateManagerGL.h
index 82978bb..c4fe748 100644
--- a/src/libANGLE/renderer/gl/StateManagerGL.h
+++ b/src/libANGLE/renderer/gl/StateManagerGL.h
@@ -146,6 +146,8 @@
void syncState(const gl::State &state, const gl::State::DirtyBits &glDirtyBits);
+ GLuint getBoundBuffer(GLenum type);
+
private:
gl::Error setGenericDrawState(const gl::ContextState &data);
diff --git a/src/libANGLE/renderer/gl/TextureGL.cpp b/src/libANGLE/renderer/gl/TextureGL.cpp
index cc8c14b..e9db0ad 100644
--- a/src/libANGLE/renderer/gl/TextureGL.cpp
+++ b/src/libANGLE/renderer/gl/TextureGL.cpp
@@ -137,6 +137,30 @@
gl::Error TextureGL::setImage(GLenum target, size_t level, GLenum internalFormat, const gl::Extents &size, GLenum format, GLenum type,
const gl::PixelUnpackState &unpack, const uint8_t *pixels)
{
+ if (mWorkarounds.unpackOverlappingRowsSeparatelyUnpackBuffer && unpack.pixelBuffer.get() &&
+ unpack.rowLength != 0 && unpack.rowLength < size.width)
+ {
+ // The rows overlap in unpack memory. Upload the texture row by row to work around
+ // driver bug.
+ reserveTexImageToBeFilled(target, level, internalFormat, size, format, type);
+ gl::Box area(0, 0, 0, size.width, size.height, size.depth);
+ ANGLE_TRY(setSubImageRowByRowWorkaround(target, level, area, format, type, unpack, pixels));
+ }
+ else
+ {
+ setImageHelper(target, level, internalFormat, size, format, type, pixels);
+ }
+ return gl::NoError();
+}
+
+void TextureGL::setImageHelper(GLenum target,
+ size_t level,
+ GLenum internalFormat,
+ const gl::Extents &size,
+ GLenum format,
+ GLenum type,
+ const uint8_t *pixels)
+{
UNUSED_ASSERTION_VARIABLE(&CompatibleTextureTarget); // Reference this function to avoid warnings.
ASSERT(CompatibleTextureTarget(mState.mTarget, target));
@@ -144,6 +168,7 @@
nativegl::GetTexImageFormat(mFunctions, mWorkarounds, internalFormat, format, type);
mStateManager->bindTexture(mState.mTarget, mTextureID);
+
if (UseTexImage2D(mState.mTarget))
{
ASSERT(size.depth == 1);
@@ -163,8 +188,20 @@
}
mLevelInfo[level] = GetLevelInfo(internalFormat, texImageFormat.internalFormat);
+}
- return gl::Error(GL_NO_ERROR);
+void TextureGL::reserveTexImageToBeFilled(GLenum target,
+ size_t level,
+ GLenum internalFormat,
+ const gl::Extents &size,
+ GLenum format,
+ GLenum type)
+{
+ GLuint unpackBuffer = mStateManager->getBoundBuffer(GL_PIXEL_UNPACK_BUFFER);
+ mStateManager->bindBuffer(GL_PIXEL_UNPACK_BUFFER, 0);
+ gl::PixelUnpackState unpack;
+ setImageHelper(target, level, internalFormat, size, format, type, nullptr);
+ mStateManager->bindBuffer(GL_PIXEL_UNPACK_BUFFER, unpackBuffer);
}
gl::Error TextureGL::setSubImage(GLenum target, size_t level, const gl::Box &area, GLenum format, GLenum type,
@@ -176,7 +213,12 @@
nativegl::GetTexSubImageFormat(mFunctions, mWorkarounds, format, type);
mStateManager->bindTexture(mState.mTarget, mTextureID);
- if (UseTexImage2D(mState.mTarget))
+ if (mWorkarounds.unpackOverlappingRowsSeparatelyUnpackBuffer && unpack.pixelBuffer.get() &&
+ unpack.rowLength != 0 && unpack.rowLength < area.width)
+ {
+ ANGLE_TRY(setSubImageRowByRowWorkaround(target, level, area, format, type, unpack, pixels));
+ }
+ else if (UseTexImage2D(mState.mTarget))
{
ASSERT(area.z == 0 && area.depth == 1);
mFunctions->texSubImage2D(target, static_cast<GLint>(level), area.x, area.y, area.width,
@@ -200,6 +242,69 @@
return gl::Error(GL_NO_ERROR);
}
+gl::Error TextureGL::setSubImageRowByRowWorkaround(GLenum target,
+ size_t level,
+ const gl::Box &area,
+ GLenum format,
+ GLenum type,
+ const gl::PixelUnpackState &unpack,
+ const uint8_t *pixels)
+{
+ gl::PixelUnpackState unpackToUse;
+ unpackToUse.pixelBuffer = unpack.pixelBuffer;
+ mStateManager->setPixelUnpackState(unpackToUse);
+ unpackToUse.pixelBuffer.set(nullptr);
+ GLenum sizedFormat =
+ gl::GetSizedInternalFormat(mState.getImageDesc(mState.mTarget, level).internalFormat, type);
+ const gl::InternalFormat &formatInfo = gl::GetInternalFormatInfo(sizedFormat);
+ GLuint rowBytes = 0;
+ ANGLE_TRY_RESULT(
+ formatInfo.computeRowPitch(GL_NONE, area.width, unpack.alignment, unpack.rowLength),
+ rowBytes);
+ GLuint imageBytes = 0;
+ ANGLE_TRY_RESULT(
+ formatInfo.computeDepthPitch(GL_NONE, area.width, area.height, unpack.alignment,
+ unpack.rowLength, unpack.imageHeight),
+ imageBytes);
+ bool useTexImage3D = UseTexImage3D(mState.mTarget);
+ GLuint skipBytes = 0;
+ ANGLE_TRY_RESULT(formatInfo.computeSkipBytes(rowBytes, imageBytes, unpack.skipImages,
+ unpack.skipRows, unpack.skipPixels, useTexImage3D),
+ skipBytes);
+
+ const uint8_t *pixelsWithSkip = pixels + skipBytes;
+ if (useTexImage3D)
+ {
+ for (GLint image = 0; image < area.depth; ++image)
+ {
+ GLint imageByteOffset = image * imageBytes;
+ for (GLint row = 0; row < area.height; ++row)
+ {
+ GLint byteOffset = imageByteOffset + row * rowBytes;
+ const GLubyte *rowPixels = pixelsWithSkip + byteOffset;
+ mFunctions->texSubImage3D(target, static_cast<GLint>(level), area.x, row + area.y,
+ image + area.z, area.width, 1, 1, format, type,
+ rowPixels);
+ }
+ }
+ }
+ else if (UseTexImage2D(mState.mTarget))
+ {
+ for (GLint row = 0; row < area.height; ++row)
+ {
+ GLint byteOffset = row * rowBytes;
+ const GLubyte *rowPixels = pixelsWithSkip + byteOffset;
+ mFunctions->texSubImage2D(target, static_cast<GLint>(level), area.x, row + area.y,
+ area.width, 1, format, type, rowPixels);
+ }
+ }
+ else
+ {
+ UNREACHABLE();
+ }
+ return gl::NoError();
+}
+
gl::Error TextureGL::setCompressedImage(GLenum target, size_t level, GLenum internalFormat, const gl::Extents &size,
const gl::PixelUnpackState &unpack, size_t imageSize, const uint8_t *pixels)
{
diff --git a/src/libANGLE/renderer/gl/TextureGL.h b/src/libANGLE/renderer/gl/TextureGL.h
index b688a19..540e6c3 100644
--- a/src/libANGLE/renderer/gl/TextureGL.h
+++ b/src/libANGLE/renderer/gl/TextureGL.h
@@ -99,6 +99,27 @@
void setBaseLevel(GLuint) override {}
private:
+ void setImageHelper(GLenum target,
+ size_t level,
+ GLenum internalFormat,
+ const gl::Extents &size,
+ GLenum format,
+ GLenum type,
+ const uint8_t *pixels);
+ void reserveTexImageToBeFilled(GLenum target,
+ size_t level,
+ GLenum internalFormat,
+ const gl::Extents &size,
+ GLenum format,
+ GLenum type);
+ gl::Error setSubImageRowByRowWorkaround(GLenum target,
+ size_t level,
+ const gl::Box &area,
+ GLenum format,
+ GLenum type,
+ const gl::PixelUnpackState &unpack,
+ const uint8_t *pixels);
+
const FunctionsGL *mFunctions;
const WorkaroundsGL &mWorkarounds;
StateManagerGL *mStateManager;
diff --git a/src/libANGLE/renderer/gl/WorkaroundsGL.h b/src/libANGLE/renderer/gl/WorkaroundsGL.h
index a79da25..2549a2c 100644
--- a/src/libANGLE/renderer/gl/WorkaroundsGL.h
+++ b/src/libANGLE/renderer/gl/WorkaroundsGL.h
@@ -20,7 +20,8 @@
doesSRGBClearsOnLinearFramebufferAttachments(false),
doWhileGLSLCausesGPUHang(false),
finishDoesNotCauseQueriesToBeAvailable(false),
- alwaysCallUseProgramAfterLink(false)
+ alwaysCallUseProgramAfterLink(false),
+ unpackOverlappingRowsSeparatelyUnpackBuffer(false)
{
}
@@ -63,6 +64,9 @@
// workaround in Chromium (http://crbug.com/110263). It has been shown that this workaround is
// not necessary for MacOSX 10.9 and higher (http://crrev.com/39eb535b).
bool alwaysCallUseProgramAfterLink;
+
+ // In the case of unpacking from a pixel unpack buffer, unpack overlapping rows row by row.
+ bool unpackOverlappingRowsSeparatelyUnpackBuffer;
};
}
diff --git a/src/libANGLE/renderer/gl/renderergl_utils.cpp b/src/libANGLE/renderer/gl/renderergl_utils.cpp
index 006d81d..9668c70 100644
--- a/src/libANGLE/renderer/gl/renderergl_utils.cpp
+++ b/src/libANGLE/renderer/gl/renderergl_utils.cpp
@@ -697,6 +697,8 @@
// TODO(cwallez): Disable this workaround for MacOSX versions 10.9 or later.
workarounds->alwaysCallUseProgramAfterLink = true;
+
+ workarounds->unpackOverlappingRowsSeparatelyUnpackBuffer = vendor == VENDOR_ID_NVIDIA;
}
}
diff --git a/src/tests/gl_tests/TextureTest.cpp b/src/tests/gl_tests/TextureTest.cpp
index c86bdfb..85dc554 100644
--- a/src/tests/gl_tests/TextureTest.cpp
+++ b/src/tests/gl_tests/TextureTest.cpp
@@ -3388,6 +3388,67 @@
EXPECT_GL_ERROR(GL_INVALID_OPERATION);
}
+// Test that unpacking rows that overlap in a pixel unpack buffer works as expected.
+TEST_P(Texture2DTestES3, UnpackOverlappingRowsFromUnpackBuffer)
+{
+ if (IsD3D11())
+ {
+ std::cout << "Test skipped on D3D." << std::endl;
+ return;
+ }
+ if (IsOSX() && IsAMD())
+ {
+ // Incorrect rendering results seen on OSX AMD.
+ std::cout << "Test skipped on OSX AMD." << std::endl;
+ return;
+ }
+
+ const GLuint width = 8u;
+ const GLuint height = 8u;
+ const GLuint unpackRowLength = 5u;
+ const GLuint unpackSkipPixels = 1u;
+
+ setWindowWidth(width);
+ setWindowHeight(height);
+
+ glBindTexture(GL_TEXTURE_2D, mTexture2D);
+ glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
+ glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
+ ASSERT_GL_NO_ERROR();
+
+ GLBuffer buf;
+ glBindBuffer(GL_PIXEL_UNPACK_BUFFER, buf.get());
+ std::vector<GLColor> pixelsGreen((height - 1u) * unpackRowLength + width + unpackSkipPixels,
+ GLColor::green);
+
+ for (GLuint skippedPixel = 0u; skippedPixel < unpackSkipPixels; ++skippedPixel)
+ {
+ pixelsGreen[skippedPixel] = GLColor(255, 0, 0, 255);
+ }
+
+ glBufferData(GL_PIXEL_UNPACK_BUFFER, pixelsGreen.size() * 4u, pixelsGreen.data(),
+ GL_DYNAMIC_COPY);
+ ASSERT_GL_NO_ERROR();
+
+ glPixelStorei(GL_UNPACK_ROW_LENGTH, unpackRowLength);
+ glPixelStorei(GL_UNPACK_SKIP_PIXELS, unpackSkipPixels);
+ ASSERT_GL_NO_ERROR();
+
+ glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA8, width, height, 0, GL_RGBA, GL_UNSIGNED_BYTE, 0);
+ ASSERT_GL_NO_ERROR();
+
+ glUseProgram(mProgram);
+ drawQuad(mProgram, "position", 0.5f);
+ ASSERT_GL_NO_ERROR();
+
+ GLuint windowPixelCount = getWindowWidth() * getWindowHeight();
+ std::vector<GLColor> actual(windowPixelCount, GLColor::black);
+ glReadPixels(0, 0, getWindowWidth(), getWindowHeight(), GL_RGBA, GL_UNSIGNED_BYTE,
+ actual.data());
+ std::vector<GLColor> expected(windowPixelCount, GLColor::green);
+ EXPECT_EQ(expected, actual);
+}
+
// Use this to select which configurations (e.g. which renderer, which GLES major version) these tests should be run against.
// TODO(oetuaho): Enable all below tests on OpenGL. Requires a fix for ANGLE bug 1278.
ANGLE_INSTANTIATE_TEST(Texture2DTest,