Re: [Mesa-dev] [Mesa-stable] [PATCH] egl: Partially revert 23c86c74, fix eglMakeCurrent

2017-05-18 Thread Chad Versace 
On Thu 18 May 2017, Tapani Pälli wrote:
> On 05/18/2017 12:16 AM, Tomasz Figa wrote:
> > Hi,
> > 
> > On Tue, May 16, 2017 at 11:35 PM, Tapani Pälli  
> > wrote:
> > > 
> > > On 05/16/2017 08:10 PM, Chad Versace wrote:
> > > > On Tue 16 May 2017, Tapani Pälli wrote:
> > > > > 
> > > > > 
> > > > > On 05/16/2017 02:04 AM, Chad Versace wrote:
> > > > > > Fixes regressions in Android CtsVerifier.apk on Intel Chrome OS 
> > > > > > devices
> > > > > > due to incorrect error handling in eglMakeCurrent. See below on how 
> > > > > > to
> > > > > > confirm the regression is fixed.
> > > > > > 
> > > > > > This partially reverts
> > > > > > 
> > > > > >commit 23c86c74cc450a23848b85cfe914376caede1cdf
> > > > > >Author:  Chad Versace 
> > > > > >Subject: egl: Emit error when EGLSurface is lost
> > > > > > 
> > > > > > The bad commit added the error handling below. #2 and #3 were right,
> > > > > > but #1 was wrong.
> > > > > > 
> > > > > >1. eglMakeCurrent emits EGL_BAD_CURRENT_SURFACE if the 
> > > > > > calling
> > > > > >   thread has unflushed commands and either previous surface 
> > > > > > is no
> > > > > >   longer valid.
> > > > > > 
> > > > > >2. eglMakeCurrent emits EGL_BAD_NATIVE_WINDOW if either new
> > > > > > surface
> > > > > >   is no longer valid.
> > > > > > 
> > > > > >3. eglSwapBuffers emits EGL_BAD_NATIVE_WINDOW if the swapped
> > > > > > surface
> > > > > >   is no longer valid.
> > > > > > 
> > > > > > Whe I wrote the bad commit, I misunderstood the EGL spec language
> > > > > > for #1. The correct behavior is, if I understand correctly now:
> > > > > > 
> > > > > >- Assume a bound EGLSurface is no longer valid.
> > > > > >- Assume the bound EGLContext has unflushed commands.
> > > > > >- The app calls eglMakeCurrent. The spec requires 
> > > > > > eglMakeCurrent
> > > > > > to
> > > > > >  implicitly flush. After flushing, eglMakeCurrent emits
> > > > > >  EGL_BAD_CURRENT_SURFACE and does *not* alter the thread's
> > > > > >  current bindings.
> > > > > >- If the app calls eglMakeCurrent again, and the app inserts 
> > > > > > no
> > > > > >  commands into the GL command stream between the two
> > > > > > eglMakeCurrent
> > > > > >  calls, then this second eglMakeCurrent succeeds without 
> > > > > > emitting
> > > > > > an
> > > > > >  error.
> > > > > > 
> > > > > > How to confirm this fixes the regression:
> > > > > > 
> > > > > >Download android-cts-verifier-7.1_r5-linux_x86-x86.zip from
> > > > > >source.android.com, unpack, and `adb install 
> > > > > > CtsVerifier.apk`.
> > > > > >Run test "Projection Cube". Click the Pass button (a
> > > > > >green checkmark). Then run test "Projection Widget". Confirm 
> > > > > > that
> > > > > >widgets are visible and that logcat does not complain about
> > > > > >eglMakeCurrent failure.
> > > > > > 
> > > > > >Then confirm there are no regressions in the cts-traded 
> > > > > > module
> > > > > > that
> > > > > >commit 263243b1 fixed:
> > > > > > 
> > > > > >cts-tf > run cts --skip-preconditions --skip-device-info 
> > > > > > \
> > > > > > -m CtsCameraTestCases \
> > > > > > -t android.hardware.camera2.cts.RobustnessTest
> > > > > > 
> > > > > >Tested with Chrome OS board "reef".
> > > > > 
> > > > > both tests passed on Android-IA with this patch ... but if I minimize
> > > > > "Projection Widget" test it starts to bang EGL_BAD_NATIVE_WINDOW 
> > > > > heavily.
> > > > > Is
> > > > > this expected behavior?
> > > > 
> > > > I'm unsure. I haven't yet tried that experiment. I'll give it a try when
> > > > I'm back at my desk.
> > > > 
> > > > Which EGL function is emitting EGL_BAD_NATIVE_WINDOW in logcat?
> > > > eglMakeCurrent or eglSwapBuffers, or something else?
> > > 
> > > doh sorry, I actually meant 'Projection Cube' test, not the widget test. 
> > > But
> > > yep, when tapping switcher button eglMakeCurrent starts to emit the error
> > > (likely the app just continues to submit frames even when it's
> > > 'minimized'?). When switching back to app it cannot draw anymore as it 
> > > keeps
> > > getting the error.
> > I don't remember exactly and I'm not an Android app expert either, but
> > I remember hearing something that the app needs to stop drawing when
> > the activity is paused (stopped?). Isn't that what's happening when an
> > app gets minimized?
> 
> Yep, this is very likely the case and the app here is just not obeying the
> rules. Considering that Android wraps EGL layer it seems weird that it just
> let's application to do this. Any case, since everything else works fine
> this is definitely not a blocker for me for this patch;
> 
> Acked-by: Tapani Pälli 

