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

[Tapani Pälli]

On 23.03.2018 12:18, Alejandro Piñeiro wrote:
> 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.

True, commit message could include the part mentioning when 
INVALID_OPERATION is generated as that mentions both FLOAT and HALF FLOAT.



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