[Mesa-dev] [v4 PATCH 03/10] mesa: Complete ARB_framebuffer_no_attachments in Mesa core
Ian Romanick
idr at freedesktop.org
Tue Jun 9 15:05:07 PDT 2015
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.rogovin at 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.romanick at intel.com>
> + fb = ctx->WinSysDrawBuffer;
> + }
> +
> + if (fb) {
> + get_framebuffer_parameteriv(ctx, fb, pname, param,
> + "glGetNamedFramebufferParameteriv");
> + }
> }
>
>
More information about the mesa-dev
mailing list