<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<meta http-equiv="Content-Style-Type" content="text/css" />
<meta name="generator" content="pandoc" />
<title></title>
<style type="text/css">
code{white-space: pre-wrap;}
span.smallcaps{font-variant: small-caps;}
span.underline{text-decoration: underline;}
div.column{display: inline-block; vertical-align: top; width: 50%;}
</style>
</head>
<body>
<p>Hi Olivier,</p>
<p>On 2018-07-13 18:25, Olivier Crête wrote:</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> From: Roman Diouskine <rdiouskine@haivision.com>
It is possible to get an empty read from libsrt and it should not be
treated as EOF.
libsrt input is asynchonously buffered internally and it makes sense
to empty those receive buffers as much as possible on every signaled
receive event from epoll. Doing so reduces context
switching/re-scheduling and improves performance.</code></pre>
</blockquote>
<p>As we are no longer reading a fixed-number of bytes, some calls to <code>msg_Dbg</code> with relevant information about how many bytes have been/are about to be read seems relevant.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> ---
modules/access/srt.c | 62 ++++++++++++++++++++++++++++++++++----------
1 file changed, 48 insertions(+), 14 deletions(-)
diff --git a/modules/access/srt.c b/modules/access/srt.c
index a1c9acca50..b21a9b64c7 100644
--- a/modules/access/srt.c
+++ b/modules/access/srt.c
@@ -40,6 +40,9 @@
/* libsrt defines default packet size as 1316 internally
* so srt module takes same value. */
#define SRT_DEFAULT_CHUNK_SIZE 1316
+/* Minimum/Maximum chunks to allow reading at a time from libsrt */
+#define SRT_MIN_CHUNKS_TRYREAD 10
+#define SRT_MAX_CHUNKS_TRYREAD 100
/* The default timeout is -1 (infinite) */
#define SRT_DEFAULT_POLL_TIMEOUT -1
/* The default latency is 125
@@ -64,6 +67,7 @@ typedef struct
bool b_interrupted;
char *psz_host;
int i_port;
+ int i_chunks;
} stream_sys_t;
static void srt_wait_interrupted(void *p_data)
@@ -193,6 +197,11 @@ static bool srt_schedule_reconnect(stream_t *p_stream)
failed = true;
}
+ /* Reset read chunks number because
+ * the stream might have changed
+ */
+ p_sys->i_chunks = SRT_MIN_CHUNKS_TRYREAD;</code></pre>
</blockquote>
<p>The above comment is somewhat confusing, it is probably better to add a comment to the data-member declaration within <code>stream_sys</code> explaining what <em>“i_chunks”</em> denotes, and then simplify the above comment, or remove it.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +
out:
if (failed && p_sys->sock != SRT_INVALID_SOCK)
{
@@ -212,6 +221,8 @@ static block_t *BlockSRT(stream_t *p_stream, bool *restrict eof)
stream_sys_t *p_sys = p_stream->p_sys;
int i_chunk_size = var_InheritInteger( p_stream, "chunk-size" );
int i_poll_timeout = var_InheritInteger( p_stream, "poll-timeout" );
+ /* SRT doesn't have a concept of EOF for live streams. */
+ VLC_UNUSED(eof);</code></pre>
</blockquote>
<p>The above has nothing to do with the apparent goal of this patch, please make the change (if you find it necessary) in a separate commit.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> if ( vlc_killed() )
{
@@ -219,13 +230,18 @@ static block_t *BlockSRT(stream_t *p_stream, bool *restrict eof)
return NULL;
}
- block_t *pkt = block_Alloc( i_chunk_size );
+ size_t i_chunk_size_actual = ( i_chunk_size > 0 )
+ ? i_chunk_size : SRT_DEFAULT_CHUNK_SIZE;
+ int chunks = ( p_sys->i_chunks )
+ ? p_sys->i_chunks : SRT_MIN_CHUNKS_TRYREAD;</code></pre>
</blockquote>
<p>What is the need of the local variable <code>chunks</code> here, as you will modify <code>p_sys->i_chunk</code> in either case (unless allocation fails, but you would end up trying to allocate the same number of bytes on the next iteration anyhow)?</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + size_t bufsize = i_chunk_size_actual * chunks;
+ block_t *pkt = block_Alloc( bufsize );
if ( unlikely( pkt == NULL ) )
{
return NULL;
}
- vlc_interrupt_register( srt_wait_interrupted, p_stream);
+ vlc_interrupt_register( srt_wait_interrupted, p_stream );</code></pre>
</blockquote>
<p>Your patch is about extending the number of blocks read, not doing clean-up, please do the above in a separate commit as you would not be touching the above line if it wasn’t for the cleaning (it has nothing to do with the goal of this patch).</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> SRTSOCKET ready[1];
int readycnt = 1;
@@ -257,23 +273,41 @@ static block_t *BlockSRT(stream_t *p_stream, bool *restrict eof)
continue;
}
- int stat = srt_recvmsg( p_sys->sock,
- (char *)pkt->p_buffer, i_chunk_size );
- if ( stat > 0 )
+ /* Try to read all available data from the lib up to
+ * predefined buffer size. The buffer size can grow over time
+ * up to predefined maximum.
+ */</code></pre>
</blockquote>
<p>There must be a way to write the above comment in a cleaner fashion?</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + size_t bytesread = 0;</code></pre>
</blockquote>
<p>You are allowed to shrink <code>pkt->i_buffer</code>, and with that; you could use <code>pkt->i_buffer</code> to denote the current number of read bytes, effectively replacing <code>bytesread</code>, while also removing the currently mandatory assignment to <code>pkt->i_buffer</code> when you are done.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + while ( (bufsize - bytesread) >= i_chunk_size_actual )
{
- pkt->i_buffer = stat;
- goto out;
+ int stat = srt_recvmsg( p_sys->sock,
+ (char *)( pkt->p_buffer + bytesread ), bufsize - bytesread );
+ if ( stat <= 0 )
+ {
+ break;
+ }
+ bytesread += (size_t)stat;
}
- msg_Err( p_stream, "failed to receive packet, set EOS (reason: %s)",
- srt_getlasterror_str() );
- *eof = true;
- break;
+ /* Gradually adjust number of chunks we read at a time
+ * up to a predefined maximum. The actual number we might
+ * settle on depends on stream's bit rate.
+ */
+ size_t rem = bufsize - bytesread;
+ if ( rem < i_chunk_size_actual )
+ {
+ if ( ++chunks <= SRT_MAX_CHUNKS_TRYREAD )
+ {
+ p_sys->i_chunks = chunks;
+ }
+ }
+
+ pkt->i_buffer = bytesread;
+ goto out;
}
- /* if the poll reports errors for any reason at all
- * including a timeout, or there is a read error,
- * we skip the turn.
+ /* if the poll reports errors for any reason at all,
+ * including a timeout, we skip the turn.
*/
block_Release(pkt);
--
2.17.1</code></pre>
</blockquote>
<p>Best Regards,<br />
Filip</p>
</body>
</html>