Implement robust ReadPixels entry points.

src/libANGLE/validationES.cpp

@@ -180,6 +180,137 @@ bool ValidateRobustBufferSize(ValidationContext *context, GLsizei bufSize, GLsiz
     return true;
 }
 
+bool ValidateReadPixelsBase(ValidationContext *context,
+                            GLint x,
+                            GLint y,
+                            GLsizei width,
+                            GLsizei height,
+                            GLenum format,
+                            GLenum type,
+                            GLsizei bufSize,
+                            GLsizei *length,
+                            GLvoid *pixels)
+{
+    if (length != nullptr)
+    {
+        *length = 0;
+    }
+
+    if (width < 0 || height < 0)
+    {
+        context->handleError(Error(GL_INVALID_VALUE, "width and height must be positive"));
+        return false;
+    }
+
+    auto readFramebuffer = context->getGLState().getReadFramebuffer();
+
+    if (readFramebuffer->checkStatus(context->getContextState()) != GL_FRAMEBUFFER_COMPLETE)
+    {
+        context->handleError(Error(GL_INVALID_FRAMEBUFFER_OPERATION));
+        return false;
+    }
+
+    if (readFramebuffer->id() != 0 && readFramebuffer->getSamples(context->getContextState()) != 0)
+    {
+        context->handleError(Error(GL_INVALID_OPERATION));
+        return false;
+    }
+
+    const Framebuffer *framebuffer = context->getGLState().getReadFramebuffer();
+    ASSERT(framebuffer);
+
+    if (framebuffer->getReadBufferState() == GL_NONE)
+    {
+        context->handleError(Error(GL_INVALID_OPERATION, "Read buffer is GL_NONE"));
+        return false;
+    }
+
+    const FramebufferAttachment *readBuffer = framebuffer->getReadColorbuffer();
+    if (!readBuffer)
+    {
+        context->handleError(Error(GL_INVALID_OPERATION));
+        return false;
+    }
+
+    GLenum currentFormat         = framebuffer->getImplementationColorReadFormat();
+    GLenum currentType           = framebuffer->getImplementationColorReadType();
+    GLenum currentInternalFormat = readBuffer->getFormat().asSized();
+
+    const gl::InternalFormat &internalFormatInfo = gl::GetInternalFormatInfo(currentInternalFormat);
+    bool validFormatTypeCombination =
+        ValidReadPixelsFormatType(context, internalFormatInfo.componentType, format, type);
+
+    if (!(currentFormat == format && currentType == type) && !validFormatTypeCombination)
+    {
+        context->handleError(Error(GL_INVALID_OPERATION));
+        return false;
+    }
+
+    // Check for pixel pack buffer related API errors
+    gl::Buffer *pixelPackBuffer = context->getGLState().getTargetBuffer(GL_PIXEL_PACK_BUFFER);
+    if (pixelPackBuffer != nullptr && pixelPackBuffer->isMapped())
+    {
+        // ...the buffer object's data store is currently mapped.
+        context->handleError(Error(GL_INVALID_OPERATION, "Pixel pack buffer is mapped."));
+        return false;
+    }
+
+    // ..  the data would be packed to the buffer object such that the memory writes required
+    // would exceed the data store size.
+    GLenum sizedInternalFormat       = GetSizedInternalFormat(format, type);
+    const InternalFormat &formatInfo = GetInternalFormatInfo(sizedInternalFormat);
+    const gl::Extents size(width, height, 1);
+    const auto &pack = context->getGLState().getPackState();
+
+    auto endByteOrErr = formatInfo.computePackUnpackEndByte(type, size, pack, false);
+    if (endByteOrErr.isError())
+    {
+        context->handleError(endByteOrErr.getError());
+        return false;
+    }
+
+    size_t endByte = endByteOrErr.getResult();
+    if (bufSize >= 0)
+    {
+
+        if (static_cast<size_t>(bufSize) < endByte)
+        {
+            context->handleError(
+                Error(GL_INVALID_OPERATION, "bufSize must be at least %u bytes.", endByte));
+            return false;
+        }
+    }
+
+    if (pixelPackBuffer != nullptr)
+    {
+        CheckedNumeric<size_t> checkedEndByte(endByte);
+        CheckedNumeric<size_t> checkedOffset(reinterpret_cast<size_t>(pixels));
+        checkedEndByte += checkedOffset;
+
+        if (checkedEndByte.ValueOrDie() > static_cast<size_t>(pixelPackBuffer->getSize()))
+        {
+            // Overflow past the end of the buffer
+            context->handleError(
+                Error(GL_INVALID_OPERATION, "Writes would overflow the pixel pack buffer."));
+            return false;
+        }
+    }
+
+    if (length != nullptr)
+    {
+        if (endByte > static_cast<size_t>(std::numeric_limits<GLsizei>::max()))
+        {
+            context->handleError(
+                Error(GL_INVALID_OPERATION, "length would overflow GLsizei.", endByte));
+            return false;
+        }
+
+        *length = static_cast<GLsizei>(endByte);
+    }
+
+    return true;
+}
+
 }  // anonymous namespace
 
 bool ValidTextureTarget(const ValidationContext *context, GLenum target)