Tapani, thanks for

Re: [Mesa-dev] [Mesa-stable] [PATCH] egl: Partially revert 23c86c74, fix eglMakeCurrent

2017-05-18 Thread Tapani Pälli 

On 05/18/2017 12:16 AM, Tomasz Figa wrote:

Hi,

On Tue, May 16, 2017 at 11:35 PM, Tapani Pälli  wrote:


On 05/16/2017 08:10 PM, Chad Versace wrote:

On Tue 16 May 2017, Tapani Pälli wrote:



On 05/16/2017 02:04 AM, Chad Versace wrote:

Fixes regressions in Android CtsVerifier.apk on Intel Chrome OS devices
due to incorrect error handling in eglMakeCurrent. See below on how to
confirm the regression is fixed.

This partially reverts

   commit 23c86c74cc450a23848b85cfe914376caede1cdf
   Author:  Chad Versace 
   Subject: egl: Emit error when EGLSurface is lost

The bad commit added the error handling below. #2 and #3 were right,
but #1 was wrong.

   1. eglMakeCurrent emits EGL_BAD_CURRENT_SURFACE if the calling
  thread has unflushed commands and either previous surface is no
  longer valid.

   2. eglMakeCurrent emits EGL_BAD_NATIVE_WINDOW if either new
surface
  is no longer valid.

   3. eglSwapBuffers emits EGL_BAD_NATIVE_WINDOW if the swapped
surface
  is no longer valid.

Whe I wrote the bad commit, I misunderstood the EGL spec language
for #1. The correct behavior is, if I understand correctly now:

   - Assume a bound EGLSurface is no longer valid.
   - Assume the bound EGLContext has unflushed commands.
   - The app calls eglMakeCurrent. The spec requires eglMakeCurrent
to
 implicitly flush. After flushing, eglMakeCurrent emits
 EGL_BAD_CURRENT_SURFACE and does *not* alter the thread's
 current bindings.
   - If the app calls eglMakeCurrent again, and the app inserts no
 commands into the GL command stream between the two
eglMakeCurrent
 calls, then this second eglMakeCurrent succeeds without emitting
an
 error.

How to confirm this fixes the regression:

   Download android-cts-verifier-7.1_r5-linux_x86-x86.zip from
   source.android.com, unpack, and `adb install CtsVerifier.apk`.
   Run test "Projection Cube". Click the Pass button (a
   green checkmark). Then run test "Projection Widget". Confirm that
   widgets are visible and that logcat does not complain about
   eglMakeCurrent failure.

   Then confirm there are no regressions in the cts-traded module
