[vlc-devel] [PATCH] srt: Read several chunks per blocking read call
Filip Roséen
filip at atch.se
Mon Jul 23 20:32:38 CEST 2018
Hi Olivier,
On 2018-07-13 18:25, Olivier Crête wrote:
> From: Roman Diouskine <rdiouskine at 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.
As we are no longer reading a fixed-number of bytes, some calls to
`msg_Dbg` with relevant information about how many bytes have been/are
about to be read seems relevant.
> ---
> 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;
The above comment is somewhat confusing, it is probably better to add
a comment to the data-member declaration within `stream_sys`
explaining what *"i_chunks"* denotes, and then simplify the above
comment, or remove it.
> +
> 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);
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.
>
> 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;
What is the need of the local variable `chunks` here, as you will
modify `p_sys->i_chunk` in either case (unless allocation fails, but
you would end up trying to allocate the same number of bytes on the
next iteration anyhow)?
> + 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 );
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).
>
> 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.
> + */
There must be a way to write the above comment in a cleaner fashion?
> + size_t bytesread = 0;
You are allowed to shrink `pkt->i_buffer`, and with that; you could
use `pkt->i_buffer` to denote the current number of read bytes,
effectively replacing `bytesread`, while also removing the currently
mandatory assignment to `pkt->i_buffer` when you are done.
> + 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
Best Regards,\
Filip
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20180723/b5bc444f/attachment.html>
More information about the vlc-devel
mailing list