ryanheise / just_audio

Audio Player
1.04k stars 656 forks source link

Bug: Logcat exception output #13

Closed rohansohonee closed 4 years ago

rohansohonee commented 4 years ago
java.lang.IllegalStateException
android.media.MediaExtractor.getSampleTime(Native Method)
com.ryanheise.just_audio.AudioPlayer$PlayThread.run(AudioPlayer.java:458)

Happens when stopping, seeking or playing audio sometimes. The PlayThread is the main cause of this issue.

ryanheise commented 4 years ago

Can you provide the audio file and the steps to reproduce?

rohansohonee commented 4 years ago

mp3 audio files are being used. The steps to reproduce are:

  1. Play mp3.
  2. Stop and play next mp3 immediately.
  3. Then seek and jump back to previous mp3, that is when the exception occurs.

Due to this reason I guess i am not able to perform crossfade between tracks cause app just crashes. I have two instances of audioplayer object but only one play thread is running. The minute I start a new play thread very very quickly it causes this exeption I think.

Is using a thread correct in this approach, like in this particular way?? i don't know really??

rohansohonee commented 4 years ago

I am referring to the PlayThread of just_audio in the native code.

I call play() method from dart which in turn starts the play thread on native side.

rohansohonee commented 4 years ago

I think you should readSampleData first then getSampleTime. And check if sampleSize is >0 What do you think??

ryanheise commented 4 years ago

In terms of the steps to reproduce, can you share the actual code? It is important to know the exact sequence of method calls, where await is being used, how many instances you're using, etc. Also, are the audio files local or remote?

Looking through the code, I can see that setUrl does not actually check that the play thread is stopped so that might be the problem here if calling setUrl soon after the previous track finished, and so I should insert an ensureStopped call at the top of setUrl.

rohansohonee commented 4 years ago
await audioplayer.setFilePath()
await tempplayer.setFilePath()
audioplayer.play()
//Skip to next song
await audioplayer.stop()
// Again calling setFilePath() for both players
// Skipping immediately to previous song
await audioplayer.stop()
await audioplayer.setFilePath()
await tempplayer.setFilePath()
audioplayer.play()

When performing the skip operations even in profile mode causes the exception. The reason is PlayThread that is running in native code is taking time to start and stop very quickly.

rohansohonee commented 4 years ago

With tempplayer i am only calling setFilePath and not even doing play yet. Stopping and starting audioplayer quickly is always crashing the app. Adding breakpoints only revealed that the last play command is run and then the illegal state exception occurs as mentioned in the above logcat output.

ryanheise commented 4 years ago

Sorry I edited my previous message without noticing your reply. I think calling ensureStopped at the top of setUrl should help. Reading your example though, I'm not clear on this bit:

//Skip to next song
await audioplayer.stop()
// Again calling setFilePath() for both players
// Skipping immediately to previous song
await audioplayer.stop()

Can you write out in code what you're doing between the two stops?

rohansohonee commented 4 years ago
await audioplayer.stop()
// setting new path
await audioplayer.setFilePath()
await tempplayer.setFilePath()
audioplayer.play()
await audioplayer.stop()
// again i am setting new path
await audioplayer.setFilePath()
await tempplayer.setFilePath()
audioplayer.play()

There is always a glitch in audio sound when calling play() and ofcourse the crash happens when doing stops and plays very quickly like this. (tested even in profile mode)

Let's see whether ensureStopped on top of setUrl works.

Can you also try ConcatenatingMediaSource like in exoplayer.

public final class ConcatenatingMediaSource
extends CompositeMediaSource<com.google.android.exoplayer2.source.ConcatenatingMediaSource.MediaSourceHolder>

Concatenates multiple MediaSources. The list of MediaSources can be modified during playback. It is valid for the same MediaSource instance to be present more than once in the concatenation. Access to this class is thread-safe.

ryanheise commented 4 years ago

Can you open a separate issue for the feature request of a composite media source? To help my workflow, it helps to have a separate issue for each feature request and a separate issue for each bug report. Perhaps you could close #3 also which duplicates a number of issues. Then, we can keep the present issue for resolving the IllegalStateException bug.

rohansohonee commented 4 years ago

Ok will do so.

ryanheise commented 4 years ago

With this bug, does it only occur when you involve the tempplayer.setFilePath? Also, what files are you setting in each case?

ryanheise commented 4 years ago

I have tried your code sequence with two mp3 files. When skipping to track B, I pass in the B filepath to setFilePath on both audio players, and when skipping backwards to track A, I pass the A filepath into both audio player.

