[PATCH 8/9] optionally track the GL state manually

[Imre Deak]

Hi,

On Thu, 2012-07-26 at 10:05 +0100, José Fonseca wrote:
> On Wed, Apr 11, 2012 at 4:19 PM, Imre Deak <imre.deak at intel.com> wrote:
> > At the moment apitrace uses GL/GLES calls to querry the current GL status.
> > This might not work if the underlying implementation doesn't support
> > these calls. This is the case at least with the Android software GL
> > implementation, where the following calls/arguments are not implemented:
> >
> > - glGetIntegerv with the following pname parameters, with for example
> >   GL_VERTEX_ARRAY_BUFFER_BINDING and in general any GL_*_ARRAY_BUFFER_BINDING
> >   pname parameter.
> >
> > - glEnabled with several cap parameters.
> >
> > Also glGetBufferSubData is not supported by GLES, so we need to track
> > buffer contents, at least in some cases.
> >
> > This patch won't provide tracking for the full state, only for those
> > parts that are essential for tracing GLES applications on Android. For
> > the rest fall back to calling the the platform's GL function as it's
> > used to be.
> 
> Imre,
> 
> I've been looking at the latest version of your "android manual
> tracking" patches in more detail, but there are still some issues that
> need more attention, so I'll need to tax your patience a little more.

No problem, I'm relatively new to apitrace and also in the process of
learning all the GL/GL(ES) peculiarities so all feedback from you / the
list is much appreciated.

> - what state needs to be tracked to work around android bugs? what
> state needs to be tracked to work around gles limitations?
> 
>    My understanding is that :
> 
>       - >  GL_VERTEX_ARRAY_BUFFER_BINDING/  GL_*_ARRAY_BUFFER_BINDING,
>  glEnabled  are android bugs
> 
>       -> glGetBufferSubData are GLES limitations.

Yes, this is correct.

>    Also please verify that the workarounds for android bugs are still
> necessary with latest emulator.

Ok. I finally managed to setup my Jelly Bean environment and got
apitrace working there too. I already see that some of the workarounds
will still be needed (GL_MAX_TEXTURE_UNITS returning 0 for example). I
will check the status of the rest just for our reference.

> My understanding is that emulator is
> open source, so I'd prefer that those issues were fixed there, as
> these workarounds really complicate apitrace for all platforms.

Yes, I fully agree. I will check the process of submitting patches to
Google and send the fixes to them. I guess we should still have the
workarounds for people who want to trace older Android versions though.

>     Concerning glGetBufferSubData, could we fallback to MapBufferOES instead?

MapBufferOES isn't supported by GLES :(

> -  We should only work  around andoird bugs on andoird, workaround
> gles limiitations on all gles contexts (even outside android)  that
> is, we should have helper Context methods like
> 
>     inline bool
>     gltrace:::Context::needsShadowBuffers(void) const {
>         return profile == PROFILE_ES1 || profile == PROFILE_ES2;
>     }
> 
>     inline bool
>     gltrace:::Context::needsShadowState(void) const {
> #ifdef ANDROID
>         return true;
> #else
>      return false
> #endif
>     }
> 
> and then the generated code should do
> 
>   if (ctx->needsShadowBuffers()) {
>     /* keep state shadow */
>     ...
>   }

Ok, will create these.

> in generate I'd prefer the term "shadow" instead of "manual" to refer
> to the state we need to keep ourselves.

Ok.

> 
> -  glDeleteBuffers is munging the "n" argument. That means that the
> call for the real glDeleteBuffers will always pass n=0, and buffer
> objects leak.

Oops, true. Will fix this.

> - gl_buffer should be named "Buffer. Also, please use c++
> constructor/destructor and other methods, so that the generated code
> smaller and  easier to understand

Ok.


> - please hook into glGenBuffers, and allocate our shadow buffers  there

Ok. That is allocation for the bookkeeping, while allocating for the
data itself still lazily eg. in glBufferSubData.

> Finally, the state shadowing would be useful for retrace too.
> 
> This means that all state shadowing should be in a layer immediately
> above dispatch so it can be reused everywhere.

Right, one useful case is retracing on Android, I didn't think of that.
I'll make the shadowing be wrappers around the dispatch functions
instead of what I have now in the _apitrace_XXX functions.

Thanks for the review,
Imre