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

[Tomasz Figa]

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>

Best regards,
Tomasz