@@ -1202,93 +1333,35 @@ bool ValidateReadPixels(ValidationContext *context,
                         GLenum type,
                         GLvoid *pixels)
 {
-    if (width < 0 || height < 0)
-    {
-        context->handleError(Error(GL_INVALID_VALUE, "width and height must be positive"));
-        return false;
-    }
-
-    auto readFramebuffer = context->getGLState().getReadFramebuffer();
-
-    if (readFramebuffer->checkStatus(context->getContextState()) != GL_FRAMEBUFFER_COMPLETE)
-    {
-        context->handleError(Error(GL_INVALID_FRAMEBUFFER_OPERATION));
-        return false;
-    }
-
-    if (readFramebuffer->id() != 0 && readFramebuffer->getSamples(context->getContextState()) != 0)
-    {
-        context->handleError(Error(GL_INVALID_OPERATION));
-        return false;
-    }
-
-    const Framebuffer *framebuffer = context->getGLState().getReadFramebuffer();
-    ASSERT(framebuffer);
-
-    if (framebuffer->getReadBufferState() == GL_NONE)
-    {
-        context->handleError(Error(GL_INVALID_OPERATION, "Read buffer is GL_NONE"));
-        return false;
-    }
-
-    const FramebufferAttachment *readBuffer = framebuffer->getReadColorbuffer();
-    if (!readBuffer)
-    {
-        context->handleError(Error(GL_INVALID_OPERATION));
-        return false;
-    }
-
-    GLenum currentFormat = framebuffer->getImplementationColorReadFormat();
-    GLenum currentType = framebuffer->getImplementationColorReadType();
-    GLenum currentInternalFormat = readBuffer->getFormat().asSized();
-
-    const gl::InternalFormat &internalFormatInfo = gl::GetInternalFormatInfo(currentInternalFormat);
-    bool validFormatTypeCombination =
-        ValidReadPixelsFormatType(context, internalFormatInfo.componentType, format, type);
-
-    if (!(currentFormat == format && currentType == type) && !validFormatTypeCombination)
-    {
-        context->handleError(Error(GL_INVALID_OPERATION));
-        return false;
-    }
-
-    // Check for pixel pack buffer related API errors
-    gl::Buffer *pixelPackBuffer = context->getGLState().getTargetBuffer(GL_PIXEL_PACK_BUFFER);
-    if (pixelPackBuffer != nullptr)
-    {
-        // ..  the data would be packed to the buffer object such that the memory writes required
-        // would exceed the data store size.
-        GLenum sizedInternalFormat       = GetSizedInternalFormat(format, type);
-        const InternalFormat &formatInfo = GetInternalFormatInfo(sizedInternalFormat);
-        const gl::Extents size(width, height, 1);
-        const auto &pack = context->getGLState().getPackState();
+    return ValidateReadPixelsBase(context, x, y, width, height, format, type, -1, nullptr, pixels);
+}
 
-        auto endByteOrErr = formatInfo.computePackUnpackEndByte(type, size, pack, false);
-        if (endByteOrErr.isError())
+bool ValidateReadPixelsRobustANGLE(ValidationContext *context,
+                                   GLint x,
+                                   GLint y,
+                                   GLsizei width,
+                                   GLsizei height,
+                                   GLenum format,
+                                   GLenum type,
+                                   GLsizei bufSize,
+                                   GLsizei *length,
+                                   GLvoid *pixels)
+{
+    if (!ValidateRobustEntryPoint(context, bufSize))
     {
-            context->handleError(endByteOrErr.getError());
         return false;
     }
 
-        CheckedNumeric<size_t> checkedEndByte(endByteOrErr.getResult());
-        CheckedNumeric<size_t> checkedOffset(reinterpret_cast<size_t>(pixels));
-        checkedEndByte += checkedOffset;
-
-        if (checkedEndByte.ValueOrDie() > static_cast<size_t>(pixelPackBuffer->getSize()))
+    if (!ValidateReadPixelsBase(context, x, y, width, height, format, type, bufSize, length,
+                                pixels))
     {
-            // Overflow past the end of the buffer
-            context->handleError(
-                Error(GL_INVALID_OPERATION, "Writes would overflow the pixel pack buffer."));
         return false;
     }
 
-        // ...the buffer object's data store is currently mapped.
-        if (pixelPackBuffer->isMapped())
+    if (!ValidateRobustBufferSize(context, bufSize, *length))
     {
-            context->handleError(Error(GL_INVALID_OPERATION, "Pixel pack buffer is mapped."));
         return false;
     }
-    }
 
     return true;
 }