that
   commit 263243b1 fixed:

   cts-tf > run cts --skip-preconditions --skip-device-info \
-m CtsCameraTestCases \
-t android.hardware.camera2.cts.RobustnessTest

   Tested with Chrome OS board "reef".


both tests passed on Android-IA with this patch ... but if I minimize
"Projection Widget" test it starts to bang EGL_BAD_NATIVE_WINDOW heavily.
Is
this expected behavior?


I'm unsure. I haven't yet tried that experiment. I'll give it a try when
I'm back at my desk.

Which EGL function is emitting EGL_BAD_NATIVE_WINDOW in logcat?
eglMakeCurrent or eglSwapBuffers, or something else?


doh sorry, I actually meant 'Projection Cube' test, not the widget test. But
yep, when tapping switcher button eglMakeCurrent starts to emit the error
(likely the app just continues to submit frames even when it's
'minimized'?). When switching back to app it cannot draw anymore as it keeps
getting the error.

I don't remember exactly and I'm not an Android app expert either, but
I remember hearing something that the app needs to stop drawing when
the activity is paused (stopped?). Isn't that what's happening when an
app gets minimized?


Yep, this is very likely the case and the app here is just not obeying 
the rules. Considering that Android wraps EGL layer it seems weird that 
it just let's application to do this. Any case, since everything else 
works fine this is definitely not a blocker for me for this patch;


Acked-by: Tapani Pälli 


Best regards,
Tomasz


What was the behavior before this patch? And before
commit 23c86c74 (egl: Emit error when EGLSurface is lost)?


Without this patch (and before commit 23c86c74) I can minimize app to
switcher and bring it back and it continues to draw.

Note that everything else seems to work just fine though (tested lots of GL
apps) so is this just something specific what cts-verifier is doing or not
handling properly?




Cc: "17.1" 
Cc: Tomasz Figa 
Cc: Nicolas Boichat 
Cc: Emil Velikov 
Fixes: 23c86c74 (egl: Emit error when EGLSurface is lost)
---
src/egl/main/eglapi.c | 19 ---
1 file changed, 19 deletions(-)

diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c
index aa0eb94666..9cea2f41ff 100644
--- a/src/egl/main/eglapi.c
+++ b/src/egl/main/eglapi.c
@@ -828,25 +828,6 @@ eglMakeCurrent(EGLDisplay dpy, EGLSurface draw,
EGLSurface read,
 RETURN_EGL_ERROR(disp, EGL_BAD_MATCH, EGL_FALSE);
   }
-   _EGLThreadInfo *t =_eglGetCurrentThread();
-   _EGLContext *old_ctx = t->CurrentContext;
-

Re: [Mesa-dev] [Mesa-stable] [PATCH] egl: Partially revert 23c86c74, fix eglMakeCurrent

2017-05-17 Thread Tomasz Figa 
Hi,

