[Mesa-dev] [PATCH 16/17] main: Added entry points for NamedRenderbufferStorage/Multisample
Martin Peres
martin.peres at linux.intel.com
Mon Mar 23 04:59:41 PDT 2015
On 20/03/15 19:23, Laura Ekstrand wrote:
>
>
> On Thu, Mar 19, 2015 at 11:13 AM, Martin Peres
> <martin.peres at linux.intel.com <mailto: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
> <mailto: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.)
Indeed, that allows to have a better error message. Done.
> 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?
Yep, that's what happens when you edit the wrong line in rebase -i ...
> @@ -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.
Got rid of it, it was my IDE who replaced the tabs by spaces.
> + 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.
Fallout of the same problem as before, I edited the wrong line in rebase
-i. I'll pay more attention to that from now on.
I will resend patches 15 and 16 as soon as jenkins tells me I did not
introduce any regression.
> + 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 <mailto:mesa-dev at lists.freedesktop.org>
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
More information about the mesa-dev
mailing list