Add a connection timeout to the socket #76

vavadhani · 2019-06-21T09:34:10Z

When using invalid URL, without an explicit timeout, the connection
would fail only after 60s, which is too long. Let the nativeOpen()
method set a timeout and if it doesn't, use a default timeout of 10s.

Allows this timeout value to be set from the application.

#67 and google/ExoPlayer#4249 will be solved with this commit.

When using invalid URL, without an explicit timeout, the connection would fail only after 60s, which is too long. Let the nativeOpen() method set a timeout and if it doesn't, use a default timeout of 10s.

ojw28 · 2019-06-21T10:03:47Z

rtmp-client/src/main/java/net/butterflytv/rtmp_client/RtmpClient.java

+    /* Timeout value in seconds */
+    private int timeout = TIMEOUT;
+    /* Sets the timeout variable */
+    public void setTimeout(int timeout) {


Nit: I'm guessing that this method should probably go below the static exception class (e.g. just above the open method).

Please also add proper javadoc (e.g. /** / rather than / */, and @param documenting the argument - including units).

ojw28 · 2019-06-21T10:06:41Z

rtmp-client/src/main/java/net/butterflytv/rtmp_client/RtmpClient.java

    private long rtmpPointer = 0;

+    /* Timeout value in seconds */


It seems strange that this is in whole seconds. Normally I'd expect to specify a timeout in milliseconds. Is this possible?

We initially considered setting the timeout in milliseconds. The only conflict we saw was that there is already a timeout in the rtmp library but it was being used to set only the socket's receive timeout. So we favoured using the same.
Do you think it would be better to use a different value for the send timeout (in ms), which can be set from the application and continue using the earlier timeout for receive?

The only conflict we saw was that there is already a timeout in the rtmp library but it was being used to set only the socket's receive timeout. So we favoured using the same.

I'm confused about how this is relevant to the unit being used. Please clarify. Is it that seconds is used somewhere elsewhere (if so, where abouts)?

There is a rtmp->Link.timeout variable which holds the timeout in seconds. It is being used here - https://github.com/ittiam-systems/LibRtmp-Client-for-Android/blob/bug-67/rtmp-client/src/main/cpp/librtmp/rtmp.c#L950.
I could add another timeoutInMs variable to the rtmp structure and keep the receive timeout and send timeout different.

If it's possible to set them separately, then I think they should be separate and both settable via the Java API

All Java timeouts should be in the same unit. Ideally milliseconds, since this is more standard for a Java API. However if there's already a Java API that takes seconds that we need to remain consistent with (I couldn't see one?), or if a value passed is going to be rounded to seconds internally (i.e. a millisecond value would lose precision internally) then it's indeed better to use seconds. Either way, let's make sure the unit is well documented on the Java APIs.

There are two APIs now, one to set the send timeout (in milliseconds) and another to set the receive timeout (in seconds).
The javadoc mention the units for both the timeouts and even the variable names used indicate the units.

I have kept the receive timeout unit in seconds since the variable that is used, rtmp->Link.timeout is also set using another C API called RTMP_SetupStream().

I have kept the receive timeout unit in seconds since the variable that is used, rtmp->Link.timeout is also set using another C API called RTMP_SetupStream().

Can we not just change the type of the variable, and the implementation of RTMP_SetupStream(), and then use milliseconds everywhere? I would really rather avoid mixing units for timeouts if at all possible, since doing so seems quite non-ideal.

Done. Changed to milliseconds everywhere.

ojw28 · 2019-06-21T10:08:17Z

rtmp-client/src/main/java/net/butterflytv/rtmp_client/RtmpClient.java

