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....@intel.com>; Alejandro Piñeiro >> <apinhe...@igalia.com>; mesa-dev@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 <apinhe...@igalia.com>; Lin, Johnson >>>> <johnson....@intel.com>; mesa-dev@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....@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@lists.freedesktop.org >>>>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev >>>>>> >>>>> _______________________________________________ >>>>> mesa-dev mailing list >>>>> mesa-dev@lists.freedesktop.org >>>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev >>> _______________________________________________ >>> mesa-dev mailing list >>> mesa-dev@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