On Tue, May 16, 2017 at 11:35 PM, Tapani Pälli  wrote:
>
>
> On 05/16/2017 08:10 PM, Chad Versace wrote:
>>
>> On Tue 16 May 2017, Tapani Pälli wrote:
>>>
>>>
>>>
>>> On 05/16/2017 02:04 AM, Chad Versace wrote:

 Fixes regressions in Android CtsVerifier.apk on Intel Chrome OS devices
 due to incorrect error handling in eglMakeCurrent. See below on how to
 confirm the regression is fixed.

 This partially reverts

   commit 23c86c74cc450a23848b85cfe914376caede1cdf
   Author:  Chad Versace 
   Subject: egl: Emit error when EGLSurface is lost

 The bad commit added the error handling below. #2 and #3 were right,
 but #1 was wrong.

   1. eglMakeCurrent emits EGL_BAD_CURRENT_SURFACE if the calling
  thread has unflushed commands and either previous surface is no
  longer valid.

   2. eglMakeCurrent emits EGL_BAD_NATIVE_WINDOW if either new
 surface
  is no longer valid.

   3. eglSwapBuffers emits EGL_BAD_NATIVE_WINDOW if the swapped
 surface
  is no longer valid.

 Whe I wrote the bad commit, I misunderstood the EGL spec language
 for #1. The correct behavior is, if I understand correctly now:

   - Assume a bound EGLSurface is no longer valid.
   - Assume the bound EGLContext has unflushed commands.
   - The app calls eglMakeCurrent. The spec requires eglMakeCurrent
 to
 implicitly flush. After flushing, eglMakeCurrent emits
 EGL_BAD_CURRENT_SURFACE and does *not* alter the thread's
 current bindings.
   - If the app calls eglMakeCurrent again, and the app inserts no
 commands into the GL command stream between the two
 eglMakeCurrent
 calls, then this second eglMakeCurrent succeeds without emitting
 an
 error.

 How to confirm this fixes the regression:

   Download android-cts-verifier-7.1_r5-linux_x86-x86.zip from
   source.android.com, unpack, and `adb install CtsVerifier.apk`.
   Run test "Projection Cube". Click the Pass button (a
   green checkmark). Then run test "Projection Widget". Confirm that
   widgets are visible and that logcat does not complain about
   eglMakeCurrent failure.

   Then confirm there are no regressions in the cts-traded module
 that
   commit 263243b1 fixed:

   cts-tf > run cts --skip-preconditions --skip-device-info \
-m CtsCameraTestCases \
-t android.hardware.camera2.cts.RobustnessTest

   Tested with Chrome OS board "reef".
>>>
>>>
>>> both tests passed on Android-IA with this patch ... but if I minimize
>>> "Projection Widget" test it starts to bang EGL_BAD_NATIVE_WINDOW heavily.
>>> Is
>>> this expected behavior?
>>
>>
>> I'm unsure. I haven't yet tried that experiment. I'll give it a try when
>> I'm back at my desk.
>>
>> Which EGL function is emitting EGL_BAD_NATIVE_WINDOW in logcat?
>> eglMakeCurrent or eglSwapBuffers, or something else?
>
>
> doh sorry, I actually meant 'Projection Cube' test, not the widget test. But
> yep, when tapping switcher button eglMakeCurrent starts to emit the error
> (likely the app just continues to submit frames even when it's
> 'minimized'?). When switching back to app it cannot draw anymore as it keeps
> getting the error.

I don't remember exactly and I'm not an Android app expert either, but
I remember hearing something that the app needs to stop drawing when
the activity is paused (stopped?). Isn't that what's happening when an
app gets minimized?

Best regards,
Tomasz

>
>> What was the behavior before this patch? And before
>> commit 23c86c74 (egl: Emit error when EGLSurface is lost)?
>
>
> Without this patch (and before commit 23c86c74) I can minimize app to
> switcher and bring it back and it continues to draw.
>
> Note that everything else seems to work just fine though (tested lots of GL
> apps) so is this just something specific what cts-verifier is doing or not
> handling properly?
>
>
>
 Cc: "17.1" 
 Cc: Tomasz Figa 
 Cc: Nicolas Boichat 
 Cc: Emil Velikov 
 Fixes: 23c86c74 (egl: Emit error when EGLSurface is lost)
 ---
src/egl/main/eglapi.c | 19 ---
1 file changed, 19 deletions(-)

 diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c
 index aa0eb94666..9cea2f41ff 100644
 --- a/src/egl/main/eglapi.c
 +++ b/src/egl/main/eglapi.c
 @@ -828,25 +828,6 @@ eglMakeCurrent(EGLDisplay dpy, EGLSurface draw,
 EGLSurface read,
 RETURN_EGL_ERROR(disp, EGL_BAD_MATCH, EGL_FALSE);
   }
 -   _EGLThreadInfo *t