Re: [Mesa-dev] [PATCH] egl/android: Change order of EGLConfig generation (v2)
On Thu 22 Jun 2017, Rob Herring wrote: > On Tue, Jun 20, 2017 at 5:03 PM, Chad Versace> wrote: > > 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. > > > > v2: Rebase to make patch cherry-pickable to stable. > > > > Cc: mesa-sta...@lists.freedesktop.org > > Cc: Tomasz Figa > > Cc: Rob Herring > > Cc: Tapani Pälli > > Cc: Emil Velikov > > Tested-by: Rob Herring Already pushed... > > --- > > 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 5550f580a80..eaaeff4641c 100644 > > --- a/src/egl/drivers/dri2/platform_android.c > > +++ b/src/egl/drivers/dri2/platform_android.c > > @@ -1049,20 +1049,38 @@ droid_add_configs_for_visuals(_EGLDriver *drv, > > _EGLDisplay *dpy) > > unsigned int format_count[ARRAY_SIZE(visuals)] = { 0 }; > > int count, i, 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. > > How do we find the buggy clients with this patch applied other than reverting? It's possible to detect most buggy client behavior within Mesa, but it would require sniffing the EGL calls. In the simplest case, if (((the client calls eglChooseConfig without filtering on EGL_NATIVE_VISUAL_ID) || (the client calls eglGetConfigs)) && (the client never calls eglGetConfigAttrib(EGL_NATIVE_VISUAL_ID)) && (the client calls eglCreateWindowSurface)) { the client is buggy; } If you wanted to get fancier, so as to detect more buggy clients, you could trace the above conditions per-EGLDisplay, per-EGLConfig. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] egl/android: Change order of EGLConfig generation (v2)
On Tue, Jun 20, 2017 at 5:03 PM, Chad Versacewrote: > 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. > > v2: Rebase to make patch cherry-pickable to stable. > > Cc: mesa-sta...@lists.freedesktop.org > Cc: Tomasz Figa > Cc: Rob Herring > Cc: Tapani Pälli > Cc: Emil Velikov Tested-by: Rob Herring One question though... > > --- > 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 5550f580a80..eaaeff4641c 100644 > --- a/src/egl/drivers/dri2/platform_android.c > +++ b/src/egl/drivers/dri2/platform_android.c > @@ -1049,20 +1049,38 @@ droid_add_configs_for_visuals(_EGLDriver *drv, > _EGLDisplay *dpy) > unsigned int format_count[ARRAY_SIZE(visuals)] = { 0 }; > int count, i, 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. How do we find the buggy clients with this patch applied other than reverting? Rob ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] egl/android: Change order of EGLConfig generation (v2)
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. v2: Rebase to make patch cherry-pickable to stable. Cc: mesa-sta...@lists.freedesktop.org Cc: Tomasz FigaCc: Rob Herring Cc: Tapani Pälli Cc: Emil Velikov --- 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 5550f580a80..eaaeff4641c 100644 --- a/src/egl/drivers/dri2/platform_android.c +++ b/src/egl/drivers/dri2/platform_android.c @@ -1049,20 +1049,38 @@ droid_add_configs_for_visuals(_EGLDriver *drv, _EGLDisplay *dpy) unsigned int format_count[ARRAY_SIZE(visuals)] = { 0 }; int count, i, 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. +*/ count = 0; - for (i = 0; dri2_dpy->driver_configs[i]; i++) { + for (i = 0; i < ARRAY_SIZE(visuals); 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++) { - config_attrs[1] = visuals[j].format; - config_attrs[3] = visuals[j].format; + for (j = 0; dri2_dpy->driver_configs[j]; j++) { + config_attrs[1] = visuals[i].format; + config_attrs[3] = visuals[i].format; - dri2_conf = dri2_add_config(dpy, dri2_dpy->driver_configs[i], - count + 1, surface_type, config_attrs, visuals[j].rgba_masks); + dri2_conf = dri2_add_config(dpy, dri2_dpy->driver_configs[j], + count + 1, surface_type, config_attrs, visuals[i].rgba_masks); if (dri2_conf) { count++; -format_count[j]++; +format_count[i]++; } } } -- 2.12.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev