pedroSG94 / RootEncoder

RootEncoder for Android (rtmp-rtsp-stream-client-java) is a stream encoder to push video/audio to media servers using protocols RTMP, RTSP, SRT and UDP with all code written in Java/Kotlin
Apache License 2.0
2.53k stars 772 forks source link

When the RTMP server channel is closed, the error message related to the incorrect socket is not received in the `onConnectionFailed` method. #1583

Closed AnthonyWu-kkstream closed 4 days ago

AnthonyWu-kkstream commented 1 week ago

Hi @pedroSG94

Describe the bug
When the RTMP server channel is closed, the error message related to the incorrect socket is not received in the onConnectionFailed method. I tested this on both virtual and physical devices.

To Reproduce
Steps to reproduce the issue:

  1. Start the stream successfully.
  2. Stream for about 30 seconds.
  3. Close the RTMP server channel from the backend.

Expected behavior
When the RTMP server channel is closed, an error message related to the incorrect socket should be received in the onConnectionFailed method.

Smartphone (please complete the following information):
Virtual Device:

Physical Device:

pedroSG94 commented 1 week ago

Hello,

Can you tell me your server name and version. And the command used to close the channel to reproduce the case?

AnthonyWu-kkstream commented 1 week ago

Hi @pedroSG94, sorry for the confusion.

We are using our streaming console to play the live stream, where users can click a "Close Live Stream" button to stop the stream.

I tested on several virtual devices—some worked, and some didn't. However, none of the physical devices worked. I'm not sure if this issue is related to the socket.

I suspect the exception occurs in the flush method of the TcpSocket class.

I also tested the sample app you provided, and the issue is the same.

Additionally, when I receive an error and call the retry API, the reConnect method does not work. It seems that the retry scope is not being launched.

As shown in the attached image, the log only shows "reConnect 1" without "reConnect 2": 截圖 2024-09-13 晚上9 30 55

AnthonyWu-kkstream commented 1 week ago

When RTMP server closed and an error is received successfully, the following error message is expected:

截圖 2024-09-16 下午3 46 19
pedroSG94 commented 1 week ago

Hello,

About the error, no notified: If the socket is closed and the library try to write in the socket, that error must appear. If the library continue sending packets that means that the socket is not correctly closed in server side even if you send that command. For that reason I want know the name of your server and version to create an environment to reproduce it.

About reconnect: I only can figure that the send close command on disconnect never finish because I haven't other code that could block it. Can you check if the code enter in the launch using a log before disconnect line?

AnthonyWu-kkstream commented 1 week ago

Hi @pedroSG94,

Apologies, but in order to assist you with setting up a streaming test environment on our console, I will need to get approval from my supervisor and submit a request.

Additionally, I found that getOutStream().flush() does not work when the stream is closed. It seems to be locked. When the stream is closed, the log stops at "test1" and never reaches "test2."

override fun flush(isPacket: Boolean) {
    Log.d("test","test1")
    getOutStream().flush()
    Log.d("test","test2")
  }

I tried adjusting the method like this, and it works. Do you have any thoughts on this code change?

override fun flush(isPacket: Boolean) {
    val executor = Executors.newSingleThreadExecutor()
    val future = executor.submit {
      try {
        getOutStream().flush()
      } catch (e: IOException) {
        throw RuntimeException("Flush failed", e)
      }
    }
    try {
      future.get(5, TimeUnit.SECONDS)
    } catch (e: TimeoutException) {
      future.cancel(true)
      throw TimeoutException("Flush operation timed out")
    } catch (e: Exception) {
      throw IOException("Flush failed", e.cause)
    } finally {
        executor.shutdown()
    }
  }
pedroSG94 commented 1 week ago

Hello,

Thank you for the debug and your suggestion, but create a thread each time flush is called is a really bad idea. I did a timeout in the command that fail because this only fail when the disconnect method is called. This is the fix: https://github.com/pedroSG94/RootEncoder/commit/dd4ded4223c4a0a0dbba9bdbca5dedfd7e22e47e

About the other error. I only need info about your server name (Wowza engine, mediamtx, nginx with rtmp module, etc), the version and the command used to close the stream. If it is a free server or I can create an account with a trial I can create the environment by myself.

AnthonyWu-kkstream commented 1 week ago

Hi @pedroSG94

Apologies, but our servers are self-hosted and not free.

Still can’t solve it . There are two main reasons:

pedroSG94 commented 1 week ago

Hello,

