[Mesa-dev] [PATCH 1/2] mesa: MESA_framebuffer_flip_y extension [v3]
Fritz Koenig
frkoenig at google.com
Thu Jul 19 20:05:41 UTC 2018
On Wed, Jul 11, 2018 at 3:54 PM Chad Versace <chadversary at chromium.org> wrote:
>
> +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.
>
cannot_be_winsys_fbo is set to true in the beginning of the function already.
> > + 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