Question: Is 'volatile' redundant in RecordFBOActivity.RenderThread.mHandler field?

[hsav]

Hi,
First of all I have to say that grafika really helped me improving my understanding on how OpenGLES and android work together, so thanks for all the great work!

In my question: I 've been been playing recently with RecordFBOActivity class, and I 've noticed that in the render thread the RenderThread.mHandler field is declared volatile due to thread visibility concerns (expressed in the comment below):

private static class RenderThread extends Thread {
        // Object must be created on render thread to get correct Looper, but is used from
        // UI thread, so we need to declare it volatile to ensure the UI thread sees a fully
        // constructed object.
        private volatile RenderHandler mHandler;

I believe however that the volatile declaration is redundant because of the way the two threads work together during the render thread initialization. More specifically:

• In RecordFBOActivity.surfaceCreated() the UI thread creates the render thread, starts it and waits until the render thread is ready:

mRenderThread = new RenderThread(...);
mRenderThread.setName("RecordFBO GL render");
mRenderThread.start();
mRenderThread.waitUntilReady();  // synchronizes on mRenderThread.mStartLock

• In RenderThread.run() the mHandler is assigned and then synchronizes on the same mStartLock lock the UI thread is waiting for.

public void run() {
    Looper.prepare();
    mHandler = new RenderHandler(this);
    ...
    synchronized (mStartLock) {
        mReady = true;
        mStartLock.notify();    // signal waitUntilReady()
    }

I would expect that when the UI thread resumes after waitUntilReady() the visibility of the mHandler field is guaranteed (the assignment should be visible due to happens-before relationship), so we do not need the volatile declaration. As far as the UI thread is concerned it should never be able to see a null value for the mHandler field (the field is not re-assigned anywhere else).
If the above is true, then all calls in the UI thread that check if getHandler() returns a null value, are redundant too and could be removed simplifying the code a little bit.

Is the above understanding correct or am I missing something here?

Thanks in advance

[fadden]

I think your reasoning is correct -- mReady is tested and set with the lock held, so ordering should be guaranteed.

A check for null is strictly necessary in at least the View UI callbacks, since in theory somebody could click on a UI element before the surface creation completes (which requires a round-trip to SurfaceFlinger by way of the window manager). However, the existing check is incorrect: the check for null should be on mRenderThread, rather than the return value of getHandler().

If you look at TextureFromCameraActivity, which was written later, functions like onProgressChanged() do the correct null check. However, it does have the same volatile declaration, with copy & pasted comment. HardwareScalerActivity was written at about the same time as RecordFBOActivity, and has the same faults.

My guess is that the volatile declaration and null checks are left over from a previous iteration of the code (not checked in) that didn't have waitUntilReady(), and so had to deal with situations where initialization wasn't complete. Without the ready check, mHandler would have to be volatile, and getHandler() could return null. It's hard to remember that far back, but looking at the git history suggests that I was doing a lot of experiments to figure out to get the SurfaceView and Activity lifecycles to peacefully co-exist, so this probably got rewritten a few times and was never fully cleaned up.

[hsav]

I see, that makes sense. I did thought that changes left from previous code versions could probably be the case, I just wanted to verify my understanding.

I am investigating the idea of creating a version of the RenderThread class that can be re-used in my future projects so in order to do that I have to delegate any project specific code to an external Renderer class (pretty much like GLSurfaceView does).

Regarding the co-existence of activity and surface life cycles I can only imagine. I have some experience myself from an Android game I have worked on. Even though I have used GLSurfaceView I have faced similar issues. The approach I ended-up with at the time, was based on an nvidia article where the state was tracked with a set of flags. The basic idea is to set a flag for each different state (e.g. the resume/pause flag, the surface created/destroyed flag etc). This way you don't make any assumptions about the order of the callbacks and you only check the state when it is time to render. I can say it worked pretty well in my case so I am wondering if I can apply the same principles with the RenderThread.

Anyway, thanks for answering my question and again for the great work.
If I manage to come up with a useful implementation I will let you know (I am willing to share it in case you are interested).