Ok, thank you for the info. This could explain both errors. If the flush is blocked then the socket never know that the socket is closed and then never report on connection failed or never disconnect properly.

I will create a new branch to handle this case but I need your help here to test if the error is solved.

AnthonyWu-kkstream commented 1 week ago

Hello, @pedroSG94

Thank you for your explanation. I would be happy to assist in testing the solution for this issue. Please feel free to share the details or steps you'd like me to follow, and I will promptly help to verify if the error is resolved.

pedroSG94 commented 1 week ago

Hello,

Please, compile this branch and tell me if the change fix the problem: https://github.com/pedroSG94/RootEncoder/pull/1585

AnthonyWu-kkstream commented 1 week ago

Hello, @pedroSG94

I tried the fix, but the problem is still not resolved. The sample app continues to show 0.0mb/s when I close it from our console, and logcat keeps printing "frames discarded."

Screenshot_20240918_234834

image

I noticed something interesting: when I switch the network status, it no longer locks up and throws socket errors.

https://github.com/user-attachments/assets/c5ef6f0e-2aab-4cf7-b820-2b6f1278e3df

Is this problem related to the network status?

pedroSG94 commented 1 week ago

Hello,

That socket error on the switch network is the expected way to work. I also did tests doing a server shutdown while streaming and all worked as expected (socket throw an error).

The weird case is your case, because that should works like shutdown the server but instead of that the socket is blocked.

For now, I'm looking to migrate the socket to ktor socket library but I also need to do performance tests.

Can you do a test? Can you try stream and instead of close the stream using the command, shutdown the server?

pedroSG94 commented 1 week ago

Hello,

I did a migration to ktor socket that is based on coroutines. Now, the library never should be blocked and all should be working fine.

Can you update the branch and try again?

AnthonyWu-kkstream commented 1 week ago

Hello @pedroSG94 ,

Sorry for the late reply. You’ve saved my life! 👍 I tried it again, and the new fix resolved the problem. Could you please publish a snapshot version from the branch?

https://github.com/user-attachments/assets/42ac5c8a-ef70-48b3-a631-811493c1e73f

pedroSG94 commented 1 week ago

Hello,

Nice, it is working fine but I can see that the timeout is really high (around 20s) or you are closing the stream in other moment, I'm not sure. I need to see that. Remember that this only working with rtmp and rtmps. Rtmpt, rtmpts are broken (the app will crash if you write rtmpt or rtmpts) and others protocols still use java.io sockets. Gradle version to test:

implementation 'com.github.pedroSG94.RootEncoder:library:d459b91a75'

This change is really large so this could take days. Also, I need do more performance tests using a high bitrates to confirm that the performance is similar

AnthonyWu-kkstream commented 1 week ago

Hello,

Got it. I'll wait for your publish notification. Thank you for the update and reminder!

AnthonyWu-kkstream commented 1 week ago

Hello, @pedroSG94,

I tried it again and called the retry() API when the stream is closed. However, the retry() API is retrying at intervals of about 20 to 30 seconds, even though I've set the retry interval to 5 seconds.

Do you have any insights on this issue?

//retryInterval = 5000L
    override fun onConnectionFailed(reason: String) {
        if (!genericStream.getStreamClient().reTry(retryInterval, reason)) {
            _error.tryEmit(StreamIngestException.ConnectionFailed(errorMessage = reason))
            stopStream()
        } else {
            Log.d(TAG, "Retrying connection")
        }
    }
pedroSG94 commented 1 week ago

Hello,

That should be correct. The interval is the time that the app wait until try connect again but the connection timeout when you try connect is not included. The connection could be instant failed or try about until timeout. Also, the timeout of the socket is bugged with the new socket implementation but that timeout should be 5s.

AnthonyWu-kkstream commented 1 week ago

Hello, Got it., thanks for your explanations.

AnthonyWu-kkstream commented 1 week ago

Hello @pedroSG94,
Is it possible to provide an object that differentiates between various ConnectionFailed reasons?
For example:

This would allow me to retry the connection when receiving ConnectionFailed.No_Network and immediately notify the client of an error when receiving ConnectionFailed.Socket_Time_Out.


override fun onConnectionFailed(reason: ConnectionFailed) {
    // handle connection failure
}
pedroSG94 commented 1 week ago

Hello,

Yes, it is possible but this will take time. I will mark this issue as feature

pedroSG94 commented 4 days ago

Hello,

Both features was added to master branch. Closing as completed