[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