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

[Lin, Johnson]

Cool. Thanks. Will update the patch.

-----Original Message-----
From: Palli, Tapani 
Sent: Friday, March 23, 2018 4:59 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/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.


> -----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