NPE when enabling looping after creating the video player on Android

[EmMper]

The repo(https://oss.sonatype.org/content/repositories/releases) has no path to "com.badlogicgames.gdx-video", and it causes my implementation get errors, you should update your README and add new available dependency.

[SimonIT]

If you look at the versions

there's currently only a snapshot version

[EmMper]

If you look at the versions there's currently only a snapshot version

yeah I tried snapshot version and get same error, that's why i create this issue.

 > Could not find com.badlogicgames.gdx-video:gdx-video-android:1.3.2-SNAPSHOT.
     Searched in the following locations:
       - https://repo.maven.apache.org/maven2/com/badlogicgames/gdx-video/gdx-video-android/1.3.2-SNAPSHOT/maven-metadata.xml
       - https://repo.maven.apache.org/maven2/com/badlogicgames/gdx-video/gdx-video-android/1.3.2-SNAPSHOT/gdx-video-android-1.3.2-SNAPSHOT.pom

I added implementations to my gdx module:

// Gdx module's build.gradle.kts:
dependencis {
    api("com.badlogicgames.gdx:gdx:1.12.1")
    implementation("com.badlogicgames.gdx:gdx-backend-android:1.12.1")
    implementation("com.badlogicgames.gdx-video:gdx-video:1.3.2-SNAPSHOT")
    implementation("com.badlogicgames.gdx-video:gdx-video-android:1.3.2-SNAPSHOT")
}

And added maven to build.gradle.kts in my project root folder:

// project root folder's build.gradle.kts:
buildscript {
    repositories {
        maven {
            url = uri("https://oss.sonatype.org/content/repositories/snapshots/")
        }
    }
}

BTW, your sample test Android module runs with a null pointer when the app just opened, because the function initializeMediaPlayer in VideoPlayerAndroid is initializing the MediaPlayer in Android main thread, not Gdx render thread. In GdxVideoTest class, when create function called, the VideoPlayerAndroid.setLooping() calls inmmediately after VideoPlayerAndroid's constructor, so you can get a null pointer.

[SimonIT]

And added maven to build.gradle.kts in my project root folder:

You added it to the buildscript section, so it's only available for plugins... You have to add it to allprojects.

BTW, your sample test Android module runs with a null pointer when the app just opened

You mean I change the example to this:

		videoPlayer = VideoPlayerCreator.createVideoPlayer();
		videoPlayer.setLooping(true);

then yes I get also an exception:

java.lang.NullPointerException: Attempt to invoke virtual method 'void android.media.MediaPlayer.setLooping(boolean)' on a null object reference
	at com.badlogic.gdx.video.VideoPlayerAndroid.setLooping(VideoPlayerAndroid.java:393)
	at com.badlogic.gdx.video.test.GdxVideoTest.create(GdxVideoTest.java:49)
	at com.badlogic.gdx.backends.android.AndroidGraphics.onSurfaceChanged(AndroidGraphics.java:312)
	at android.opengl.GLSurfaceView$GLThread.guardedRun(GLSurfaceView.java:1557)
	at android.opengl.GLSurfaceView$GLThread.run(GLSurfaceView.java:1272)

When I look at the commits I see @dasisdormax removed the locking. Any reason why?

[dasisdormax]

Good find! Actually, many functions (setLooping, setVolume, isPlaying, getCurrentTimestamp, ...) should cause this crash on Android when called directly after the constructor. These functions never checked if the internal player was already created (both before and after c0fbd06).

I don't think blocking the render thread until everything is created s an option. A slightly inconvenient fix would be to run these functions with handler.post on the Android looper thread.

[EmMper]

You added it to the buildscript section, so it's only available for plugins... You have to add it to allprojects.

Oh, how could I forget this, what an obvious error, damn. Thanks for reply.

[EmMper]

Good find! Actually, many functions (setLooping, setVolume, isPlaying, getCurrentTimestamp, ...) should cause this crash on Android when called directly after the constructor. These functions never checked if the internal player was already created (both before and after c0fbd06).

I don't think blocking the render thread until everything is created s an option. A slightly inconvenient fix would be to run these functions with handler.post on the Android looper thread.

I think using handler.post to run those function is not a good way tho. And MediaPlayer.setLooping will work only when MediaPlay.play is called.
So I downloaded source code and edit a little.

1. I tried to add a new flag private volatile boolean looping = false;
2. and rewrite setLooping() function

    @Override
    public void setLooping(boolean looping) {
        this.looping = looping;
        if (player != null && player.isPlaying()) {
            player.setLooping(looping);
        }
    }

3. then add player.setLooping(looping) to other place witch called player.start()

    @Override
    public void play() {
        if (prepared) {
            player.start();
            player.setLooping(looping);
        }
        playRequested = true;
    }

    private void playInternal(final FileHandle file) {
        // ...other codes
        player.setOnPreparedListener(new OnPreparedListener() {
            @Override
            public void onPrepared(MediaPlayer mp) {
                if (sizeListener != null) {
                    sizeListener.onVideoSize(mp.getVideoWidth(), mp.getVideoHeight());
                }

                Gdx.app.postRunnable(new Runnable() {
                    @Override
                    public void run() {
                        // ...other codes
                        if (playRequested) {
                            player.start();
                            player.setLooping(looping);
                        }
                    }
                });
            }
        }
        // ...other codes
    }

for other void functions, we can also try this way. And for another functions which have returning value, we could just return a default value if MediaPlayer is null.
Hope these could help.

[SimonIT]

I think using handler.post to run those function is not a good way tho.

Do you have a reason?
I like the idea more than adding a variable for each attribute you can set on the media player

[EmMper]

I think using handler.post to run those function is not a good way tho.

Do you have a reason? I like the idea more than adding a variable for each attribute you can set on the media player

handler.post may switch thread, which could cause the function runs in an unknown time, if you care about the result, you will need a callback to get the value right after it is initialized. But if I don't need the value to be that exactly, a default value before initialization is enough, for example isLooping, getCurrentTimestamp.

And when I called setLooping after constructor, I just want they to run in order, I dont care when they really invoked. For this, handler.post will work, so do adding a variable. But this is my personal opinion.