Re: [AD] android_set_current_display

[Jon Rafkind <workmin@xxxxxxxxxx>]

Trent noticed that updating the opengl_view in set_current_display could mess up the users transforms. A simpler solution is just not to set the target_backbuffer in _al_android_setup_opengl_view. I don't think any functions called from there require a target bitmap to be set.

On 11/29/2012 03:56 PM, Jon Rafkind wrote:
> There is more to the story here. The display->render_state.write_mask was not set appropriately before the opengl state was set. This is because the flow of the system is
>
> al_create_display
> android_create_display
> _al_android_setup_opengl_view
> al_set_target_backbuffer
> al_set_target_bitmap
> android_set_current_display
> _al_ogl_update_render_state
>
> And finally in _al_ogl_update_render_state is a call to glColorMask based on display->render_state.write_mask. But the write_mask is not set until after al_create_display calls android_create_display which is after that entire chain of events. Somehow android_set_current_display is not called again so the ogl state is never updated with the proper write mask.
>
> I tried moving the call to _al_android_setup_opengl_view out of android_create_display and into android_set_current_display and it works. I don't know if this is the proper solution but heres a patch anyway.
>
> BTW I noticed that android_acknowledge_drawing_resume could probably just call android_set_current_display instead of clear_current/make_current/setup_opengl_view.
>
> diff --git a/src/android/android_display.c b/src/android/android_display.c
> index 8ced6c4..a760334 100644
> --- a/src/android/android_display.c
> +++ b/src/android/android_display.c
> @@ -538,8 +538,6 @@ static ALLEGRO_DISPLAY *android_create_display(int w, int h)
>     _al_android_clear_current(_al_android_get_jnienv(), d);
>     _al_android_make_current(_al_android_get_jnienv(), d);
>    
> -   _al_android_setup_opengl_view(display);
> -
>     /* Don't need to repeat what this does */
>     android_change_display_option(display, ALLEGRO_SUPPORTED_ORIENTATIONS, al_get_new_display_option(ALLEGRO_SUPPORTED_ORIENTATIONS, NULL));
>  
> @@ -597,6 +595,7 @@ static bool android_set_current_display(ALLEGRO_DISPLAY *dpy)
>     ALLEGRO_DEBUG("make current %p", dpy);
>     if (dpy) _al_android_make_current(_al_android_get_jnienv(), (ALLEGRO_DISPLAY_ANDROID*)dpy);
>  
> +   _al_android_setup_opengl_view(dpy);
>     _al_ogl_update_render_state(dpy);
>  
>     return true;
>
>
> On 11/05/2012 10:38 AM, Thomas Fjellstrom wrote:
>> On Mon Nov 5, 2012, Elias Pschernig wrote:
>>> On Mon, Nov 5, 2012 at 9:35 AM, Jon Rafkind <workmin@xxxxxxxxxx> wrote:
>>>> There is a bug in android_set_current_display where the new display will
>>>> never be set properly. The short story is that it first checks if
>>>> al_get_current_display() == dpy, where dpy is the argument to the
>>>> function, and if they are equal then the function aborts (presumably to
>>>> save time). But these two things will always be equal.
>>>>
>>>> With this client code:
>>>>   ALLEGRO_BITMAP * old = al_get_target_bitmap();
>>>>   al_set_target_bitmap(NULL);
>>>>   al_set_target_bitmap(old);
>>>>
>>>> The old->display will be the current active display. Setting the target
>>>> to NULL will unset the current display. Then setting the target back to
>>>> old will try to reset the display to old->display.
>>>>
>>>> The logic Allegro follows is
>>>>
>>>> al_set_target_bitmap(old)
>>>> // tls.c: al_set_target_bitmap
>>>> ...
>>>> if (old_display != new_display){
>>>>
>>>>    ...
>>>>    tls->current_display = new_display;
>>>>    new_display->vt->set_current_display(new_display)
>>>>    
>>>>    // android/android_display.c:android_set_current_display
>>>>    if (al_get_current_display() == dpy) return true;
>>>>
>>>> Where al_get_current_display() is
>>>>
>>>> // tls.c:al_get_current_display
>>>>
>>>>  return tls->current_display;
>>>>
>>>> So al_get_current_display() will always be equal to the dpy parameter to
>>>> android_set_current_display. By simply commenting out the check in
>>>> android_set_current_display() and letting it always set the display
>>>> (with make_current) my application works as expected. I guess I should
>>>> mention that this bug eventually causes all opengl calls to fail because
>>>> there is no context for them.
>>> Other ports have no such check - so why does the Android port have it?
>> 2 possibilities, the tls function was changed after the fact, or potentially 
>> more likely, I was in my "CHECK ALL THE INPUTS" mode, when it wasn't wanted or 
>> desired. (or for that matter, correct)
>>
>>> ---------------------------------------------------------------------------
>>> --- LogMeIn Central: Instant, anywhere, Remote PC access and management.
>>> Stay in control, update software, and manage PCs from one command center
>>> Diagnose problems and improve visibility into emerging IT issues
>>> Automate, monitor and manage. Do more in less time with Central
>>> http://p.sf.net/sfu/logmein12331_d2d
>
> ------------------------------------------------------------------------------
> Keep yourself connected to Go Parallel: 
> VERIFY Test and improve your parallel project with help from experts 
> and peers. http://goparallel.sourceforge.net