[Mesa-dev] [PATCH 1/2] mesa: MESA_framebuffer_flip_y extension [v3]
Chad Versace
chadversary at chromium.org
Wed Jul 11 19:54:39 UTC 2018
+Ken, I had a question about GLboolean. I call you by name in the
comments below.
On Fri 29 Jun 2018, Fritz Koenig wrote:
> Adds an extension to glFramebufferParameteri
> that will specify if the framebuffer is vertically
> flipped. Historically system framebuffers are
> vertically flipped and user framebuffers are not.
> Checking to see the state was done by looking at
> the name field. This adds an explicit field.
>
> v2:
> * updated spec language [for chadv]
> * correctly specifying ES 3.1 [for chadv]
> * refactor access to rb->Name [for jason]
> * handle GetFramebufferParameteriv [for chadv]
> v3:
> * correct _mesa_GetMultisamplefv [for kusmabite]
> ---
> docs/specs/MESA_framebuffer_flip_y.spec | 84 ++++++++++++++++++++++
Use file extension '.txt'. Khronos no longer uses the '.spec' extension.
File docs/specs/enums.txt needs an update too.
> include/GLES2/gl2ext.h | 5 ++
> src/mapi/glapi/registry/gl.xml | 6 ++
> src/mesa/drivers/dri/i915/intel_fbo.c | 7 +-
> src/mesa/drivers/dri/i965/intel_fbo.c | 7 +-
> src/mesa/drivers/dri/nouveau/nouveau_fbo.c | 7 +-
> src/mesa/drivers/dri/radeon/radeon_fbo.c | 7 +-
> src/mesa/drivers/dri/radeon/radeon_span.c | 9 ++-
> src/mesa/drivers/dri/swrast/swrast.c | 7 +-
> src/mesa/drivers/osmesa/osmesa.c | 5 +-
> src/mesa/drivers/x11/xm_buffer.c | 3 +-
> src/mesa/drivers/x11/xmesaP.h | 3 +-
> src/mesa/main/accum.c | 17 +++--
> src/mesa/main/dd.h | 3 +-
> src/mesa/main/extensions_table.h | 1 +
> src/mesa/main/fbobject.c | 18 ++++-
> src/mesa/main/framebuffer.c | 1 +
> src/mesa/main/glheader.h | 3 +
> src/mesa/main/mtypes.h | 3 +
> src/mesa/main/readpix.c | 20 +++---
> src/mesa/state_tracker/st_cb_fbo.c | 7 +-
> src/mesa/swrast/s_blit.c | 17 +++--
> src/mesa/swrast/s_clear.c | 3 +-
> src/mesa/swrast/s_copypix.c | 11 +--
> src/mesa/swrast/s_depth.c | 6 +-
> src/mesa/swrast/s_drawpix.c | 26 ++++---
> src/mesa/swrast/s_renderbuffer.c | 6 +-
> src/mesa/swrast/s_renderbuffer.h | 3 +-
> src/mesa/swrast/s_stencil.c | 3 +-
> 29 files changed, 241 insertions(+), 57 deletions(-)
> create mode 100644 docs/specs/MESA_framebuffer_flip_y.spec
>
> diff --git a/docs/specs/MESA_framebuffer_flip_y.spec b/docs/specs/MESA_framebuffer_flip_y.spec
> new file mode 100644
> index 0000000000..dca77a9541
> --- /dev/null
> +++ b/docs/specs/MESA_framebuffer_flip_y.spec
> @@ -0,0 +1,84 @@
> +Name
> +
> + MESA_framebuffer_flip_y
> +
> +Name Strings
> +
> + GL_MESA_framebuffer_flip_y
> +
> +Contact
> +
> + Fritz Koenig <frkoenig at google.com>
> +
> +Contributors
> +
> + Fritz Koenig, Google
> + Kristian Høgsberg, Google
> + Chad Versace, Google
> +
> +Status
> +
> + Proposal
> +
> +Version
> +
> + Version 1, June 7, 2018
> +
> +Number
> +
> + TBD
> +
> +Dependencies
> +
> + OpenGL ES 3.1 is required, for FramebufferParameteri.
> +
> +Overview
> +
> + Rendered buffers are normally returned right side up, as accessed
> + top to bottom. This extension allows those buffers to be upside down
> + when accessed top to bottom.
> +
> + This extension defines a new framebuffer parameter,
> + GL_FRAMEBUFFER_FLIP_Y_MESA, that changes the behavior of the reads and
> + writes to the framebuffer attachment points. When GL_FRAMEBUFFER_FLIP_Y_MESA
> + is GL_TRUE, render commands and pixel transfer operations access the
> + backing store of each attachment point with an y-inverted coordinate
> + system. This y-inversion is relative to the coordinate system set when
> + GL_FRAMEBUFFER_FLIP_Y_MESA is GL_FALSE.
> +
> + Access through TexSubImage2D and similar calls will notice the effect of
> + the flip when they are not attached to framebuffer objects because
> + GL_FRAMEBUFFER_FLIP_Y_MESA is associated with the framebuffer object and
> + not the attachment points.
> +
> +IP Status
> +
> + None
> +
> +Issues
> +
> + None
> +
> +New Procedures and Functions
> +
> + None
> +
> +New Types
> +
> + None
> +
> +New Tokens
> +
> + Accepted by the <pname> argument of FramebufferParameteri and
> + GetFramebufferParameteriv:
> +
> + GL_FRAMEBUFFER_FLIP_Y_MESA 0x8BBB
> +
> +Errors
> + GL_INVALID_OPERATION is returned from GetFramebufferParameteriv if this
> + is called on a winsys framebuffer.
Above, s/on a winsys framebuffer/on the default framebuffer/
> +
> +Revision History
> +
> + Version 1, June, 2018
> + Initial draft (Fritz Koenig)
> diff --git a/include/GLES2/gl2ext.h b/include/GLES2/gl2ext.h
> index a7d19a1fc8..0a93bfb865 100644
> --- a/include/GLES2/gl2ext.h
> +++ b/include/GLES2/gl2ext.h
> @@ -2334,6 +2334,11 @@ GL_APICALL void GL_APIENTRY glGetPerfQueryInfoINTEL (GLuint queryId, GLuint quer
> #endif
> #endif /* GL_INTEL_performance_query */
>
> +#ifndef GL_MESA_framebuffer_flip_y
> +#define GL_MESA_framebuffer_flip_y 1
> +#define GL_FRAMEBUFFER_FLIP_Y_MESA 0x8BBB
> +#endif /* GL_MESA_framebuffer_flip_y */
In general, avoid adding changes to this file because they will likely
be silently clobbered by the next header update.
But, ignore that advice for now :) , and leave this hunk in the patch,
at least for now.
Most of the files in include/{GL,GLES*,EGL} come directly from Khronos.
Periodically, we fetch new headers from Khronos and commit them
verbatim. For all Mesa extensions that do not live in the Khronos
registry, we maintain the header chunks in a file at
include/${API}/*mesa*.h, and then add a one-line change to the Khronos
header to include the Mesa header. But it's a bit of a hack. Even more
of a hack when you consider that some Linux distributions (including
Chrome OS) don't install Mesa's GLES headers.
Since the public API for this extension is non-controversial, as soon as
possible please submit a merge request to Khronos that adds
GL_FRAMEBUFFER_FLIP_Y_MESA to the official gl.xml. It should get merged
quickly (less than a week) if Jon's not on vacation.
A good example to follow is:
https://github.com/KhronosGroup/OpenGL-Registry/commit/a4f25a7ec733d397d8ea78794a066ea519493688
commit a4f25a7ec733d397d8ea78794a066ea519493688
Author: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
AuthorDate: Mon Feb 26 14:31:57 2018 +0000
Commit: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
CommitDate: Mon Mar 5 13:57:25 2018 +0000
Subject: Register new INTEL_blackhole_render extension for OpenGL & OpenGL ES
While we're waiting for Jon to merge your GL patch, let's continue the
review of this series on mesa-dev, and maybe Jon will produce new
headers soon enough that you can drop this hunk from the final patch.
> diff --git a/src/mapi/glapi/registry/gl.xml b/src/mapi/glapi/registry/gl.xml
> index 833478aa51..13882eff7b 100644
> --- a/src/mapi/glapi/registry/gl.xml
> +++ b/src/mapi/glapi/registry/gl.xml
> @@ -6568,6 +6568,7 @@ typedef unsigned int GLhandleARB;
> <enum value="0x8BB5" name="GL_VERTEX_PROGRAM_CALLBACK_MESA"/>
> <enum value="0x8BB6" name="GL_VERTEX_PROGRAM_CALLBACK_FUNC_MESA"/>
> <enum value="0x8BB7" name="GL_VERTEX_PROGRAM_CALLBACK_DATA_MESA"/>
> + <enum value="0x8BBB" name="GL_FRAMEBUFFER_FLIP_Y_MESA"/>
> </enums>
>
> <enums namespace="GL" start="0x8BC0" end="0x8BFF" vendor="QCOM" comment="Reassigned from AMD to QCOM">
> @@ -44356,6 +44357,11 @@ typedef unsigned int GLhandleARB;
> <enum name="GL_TEXTURE_2D_STACK_BINDING_MESAX"/>
> </require>
> </extension>
> + <extension name="GL_MESA_framebuffer_flip_y" supported="gles2">
> + <require>
> + <enum name="GL_FRAMEBUFFER_FLIP_Y_MESA"/>
> + </require>
> + </extension>
> <extension name="GL_MESA_pack_invert" supported="gl">
> <require>
> <enum name="GL_PACK_INVERT_MESA"/>
> diff --git a/src/mesa/drivers/dri/i915/intel_fbo.c b/src/mesa/drivers/dri/i915/intel_fbo.c
> index 827a77f722..31b65fb53b 100644
> --- a/src/mesa/drivers/dri/i915/intel_fbo.c
> +++ b/src/mesa/drivers/dri/i915/intel_fbo.c
> @@ -86,7 +86,8 @@ intel_map_renderbuffer(struct gl_context *ctx,
> GLuint x, GLuint y, GLuint w, GLuint h,
> GLbitfield mode,
> GLubyte **out_map,
> - GLint *out_stride)
> + GLint *out_stride,
> + GLboolean inverted_y)
Please rename the parameter to match the extension token, 'flip_y'.
And I believe the internal APIs should use 'bool' instead of
'GLboolean'. See commit 786a6472450b50977e6906e27d5f481e00b05d73 .
Ken, should Fritz also use plain 'bool' in struct gl_framebuffer? That
is, should it be
struct gl_framebuffer {
...
GLboolean FlipY;
or
bool FlipY;
> {
> struct intel_context *intel = intel_context(ctx);
> struct swrast_renderbuffer *srb = (struct swrast_renderbuffer *)rb;
> @@ -94,6 +95,10 @@ intel_map_renderbuffer(struct gl_context *ctx,
> void *map;
> int stride;
>
> + /* driver does not support GL_FRAMEBUFFER_FLIP_Y_MESA */
> + if (rb->Name != 0)
> + assert(!inverted_y);
I think this should be a biconditional. But I'm not entirely sure.
assert((rb->Name == 0) == flip_y);
> +
> if (srb->Buffer) {
> /* this is a malloc'd renderbuffer (accum buffer), not an irb */
> GLint bpp = _mesa_get_format_bytes(rb->Format);
> diff --git a/src/mesa/drivers/dri/i965/intel_fbo.c b/src/mesa/drivers/dri/i965/intel_fbo.c
> index fb84b738c0..7ef525c121 100644
> --- a/src/mesa/drivers/dri/i965/intel_fbo.c
> +++ b/src/mesa/drivers/dri/i965/intel_fbo.c
> @@ -105,7 +105,8 @@ intel_map_renderbuffer(struct gl_context *ctx,
> GLuint x, GLuint y, GLuint w, GLuint h,
> GLbitfield mode,
> GLubyte **out_map,
> - GLint *out_stride)
> + GLint *out_stride,
> + GLboolean inverted_y)
> {
> struct brw_context *brw = brw_context(ctx);
> struct swrast_renderbuffer *srb = (struct swrast_renderbuffer *)rb;
> @@ -114,6 +115,10 @@ intel_map_renderbuffer(struct gl_context *ctx,
> void *map;
> ptrdiff_t stride;
>
> + /* driver does not support GL_FRAMEBUFFER_FLIP_Y_MESA */
> + if (rb->Name != 0)
> + assert(!inverted_y);
> +
> if (srb->Buffer) {
> /* this is a malloc'd renderbuffer (accum buffer), not an irb */
> GLint bpp = _mesa_get_format_bytes(rb->Format);
[snip]
> diff --git a/src/mesa/main/extensions_table.h b/src/mesa/main/extensions_table.h
> index 7af48a4ad9..29ef35c20a 100644
> --- a/src/mesa/main/extensions_table.h
> +++ b/src/mesa/main/extensions_table.h
> @@ -323,6 +323,7 @@ EXT(KHR_texture_compression_astc_hdr , KHR_texture_compression_astc_hdr
> EXT(KHR_texture_compression_astc_ldr , KHR_texture_compression_astc_ldr , GLL, GLC, x , ES2, 2012)
> EXT(KHR_texture_compression_astc_sliced_3d , KHR_texture_compression_astc_sliced_3d , GLL, GLC, x , ES2, 2015)
>
> +EXT(MESA_framebuffer_flip_y , MESA_framebuffer_flip_y , x, x, x , 31, 2018)
> EXT(MESA_pack_invert , MESA_pack_invert , GLL, GLC, x , x , 2002)
> EXT(MESA_shader_integer_functions , MESA_shader_integer_functions , GLL, GLC, x , 30, 2016)
> EXT(MESA_texture_signed_rgba , EXT_texture_snorm , GLL, GLC, x , x , 2009)
Yep, this table looks good.
> diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c
> index fa7a9361df..962a19446c 100644
> --- a/src/mesa/main/fbobject.c
> +++ b/src/mesa/main/fbobject.c
> @@ -1430,6 +1430,10 @@ framebuffer_parameteri(struct gl_context *ctx, struct gl_framebuffer *fb,
> if (!ctx->Extensions.ARB_sample_locations)
> goto invalid_pname_enum;
> break;
> + case GL_FRAMEBUFFER_FLIP_Y_MESA:
> + if (!ctx->Extensions.MESA_framebuffer_flip_y)
> + goto invalid_pname_enum;
> + cannot_be_winsys_fbo = true;
> default:
> goto invalid_pname_enum;
> }
> @@ -1482,6 +1486,9 @@ framebuffer_parameteri(struct gl_context *ctx, struct gl_framebuffer *fb,
> case GL_FRAMEBUFFER_SAMPLE_LOCATION_PIXEL_GRID_ARB:
> fb->SampleLocationPixelGrid = !!param;
> break;
> + case GL_FRAMEBUFFER_FLIP_Y_MESA:
> + fb->InvertedY = param;
> + break;
> }
>
> switch (pname) {
> @@ -1489,7 +1496,7 @@ framebuffer_parameteri(struct gl_context *ctx, struct gl_framebuffer *fb,
> case GL_FRAMEBUFFER_SAMPLE_LOCATION_PIXEL_GRID_ARB:
> if (fb == ctx->DrawBuffer)
> ctx->NewDriverState |= ctx->DriverFlags.NewSampleLocations;
> - break;
> + break;
> default:
> invalidate_framebuffer(fb);
> ctx->NewState |= _NEW_BUFFERS;
> @@ -1574,6 +1581,12 @@ validate_get_framebuffer_parameteriv_pname(struct gl_context *ctx,
> goto invalid_pname_enum;
> cannot_be_winsys_fbo = false;
> break;
> + case GL_FRAMEBUFFER_FLIP_Y_MESA:
> + if (!ctx->Extensions.MESA_framebuffer_flip_y) {
> + _mesa_error(ctx, GL_INVALID_ENUM, "%s(pname=0x%x)", func, pname);
> + return false;
> + }
I think you should cannot_be_winsys_fbo here.
> + break;
[snip]
> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> index 0dfff31396..605fa8a564 100644
> --- a/src/mesa/main/mtypes.h
> +++ b/src/mesa/main/mtypes.h
> @@ -3509,6 +3509,8 @@ struct gl_framebuffer
>
> /** Delete this framebuffer */
> void (*Delete)(struct gl_framebuffer *fb);
> +
> + GLboolean InvertedY;
Again, please rename the field to match the token name, 'FlipY'.
And try to keep the date fields above the non-extension pointers, such
as 'Delete'.
And add a comment with the extension name too.
In total,
= /* GL_ARB_sample_locations */
= GLfloat *SampleLocationTable; /**< If NULL, no table has been specified */
= GLboolean ProgrammableSampleLocations;
= GLboolean SampleLocationPixelGrid;
=
+ /* GL_MESA_framebuffer_flip_y */
+ GLboolean FlipY;
[snip]
The rest of the patch looks good to me.
More information about the mesa-dev
mailing list