[Mesa-dev] [Patch v3] Mesa: Add support for GL_OES_texture_*float* extensions.

[Ilia Mirkin]

On Tue, Dec 2, 2014 at 3:36 PM, Ian Romanick <idr at freedesktop.org> wrote:
> On 11/28/2014 01:08 PM, Kalyan Kondapally wrote:
>> This patch adds support for following GLES2 Texture Float extensions:
>> 1)GL_OES_texture_float,
>> 2)GL_OES_texture_half_float,
>> 3)GL_OES_texture_float_linear,
>> 4)GL_OES_texture_half_float_linear.
>>
>> If we are using GLES context and the driver has advertised support for
>> ARB_texture_float, then support for all texture_float* extensions is
>> enabled. Here, we are assuming that when driver advertises support for
>> ARB_texture_float, it should be able to support all these extensions.
>>
>> v2: Indentation fixes. (Brian Paul)
>>     Fixed Comments and added some to new private functions.(Brian Paul)
>>     Added assert in valid_filter_for_float.(Brian Paul)
>>     Renamed Float and HALF_FLOAT_OES as IsFloatOES and IsHalfFloatOES.(Brian Paul)
>>     adjust_for_oes_float_texture to return GLenum. (Brian Paul)
>>     Use RGB_32F internal floating point format for RGB base format.
>>
>> v3: Don't indent case. (Matt Turner)
>>     Enable support for float extensions in case driver supports
>>     ARB_texture_float. (Fredrik Höglund)
>
> At this point, I think this patch should be split in three:
>
> 1. Extension infrastructure bits.
>
> 2. GL_HALF_FLOAT_OES fixes.
>
> 3. The rest.
>
> I suggest this because I think the first two should be pretty much
> landable.  "The rest" may still need some changes.
>
> I'd also accept a 0th patch.
>
> 0. Global s/GL_HALF_FLOAT_ARB/GL_HALF_FLOAT/g;s/GLhalfARB/GLhalf/g
>
> :)
>
>> Signed-off-by: Kevin Rogovin <kevin.rogovin at intel.com>
>> Signed-off-by: Kalyan Kondapally <kalyan.kondapally at intel.com>
>> ---
>>  src/mesa/main/context.c    | 15 +++++++++++
>>  src/mesa/main/extensions.c |  4 +++
>>  src/mesa/main/glformats.c  | 46 +++++++++++++++++++++++++++++---
>>  src/mesa/main/glformats.h  |  3 ++-
>>  src/mesa/main/mtypes.h     |  6 +++++
>>  src/mesa/main/pack.c       | 16 +++++++++++
>>  src/mesa/main/teximage.c   | 66 +++++++++++++++++++++++++++++++++++++++++++++-
>>  src/mesa/main/texobj.c     | 53 +++++++++++++++++++++++++++++++++++++
>>  8 files changed, 203 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/mesa/main/context.c b/src/mesa/main/context.c
>> index 400c158..8b54967 100644
>> --- a/src/mesa/main/context.c
>> +++ b/src/mesa/main/context.c
>> @@ -1544,6 +1544,21 @@ handle_first_current(struct gl_context *ctx)
>>        return;
>>     }
>>
>> +   /* If we are using GLES context and the driver has advertised support for
>> +    * ARB_texture_float, then enable support for all texture_float* extensions.
>> +    * Here, we are assuming that when driver advertises support for
>> +    * ARB_texture_float, it should be able to support all these extensions. In
>> +    * case any particular driver doesn't want to enable these extensions or
>> +    * only a subset on GLES, then it shouldn't advertise support for
>> +    * ARB_texture_float and toggle OES_texture_float* flags accordingly.
>> +    */
>> +   if (ctx->Extensions.ARB_texture_float && _mesa_is_gles(ctx)) {
>> +      ctx->Extensions.OES_texture_float = GL_TRUE;
>> +      ctx->Extensions.OES_texture_float_linear = GL_TRUE;
>> +      ctx->Extensions.OES_texture_half_float = GL_TRUE;
>> +      ctx->Extensions.OES_texture_half_float_linear = GL_TRUE;
>> +   }
>
> I think this is a bad idea.  Right now OpenGL ES 3.0 support depends on
> ARB_texture_float (see compute_version_es2 in src/mesa/main/version.c).
>  That's a superset of OpenGL ES 3.0: GL_OES_texture_float_linear is not
> required.  Now when someone wants to enable ES3 on their GPU that
> supports everything except GL_OES_texture_float_linear they will have to
> find-and-move this code because this code occurs after the version is
> calculated.  That person will also have to modify other drivers (that
> they probably cannot test).  Gosh, what could go wrong? :)
>
> There are a bunch of other cases where an extension can be automatically
> enabled if some other conditions are met.  If we care about saving a
> couple lines of code here and there, we should make a function that
> drivers explicitly call to do that.  Drivers that opt-in can call this
> function after they have enabled all of their other extensions in their
> create-context path.
>
> Moreover, this code is already broken for r300.  Marek said in the
> previous thread that r300 advertises GL_ARB_texture_float (and
> developers know the documented caveats there) but does not want to
> advertise GL_OES_texture_*_linear.
>
> For the most part in Mesa, extensions are only enabled by default in
> three cases:
>
> 1. All in-tree drivers were already enabling the extension.
> GL_ARB_texture_env_add is an example.  In the long past this extension
> was not enabled by default.  Eventually all drivers either grew support
> or were removed from the tree.
>
> 2. The extension is just API "syntactic sugar" that the driver won't
> even see.  GL_ARB_multi_bind is an example.
>
> 3. The extension has a secondary enable, such as a number of available
> formats, that drivers can control.  GL_ARB_get_program_binary,
> GL_ARB_multisample, and GL_ARB_texture_compression are examples.
>
> In these cases we have a compelling testing story.
>
> - If the extension was already enabled by the driver, then we can assume
> that it has already been tested.
>
> - If an extension is syntactic sugar, then testing on driver X should be
> good enough for driver Y.
>
> - If an extension has a secondary enable, tests (and applications)
> should skip the functionality when the secondary enable is not set.
> Alas, this does not always hold, and this is why GL_ARB_occlusion_query
> is not enabled by default with GL_QUERY_COUNTER_BITS = 0.
>
> Especially given the existence of things like meta, enabling existing
> functionaly in a new API can have surprising results.  I'd much prefer
> to get buy-in from other driver maintainers before enabling this by
> default.  That buy-in generally comes in the form of patches that enable
> the functionality. :)  See the GL_ARB_texture_env_add example above.
>
> Another way to think of it is that we have two competing goals.  On one
> hand, we want to generally raise up the functionality supported by Mesa
> drivers, share common code, and avoid unnecessary extension checks.  On
> the other hand, we don't want to cause bug reports that other people
> will have to field.
>
> My suggestion: just enable the full set of extensions in the i965
> driver.  If someone that maintains a Gallium-based driver cares, they
> can add a cap bit to selectively enable the GL_OES_texture_*_linear
> extensions.  They also have a different mechanism to independently
> enable GL_OES_texture_float and / or GL_OES_texture_half_float when the
> hardware natively supports those formats.  I don't think you want to
> enable GL_OES_texture_half_float on hardware that only does 32-bit float
> textures, for example.

I've been following this discussion since I've also been working on
freedreno, which targets (among others) the Adreno A3xx gpu line. This
line of GPUs was designed for GLES2/3, but there's some desire to use
it with desktop GL as well, since that's what all "usual" software
expects. So in a GLES context, it's be nice to be specific about
things you enable (e.g. leaving out OES_texture_float_linear), but
also having a big hammer of enabling things for desktop GL which will
have a 90%-compliant implementation, and where the incompliance is
acceptable (weird quirks in the API, rarely used features, etc).
Another example is the GLES EXT_color_buffer_float doesn't require
32-bit float blending, while the GL ARB_color_buffer_float does (and
the A3xx hw doesn't appear to support it). However in desktop GL, we
may still want to expose ARB_color_buffer_float and some other things
required for GL3.0.

I guess the main take-away from that ranty comment, is that it's nice
to let drivers choose :)

  -ilia