[Mesa-dev] [PATCH 1/2] mesa: MESA_framebuffer_flip_y extension [v3]
Fritz Koenig
frkoenig at google.com
Wed Jul 18 15:20:57 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'.
>
My reasoning for naming the variable 'inverted_y' is that it
represents the state of the buffer, where as the extension is 'flip_y'
because that is the action. Maybe that is too abstract? I can see
that having the same name through out makes it easier to correlate the
variable with the extension. But there is a loose coupling of
extension to state. Adding the variable makes it clearer what the
state of the framebuffer is.
I'm trying to say is that I'm open to changing the variable name to '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