Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added code to stop gl thread #8871

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Devashishbasu
Copy link
Contributor

fixes #7194

I added fields to hold the renderer and the GL thread.

In onPause(), I interrupt the GL thread if it's not null.

In onResume(), I start a new GL thread only if the renderer is not null.

I added a setRenderer() method to set the renderer externally.

I introduced a reset() method, annotated with @resetter, which you can use to clean up resources and terminate the GL thread if needed after each test.

@Devashishbasu Devashishbasu marked this pull request as ready for review February 25, 2024 18:36
@utzcoz
Copy link
Member

utzcoz commented Feb 27, 2024

@Devashishbasu Please check CI jobs and fix failures.

@utzcoz
Copy link
Member

utzcoz commented Feb 27, 2024

If you're a new contributor, it's recommended to run related tasks like building, running tests, formatting changes, and etc. It can help reduce extra discussion for simple changes.

@utzcoz utzcoz requested a review from hoisie February 27, 2024 04:46
@Devashishbasu
Copy link
Contributor Author

@Devashishbasu Please check CI jobs and fix failures.

i have tried Another option to terminate the thread in callOnDetachFromWindow...could you please review...build was successful in local

@utzcoz
Copy link
Member

utzcoz commented Feb 27, 2024

@Devashishbasu Please squash your commits to one as Robolectric prefers single commit PR for many occasions.

@Devashishbasu Devashishbasu force-pushed the Calling-GLSurfaceView-stops-the-created-gl-thread branch 2 times, most recently from 9c5157b to 0f1b308 Compare February 27, 2024 05:40
@Devashishbasu
Copy link
Contributor Author

@Devashishbasu Please squash your commits to one as Robolectric prefers single commit PR for many occasions.

Done

@hoisie
Copy link
Contributor

hoisie commented Feb 27, 2024

Hi @Devashishbasu ,

Thanks for this PR. I believe the problem is that GLSurfaceView starts a thread when setRenderer is called:

https://cs.android.com/android/platform/superproject/main/+/main:frameworks/base/opengl/java/android/opengl/GLSurfaceView.java;l=360

This thread is contained in the GLSurfaceView.mGLThread member. There is currently nothing stopping that thread. This PR creates a new thread member in the shadow and stops that, but that isn't really doing anything to stop the existing thread.

I think the first step would be to write a test that captures this issue. This would involve creating a new activity and then adding a GLSurfaceView to it, and then making the activity visible. I believe this would start the mGLThread member. The test would be similar to https://github.com/robolectric/robolectric/blob/master/robolectric/src/test/java/org/robolectric/shadows/ShadowSurfaceViewTest.java.

@Devashishbasu
Copy link
Contributor Author

Hi @Devashishbasu ,

Thanks for this PR. I believe the problem is that GLSurfaceView starts a thread when setRenderer is called:

https://cs.android.com/android/platform/superproject/main/+/main:frameworks/base/opengl/java/android/opengl/GLSurfaceView.java;l=360

This thread is contained in the GLSurfaceView.mGLThread member. There is currently nothing stopping that thread. This PR creates a new thread member in the shadow and stops that, but that isn't really doing anything to stop the existing thread.

I think the first step would be to write a test that captures this issue. This would involve creating a new activity and then adding a GLSurfaceView to it, and then making the activity visible. I believe this would start the mGLThread member. The test would be similar to https://github.com/robolectric/robolectric/blob/master/robolectric/src/test/java/org/robolectric/shadows/ShadowSurfaceViewTest.java.

Got it.. thankyou

@Devashishbasu
Copy link
Contributor Author

Hi @hoisie , could you please review the test? and in which folder do i need to place the test?

import android.app.Activity;
import android.opengl.GLSurfaceView;
import android.os.Bundle;
import androidx.test.ext.junit.runners.AndroidJUnit4;
import org.junit.Test;
import org.junit.runner.RunWith;

import static org.robolectric.Robolectric.buildActivity;
import static org.robolectric.Shadows.shadowOf;
import static org.junit.Assert.assertTrue;

@RunWith(AndroidJUnit4.class)
public class GLSurfaceViewThreadTest {

@Test
public void testGLSurfaceViewThreadStopped() {
    // Create the activity
    TestActivity activity = buildActivity(TestActivity.class).create().start().resume().visible().get();

    // Get the GLSurfaceView
    GLSurfaceView glSurfaceView = activity.findViewById(R.id.glSurfaceView);

    // Get the shadow of GLSurfaceView
    ShadowGLSurfaceView shadowGLSurfaceView = shadowOf(glSurfaceView);

    // Ensure that the GLSurfaceView's internal thread is running
    assertTrue(shadowGLSurfaceView.getGlThread().isAlive());

    // Finish the activity
    activity.finish();

    // Ensure that the GLSurfaceView's internal thread is stopped after the activity is finished
    assertTrue(shadowGLSurfaceView.getGlThread().isInterrupted());
}

public static class TestActivity extends Activity {
    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(new GLSurfaceView(this));
    }
}

}

@utzcoz
Copy link
Member

utzcoz commented Feb 29, 2024

@Devashishbasu You can create a new test file called ShadowGLSurfaceViewTest like other shadow class's tests if it doesn't exist.

@Devashishbasu
Copy link
Contributor Author

Hi @utzcoz @MGaetan89 ...how can i retrieve the GLThread associated with GLSurfaceView?

@utzcoz
Copy link
Member

utzcoz commented Mar 16, 2024

@Devashishbasu I think you can use reflector or common reflection code to retrieve mGLThread from a GLSurfaceView instance.

@Devashishbasu
Copy link
Contributor Author

@Devashishbasu I think you can use reflector or common reflection code to retrieve mGLThread from a GLSurfaceView instance.

Hi, @utzcoz could you please review if the code added by me is correctly terminating the thread or not after the test is finished?

@utzcoz
Copy link
Member

utzcoz commented Apr 16, 2024

@Devashishbasu Please keep your commit history clean always.

 terminate the thread using callOnDetachFromWindow and finalize method
 present in GLSurafceView
@Devashishbasu Devashishbasu force-pushed the Calling-GLSurfaceView-stops-the-created-gl-thread branch from bbedd78 to 11f0d4a Compare April 16, 2024 09:17
@Devashishbasu
Copy link
Contributor Author

@Devashishbasu Please keep your commit history clean always.

Hi @utzcoz, done.... is there anything else that I need to do to fix this issue?

@Devashishbasu
Copy link
Contributor Author

@Devashishbasu Please keep your commit history clean always.

Hi @utzcoz, done.... is there anything else that I need to do to fix this issue?

Hi utzcoz, should I add a test class for ShadowGLSurafceView?

@utzcoz
Copy link
Member

utzcoz commented Apr 18, 2024

should I add a test class for ShadowGLSurafceView?

I recommend you adding necessary tests for it. You can refer other tests in ShadowGLSurfaceViewTest.

}
} catch (Exception e) {
Log.e(TAG, "Exception occurred while stopping GLThread: " + e.getMessage());
e.printStackTrace();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can add e to Log.e, and remove e.printStackTrace, like:

Log.e(TAG, "....", e);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Calling GLSurfaceView does not stop the created gl thread
4 participants