@@ -1309,25 +1382,37 @@ bool ValidateReadnPixelsEXT(Context *context,
         return false;
     }
 
-    GLenum sizedInternalFormat = GetSizedInternalFormat(format, type);
-    const InternalFormat &formatInfo = GetInternalFormatInfo(sizedInternalFormat);
-    const gl::Extents size(width, height, 1);
-    const auto &pack = context->getGLState().getPackState();
+    return ValidateReadPixelsBase(context, x, y, width, height, format, type, bufSize, nullptr,
+                                  pixels);
+}
 
-    auto endByteOrErr = formatInfo.computePackUnpackEndByte(type, size, pack, false);
-    if (endByteOrErr.isError())
+bool ValidateReadnPixelsRobustANGLE(ValidationContext *context,
+                                    GLint x,
+                                    GLint y,
+                                    GLsizei width,
+                                    GLsizei height,
+                                    GLenum format,
+                                    GLenum type,
+                                    GLsizei bufSize,
+                                    GLsizei *length,
+                                    GLvoid *data)
+{
+    if (!ValidateRobustEntryPoint(context, bufSize))
     {
-        context->handleError(endByteOrErr.getError());
         return false;
     }
 
-    if (endByteOrErr.getResult() > static_cast<GLuint>(bufSize))
+    if (!ValidateReadPixelsBase(context, x, y, width, height, format, type, bufSize, length, data))
     {
-        context->handleError(Error(GL_INVALID_OPERATION, "Writes would overflow past bufSize."));
         return false;
     }
 
-    return ValidateReadPixels(context, x, y, width, height, format, type, pixels);
+    if (!ValidateRobustBufferSize(context, bufSize, *length))
+    {
+        return false;
+    }
+
+    return true;
 }
 
 bool ValidateGenQueriesEXT(gl::Context *context, GLsizei n)

src/libANGLE/validationES.h

@@ -107,6 +107,16 @@ bool ValidateReadPixels(ValidationContext *context,
                         GLenum format,
                         GLenum type,
                         GLvoid *pixels);