However, I was unable to reproduce the bug, so there may be some other condition required to reproduce this. Can you share a minimal reproduction project?

ryanheise commented 4 years ago

Or, perhaps since you can reproduce it, have you tried inserting ensureStopped(); right after the precondition check in setUrl? i.e.:

    public void setUrl(final String url, final Result result) throws IOException {
        if (state != PlaybackState.none && state != PlaybackState.stopped && state != PlaybackState.completed) {
            throw new IllegalStateException("Can call setUrl only from none/stopped/completed states");
        }
        ensureStopped(); // ADD THIS

Anyway, if I look at what you're ultimately trying to do, I think if you fully implement the cross fade, then the same instance of AudioPlayer would not immediately try to initialise the next track because that's what the second instance is for, and this illegal state might be avoided. Even so, I still would be interested to know if ensureStopped prevents the illegal state from happening.

rohansohonee commented 4 years ago

Will add ensureStopped and test.

rohansohonee commented 4 years ago

Skipping songs back and forth slightly fast causes illegalstateexceptions such as: "Illegal state: Can call stop only from playing, paused and buffering states" This means even when i am in one of the legal states, doing fast skipping of songs will cause a delay and thus the illegal state gets formed.

rohansohonee commented 4 years ago

I also added ensureStopped

rohansohonee commented 4 years ago

Not getting this error anymore:

java.lang.IllegalStateException
android.media.MediaExtractor.getSampleTime(Native Method)
com.ryanheise.just_audio.AudioPlayer$PlayThread.run(AudioPlayer.java:458)

There was bug in my dart code. I was awaiting stop() which was in a function. That function I was not awaiting, that is why audio continued playing and thus caused this crash.

Now I am adding if conditions to all the await player code and testing.

rohansohonee commented 4 years ago

Since you are not returning any result the dart code keeps awaiting. Eg:

public void stop(final Result result) {
        switch (state) {
        case stopped:
            break;
        case completed:
            transition(PlaybackState.stopped);
            break;

The stopped and completed case do not return result thus at dart side it keeps awaiting for result. This is tested with adding if conditions to check whether the state is legal.

rohansohonee commented 4 years ago

You need to return result.success(null) or something.

rohansohonee commented 4 years ago

I added the return statements and finally found the final issue. We cannot add if condition for setFilePath at all since playbackstate is null.

Merging this pull request shall solve the issue: https://github.com/ryanheise/just_audio/pull/11

Also I will test and report.

ryanheise commented 4 years ago

Well spotted on the missed result.success. I'll fix it and commit.

rohansohonee commented 4 years ago

Just caught a new error: Error: java.lang.NullPointerException: Attempt to invoke virtual method 'void android.media.AudioTrack.pause()' on a null object reference

rohansohonee commented 4 years ago

After adding all the if statements and making sure i am not in illegal state and also adding the return statements and https://github.com/ryanheise/just_audio/pull/11 using this fix

The new error is audioTrack is null!!

ryanheise commented 4 years ago

I've just committed those changes above. I'm about to have dinner, so I'll continue looking into this afterwards. Can you copy and paste the full stacktrace for the null pointer exception? Which line does it occur on?

rohansohonee commented 4 years ago

It's in the stop method:

case paused:
                synchronized (monitor) {
                    // It takes some time for the PlayThread to actually wind down
                    // so other methods that transition from the stopped state should
                    // wait for playThread == null with ensureStopped().
                    PlaybackState oldState = state;
                    transition(PlaybackState.stopped);
                    if (oldState == PlaybackState.paused) {
                        monitor.notifyAll();
                    } else {
                        if(audioTrack!=null)
                        audioTrack.pause();
                    }
                    new Thread(() -> {
                        ensureStopped();
                        handler.post(() -> result.success(null));
                    }).start();
                }
                break;

i just added a simple check for if(audioTrack!=null)

ryanheise commented 4 years ago

After adding the null check, were there any further errors?

rohansohonee commented 4 years ago

Yes the final error is this: IllegalStateException("Can call stop only from playing, paused and buffering states");

rohansohonee commented 4 years ago

I am handling all the cases for stop yet I keep getting this exception. Step to reproduce this: Skip songs quickly i.e calls to stop() are done very quickly.

This causes the synchronized block here

case paused:
                synchronized (monitor) {
                    // It takes some time for the PlayThread to actually wind down
                    // so other methods that transition from the stopped state should
                    // wait for playThread == null with ensureStopped().
                    PlaybackState oldState = state;
                    transition(PlaybackState.stopped);
                    if (oldState == PlaybackState.paused) {
                        monitor.notifyAll();
                    } else {
                        if (audioTrack != null)
                            audioTrack.pause();
                    }...........

to cause this exception

rohansohonee commented 4 years ago

Is there a better way to handle PlayThread ????

ryanheise commented 4 years ago

I've just added the null check, and also modified the exception message to show the current state, because I'm curious what state you are calling "stop" from.

When you say you keep getting this exception, are you talking about IllegalStateException("Can call stop only from playing, paused and buffering states") or the NullPointerException?

As for whether a thread can be avoided, there is another way to implement it using asynchronous callbacks. The API docs actually recommend new code to be written that way. However, as some of the method calls involved in the processing pipeline are blocking, and Flutter would prefer you not to run blocking code on the main thread, it would still be necessary to use a separate thread for that blocking code. So, it may be possible to structure the code that way, but it wouldn't necessarily solve any problems.

rohansohonee commented 4 years ago

I am talking about IllegalStateException("Can call stop only from playing, paused and buffering states")

rohansohonee commented 4 years ago

And it has this IllegalStateException("Can call stop only from playing, paused and buffering states",null)

a null at the end. I guess the state is null thus the error!!

ryanheise commented 4 years ago

That's not the state, the new code in the latest commit looks like this:

throw new IllegalStateException("Can call stop only from playing, paused and buffering states (" + state + ")");

So the state should appear in parentheses, and if it isn't, you musn't have that code.

rohansohonee commented 4 years ago

Exception has occurred. PlatformException (PlatformException(Illegal state: Can call setUrl only from none/stopped/completed states ::(connecting), null, null))

rohansohonee commented 4 years ago

I added "::" before the brackets just cause

ryanheise commented 4 years ago

Interesting, so basically you are calling stop right after calling setUrl and without waiting for setUrl to complete. This error shouldn't happen if you await the setUrl call before doing anything else. The error message is slightly wrong about which are valid states, although it is at least true that you can't call stop while setUrl is in progress (i.e. the connecting state).

(I think it would actually be better to be able to call stop from any state, but I figure that there are much more important things to work on first, and this illegal state exception can be avoided by waiting for setUrl to complete before doing anything else.)

rohansohonee commented 4 years ago

I have put await before setFilePath

rohansohonee commented 4 years ago

state is in connecting whenever the exception happens

I figure that there are much more important things to work on first

Ok

ryanheise commented 4 years ago

I've checked the code, and it should be impossible for this illegal state error to occur unless you call stop before setFilePath completes. So this would give the error:

player.setFilePath(...); // state becomes connecting
await player.stop(); // still connecting, so illegal state

And the error would go away with an await on the first line.

await player.setFilePath(...); // state is connecting
// now state is stopped
await player.stop(); // we're in a legal state, so it's fine

The problem, though, is that your stop call is unlikely to be right there in line 2, because why would anyone want to stop the player before even entering the playing state? So more likely, you have called stop asynchronously in some other part of code without regard for checking the current state to see if it's legal. You can inspect player.playbackState to ensure that it's no longer connecting, or you can listen to the stream to know when it's finished connecting.

rohansohonee commented 4 years ago

I am listening to player.playbackState and i also have await player.setFilePath(..) before await player.stop().

If I perform these operations very quickly only then do I get a state of connecting.

ryanheise commented 4 years ago

But why would you want to stop the player right after setting the file path? That was what I asked in my previous post.

rohansohonee commented 4 years ago

When I am skipping I stop the player and then setpath and play music.

now what i did was press skip multiple times, this then causes the error.

ryanheise commented 4 years ago

I see, I suspected something like that. I think this is a good reason why it would be convenient to be able to stop in the middle of connecting. But unfortunately there are a number of other things that need to be implemented first.

In the meantime, I would suggest that you queue up requests on the player and process the queue serially with await. Hopefully that makes sense.

ryanheise commented 4 years ago

@rohansohonee The latest commit makes state transitions more permissive, so hopefully that should allow you to skip between tracks more easily.

rohansohonee commented 4 years ago

@ryanheise Two exceptions:

ryanheise commented 4 years ago

I should have mentioned, you must now put a try/catch around setUrl since if you stop the player in the middle of this call, the call will fail. But if you catch that exception, it should work.

ryanheise commented 4 years ago

Perhaps an alternative to this behaviour would be to make this method return a duration of null rather than throw an exception.

ryanheise commented 4 years ago

I've just updated it to return null.

rohansohonee commented 4 years ago

Version 0.0.5

Exception has occurred. NoSuchMethodError (NoSuchMethodError: The method '_mulFromInteger' was called on null. Receiver: null Tried calling: _mulFromInteger(1000))

Consequences of null