On 05/27/2015 02:49 AM, Kevin Rogovin wrote: > Implement GL_ARB_framebuffer_no_attachments in Mesa core > - changes to conditions for framebuffer completenss > - implement set/get functions for framebuffers for > new functions in GL_ARB_framebuffer_no_attachments > > v2: > Spacing and exceed 80 characters per line fixes. > > v3: > Implement DSA functions of extension. > > v4: > Formatting fixes. > Add early return to functions when extension(s) not present. > > Signed-off-by: Kevin Rogovin <kevin.rogo...@intel.com> > --- > src/mesa/main/fbobject.c | 220 > ++++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 187 insertions(+), 33 deletions(-) > > diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c > index 4ac3f20..f9858ad 100644 > --- a/src/mesa/main/fbobject.c > +++ b/src/mesa/main/fbobject.c > @@ -1156,14 +1156,48 @@ _mesa_test_framebuffer_completeness(struct gl_context > *ctx, > } else if (att_layer_count > max_layer_count) { > max_layer_count = att_layer_count; > } > + > + /** > + * The extension GL_ARB_framebuffer_no_attachments places additional
This isn't a comment that Doxygen will pick up, so just start with /* on the same line as the initial text. /* The extension GL_ARB_framebuffer_no_attachments places additional > + * requirement on each attachment. Those additional requirements are > + * tighter that those of previous versions of GL. In interest of better > + * compatibility, we will not enforce these restrictions. For the > record > + * those additional restrictions are quoted below: > + * > + * "The width and height of image are greater than zero and less than > or > + * equal to the values of the implementation-dependent limits > + * MAX_FRAMEBUFFER_WIDTH and MAX_FRAMEBUFFER_HEIGHT, respectively." > + * > + * "If <image> is a three-dimensional texture or a one- or > two-dimensional > + * array texture and the attachment is layered, the depth or layer > count > + * of the texture is less than or equal to the > implementation-dependent > + * limit MAX_FRAMEBUFFER_LAYERS." > + * > + * "If image has multiple samples, its sample count is less than or > equal > + * to the value of the implementation-dependent limit > + * MAX_FRAMEBUFFER_SAMPLES." > + * > + * The same requirements are also in place for GL 4.5, > + * Section 9.4.1 "Framebuffer Attachment Completeness", pg 310-311 > + */ > } > > fb->MaxNumLayers = max_layer_count; > > if (numImages == 0) { > - fb->_Status = GL_FRAMEBUFFER_INCOMPLETE_MISSING_ATTACHMENT_EXT; > - fbo_incomplete(ctx, "no attachments", -1); > - return; > + fb->_HasAttachments = GL_FALSE; Assuming patch 1 is changed, s/GL_FALSE/false/ here. > + > + if (!ctx->Extensions.ARB_framebuffer_no_attachments) { > + fb->_Status = GL_FRAMEBUFFER_INCOMPLETE_MISSING_ATTACHMENT_EXT; > + fbo_incomplete(ctx, "no attachments", -1); > + return; > + } > + > + if (fb->DefaultGeometry.Width == 0 || fb->DefaultGeometry.Height == 0) > { > + fb->_Status = GL_FRAMEBUFFER_INCOMPLETE_MISSING_ATTACHMENT_EXT; > + fbo_incomplete(ctx, "no attachments and default width or height is > 0", -1); > + return; > + } > } > > if (_mesa_is_desktop_gl(ctx) && !ctx->Extensions.ARB_ES2_compatibility) { > @@ -1228,8 +1262,10 @@ _mesa_test_framebuffer_completeness(struct gl_context > *ctx, > * renderbuffers/textures are different sizes, the framebuffer > * width/height will be set to the smallest width/height. > */ > - fb->Width = minWidth; > - fb->Height = minHeight; > + if (numImages != 0) { > + fb->Width = minWidth; > + fb->Height = minHeight; > + } > > /* finally, update the visual info for the framebuffer */ > _mesa_update_framebuffer_visual(ctx, fb); > @@ -1335,32 +1371,129 @@ _mesa_BindRenderbufferEXT(GLenum target, GLuint > renderbuffer) > bind_renderbuffer(target, renderbuffer, true); > } > > -extern void GLAPIENTRY> +static void > +framebuffer_parameteri(struct gl_context *ctx, struct gl_framebuffer *fb, > + GLenum pname, GLint param, const char *func) > +{ > + switch (pname) { > + case GL_FRAMEBUFFER_DEFAULT_WIDTH: > + if (param < 0 || param > ctx->Const.MaxFramebufferWidth) > + _mesa_error(ctx, GL_INVALID_VALUE, "%s", func); > + else > + fb->DefaultGeometry.Width = param; > + break; The common idiom in Mesa when you have the same error message over and over again is: if (some error condition) goto invalid_value; fb->DefaultGeometry.Width = param; break; It's probably not worth re-spinning this patch for that, but keep it in mind for future reference. > + case GL_FRAMEBUFFER_DEFAULT_HEIGHT: > + if (param < 0 || param > ctx->Const.MaxFramebufferHeight) > + _mesa_error(ctx, GL_INVALID_VALUE, "%s", func); > + else > + fb->DefaultGeometry.Height = param; > + break; > + case GL_FRAMEBUFFER_DEFAULT_LAYERS: > + if (param < 0 || param > ctx->Const.MaxFramebufferLayers) > + _mesa_error(ctx, GL_INVALID_VALUE, "%s", func); > + else > + fb->DefaultGeometry.Layers = param; > + break; > + case GL_FRAMEBUFFER_DEFAULT_SAMPLES: > + if (param < 0 || param > ctx->Const.MaxFramebufferSamples) > + _mesa_error(ctx, GL_INVALID_VALUE, "%s", func); > + else > + fb->DefaultGeometry.NumSamples = param; > + break; > + case GL_FRAMEBUFFER_DEFAULT_FIXED_SAMPLE_LOCATIONS: > + fb->DefaultGeometry.FixedSampleLocations = param; > + break; > + default: > + _mesa_error(ctx, GL_INVALID_ENUM, > + "%s(pname=0x%x)", func, pname); > + } > +} > + > +void GLAPIENTRY > _mesa_FramebufferParameteri(GLenum target, GLenum pname, GLint param) > { > GET_CURRENT_CONTEXT(ctx); > + struct gl_framebuffer *fb; > > - (void) target; > - (void) pname; > - (void) param; > + if (!ctx->Extensions.ARB_framebuffer_no_attachments) { > + _mesa_error(ctx, GL_INVALID_OPERATION, > + "glFramebufferParameteriv not supported " > + "(ARB_framebuffer_no_attachments not implemented)"); > + return; > + } > > - _mesa_error(ctx, GL_INVALID_OPERATION, > - "glFramebufferParameteri not supported " > - "(ARB_framebuffer_no_attachments not implemented)"); > + fb = get_framebuffer_target(ctx, target); > + if (!fb) { > + _mesa_error(ctx, GL_INVALID_ENUM, > + "glFramebufferParameteri(target=0x%x)", target); > + return; > + } > + > + /* check framebuffer binding */ > + if (_mesa_is_winsys_fbo(fb)) { > + _mesa_error(ctx, GL_INVALID_OPERATION, > + "glFramebufferParameteri"); > + return; > + } > + > + framebuffer_parameteri(ctx, fb, pname, param, "glFramebufferParameteri"); > +} > + > +static void > +get_framebuffer_parameteriv(struct gl_context *ctx, struct gl_framebuffer > *fb, > + GLenum pname, GLint *params, const char *func) > +{ > + switch (pname) { > + case GL_FRAMEBUFFER_DEFAULT_WIDTH: > + *params = fb->DefaultGeometry.Width; > + break; > + case GL_FRAMEBUFFER_DEFAULT_HEIGHT: > + *params = fb->DefaultGeometry.Height; > + break; > + case GL_FRAMEBUFFER_DEFAULT_LAYERS: > + *params = fb->DefaultGeometry.Layers; > + break; > + case GL_FRAMEBUFFER_DEFAULT_SAMPLES: > + *params = fb->DefaultGeometry.NumSamples; > + break; > + case GL_FRAMEBUFFER_DEFAULT_FIXED_SAMPLE_LOCATIONS: > + *params = fb->DefaultGeometry.FixedSampleLocations; > + break; > + default: > + _mesa_error(ctx, GL_INVALID_ENUM, > + "%s(pname=0x%x)", func, pname); > + } > } > > -extern void GLAPIENTRY > +void GLAPIENTRY > _mesa_GetFramebufferParameteriv(GLenum target, GLenum pname, GLint *params) > { > GET_CURRENT_CONTEXT(ctx); > + struct gl_framebuffer *fb; > + > + if (!ctx->Extensions.ARB_framebuffer_no_attachments) { > + _mesa_error(ctx, GL_INVALID_OPERATION, > + "glGetFramebufferParameteriv not supported " > + "(ARB_framebuffer_no_attachments not implemented)"); > + return; > + } > > - (void) target; > - (void) pname; > - (void) param; > + fb = get_framebuffer_target(ctx, target); > + if (!fb) { > + _mesa_error(ctx, GL_INVALID_ENUM, > + "glGetFramebufferParameteriv(target=0x%x)", target); > + return; > + } > > - _mesa_error(ctx, GL_INVALID_OPERATION, > - "glGetNamedFramebufferParameteriv not supported " > - "(ARB_framebuffer_no_attachments not implemented)"); > + /* check framebuffer binding */ > + if (_mesa_is_winsys_fbo(fb)) { > + _mesa_error(ctx, GL_INVALID_OPERATION, > + "glGetFramebufferParameteriv"); > + return; > + } > + > + get_framebuffer_parameteriv(ctx, fb, pname, params, > + "glGetFramebufferParameteriv"); > } > > > @@ -3757,10 +3890,7 @@ _mesa_NamedFramebufferParameteri(GLuint framebuffer, > GLenum pname, > GLint param) > { > GET_CURRENT_CONTEXT(ctx); > - > - (void) framebuffer; > - (void) pname; > - (void) param; > + struct gl_framebuffer *fb = NULL; > > if (!ctx->Extensions.ARB_direct_state_access) { > _mesa_error(ctx, GL_INVALID_OPERATION, > @@ -3769,9 +3899,20 @@ _mesa_NamedFramebufferParameteri(GLuint framebuffer, > GLenum pname, > return; > } > > - _mesa_error(ctx, GL_INVALID_OPERATION, > - "glNamedFramebufferParameteri not supported " > - "(ARB_framebuffer_no_attachments not implemented)"); > + if (!ctx->Extensions.ARB_framebuffer_no_attachments) { This extension check should just get squashed with the DSA extension check. We don't provide fine-grained extension-not-supported error messages anywhere else. Same comment applies to the other DSA functions below. > + _mesa_error(ctx, GL_INVALID_OPERATION, > + "glNamedFramebufferParameteri not supported " > + "(ARB_framebuffer_no_attachments not implemented)"); > + return; > + } > + > + fb = _mesa_lookup_framebuffer_err(ctx, framebuffer, > + "glNamedFramebufferParameteri"); > + > + if (fb) { > + framebuffer_parameteri(ctx, fb, pname, param, > + "glNamedFramebufferParameteriv"); > + } > } > > > @@ -3780,10 +3921,7 @@ _mesa_GetNamedFramebufferParameteriv(GLuint > framebuffer, GLenum pname, > GLint *param) > { > GET_CURRENT_CONTEXT(ctx); > - > - (void) framebuffer; > - (void) pname; > - (void) param; > + struct gl_framebuffer *fb; > > if (!ctx->Extensions.ARB_direct_state_access) { > _mesa_error(ctx, GL_INVALID_OPERATION, > @@ -3792,9 +3930,25 @@ _mesa_GetNamedFramebufferParameteriv(GLuint > framebuffer, GLenum pname, > return; > } > > - _mesa_error(ctx, GL_INVALID_OPERATION, > - "glGetNamedFramebufferParameteriv not supported " > - "(ARB_framebuffer_no_attachments not implemented)"); > + if (!ctx->Extensions.ARB_framebuffer_no_attachments) { > + _mesa_error(ctx, GL_INVALID_OPERATION, > + "glGetNamedFramebufferParameteriv not supported " > + "(ARB_framebuffer_no_attachments not implemented)"); > + return; > + } > + > + if (framebuffer) { > + fb = _mesa_lookup_framebuffer_err(ctx, framebuffer, > + "glGetNamedFramebufferParameteriv"); > + } > + else { Either } else { or use ?:. With all of the above issues fixed, this patch is Reviewed-by: Ian Romanick <ian.d.roman...@intel.com> > + fb = ctx->WinSysDrawBuffer; > + } > + > + if (fb) { > + get_framebuffer_parameteriv(ctx, fb, pname, param, > + "glGetNamedFramebufferParameteriv"); > + } > } > > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev