main/framebuffer: refactor _mesa_get_color_read_format/type

Current implementation returns the value for the currently bound read
framebuffer. GetNamedFramebufferParameteriv allows to get it for any
given framebuffer. GetFramebufferParameteriv would be also interested
on that method

It was refactored by allowing to pass a given framebuffer. If NULL is
passed, it used the currently bound framebuffer.

It also adds a call to _mesa_update_state. When used only by
GetIntegerv, this one was called as part of the extra checks defined
at get_hash. But now that the method is used by more methods, and the
update is needed, it makes sense (and it is safer) just calling it on
the method itself, instead of rely on the caller.

Reviewed-by: Anuj Phogat <[email protected]>

4 files changed, 71 insertions, 22 deletions

diff --git a/src/mesa/main/framebuffer.c b/src/mesa/main/framebuffer.c
index c06130dc8d7..9002020b7a7 100644
--- a/src/mesa/main/framebuffer.c
+++ b/src/mesa/main/framebuffer.c
@@ -44,6 +44,7 @@
 #include "renderbuffer.h"
 #include "texobj.h"
 #include "glformats.h"
+#include "state.h"
 
 
 
@@ -835,22 +836,54 @@ _mesa_dest_buffer_exists(struct gl_context *ctx, GLenum format)
 
 
 /**
- * Used to answer the GL_IMPLEMENTATION_COLOR_READ_FORMAT_OES query.
+ * Used to answer the GL_IMPLEMENTATION_COLOR_READ_FORMAT_OES queries (using
+ * GetIntegerv, GetFramebufferParameteriv, etc)
+ *
+ * If @fb is NULL, the method returns the value for the current bound
+ * framebuffer.
  */
 GLenum