+    private int timeout = TIMEOUT;
+    /* Sets the timeout variable */
+    public void setTimeout(int timeout) {
+        if (timeout > 0) {


If it's considered an error to pass a non-positive values, then this should IMO throw IllegalArgumentException rather than silently doing nothing, if such a value is passed.

Alternatively, if it's possible for passing a non-positive value to return the client to whatever its default is, rather than doing nothing, then that would also be fine.

Zero or negative values will now reset the timeout to default values.

ojw28 · 2019-06-21T10:09:14Z

rtmp-client/src/main/java/net/butterflytv/rtmp_client/RtmpClient.java

@@ -50,7 +60,7 @@ public RtmpIOException(int errorCode) {

    public void open(String url, boolean isPublishMode) throws RtmpIOException {
        rtmpPointer = nativeAlloc();
-        int result = nativeOpen(url, isPublishMode, rtmpPointer);
+        int result = nativeOpen(url, isPublishMode, rtmpPointer, timeout);


Ideally these timeout variables will have the unit in the variable name, so it's clear as its passed around what unit it's in. If we can do milliseconds then timeoutMs would be a good name. Ditto elsewhere in this change.

mekya · 2019-06-23T06:40:06Z

I've just make a quick review. Just let me know when it's ok to merge @ojw28 @vavadhani

Send timeout can be configured in milliseconds and receive timeout can be set in seconds.

ojw28 · 2019-06-24T11:28:05Z

See inline comment:

Can we not just change the type of the variable, and the implementation of RTMP_SetupStream(), and then use milliseconds everywhere? I would really rather avoid mixing units for timeouts if at all possible, since doing so seems quite non-ideal.

ojw28

Looks better, thanks!

ojw28 · 2019-06-24T12:13:00Z

rtmp-client/src/main/cpp/librtmp/rtmp.c

@@ -338,8 +338,8 @@ RTMP_Init(RTMP *r)
  r->m_nServerBW = 2500000;
  r->m_fAudioCodecs = 3191.0;
  r->m_fVideoCodecs = 252.0;
-  //making timeout value to 10 from 30
-  r->Link.timeout = 10;
+  //Changing units to milliseconds. Changing from 10 to 10000.


Remove this line (it's not a comment that makes sense when someone looks at the file - it only makes sense when they look at this change diff specifically)

ojw28 · 2019-06-24T12:13:48Z

rtmp-client/src/main/cpp/librtmp/rtmp.h

@@ -176,7 +176,8 @@ extern "C"
    int swfAge;

    int protocol;
-    int timeout;		/* connection timeout in seconds */
+    int timeout;                /* connection timeout in milliseconds */


Can this be renamed to receiveTimeoutInMs, for consistency?

Note that you can remove the comment on this line and the one below if so, because the names are then clear enough to be self documenting.

ojw28 · 2019-06-24T12:15:36Z

rtmp-client/src/main/java/net/butterflytv/rtmp_client/RtmpClient.java

+     * Parameter expects a non-zero positive integer and will reset timeout to the default value
+     * (10000 ms) if zero or a negative integer is passed.
+     *  */
+    public void setSendTimeoutInMs(int sendTimeoutInMs) {


nit: Can we call these methods setSendTimeout and setReceiveTimeout? The names of the arguments are sufficient to make the unit clear, so there's no need to also document this in the method name.

Changing timeout in Link structure to receiveTimeoutInMs. Remove InMs suffix from Java API that set the timeout values.

ojw28 · 2019-06-25T08:03:25Z

Looks good!

@mekya - I think this can be merged

Add a connection timeout to the socket

4f71f47

When using invalid URL, without an explicit timeout, the connection would fail only after 60s, which is too long. Let the nativeOpen() method set a timeout and if it doesn't, use a default timeout of 10s.

ojw28 reviewed Jun 21, 2019

View reviewed changes

Add Receive Timeout Configurability

3cf8462

Send timeout can be configured in milliseconds and receive timeout can be set in seconds.

Change receive timeout unit to milliseconds

9a138b5

ojw28 reviewed Jun 24, 2019

View reviewed changes

Code refactor

d537621

Changing timeout in Link structure to receiveTimeoutInMs. Remove InMs suffix from Java API that set the timeout values.

mekya merged commit 4a07dfa into ant-media:master Jun 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a connection timeout to the socket #76

Add a connection timeout to the socket #76

vavadhani commented Jun 21, 2019

ojw28 Jun 21, 2019

vavadhani Jun 24, 2019

ojw28 Jun 21, 2019

vavadhani Jun 21, 2019

ojw28 Jun 21, 2019

vavadhani Jun 21, 2019

ojw28 Jun 21, 2019

vavadhani Jun 24, 2019

ojw28 Jun 24, 2019 •

edited

Loading

vavadhani Jun 24, 2019

ojw28 Jun 21, 2019

vavadhani Jun 24, 2019

ojw28 Jun 21, 2019

vavadhani Jun 24, 2019

mekya commented Jun 23, 2019

ojw28 commented Jun 24, 2019

ojw28 left a comment

ojw28 Jun 24, 2019

vavadhani Jun 25, 2019

ojw28 Jun 24, 2019

vavadhani Jun 25, 2019

ojw28 Jun 24, 2019

vavadhani Jun 25, 2019

ojw28 commented Jun 25, 2019

Add a connection timeout to the socket #76

Add a connection timeout to the socket #76

Conversation

vavadhani commented Jun 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ojw28 Jun 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mekya commented Jun 23, 2019

ojw28 commented Jun 24, 2019

ojw28 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ojw28 commented Jun 25, 2019

ojw28 Jun 24, 2019 •

edited

Loading