On 20/03/15 19:23, Laura Ekstrand wrote:
On Thu, Mar 19, 2015 at 11:13 AM, Martin Peres
<martin.pe...@linux.intel.com <mailto:martin.pe...@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.pe...@linux.intel.com
<mailto:martin.pe...@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@lists.freedesktop.org <mailto:mesa-dev@lists.freedesktop.org>
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