+bool ValidateReadPixelsRobustANGLE(ValidationContext *context,
+                                   GLint x,
+                                   GLint y,
+                                   GLsizei width,
+                                   GLsizei height,
+                                   GLenum format,
+                                   GLenum type,
+                                   GLsizei bufSize,
+                                   GLsizei *length,
+                                   GLvoid *pixels);
 bool ValidateReadnPixelsEXT(Context *context,
                             GLint x,
                             GLint y,
@@ -116,6 +126,16 @@ bool ValidateReadnPixelsEXT(Context *context,
                             GLenum type,
                             GLsizei bufSize,
                             GLvoid *pixels);
+bool ValidateReadnPixelsRobustANGLE(ValidationContext *context,
+                                    GLint x,
+                                    GLint y,
+                                    GLsizei width,
+                                    GLsizei height,
+                                    GLenum format,
+                                    GLenum type,
+                                    GLsizei bufSize,
+                                    GLsizei *length,
+                                    GLvoid *data);
 
 bool ValidateGenQueriesEXT(gl::Context *context, GLsizei n);
 bool ValidateDeleteQueriesEXT(gl::Context *context, GLsizei n);

src/libGLESv2/entry_points_gles_2_0_ext.cpp

@@ -2291,7 +2291,21 @@ ANGLE_EXPORT void GL_APIENTRY ReadPixelsRobustANGLE(GLint x,
         "GLenum format = 0x%X, GLenum type = 0x%X, GLsizei bufsize = %d, GLsizei* length = "
         "0x%0.8p, GLvoid* pixels = 0x%0.8p)",
         x, y, width, height, format, type, bufSize, length, pixels);
-    UNIMPLEMENTED();
+
+    Context *context = GetValidGlobalContext();
+    if (context)
+    {
+        GLsizei writeLength = 0;
+        if (!ValidateReadPixelsRobustANGLE(context, x, y, width, height, format, type, bufSize,
+                                           &writeLength, pixels))
+        {
+            return;
+        }
+
+        context->readPixels(x, y, width, height, format, type, pixels);
+
+        SetRobustLengthParam(length, writeLength);
+    }
 }
 
 ANGLE_EXPORT void GL_APIENTRY TexImage2DRobustANGLE(GLenum target,
@@ -2695,7 +2709,21 @@ ANGLE_EXPORT void GL_APIENTRY ReadnPixelsRobustANGLE(GLint x,
         "GLenum format = 0x%X, GLenum type = 0x%X, GLsizei bufsize = %d, GLsizei* length = "
         "0x%0.8p, GLvoid *data = 0x%0.8p)",
         x, y, width, height, format, type, bufSize, length, data);
-    UNIMPLEMENTED();
+
+    Context *context = GetValidGlobalContext();
+    if (context)
+    {
+        GLsizei writeLength = 0;
+        if (!ValidateReadnPixelsRobustANGLE(context, x, y, width, height, format, type, bufSize,
+                                            &writeLength, data))
+        {
+            return;
+        }
+
+        context->readPixels(x, y, width, height, format, type, data);
+
+        SetRobustLengthParam(length, writeLength);
+    }
 }
 
 ANGLE_EXPORT void GL_APIENTRY GetnUniformfvRobustANGLE(GLuint program,

src/tests/gl_tests/RobustClientMemoryTest.cpp

@@ -334,6 +334,39 @@ TEST_P(RobustClientMemoryTest, TexImage2D)
     }
 }
 
+// Test basic usage and validation of glReadPixelsRobustANGLE
+TEST_P(RobustClientMemoryTest, ReadPixels)
+{
+    if (!extensionsPresent())
+    {
+        return;
+    }
+
+    GLsizei dataDimension = 16;
+    std::vector<GLubyte> rgbaData(dataDimension * dataDimension * 4);
+
+    // Test the regular case
+    GLsizei length = 0;
+    glReadPixelsRobustANGLE(0, 0, dataDimension, dataDimension, GL_RGBA, GL_UNSIGNED_BYTE,
+                            static_cast<GLsizei>(rgbaData.size()), &length, rgbaData.data());
+    EXPECT_GL_NO_ERROR();
+    EXPECT_EQ(static_cast<GLsizei>(rgbaData.size()), length);
+
+    // Test with a data size that is too small
+    glReadPixelsRobustANGLE(0, 0, dataDimension, dataDimension, GL_RGBA, GL_UNSIGNED_BYTE,
+                            static_cast<GLsizei>(rgbaData.size()) - 1, &length, rgbaData.data());
+    EXPECT_GL_ERROR(GL_INVALID_OPERATION);
+
+    if (getClientMajorVersion() >= 3)
+    {
+        // Set a pack parameter that would cause the driver to write past the end of the buffer
+        glPixelStorei(GL_PACK_ROW_LENGTH, dataDimension + 1);
+        glReadPixelsRobustANGLE(0, 0, dataDimension, dataDimension, GL_RGBA, GL_UNSIGNED_BYTE,
+                                static_cast<GLsizei>(rgbaData.size()), &length, rgbaData.data());
+        EXPECT_GL_ERROR(GL_INVALID_OPERATION);
+    }
+}
+
 // Use this to select which configurations (e.g. which renderer, which GLES major version) these
 // tests should be run against.
 ANGLE_INSTANTIATE_TEST(RobustClientMemoryTest,