<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Thu, Mar 19, 2015 at 11:13 AM, Martin Peres <<a href="mailto:martin.peres@linux.intel.com" target="_blank">martin.peres@linux.intel.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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 <<a href="mailto:martin.peres@linux.intel.com">martin.peres@linux.intel.com</a>> --- 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) <div><div class="h5"> /** - * 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) { </div></div>@@ -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; </blockquote><div>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.) </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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*(). <div><div class="h5">+ * 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) </div></div>@@ -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"); } </blockquote><div>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? </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> @@ -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); </blockquote><div>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. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + 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); </blockquote><div>See above comment. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + 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 <div class="HOEnZb"><div class="h5"> _______________________________________________ mesa-dev mailing list <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </div></div></blockquote></div> </div></div>