[Mesa-dev] [PATCH 16/17] main: Added entry points for NamedRenderbufferStorage/Multisample

Laura Ekstrand laura at jlekstrand.net
Fri Mar 20 10:23:29 PDT 2015


On Thu, Mar 19, 2015 at 11:13 AM, Martin Peres <martin.peres at linux.intel.com
> wrote:

> v2: Review from Laura Ekstrand
> - get rid of a change that should not have happened in this patch
> - improve the error messages
> - fix alignments
> - fix a capitalization in a function name in an error message
>
> Signed-off-by: Martin Peres <martin.peres at linux.intel.com>
> ---
>  src/mapi/glapi/gen/ARB_direct_state_access.xml |  15 +++
>  src/mesa/main/fbobject.c                       | 161
> ++++++++++++++++++-------
>  src/mesa/main/fbobject.h                       |  13 +-
>  src/mesa/main/tests/dispatch_sanity.cpp        |   2 +
>  4 files changed, 148 insertions(+), 43 deletions(-)
>
> diff --git a/src/mapi/glapi/gen/ARB_direct_state_access.xml
> b/src/mapi/glapi/gen/ARB_direct_state_access.xml
> index d4e1f7c..8a092d6 100644
> --- a/src/mapi/glapi/gen/ARB_direct_state_access.xml
> +++ b/src/mapi/glapi/gen/ARB_direct_state_access.xml
> @@ -159,6 +159,21 @@
>        <param name="renderbuffers" type="GLuint *" />
>     </function>
>
> +   <function name="NamedRenderbufferStorage" offset="assign">
> +      <param name="renderbuffer" type="GLuint" />
> +      <param name="internalformat" type="GLenum" />
> +      <param name="width" type="GLsizei" />
> +      <param name="height" type="GLsizei" />
> +   </function>
> +
> +   <function name="NamedRenderbufferStorageMultisample" offset="assign">
> +      <param name="renderbuffer" type="GLuint" />
> +      <param name="samples" type="GLsizei" />
> +      <param name="internalformat" type="GLenum" />
> +      <param name="width" type="GLsizei" />
> +      <param name="height" type="GLsizei" />
> +   </function>
> +
>     <function name="GetNamedRenderbufferParameteriv" offset="assign">
>        <param name="renderbuffer" type="GLuint" />
>        <param name="pname" type="GLenum" />
> diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c
> index ae0dd76..fc76c4a 100644
> --- a/src/mesa/main/fbobject.c
> +++ b/src/mesa/main/fbobject.c
> @@ -1785,40 +1785,17 @@ invalidate_rb(GLuint key, void *data, void
> *userData)
>
>
>  /**
> - * Helper function used by _mesa_RenderbufferStorage() and
> - * _mesa_RenderbufferStorageMultisample().
> - * samples will be NO_SAMPLES if called by _mesa_RenderbufferStorage().
> + * Helper function used by renderbuffer_storage_direct() and
> + * renderbuffer_storage_target().
> + * samples will be NO_SAMPLES if called by a non-multisample function.
>   */
>  static void
> -renderbuffer_storage(GLenum target, GLenum internalFormat,
> -                     GLsizei width, GLsizei height, GLsizei samples)
> +renderbuffer_storage(struct gl_context *ctx, struct gl_renderbuffer *rb,
> +                     GLenum internalFormat, GLsizei width,
> +                     GLsizei height, GLsizei samples, const char *func)
>  {
> -   const char *func = samples == NO_SAMPLES ?
> -      "glRenderbufferStorage" : "glRenderbufferStorageMultisample";
> -   struct gl_renderbuffer *rb;
>     GLenum baseFormat;
>     GLenum sample_count_error;
> -   GET_CURRENT_CONTEXT(ctx);
> -
> -   if (MESA_VERBOSE & VERBOSE_API) {
> -      if (samples == NO_SAMPLES)
> -         _mesa_debug(ctx, "%s(%s, %s, %d, %d)\n",
> -                     func,
> -                     _mesa_lookup_enum_by_nr(target),
> -                     _mesa_lookup_enum_by_nr(internalFormat),
> -                     width, height);
> -      else
> -         _mesa_debug(ctx, "%s(%s, %s, %d, %d, %d)\n",
> -                     func,
> -                     _mesa_lookup_enum_by_nr(target),
> -                     _mesa_lookup_enum_by_nr(internalFormat),
> -                     width, height, samples);
> -   }
> -
> -   if (target != GL_RENDERBUFFER_EXT) {
> -      _mesa_error(ctx, GL_INVALID_ENUM, "%s(target)", func);
> -      return;
> -   }
>
>     baseFormat = _mesa_base_fbo_format(ctx, internalFormat);
>     if (baseFormat == 0) {
> @@ -1828,12 +1805,14 @@ renderbuffer_storage(GLenum target, GLenum
> internalFormat,
>     }
>
>     if (width < 0 || width > (GLsizei) ctx->Const.MaxRenderbufferSize) {
> -      _mesa_error(ctx, GL_INVALID_VALUE, "%s(width)", func);
> +      _mesa_error(ctx, GL_INVALID_VALUE, "%s(invalid width %d)", func,
> +                  width);
>        return;
>     }
>
>     if (height < 0 || height > (GLsizei) ctx->Const.MaxRenderbufferSize) {
> -      _mesa_error(ctx, GL_INVALID_VALUE, "%s(height)", func);
> +      _mesa_error(ctx, GL_INVALID_VALUE, "%s(invalid height %d)", func,
> +                  height);
>        return;
>     }
>
> @@ -1845,7 +1824,7 @@ renderbuffer_storage(GLenum target, GLenum
> internalFormat,
>        /* check the sample count;
>         * note: driver may choose to use more samples than what's requested
>         */
> -      sample_count_error = _mesa_check_sample_count(ctx, target,
> +      sample_count_error = _mesa_check_sample_count(ctx, GL_RENDERBUFFER,
>              internalFormat, samples);
>        if (sample_count_error != GL_NO_ERROR) {
>           _mesa_error(ctx, sample_count_error, "%s(samples)", func);
> @@ -1853,7 +1832,6 @@ renderbuffer_storage(GLenum target, GLenum
> internalFormat,
>        }
>     }
>
> -   rb = ctx->CurrentRenderbuffer;
>
I would move this check up into renderbuffer_storage_target and
renderbuffer_storage_named since you can customize it to those functions.
Also, I think it makes more sense to check this right away before passing
it into another function.  (Unless order of checking matters a lot for this
entry point.)

>     if (!rb) {
>        _mesa_error(ctx, GL_INVALID_OPERATION, "%s", func);
>        return;
> @@ -1900,6 +1878,75 @@ renderbuffer_storage(GLenum target, GLenum
> internalFormat,
>     }
>  }
>
> +/**
> + * Helper function used by _mesa_NamedRenderbufferStorage*().
> + * samples will be NO_SAMPLES if called by a non-multisample function.
> + */
> +static void
> +renderbuffer_storage_named(GLuint renderbuffer, GLenum internalFormat,
> +                           GLsizei width, GLsizei height, GLsizei samples,
> +                           const char *func)
> +{
> +   GET_CURRENT_CONTEXT(ctx);
> +
> +   if (MESA_VERBOSE & VERBOSE_API) {
> +      if (samples == NO_SAMPLES)
> +         _mesa_debug(ctx, "%s(%u, %s, %d, %d)\n",
> +                     func, renderbuffer,
> +                     _mesa_lookup_enum_by_nr(internalFormat),
> +                     width, height);
> +      else
> +         _mesa_debug(ctx, "%s(%u, %s, %d, %d, %d)\n",
> +                     func, renderbuffer,
> +                     _mesa_lookup_enum_by_nr(internalFormat),
> +                     width, height, samples);
> +   }
> +
> +   struct gl_renderbuffer *rb = _mesa_lookup_renderbuffer(ctx,
> renderbuffer);
> +   if (rb == &DummyRenderbuffer) {
> +      /* ID was reserved, but no real renderbuffer object made yet */
> +      rb = NULL;
> +   }
> +
> +   renderbuffer_storage(ctx, rb, internalFormat, width, height, samples,
> func);
> +}
> +
> +/**
> + * Helper function used by _mesa_RenderbufferStorage() and
> + * _mesa_RenderbufferStorageMultisample().
> + * samples will be NO_SAMPLES if called by _mesa_RenderbufferStorage().
> + */
> +static void
> +renderbuffer_storage_target(GLenum target, GLenum internalFormat,
> +                            GLsizei width, GLsizei height, GLsizei
> samples,
> +                            const char *func)
> +{
> +   GET_CURRENT_CONTEXT(ctx);
> +
> +   if (MESA_VERBOSE & VERBOSE_API) {
> +      if (samples == NO_SAMPLES)
> +         _mesa_debug(ctx, "%s(%s, %s, %d, %d)\n",
> +                     func,
> +                     _mesa_lookup_enum_by_nr(target),
> +                     _mesa_lookup_enum_by_nr(internalFormat),
> +                     width, height);
> +      else
> +         _mesa_debug(ctx, "%s(%s, %s, %d, %d, %d)\n",
> +                     func,
> +                     _mesa_lookup_enum_by_nr(target),
> +                     _mesa_lookup_enum_by_nr(internalFormat),
> +                     width, height, samples);
> +   }
> +
> +   if (target != GL_RENDERBUFFER_EXT) {
> +      _mesa_error(ctx, GL_INVALID_ENUM, "%s(target)", func);
> +      return;
> +   }
> +
> +   renderbuffer_storage(ctx, ctx->CurrentRenderbuffer, internalFormat,
> width,
> +                        height, samples, func);
> +}
> +
>
>  void GLAPIENTRY
>  _mesa_EGLImageTargetRenderbufferStorageOES(GLenum target, GLeglImageOES
> image)
> @@ -1959,7 +2006,8 @@ _mesa_RenderbufferStorage(GLenum target, GLenum
> internalFormat,
>      * glRenderbufferStorageMultisample() with samples=0.  We pass in
>      * a token value here just for error reporting purposes.
>      */
> -   renderbuffer_storage(target, internalFormat, width, height,
> NO_SAMPLES);
> +   renderbuffer_storage_target(target, internalFormat, width, height,
> +                               NO_SAMPLES, "glRenderbufferStorage");
>  }
>
>
> @@ -1968,10 +2016,10 @@ _mesa_RenderbufferStorageMultisample(GLenum
> target, GLsizei samples,
>                                       GLenum internalFormat,
>                                       GLsizei width, GLsizei height)
>  {
> -   renderbuffer_storage(target, internalFormat, width, height, samples);
> +   renderbuffer_storage_target(target, internalFormat, width, height,
> +                               samples,
> "glRenderbufferStorageMultisample");
>  }
>
> -
>  /**
>   * OpenGL ES version of glRenderBufferStorage.
>   */
> @@ -1989,7 +2037,30 @@ _es_RenderbufferStorageEXT(GLenum target, GLenum
> internalFormat,
>        break;
>     }
>
> -   renderbuffer_storage(target, internalFormat, width, height, 0);
> +   renderbuffer_storage_target(target, internalFormat, width, height, 0,
> +                               "glRenderbufferStorageEXT");
> +}
> +
> +void GLAPIENTRY
> +_mesa_NamedRenderbufferStorage(GLuint renderbuffer, GLenum internalformat,
> +                               GLsizei width, GLsizei height)
> +{
> +   /* GL_ARB_fbo says calling this function is equivalent to calling
> +    * glRenderbufferStorageMultisample() with samples=0.  We pass in
> +    * a token value here just for error reporting purposes.
> +    */
> +   renderbuffer_storage_named(renderbuffer, internalformat, width, height,
> +                              NO_SAMPLES, "glNamedRenderbufferStorage");
> +}
> +
> +void GLAPIENTRY
> +_mesa_NamedRenderbufferStorageMultisample(GLuint renderbuffer, GLsizei
> samples,
> +                                          GLenum internalformat,
> +                                          GLsizei width, GLsizei height)
> +{
> +   renderbuffer_storage_named(renderbuffer, internalformat, width, height,
> +                              samples,
> +                              "glNamedRenderbufferStorageMultisample");
>  }
>
>
> These changes to render buffer parameteriv don't look like they belong in
this commit.  Perhaps you meant to put them in your render buffer
parameteriv commit and they slipped in here?

> @@ -2000,10 +2071,6 @@ get_render_buffer_parameteriv(struct gl_context
> *ctx,
>  {
>     const char *func = dsa ? "glGetNamedRenderbufferParameteriv" :
>                              "glGetRenderbufferParameteriv";
> -   if (!rb) {
> -      _mesa_error(ctx, GL_INVALID_OPERATION, "%s", func);
> -      return;
> -   }
>
>     /* No need to flush here since we're just quering state which is
>      * not effected by rendering.
> @@ -2053,6 +2120,12 @@ _mesa_GetRenderbufferParameteriv(GLenum target,
> GLenum pname, GLint *params)
>        return;
>     }
>
> +   if (!ctx->CurrentRenderbuffer) {
> +      _mesa_error(ctx, GL_INVALID_OPERATION,
> "glGetRenderbufferParameteriv"
> +                  "(invalid renderbuffer)");
> +      return;
> +   }
> +
>     get_render_buffer_parameteriv(ctx, ctx->CurrentRenderbuffer, pname,
>                                   params, false);
>  }
> @@ -2070,6 +2143,12 @@ _mesa_GetNamedRenderbufferParameteriv(GLuint
> renderbuffer, GLenum pname,
>        rb = NULL;
>     }
>
> +   if (!rb) {
> +      _mesa_error(ctx, GL_INVALID_OPERATION,
> "glGetNamedRenderbufferParameteriv"
> +                  "(invalid renderbuffer name %i)", renderbuffer);
> +      return;
> +   }
> +
>     get_render_buffer_parameteriv(ctx, rb, pname, params, true);
>  }
>
> diff --git a/src/mesa/main/fbobject.h b/src/mesa/main/fbobject.h
> index b92149b..2c5273f 100644
> --- a/src/mesa/main/fbobject.h
> +++ b/src/mesa/main/fbobject.h
> @@ -128,14 +128,23 @@ _mesa_RenderbufferStorageMultisample(GLenum target,
> GLsizei samples,
>
>  extern void GLAPIENTRY
>  _es_RenderbufferStorageEXT(GLenum target, GLenum internalFormat,
> -                          GLsizei width, GLsizei height);
>
Other people frown on changing other function prototypes in new entry point
commits.  "It's not part of this commit."  I don't think it's a big deal,
though, so I'll leave it up to you whether you get rid of this or not.
It's a nice cleanup.

> +                           GLsizei width, GLsizei height);
> +
> +extern void GLAPIENTRY
> +_mesa_NamedRenderbufferStorage(GLuint renderbuffer, GLenum internalformat,
> +                               GLsizei width, GLsizei height);
> +
> +extern void GLAPIENTRY
> +_mesa_NamedRenderbufferStorageMultisample(GLuint renderbuffer, GLsizei
> samples,
> +                                          GLenum internalformat,
> +                                          GLsizei width, GLsizei height);
>
>  extern void GLAPIENTRY
>  _mesa_EGLImageTargetRenderbufferStorageOES(GLenum target, GLeglImageOES
> image);
>
>  extern void GLAPIENTRY
>  _mesa_GetRenderbufferParameteriv(GLenum target, GLenum pname,
> -                                    GLint *params);
>
See above comment.

> +                                 GLint *params);
>
>  void GLAPIENTRY
>  _mesa_GetNamedRenderbufferParameteriv(GLuint renderbuffer, GLenum pname,
> diff --git a/src/mesa/main/tests/dispatch_sanity.cpp
> b/src/mesa/main/tests/dispatch_sanity.cpp
> index bb573d4..eb83e4d 100644
> --- a/src/mesa/main/tests/dispatch_sanity.cpp
> +++ b/src/mesa/main/tests/dispatch_sanity.cpp
> @@ -944,6 +944,8 @@ const struct function gl_core_functions_possible[] = {
>     { "glGetNamedBufferPointerv", 45, -1 },
>     { "glGetNamedBufferSubData", 45, -1 },
>     { "glCreateRenderbuffers", 45, -1 },
> +   { "glNamedRenderbufferStorage", 45, -1 },
> +   { "glNamedRenderbufferStorageMultisample", 45, -1 },
>     { "glGetNamedRenderbufferParameteriv", 45, -1 },
>     { "glCreateTextures", 45, -1 },
>     { "glTextureStorage1D", 45, -1 },
> --
> 2.3.3
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150320/a88673ee/attachment-0001.html>


More information about the mesa-dev mailing list