[Mesa-dev] [PATCH] st/mesa: remove ARB_color_buffer_float from core profile contexts

[Nicolai Hähnle]

On 10.01.2017 16:21, Ilia Mirkin wrote:
> This will just cause shader based workarounds in the affected
> applications, no? What's the benefit of removing this? Fwiw, Nvidia hw
> has support for this.

It's only supposed to remove the extension string, nothing else. Can you 
explain in which scenario you would expect a shader-based workaround?

The benefit of removing this is that it sidesteps a silly spec lawyering 
issue, which is that:

1. If we advertise an OpenGL core context, we're supposed to _not_ 
support GL_CLAMP_VERTEX/FRAGMENT_COLOR_ARB in ClampColor.

2. ARB_color_buffer_float on the other hand says that those enums 
_should_ be supported.

It's not clear how to reconcile these two when both are advertised. The 
GLCTS expects the enums to be unsupported in core contexts, no matter 
which extensions are present.

If the NVidia blob advertises ARB_color_buffer_float also in core 
contexts, it probably just doesn't "fully implement" the extension. We 
could do the same by changing the check in _mesa_ClampColor instead of 
this patch

I don't care much either way, but this patch brings us in line with what 
the Intel driver has been doing since 2013...

Nicolai

>
> On Jan 10, 2017 9:59 AM, "Nicolai Hähnle" <nhaehnle at gmail.com
> <mailto:nhaehnle at gmail.com>> wrote:
>
>     From: Nicolai Hähnle <nicolai.haehnle at amd.com
>     <mailto:nicolai.haehnle at amd.com>>
>
>     Some parts of the extension were explicitly removed for core profiles,
>     and all the remaining functionality has been in core since core profiles
>     exist. So there's no loss of exposed functionality.
>
>     The corresponding change was applied to i965 in 2013 (commit
>     bd850cb4f2c77e2eb6716c865c40b9976633fc23), so it's not like
>     applications could be surprised by this behavior either.
>
>     Fixes GL45-CTS.gtf30.GL3Tests.half_float.half_float_textures.
>     ---
>      src/mesa/state_tracker/st_context.c | 16 ++++++----------
>      1 file changed, 6 insertions(+), 10 deletions(-)
>
>     diff --git a/src/mesa/state_tracker/st_context.c
>     b/src/mesa/state_tracker/st_context.c
>     index 0eae971..a2df704 100644
>     --- a/src/mesa/state_tracker/st_context.c
>     +++ b/src/mesa/state_tracker/st_context.c
>     @@ -450,32 +450,28 @@ st_create_context_priv( struct gl_context
>     *ctx, struct pipe_context *pipe,
>
>         /* Enable shader-based fallbacks for ARB_color_buffer_float if
>     needed. */
>         if (screen->get_param(screen, PIPE_CAP_VERTEX_COLOR_UNCLAMPED)) {
>            if (!screen->get_param(screen, PIPE_CAP_VERTEX_COLOR_CLAMPED)) {
>               st->clamp_vert_color_in_shader = GL_TRUE;
>            }
>
>            if (!screen->get_param(screen,
>     PIPE_CAP_FRAGMENT_COLOR_CLAMPED)) {
>               st->clamp_frag_color_in_shader = GL_TRUE;
>            }
>     -
>     -      /* For drivers which cannot do color clamping, it's better to
>     just
>     -       * disable ARB_color_buffer_float in the core profile, because
>     -       * the clamping is deprecated there anyway. */
>     -      if (ctx->API == API_OPENGL_CORE &&
>     -          (st->clamp_frag_color_in_shader ||
>     st->clamp_vert_color_in_shader)) {
>     -         st->clamp_vert_color_in_shader = GL_FALSE;
>     -         st->clamp_frag_color_in_shader = GL_FALSE;
>     -         ctx->Extensions.ARB_color_buffer_float = GL_FALSE;
>     -      }
>         }
>
>     +   /* Disable ARB_color_buffer_float for core contexts, since some
>     of its
>     +    * functionality was explicitly removed.
>     +    */
>     +   if (ctx->API == API_OPENGL_CORE)
>     +      ctx->Extensions.ARB_color_buffer_float = GL_FALSE;
>     +
>         /* called after _mesa_create_context/_mesa_init_point, fix
>     default user
>          * settable max point size up
>          */
>         ctx->Point.MaxSize = MAX2(ctx->Const.MaxPointSize,
>                                   ctx->Const.MaxPointSizeAA);
>         /* For vertex shaders, make sure not to emit saturate when SM
>     3.0 is not supported */
>         ctx->Const.ShaderCompilerOptions[MESA_SHADER_VERTEX].EmitNoSat =
>     !st->has_shader_model3;
>
>         if (!ctx->Extensions.ARB_gpu_shader5) {
>            for (i = 0; i < MESA_SHADER_STAGES; i++)
>     --
>     2.7.4
>
>     _______________________________________________
>     mesa-dev mailing list
>     mesa-dev at lists.freedesktop.org <mailto:mesa-dev at lists.freedesktop.org>
>     https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>     <https://lists.freedesktop.org/mailman/listinfo/mesa-dev>
>
>