[Mesa-stable] [Mesa-dev] [PATCH 1/2] egl/android: remove HAL_PIXEL_FORMAT_BGRA_8888 support

[Tomasz Figa]

On Mon, Jul 24, 2017 at 1:00 PM, Tomasz Figa <tfiga at chromium.org> wrote:
> On Wed, Jul 12, 2017 at 1:34 AM, Rob Herring <robh at kernel.org> wrote:
>> On Tue, Jul 11, 2017 at 9:34 AM, Tomasz Figa <tfiga at chromium.org> wrote:
>>> On Tue, Jul 11, 2017 at 11:16 PM, Rob Herring <robh at kernel.org> wrote:
>>>> On Tue, Jul 11, 2017 at 8:27 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>>>> From: Emil Velikov <emil.velikov at collabora.com>
>>>>>
>>>>> As said in the EGL_KHR_platform_android extensions
>>>>>
>>>>>     For each EGLConfig that belongs to the Android platform, the
>>>>>     EGL_NATIVE_VISUAL_ID attribute is an Android window format, such as
>>>>>     WINDOW_FORMAT_RGBA_8888.
>>>>>
>>>>> Although it should be applicable overall.
>>>>>
>>>>> Even though we use HAL_PIXEL_FORMAT here, those are numerically
>>>>> identical to the  WINDOW_FORMAT_ and AHARDWAREBUFFER_FORMAT_ ones.
>>>>>
>>>>> Barring the said format of course. That one is only listed in HAL.
>>>>>
>>>>> Keep in mind that even if we try to use the said format, you'll get
>>>>> caught by droid_create_surface(). The function compares the format of
>>>>> the underlying window, against the NATIVE_VISUAL_ID of the config.
>>>>>
>>>>> Unfortunatelly it only prints a warning, rather than error out, likely
>>>>> leading to visual corruption.
>>>>>
>>>>> While SDL will even call ANativeWindow_setBuffersGeometry() with the
>>>>> wrong format, and conviniently ignore the [expected] failure.
>>>>>
>>>>> Cc: mesa-stable at lists.freedesktop.org
>>>>> Cc: Chad Versace <chadversary at google.com>
>>>>> Cc: Tomasz Figa <tfiga at chromium.org>
>>>>> Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
>>>>> ---
>>>>> I'm about 99.99% sure the above is correct, but I haven't tested it.
>>>>
>>>> Isn't this going to break if there's no driver support for RGBA/RGBX
>>>> which is the case for stable (and master for gallium drvs).
>>>
>>> First of all, Android hardcodes HAL_PIXEL_FORMAT_RGBA_8888 in a number
>>> of places, which means that those users use a patched Android. However
>>> I'm not sure if we can just break them like this. I'll leave it to you
>>> guys, though.
>>
>> Yes, patched to work around mesa's lack of RGBA/X support. Not sure
>> why they went this route. Maybe RGBA/X support in mesa was attempted
>> before.
>>
>>> Other than that, CTS seems to require only RGBA_8888 and RGB_565, so
>>> this change is not going to affect compliance with unpatched Android.
>>
>> Okay, good to know.
>
> I just found some interesting evidence that proves we should go
> forward with this patch:
>
> https://android.googlesource.com/platform/frameworks/native/+/master/opengl/libs/EGL/eglApi.cpp#455
>
> Basically this is the EGL wrapper that is used for everything on
> Android. Whenever eglCreateWindowSurface() is called, it goes through
> the linked function first, which explicitly limits all the supported
> formats to RGBA_8888, RGBX_8888 and RGB_565, by forcing one of those
> matching EGL config's component sizes on the given native window (the
> native_window_set_buffers_format() call).
>
> I don't know how this worked for me before, but essentially it makes
> it impossible to pass dEQP-EGL tests with RGBA8888 (as per EGL config
> component widths only) window surface, because it's impossible to have
> eglCreateWindowSurface() accept BGRA_8888 and in result the config
> doesn't match the surface, which gives swapped channels in the
> buffers.
>
> Since the support for Gallium landed already, can we move forward with
> this patch?
>
> (If it's worth anything:)
> Acked-by: Tomasz Figa <tfiga at chromium.org>

Ah, just one side note (should have checked on the patch again,
instead of relying on my apparently already incomplete memories). We
should keep EGL image support for the format, as it is surprisingly
used for some camera use cases, at least on some of our platforms. So
the only thing that should go away is the array item in
droid_add_configs_for_visuals().

Best regards,
Tomasz