[PATCH] Android: Port eglretracer for Android

[Chad Versace]

On 07/23/2013 08:28 AM, Alexander Monakov wrote:
> On Tue, 23 Jul 2013, Juha-Pekka Heikkilä wrote:
>>> Why do you need Waffle to handle symbol lookups for Apitrace?  As far as I
>>> understand, you only need the surface+context handling from Waffle, and
>>> Apitrace's own dynamic loading code should be sufficient.  Can you drop
>>> all
>>> code that references waffle_gl_api from this patch?
>>
>> If the intention is to go Waffle way for Android why would I go
>> sideloading with different APIs?
>
> I've asked the code to be removed because the way it introduces waffle_dl_* is
> very inelegant and does not appear to add anything useful.  If there are
> reasons you want Waffle's dynamic loading be used in Apitrace, please make
> that a separate patch, but for retracing just the glws_waffle.cpp code should
> be sufficient.

I agree with Alexander here. Apitrace already has code to handle dynamic loading,
and I would prefer if the Apitrace Waffle codepaths used the same dynamic loading
as the other parts of Apitrace.

>>> Can this code go into the constructor instead and the static method be
>>> removed?
>>
>> I prefer the two stage constructor so I don't need to start throwing
>> exceptions. All waffle functions can fail so I take them the C-ish way.
>
> But you don't have any recovery code, you just immediately terminate the
> application with assert(0) when Waffle fails.  You can do that in the
> constructor as well.

Are exceptions enabled in Apitrace? That is, is Apitrace compiled with -fno-exceptions?
Do other components of Apitrace throw exceptions? What is the common pattern in Apitrace
for construction failure? (I'm not an Apitrace developer, so these are all open questions
to me).

For the sake of uniformity with the rest of the Apitrace codebase, I feel taht glws_waffle should
attempt to follow the same patterns used elsewhere in Apitrace. However, I don't know what
those patterns are.

>>> The comment basically says that Waffle's make_current(NULL, NULL) has a
>>> totally reasonable and expected behavior.  Combined with the lack of code
>>> comments in the rest of the file, this creates a somewhat comical effect,
>>> at
>>> least to me. :)  No offence meant, just pointing out the imbalance.
>>
>> No offence taken :) The thing here is that..
>>
>> waffle_make_current(dpy, NULL, NULL);
>>
>> ..look really straightforward as if would just do..
>>
>> eglMakeCurrent(dpy, NULL, NULL, NULL);
>>
>> ..but it does more than that and is platform dependant. This is
>> undocumented behaviour within Waffle and looking at the Waffle code I do
>> think it should be underlined here is used something which was not
>> promised.
>
> Can you ask Chad to document that, because, honestly, I don't see how Waffle's
> make_current(?, NULL, NULL) could possibly have some effect other than "unbind
> current context from the current thread".

That behavior is documented in the waffle_make_current manpage, both online [1] and in
the installed real manpages.

   "To unbind the current context without binding a new one, set window and context to NULL."

[1] http://people.freedesktop.org/~chadversary/waffle/man/waffle_make_current.3.html

So, I agree that the comment should be removed.