On Thu, Feb 2, 2017 at 10:51 AM, Alejandro Piñeiro <apinhe...@igalia.com> wrote:
> 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.
> ---
>
> I also updated the spec quotes, as now there is a quote that justifies
> INVALID_OPERATION when readbuffer is not available with GetIntegerv
>
> As mentioned on the patch, there is no equivalent for
> GetFramebufferParameteriv
> or GetNamedFramebufferParameteriv, but there is another quote that links
> the value of those with GetIntegerv, so I think that makes sense that in
> the situations that GetIntegerv raises INVALID_OPERATION the other two too.
>
> If the patch is accepted, I would open a spec bug.
>
>
> src/mesa/main/framebuffer.c | 77
> +++++++++++++++++++++++++++++++++++----------
> src/mesa/main/framebuffer.h | 8 +++--
> src/mesa/main/get.c | 4 +--
> src/mesa/main/readpix.c | 4 +--
> 4 files changed, 71 insertions(+), 22 deletions(-)
>
> diff --git a/src/mesa/main/framebuffer.c b/src/mesa/main/framebuffer.c
> index c06130d..9002020 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 745c1da..ee0690b 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 f0bb041..397f4a3 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 1cb06c7..2582323 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);
> --
> 2.9.3
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Series-is:
Reviewed-by: Anuj Phogat <anuj.pho...@gmail.com>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