-_mesa_get_color_read_format(struct gl_context *ctx)
+_mesa_get_color_read_format(struct gl_context *ctx,
+                            struct gl_framebuffer *fb,
+                            const char *caller)
 {
-   if (!ctx->ReadBuffer || !ctx->ReadBuffer->_ColorReadBuffer) {
-      /* The spec is unclear how to handle this case, but NVIDIA's
-       * driver generates GL_INVALID_OPERATION.
+   if (ctx->NewState)
+      _mesa_update_state(ctx);
+
+   if (fb == NULL)
+      fb = ctx->ReadBuffer;
+
+   if (!fb || !fb->_ColorReadBuffer) {
+      /*
+       * From OpenGL 4.5 spec, section 18.2.2 "ReadPixels":
+       *
+       *    "An INVALID_OPERATION error is generated by GetIntegerv if pname
+       *     is IMPLEMENTATION_COLOR_READ_FORMAT or IMPLEMENTATION_COLOR_-
+       *     READ_TYPE and any of:
+       *      * the read framebuffer is not framebuffer complete.
+       *      * the read framebuffer is a framebuffer object, and the selected
+       *        read buffer (see section 18.2.1) has no image attached.
+       *      * the selected read buffer is NONE."
+       *
+       * There is not equivalent quote for GetFramebufferParameteriv or
+       * GetNamedFramebufferParameteriv, but from section 9.2.3 "Framebuffer
+       * Object Queries":
+       *
+       *    "Values of framebuffer-dependent state are identical to those that
+       *     would be obtained were the framebuffer object bound and queried
+       *     using the simple state queries in that table."
+       *
+       * Where "using the simple state queries" refer to use GetIntegerv. So
+       * we will assume that on that situation the same error should be
+       * triggered too.
        */
       _mesa_error(ctx, GL_INVALID_OPERATION,
-                  "glGetIntegerv(GL_IMPLEMENTATION_COLOR_READ_FORMAT: "
-                  "no GL_READ_BUFFER)");
+                  "%s(GL_IMPLEMENTATION_COLOR_READ_FORMAT: no GL_READ_BUFFER)",
+                  caller);
       return GL_NONE;
    }
    else {
-      const mesa_format format = ctx->ReadBuffer->_ColorReadBuffer->Format;
+      const mesa_format format = fb->_ColorReadBuffer->Format;
       const GLenum data_type = _mesa_get_format_datatype(format);
 
       if (format == MESA_FORMAT_B8G8R8A8_UNORM)
@@ -872,22 +905,34 @@ _mesa_get_color_read_format(struct gl_context *ctx)
 
 
 /**
- * Used to answer the GL_IMPLEMENTATION_COLOR_READ_TYPE_OES query.
+ * Used to answer the GL_IMPLEMENTATION_COLOR_READ_TYPE_OES queries (using
+ * GetIntegerv, GetFramebufferParameteriv, etc)
+ *
+ * If @fb is NULL, the method returns the value for the current bound
+ * framebuffer.
  */
 GLenum
-_mesa_get_color_read_type(struct gl_context *ctx)
+_mesa_get_color_read_type(struct gl_context *ctx,
+                          struct gl_framebuffer *fb,
+                          const char *caller)
 {
-   if (!ctx->ReadBuffer || !ctx->ReadBuffer->_ColorReadBuffer) {
-      /* The spec is unclear how to handle this case, but NVIDIA's
-       * driver generates GL_INVALID_OPERATION.
+   if (ctx->NewState)
+      _mesa_update_state(ctx);
+
+   if (fb == NULL)
+      fb = ctx->ReadBuffer;
+
+   if (!fb || !fb->_ColorReadBuffer) {
+      /*
+       * See comment on _mesa_get_color_read_format
        */
       _mesa_error(ctx, GL_INVALID_OPERATION,
-                  "glGetIntegerv(GL_IMPLEMENTATION_COLOR_READ_TYPE: "
-                  "no GL_READ_BUFFER)");
+                  "%s(GL_IMPLEMENTATION_COLOR_READ_TYPE: no GL_READ_BUFFER)",
+                  caller);
       return GL_NONE;
    }
    else {
-      const GLenum format = ctx->ReadBuffer->_ColorReadBuffer->Format;
+      const GLenum format = fb->_ColorReadBuffer->Format;
       const GLenum data_type = _mesa_get_format_datatype(format);
 
       if (format == MESA_FORMAT_B5G6R5_UNORM)
diff --git a/src/mesa/main/framebuffer.h b/src/mesa/main/framebuffer.h
index 745c1dabf64..ee0690b068b 100644
--- a/src/mesa/main/framebuffer.h
+++ b/src/mesa/main/framebuffer.h
@@ -128,10 +128,14 @@ extern GLboolean
 _mesa_dest_buffer_exists(struct gl_context *ctx, GLenum format);
 
 extern GLenum
-_mesa_get_color_read_type(struct gl_context *ctx);
+_mesa_get_color_read_type(struct gl_context *ctx,
+                          struct gl_framebuffer *fb,
+                          const char *caller);
 
 extern GLenum
-_mesa_get_color_read_format(struct gl_context *ctx);
+_mesa_get_color_read_format(struct gl_context *ctx,
+                            struct gl_framebuffer *fb,
+                            const char *caller);
 
 extern struct gl_renderbuffer *
 _mesa_get_read_renderbuffer_for_format(const struct gl_context *ctx,
diff --git a/src/mesa/main/get.c b/src/mesa/main/get.c
index f0bb041fc38..397f4a3547e 100644
--- a/src/mesa/main/get.c
+++ b/src/mesa/main/get.c
@@ -787,10 +787,10 @@ find_custom_value(struct gl_context *ctx, const struct value_desc *d, union valu
       break;
 
    case GL_IMPLEMENTATION_COLOR_READ_TYPE_OES:
-      v->value_int = _mesa_get_color_read_type(ctx);
+      v->value_int = _mesa_get_color_read_type(ctx, NULL, "glGetIntegerv");
       break;
    case GL_IMPLEMENTATION_COLOR_READ_FORMAT_OES:
-      v->value_int = _mesa_get_color_read_format(ctx);
+      v->value_int = _mesa_get_color_read_format(ctx, NULL, "glGetIntegerv");
       break;
 
    case GL_CURRENT_MATRIX_STACK_DEPTH_ARB:
diff --git a/src/mesa/main/readpix.c b/src/mesa/main/readpix.c
index 1cb06c78a5a..25823230d62 100644
--- a/src/mesa/main/readpix.c
+++ b/src/mesa/main/readpix.c
@@ -1033,8 +1033,8 @@ _mesa_ReadnPixelsARB( GLint x, GLint y, GLsizei width, GLsizei height,
    if (_mesa_is_gles(ctx)) {
       if (ctx->API == API_OPENGLES2 &&
           _mesa_is_color_format(format) &&
-          _mesa_get_color_read_format(ctx) == format &&
-          _mesa_get_color_read_type(ctx) == type) {
+          _mesa_get_color_read_format(ctx, NULL, "glReadPixels") == format &&
+          _mesa_get_color_read_type(ctx, NULL, "glReadPixels") == type) {
          err = GL_NO_ERROR;
       } else if (ctx->Version < 30) {
          err = _mesa_es_error_check_format_and_type(ctx, format, type, 2);