[Mesa-dev] [PATCH 0/5] egl/android: Change order of EGLConfig generation

Discussion:

Chad Versace

2017-06-17 02:37:45 UTC

Patches 1-4 are little cleanups. The real change is in patch 5.

I wrote this series while debugging issues that Rob Herring found [1]
while testing my i965 RGBX patch series [2]. *This* patch series
fixes those errors, and is also independent of my RGBX series.

[1]: https://lists.freedesktop.org/archives/mesa-dev/2017-June/158400.html
[2]: https://lists.freedesktop.org/archives/mesa-dev/2017-June/158142.html

Many Android apps (such as Google's official NDK GLES2 example app), and
even portions the core framework code (such as SystemServiceManager in
Nougat), incorrectly choose their EGLConfig. They neglect to match the
EGLConfig's EGL_NATIVE_VISUAL_ID against the window's native format, and
instead choose the first EGLConfig whose channel sizes match those of
the native window format while ignoring the channel *ordering*.

We can detect such buggy clients in logcat when they call
eglCreateSurface, by detecting the mismatch between the EGLConfig's
format and the window's format.

As a workaround, this patch series changes the order of EGLConfig
generation such that all EGLConfigs for HAL pixel format i precede those
for HAL pixel format i+1. In my testing on Android Nougat, this was good
enough to pacify the buggy clients.

This patch series lives on a git tag:
http://git.kiwitree.net/cgit/~chadv/mesa/tag/?h=chadv/review/2017-06-16/egl-android-config-order-v01

Chad Versace (5):
egl/android: Declare loop vars inside their loops
egl/android: Declare 'const' the EGLConfig attribs template array
egl/android: Rename var in droid_add_configs_for_visuals()
egl/android: Pull invariant var outside of loop
egl/android: Change order of EGLConfig generation

src/egl/drivers/dri2/platform_android.c | 79 +++++++++++++++++++--------------
1 file changed, 45 insertions(+), 34 deletions(-)

--
2.13.0

Chad Versace

2017-06-17 02:37:46 UTC

Permalink

That is, consistently do this:

for (int i = 0; ...)

No behavioral change.
---
src/egl/drivers/dri2/platform_android.c | 32 ++++++++++++++------------------
1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/src/egl/drivers/dri2/platform_android.c b/src/egl/drivers/dri2/platform_android.c
index 5550f580a80..a5f45a0bfbe 100644
--- a/src/egl/drivers/dri2/platform_android.c
+++ b/src/egl/drivers/dri2/platform_android.c
@@ -64,9 +64,7 @@ static const struct droid_yuv_format droid_yuv_formats[] = {
static int
get_fourcc_yuv(int native, int is_ycrcb, int chroma_step)
{
- int i;
-
- for (i = 0; i < ARRAY_SIZE(droid_yuv_formats); ++i)
+ for (int i = 0; i < ARRAY_SIZE(droid_yuv_formats); ++i)
if (droid_yuv_formats[i].native == native &&
droid_yuv_formats[i].is_ycrcb == is_ycrcb &&
droid_yuv_formats[i].chroma_step == chroma_step)
@@ -78,9 +76,7 @@ get_fourcc_yuv(int native, int is_ycrcb, int chroma_step)
static bool
is_yuv(int native)
{
- int i;
-
- for (i = 0; i < ARRAY_SIZE(droid_yuv_formats); ++i)
+ for (int i = 0; i < ARRAY_SIZE(droid_yuv_formats); ++i)
if (droid_yuv_formats[i].native == native)
return true;

@@ -299,9 +295,8 @@ droid_free_local_buffers(struct dri2_egl_surface *dri2_surf)
{
struct dri2_egl_display *dri2_dpy =
dri2_egl_display(dri2_surf->base.Resource.Display);
- int i;

- for (i = 0; i < ARRAY_SIZE(dri2_surf->local_buffers); i++) {
+ for (int i = 0; i < ARRAY_SIZE(dri2_surf->local_buffers); i++) {
if (dri2_surf->local_buffers[i]) {
dri2_dpy->dri2->releaseBuffer(dri2_dpy->dri_screen,
dri2_surf->local_buffers[i]);
@@ -951,10 +946,10 @@ static int
droid_get_buffers_parse_attachments(struct dri2_egl_surface *dri2_surf,
unsigned int *attachments, int count)
{
- int num_buffers = 0, i;
+ int num_buffers = 0;

/* fill dri2_surf->buffers */
- for (i = 0; i < count * 2; i += 2) {
+ for (int i = 0; i < count * 2; i += 2) {
__DRIbuffer *buf, *local;

assert(num_buffers < ARRAY_SIZE(dri2_surf->buffers));
@@ -1046,20 +1041,21 @@ droid_add_configs_for_visuals(_EGLDriver *drv, _EGLDisplay *dpy)
EGL_RECORDABLE_ANDROID, EGL_TRUE,
EGL_NONE
};
+
unsigned int format_count[ARRAY_SIZE(visuals)] = { 0 };
- int count, i, j;
+ int count = 0;

- count = 0;
- for (i = 0; dri2_dpy->driver_configs[i]; i++) {
+ for (int i = 0; dri2_dpy->driver_configs[i]; i++) {
const EGLint surface_type = EGL_WINDOW_BIT | EGL_PBUFFER_BIT;
- struct dri2_egl_config *dri2_conf;

- for (j = 0; j < ARRAY_SIZE(visuals); j++) {
+ for (int j = 0; j < ARRAY_SIZE(visuals); j++) {
config_attrs[1] = visuals[j].format;
config_attrs[3] = visuals[j].format;

- dri2_conf = dri2_add_config(dpy, dri2_dpy->driver_configs[i],
- count + 1, surface_type, config_attrs, visuals[j].rgba_masks);
+ struct dri2_egl_config *dri2_conf =
+ dri2_add_config(dpy, dri2_dpy->driver_configs[i], count + 1,
+ surface_type, config_attrs,
+ visuals[j].rgba_masks);
if (dri2_conf) {
count++;
format_count[j]++;
@@ -1067,7 +1063,7 @@ droid_add_configs_for_visuals(_EGLDriver *drv, _EGLDisplay *dpy)
}
}

- for (i = 0; i < ARRAY_SIZE(format_count); i++) {
+ for (int i = 0; i < ARRAY_SIZE(format_count); i++) {
if (!format_count[i]) {
_eglLog(_EGL_DEBUG, "No DRI config supports native format 0x%x",
visuals[i].format);

--
2.13.0

Chad Versace

2017-06-17 02:37:47 UTC

Permalink

No behavioral change. Just a cleanup.

Post-patch, we no longer modify the same array on each iteration of the
inner loop of droid_add_configs_for_visuals(). Instead, we just declare
the array as const inside the inner loop.
---
src/egl/drivers/dri2/platform_android.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/src/egl/drivers/dri2/platform_android.c b/src/egl/drivers/dri2/platform_android.c
index a5f45a0bfbe..f309fcea11f 100644
--- a/src/egl/drivers/dri2/platform_android.c
+++ b/src/egl/drivers/dri2/platform_android.c
@@ -1034,13 +1034,6 @@ droid_add_configs_for_visuals(_EGLDriver *drv, _EGLDisplay *dpy)
{ HAL_PIXEL_FORMAT_RGB_565, { 0x0000f800, 0x000007e0, 0x0000001f, 0x00000000 } },
{ HAL_PIXEL_FORMAT_BGRA_8888, { 0x00ff0000, 0x0000ff00, 0x000000ff, 0xff000000 } },
};
- EGLint config_attrs[] = {
- EGL_NATIVE_VISUAL_ID, 0,
- EGL_NATIVE_VISUAL_TYPE, 0,
- EGL_FRAMEBUFFER_TARGET_ANDROID, EGL_TRUE,
- EGL_RECORDABLE_ANDROID, EGL_TRUE,
- EGL_NONE
- };

unsigned int format_count[ARRAY_SIZE(visuals)] = { 0 };
int count = 0;
@@ -1049,8 +1042,13 @@ droid_add_configs_for_visuals(_EGLDriver *drv, _EGLDisplay *dpy)
const EGLint surface_type = EGL_WINDOW_BIT | EGL_PBUFFER_BIT;

for (int j = 0; j < ARRAY_SIZE(visuals); j++) {
- config_attrs[1] = visuals[j].format;
- config_attrs[3] = visuals[j].format;
+ const EGLint config_attrs[] = {
+ EGL_NATIVE_VISUAL_ID, visuals[j].format,
+ EGL_NATIVE_VISUAL_TYPE, visuals[j].format,
+ EGL_FRAMEBUFFER_TARGET_ANDROID, EGL_TRUE,
+ EGL_RECORDABLE_ANDROID, EGL_TRUE,
+ EGL_NONE
+ };

struct dri2_egl_config *dri2_conf =
dri2_add_config(dpy, dri2_dpy->driver_configs[i], count + 1,

--
2.13.0

Emil Velikov

2017-06-20 08:43:00 UTC

Permalink

Post by Chad Versace
No behavioral change. Just a cleanup.
Post-patch, we no longer modify the same array on each iteration of the
inner loop of droid_add_configs_for_visuals(). Instead, we just declare
the array as const inside the inner loop.
---
src/egl/drivers/dri2/platform_android.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/src/egl/drivers/dri2/platform_android.c b/src/egl/drivers/dri2/platform_android.c
index a5f45a0bfbe..f309fcea11f 100644
--- a/src/egl/drivers/dri2/platform_android.c
+++ b/src/egl/drivers/dri2/platform_android.c
@@ -1034,13 +1034,6 @@ droid_add_configs_for_visuals(_EGLDriver *drv, _EGLDisplay *dpy)
{ HAL_PIXEL_FORMAT_RGB_565, { 0x0000f800, 0x000007e0, 0x0000001f, 0x00000000 } },
{ HAL_PIXEL_FORMAT_BGRA_8888, { 0x00ff0000, 0x0000ff00, 0x000000ff, 0xff000000 } },
};
- EGLint config_attrs[] = {
- EGL_NATIVE_VISUAL_ID, 0,
- EGL_NATIVE_VISUAL_TYPE, 0,
- EGL_FRAMEBUFFER_TARGET_ANDROID, EGL_TRUE,
- EGL_RECORDABLE_ANDROID, EGL_TRUE,
- EGL_NONE
- };
unsigned int format_count[ARRAY_SIZE(visuals)] = { 0 };
int count = 0;
@@ -1049,8 +1042,13 @@ droid_add_configs_for_visuals(_EGLDriver *drv, _EGLDisplay *dpy)
const EGLint surface_type = EGL_WINDOW_BIT | EGL_PBUFFER_BIT;
for (int j = 0; j < ARRAY_SIZE(visuals); j++) {
- config_attrs[1] = visuals[j].format;
- config_attrs[3] = visuals[j].format;
+ const EGLint config_attrs[] = {
+ EGL_NATIVE_VISUAL_ID, visuals[j].format,
+ EGL_NATIVE_VISUAL_TYPE, visuals[j].format,
+ EGL_FRAMEBUFFER_TARGET_ANDROID, EGL_TRUE,
+ EGL_RECORDABLE_ANDROID, EGL_TRUE,
+ EGL_NONE
+ };

My moving it here, we'll write the hunk i*j times, as opposed to once.
Not a huge deal, but like 3/5 it breaks the consistency with other
platforms' code.

Can we please update all [as one patch or not], please?
Emil

Chad Versace

2017-06-20 17:26:38 UTC

Permalink

Post by Emil Velikov

My moving it here, we'll write the hunk i*j times, as opposed to once.
Not a huge deal, but like 3/5 it breaks the consistency with other
platforms' code.
Can we please update all [as one patch or not], please?
Emil

Sure, I'll resubmit with a patch that touches all drivers.

Chad Versace

2017-06-17 02:37:49 UTC

Permalink

This makes the nested loops in droid_add_configs_for_visuals() easier to
read.

No behavioral change.
---
src/egl/drivers/dri2/platform_android.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/egl/drivers/dri2/platform_android.c b/src/egl/drivers/dri2/platform_android.c
index 9dc2d831b49..fcf29bce713 100644
--- a/src/egl/drivers/dri2/platform_android.c
+++ b/src/egl/drivers/dri2/platform_android.c
@@ -1035,12 +1035,11 @@ droid_add_configs_for_visuals(_EGLDriver *drv, _EGLDisplay *dpy)
{ HAL_PIXEL_FORMAT_BGRA_8888, { 0x00ff0000, 0x0000ff00, 0x000000ff, 0xff000000 } },
};

+ const EGLint surface_type = EGL_WINDOW_BIT | EGL_PBUFFER_BIT;
unsigned int format_count[ARRAY_SIZE(visuals)] = { 0 };
int config_count = 0;

for (int i = 0; dri2_dpy->driver_configs[i]; i++) {
- const EGLint surface_type = EGL_WINDOW_BIT | EGL_PBUFFER_BIT;
-
for (int j = 0; j < ARRAY_SIZE(visuals); j++) {
const EGLint config_attrs[] = {
EGL_NATIVE_VISUAL_ID, visuals[j].format,

--
2.13.0

Chad Versace

2017-06-17 02:37:48 UTC

Permalink

Rename 'config' to 'config_count'. I didn't understand what the variable
did until I untangled the for-loops. Now the next person won't have that
problem.
---
src/egl/drivers/dri2/platform_android.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/egl/drivers/dri2/platform_android.c b/src/egl/drivers/dri2/platform_android.c
index f309fcea11f..9dc2d831b49 100644
--- a/src/egl/drivers/dri2/platform_android.c
+++ b/src/egl/drivers/dri2/platform_android.c
@@ -1036,7 +1036,7 @@ droid_add_configs_for_visuals(_EGLDriver *drv, _EGLDisplay *dpy)
};

unsigned int format_count[ARRAY_SIZE(visuals)] = { 0 };
- int count = 0;
+ int config_count = 0;

for (int i = 0; dri2_dpy->driver_configs[i]; i++) {
const EGLint surface_type = EGL_WINDOW_BIT | EGL_PBUFFER_BIT;
@@ -1051,11 +1051,11 @@ droid_add_configs_for_visuals(_EGLDriver *drv, _EGLDisplay *dpy)
};

struct dri2_egl_config *dri2_conf =
- dri2_add_config(dpy, dri2_dpy->driver_configs[i], count + 1,
- surface_type, config_attrs,
+ dri2_add_config(dpy, dri2_dpy->driver_configs[i],
+ config_count + 1, surface_type, config_attrs,
visuals[j].rgba_masks);
if (dri2_conf) {
- count++;
+ config_count++;
format_count[j]++;
}
}
@@ -1068,7 +1068,7 @@ droid_add_configs_for_visuals(_EGLDriver *drv, _EGLDisplay *dpy)
}
}

- return (count != 0);
+ return (config_count != 0);
}

static int

--
2.13.0

Eric Engestrom

2017-06-21 21:37:50 UTC

Permalink

Post by Chad Versace
Rename 'config' to 'config_count'. I didn't understand what the variable

I think you mean "Rename 'count' to 'config_count'" :)

Agreed with Emil about making the same change across all platforms (one
of these days I'll dedup those, although I kinda hope someone will beat
me to it :)
Fair warning though, this will conflict will a patch of mine; the
slowest one of us will have to resolve it :P

Post by Chad Versace
did until I untangled the for-loops. Now the next person won't have that
problem.
---
src/egl/drivers/dri2/platform_android.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/egl/drivers/dri2/platform_android.c b/src/egl/drivers/dri2/platform_android.c
index f309fcea11f..9dc2d831b49 100644
--- a/src/egl/drivers/dri2/platform_android.c
+++ b/src/egl/drivers/dri2/platform_android.c
@@ -1036,7 +1036,7 @@ droid_add_configs_for_visuals(_EGLDriver *drv, _EGLDisplay *dpy)
};
unsigned int format_count[ARRAY_SIZE(visuals)] = { 0 };
- int count = 0;
+ int config_count = 0;
for (int i = 0; dri2_dpy->driver_configs[i]; i++) {
const EGLint surface_type = EGL_WINDOW_BIT | EGL_PBUFFER_BIT;
@@ -1051,11 +1051,11 @@ droid_add_configs_for_visuals(_EGLDriver *drv, _EGLDisplay *dpy)
};
struct dri2_egl_config *dri2_conf =
- dri2_add_config(dpy, dri2_dpy->driver_configs[i], count + 1,
- surface_type, config_attrs,
+ dri2_add_config(dpy, dri2_dpy->driver_configs[i],
+ config_count + 1, surface_type, config_attrs,
visuals[j].rgba_masks);
if (dri2_conf) {
- count++;
+ config_count++;
format_count[j]++;
}
}
@@ -1068,7 +1068,7 @@ droid_add_configs_for_visuals(_EGLDriver *drv, _EGLDisplay *dpy)
}
}
- return (count != 0);
+ return (config_count != 0);
}
static int
--
2.13.0

Chad Versace

2017-06-17 02:37:50 UTC

Permalink

Many Android apps (such as Google's official NDK GLES2 example app), and
even portions the core framework code (such as SystemServiceManager in
Nougat), incorrectly choose their EGLConfig. They neglect to match the
EGLConfig's EGL_NATIVE_VISUAL_ID against the window's native format, and
instead choose the first EGLConfig whose channel sizes match those of
the native window format while ignoring the channel *ordering*.

We can detect such buggy clients in logcat when they call
eglCreateSurface, by detecting the mismatch between the EGLConfig's
format and the window's format.

As a workaround, this patch changes the order of EGLConfig generation
such that all EGLConfigs for HAL pixel format i precede those for HAL
pixel format i+1. In my (chadversary) testing on Android Nougat, this
was good enough to pacify the buggy clients.
---
src/egl/drivers/dri2/platform_android.c | 32 +++++++++++++++++++++++++-------
1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/src/egl/drivers/dri2/platform_android.c b/src/egl/drivers/dri2/platform_android.c
index fcf29bce713..c294691f291 100644
--- a/src/egl/drivers/dri2/platform_android.c
+++ b/src/egl/drivers/dri2/platform_android.c
@@ -1039,23 +1039,41 @@ droid_add_configs_for_visuals(_EGLDriver *drv, _EGLDisplay *dpy)
unsigned int format_count[ARRAY_SIZE(visuals)] = { 0 };
int config_count = 0;

- for (int i = 0; dri2_dpy->driver_configs[i]; i++) {
- for (int j = 0; j < ARRAY_SIZE(visuals); j++) {
+ /* The nesting of loops is significant here. Also significant is the order
+ * of the HAL pixel formats. Many Android apps (such as Google's official
+ * NDK GLES2 example app), and even portions the core framework code (such
+ * as SystemServiceManager in Nougat), incorrectly choose their EGLConfig.
+ * They neglect to match the EGLConfig's EGL_NATIVE_VISUAL_ID against the
+ * window's native format, and instead choose the first EGLConfig whose
+ * channel sizes match those of the native window format while ignoring the
+ * channel *ordering*.
+ *
+ * We can detect such buggy clients in logcat when they call
+ * eglCreateSurface, by detecting the mismatch between the EGLConfig's
+ * format and the window's format.
+ *
+ * As a workaround, we generate EGLConfigs such that all EGLConfigs for HAL
+ * pixel format i precede those for HAL pixel format i+1. In my
+ * (chadversary) testing on Android Nougat, this was good enough to pacify
+ * the buggy clients.
+ */
+ for (int i = 0; i < ARRAY_SIZE(visuals); i++) {
+ for (int j = 0; dri2_dpy->driver_configs[j]; j++) {
const EGLint config_attrs[] = {
- EGL_NATIVE_VISUAL_ID, visuals[j].format,
- EGL_NATIVE_VISUAL_TYPE, visuals[j].format,
+ EGL_NATIVE_VISUAL_ID, visuals[i].format,
+ EGL_NATIVE_VISUAL_TYPE, visuals[i].format,
EGL_FRAMEBUFFER_TARGET_ANDROID, EGL_TRUE,
EGL_RECORDABLE_ANDROID, EGL_TRUE,
EGL_NONE
};

struct dri2_egl_config *dri2_conf =
- dri2_add_config(dpy, dri2_dpy->driver_configs[i],
+ dri2_add_config(dpy, dri2_dpy->driver_configs[j],
config_count + 1, surface_type, config_attrs,
- visuals[j].rgba_masks);
+ visuals[i].rgba_masks);
if (dri2_conf) {
config_count++;
- format_count[j]++;
+ format_count[i]++;
}
}
}

--
2.13.0

Emil Velikov

2017-06-20 08:32:35 UTC

Permalink

Post by Chad Versace
Many Android apps (such as Google's official NDK GLES2 example app), and
even portions the core framework code (such as SystemServiceManager in
Nougat), incorrectly choose their EGLConfig. They neglect to match the
EGLConfig's EGL_NATIVE_VISUAL_ID against the window's native format, and
instead choose the first EGLConfig whose channel sizes match those of
the native window format while ignoring the channel *ordering*.
We can detect such buggy clients in logcat when they call
eglCreateSurface, by detecting the mismatch between the EGLConfig's
format and the window's format.
As a workaround, this patch changes the order of EGLConfig generation
such that all EGLConfigs for HAL pixel format i precede those for HAL
pixel format i+1. In my (chadversary) testing on Android Nougat, this
was good enough to pacify the buggy clients.
---
src/egl/drivers/dri2/platform_android.c | 32 +++++++++++++++++++++++++-------
1 file changed, 25 insertions(+), 7 deletions(-)
diff --git a/src/egl/drivers/dri2/platform_android.c b/src/egl/drivers/dri2/platform_android.c
index fcf29bce713..c294691f291 100644
--- a/src/egl/drivers/dri2/platform_android.c
+++ b/src/egl/drivers/dri2/platform_android.c
@@ -1039,23 +1039,41 @@ droid_add_configs_for_visuals(_EGLDriver *drv, _EGLDisplay *dpy)
unsigned int format_count[ARRAY_SIZE(visuals)] = { 0 };
int config_count = 0;
- for (int i = 0; dri2_dpy->driver_configs[i]; i++) {
- for (int j = 0; j < ARRAY_SIZE(visuals); j++) {
+ /* The nesting of loops is significant here. Also significant is the order
+ * of the HAL pixel formats. Many Android apps (such as Google's official
+ * NDK GLES2 example app), and even portions the core framework code (such
+ * as SystemServiceManager in Nougat), incorrectly choose their EGLConfig.
+ * They neglect to match the EGLConfig's EGL_NATIVE_VISUAL_ID against the
+ * window's native format, and instead choose the first EGLConfig whose
+ * channel sizes match those of the native window format while ignoring the
+ * channel *ordering*.
+ *
+ * We can detect such buggy clients in logcat when they call
+ * eglCreateSurface, by detecting the mismatch between the EGLConfig's
+ * format and the window's format.
+ *
+ * As a workaround, we generate EGLConfigs such that all EGLConfigs for HAL
+ * pixel format i precede those for HAL pixel format i+1. In my
+ * (chadversary) testing on Android Nougat, this was good enough to pacify
+ * the buggy clients.

Huge thanks for the extensive comment Chad. At the same time - how unfortunate.
I doubt that you/others will have a chance to address the said bugs?

Humble request - can we please make this 1/5 and Cc mesa-stable?

Thanks
Emil

Chad Versace

2017-06-20 17:24:45 UTC

Permalink

Post by Emil Velikov

Huge thanks for the extensive comment Chad. At the same time - how unfortunate.
I doubt that you/others will have a chance to address the said bugs?

There is a chance to fix these bugs in the core Android codebase. But
many buggy apps, of course, are out of our control.

Post by Emil Velikov
Humble request - can we please make this 1/5 and Cc mesa-stable?

Sure, I will re-order the series to place this patch first.

Eric Engestrom

2017-06-21 21:25:30 UTC

Permalink

Post by Chad Versace
Many Android apps (such as Google's official NDK GLES2 example app), and
even portions the core framework code (such as SystemServiceManager in
Nougat), incorrectly choose their EGLConfig. They neglect to match the
EGLConfig's EGL_NATIVE_VISUAL_ID against the window's native format, and
instead choose the first EGLConfig whose channel sizes match those of
the native window format while ignoring the channel *ordering*.
We can detect such buggy clients in logcat when they call
eglCreateSurface, by detecting the mismatch between the EGLConfig's
format and the window's format.
As a workaround, this patch changes the order of EGLConfig generation
such that all EGLConfigs for HAL pixel format i precede those for HAL
pixel format i+1. In my (chadversary) testing on Android Nougat, this
was good enough to pacify the buggy clients.
EGL_NATIVE_VISUAL_TYPE (the actual sort order is
implementation-defined, depending on the meaning of
native visual types).
---
src/egl/drivers/dri2/platform_android.c | 32 +++++++++++++++++++++++++-------
1 file changed, 25 insertions(+), 7 deletions(-)
diff --git a/src/egl/drivers/dri2/platform_android.c b/src/egl/drivers/dri2/platform_android.c
index fcf29bce713..c294691f291 100644
--- a/src/egl/drivers/dri2/platform_android.c
+++ b/src/egl/drivers/dri2/platform_android.c
@@ -1039,23 +1039,41 @@ droid_add_configs_for_visuals(_EGLDriver *drv, _EGLDisplay *dpy)
unsigned int format_count[ARRAY_SIZE(visuals)] = { 0 };
int config_count = 0;
- for (int i = 0; dri2_dpy->driver_configs[i]; i++) {
- for (int j = 0; j < ARRAY_SIZE(visuals); j++) {
+ /* The nesting of loops is significant here. Also significant is the order
+ * of the HAL pixel formats. Many Android apps (such as Google's official
+ * NDK GLES2 example app), and even portions the core framework code (such
+ * as SystemServiceManager in Nougat), incorrectly choose their EGLConfig.
+ * They neglect to match the EGLConfig's EGL_NATIVE_VISUAL_ID against the
+ * window's native format, and instead choose the first EGLConfig whose
+ * channel sizes match those of the native window format while ignoring the
+ * channel *ordering*.
+ *
+ * We can detect such buggy clients in logcat when they call
+ * eglCreateSurface, by detecting the mismatch between the EGLConfig's
+ * format and the window's format.
+ *
+ * As a workaround, we generate EGLConfigs such that all EGLConfigs for HAL
+ * pixel format i precede those for HAL pixel format i+1. In my
+ * (chadversary) testing on Android Nougat, this was good enough to pacify
+ * the buggy clients.
+ */
+ for (int i = 0; i < ARRAY_SIZE(visuals); i++) {
+ for (int j = 0; dri2_dpy->driver_configs[j]; j++) {
const EGLint config_attrs[] = {
- EGL_NATIVE_VISUAL_ID, visuals[j].format,
- EGL_NATIVE_VISUAL_TYPE, visuals[j].format,
+ EGL_NATIVE_VISUAL_ID, visuals[i].format,
+ EGL_NATIVE_VISUAL_TYPE, visuals[i].format,
EGL_FRAMEBUFFER_TARGET_ANDROID, EGL_TRUE,
EGL_RECORDABLE_ANDROID, EGL_TRUE,
EGL_NONE
};
struct dri2_egl_config *dri2_conf =
- dri2_add_config(dpy, dri2_dpy->driver_configs[i],
+ dri2_add_config(dpy, dri2_dpy->driver_configs[j],
config_count + 1, surface_type, config_attrs,
- visuals[j].rgba_masks);
+ visuals[i].rgba_masks);
if (dri2_conf) {
config_count++;
- format_count[j]++;
+ format_count[i]++;
}
}
}
--
2.13.0