[Mesa-dev] [PATCH] mesa: readpixels add support for GL_HALF_FLOAT

[Alejandro Piñeiro]

On 23/03/18 09:58, Tapani Pälli wrote:
>
> On 03/23/2018 07:54 AM, Lin, Johnson wrote:
>> So the solution will be query if EXT_color_buffer_half_float is
>> supported?
>
> I think I found a proof that we don't actually need anything. Spec for
> EXT_color_buffer_float adds following text:
>
> --- 8< ---
> An INVALID_OPERATION error is generated ... if the color buffer is a
> floating-point format and type is not FLOAT, HALF FLOAT, or
> UNSIGNED_INT_10F_11F_11F_REV.
> --- 8< ---
>
> This means that type can be HALF FLOAT when color buffer has
> floating-point format.
>
> (even though for some weird reason spec does not add it to earlier
> sentence that says " "For floating-point rendering surfaces, the
> combination format RGBA and type FLOAT is accepted.")
>
> Since EXT_color_buffer_float is enabled, we can just use this patch.
> Please remove the comment about EXT_color_buffer_half_float extension
> though, that extension is not enabled and would need more work elsewhere.

Taking into account that the conclusion that we don't need a extension
check required some code+spec research, how about it we mention it
somewhere? In a comment at the code, or perhaps in the commit message is
enough.

>
>
>> -----Original Message-----
>> From: Palli, Tapani
>> Sent: Friday, March 23, 2018 1:53 PM
>> To: Lin, Johnson <johnson.lin at intel.com>; Alejandro Piñeiro
>> <apinheiro at igalia.com>; mesa-dev at lists.freedesktop.org
>> Subject: Re: [Mesa-dev] [PATCH] mesa: readpixels add support for
>> GL_HALF_FLOAT
>>
>>
>> On 03/22/2018 07:53 AM, Tapani Pälli wrote:
>>>
>>>
>>> On 03/22/2018 04:43 AM, Lin, Johnson wrote:
>>>> Hi,  Thanks for the comments.
>>>>
>>>> I just noticed it does not check the extension support for
>>>> EXT_color_buffer_float neither?
>>>
>>> That is probably because it is enabled as 'dummy_true' (see
>>> extensions_table.h) so it's always enabled on any driver. I wonder if
>>> we can just go and do the same for EXT_color_buffer_half_float? Is
>>> there any driver that would not support this?
>>
>> Took a brief look and no, we can't simply toggle it on. There is also
>> some API interaction defined by the spec that would need to be
>> enabled, querying component type via
>> glGetFramebufferAttachmentParameteriv and so on.
>>
>>>
>>>> -----Original Message-----
>>>> From: Palli, Tapani
>>>> Sent: Wednesday, March 21, 2018 6:57 PM
>>>> To: Alejandro Piñeiro <apinheiro at igalia.com>; Lin, Johnson
>>>> <johnson.lin at intel.com>; mesa-dev at lists.freedesktop.org
>>>> Subject: Re: [Mesa-dev] [PATCH] mesa: readpixels add support for
>>>> GL_HALF_FLOAT
>>>>
>>>>
>>>>
>>>> On 21.03.2018 12:45, Tapani Pälli wrote:
>>>>>
>>>>>
>>>>> On 21.03.2018 08:52, Alejandro Piñeiro wrote:
>>>>>> On 21/03/18 06:57, Lin Johnson wrote:
>>>>>>> Ext_color_buffer_half_float is using type GL_HALF_FLOAT and
>>>>>>> data_type GL_FLOAT. This fix Android CTS test
>>>>>>> android.view.cts.PixelCopyTest #TestWindowProducerCopyToRGBA16F
>>>>>>>
>>>>>>> Signed-off-by: Lin Johnson <johnson.lin at intel.com>
>>>>>>> ---
>>>>>>>     src/mesa/main/readpix.c | 2 ++
>>>>>>>     1 file changed, 2 insertions(+)
>>>>>>>
>>>>>>> diff --git a/src/mesa/main/readpix.c b/src/mesa/main/readpix.c
>>>>>>> index 6ce340ddf9bb..51331dd095ab 100644
>>>>>>> --- a/src/mesa/main/readpix.c
>>>>>>> +++ b/src/mesa/main/readpix.c
>>>>>>> @@ -920,6 +920,8 @@ read_pixels_es3_error_check(GLenum format,
>>>>>>> GLenum type,
>>>>>>>        case GL_RGBA:
>>>>>>>           if (type == GL_FLOAT && data_type == GL_FLOAT)
>>>>>>>              return GL_NO_ERROR; /* EXT_color_buffer_float */
>>>>>>> +      if (type == GL_HALF_FLOAT && data_type == GL_FLOAT)
>>>>>>> +         return GL_NO_ERROR; /* EXT_color_buffer_half_float */
>>>>>>
>>>>>> If this combination is allowed thanks to that extension, what would
>>>>>> happen if that extension is not supported? shouldn't include a
>>>>>> extension check? Or that is checked in a different place?
>>>>>
>>>>> I was thinking the same. Having seen the test it does not seem to
>>>>> make any kind of checks what is supported (like asking for
>>>>> extension, or maybe asking for GL_IMPLEMENTATION_COLOR_READ_TYPE)
>>>>> but attempts glReadPixels using GL_HALF_FLOAT type, I think it
>>>>> should verify first that such reads are supported. Currently we
>>>>> don't seem to support this extension.
>>>>
>>>> ... but probably support the functionality (OpenGL ES 3.2), so maybe
>>>> some checks needed for ES version (?)
>>>>
>>>>
>>>>>
>>>>>
>>>>>>>           if (type == GL_UNSIGNED_BYTE && data_type ==
>>>>>>> GL_UNSIGNED_NORMALIZED)
>>>>>>>              return GL_NO_ERROR;
>>>>>>>           if (internalFormat == GL_RGB10_A2 &&
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> mesa-dev mailing list
>>>>>> mesa-dev at lists.freedesktop.org
>>>>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>>>>>
>>>>> _______________________________________________
>>>>> mesa-dev mailing list
>>>>> mesa-dev at lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>