[vlc-devel] [PATCH 1/2] access{_out}:srt: introduce SRT input/ouput

Rémi Denis-Courmont remi at remlab.net
Tue Aug 8 21:13:25 CEST 2017


	Hi,

Le keskiviikkona 9. elokuuta 2017, 3.49.17 EEST Justin Kim a écrit :
> Secure Reliable Transport (SRT) is a proprietary transport technology
> that optimizes streaming performance across unpredictable networks.
> 
> Signed-off-by: Justin Kim <justin.kim at collabora.com>
> ---
>  configure.ac                      |  29 +++
>  modules/access/Makefile.am        |   8 +
>  modules/access/srt.c              | 237 +++++++++++++++++++++
>  modules/access_output/Makefile.am |   7 +
>  modules/access_output/srt.c       | 428
> ++++++++++++++++++++++++++++++++++++++ 5 files changed, 709 insertions(+)
>  create mode 100644 modules/access/srt.c
>  create mode 100644 modules/access_output/srt.c
> 
> diff --git a/configure.ac b/configure.ac
> index 8487fea895..05e01c544f 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -3859,6 +3859,35 @@ AS_IF([test "${enable_lirc}" = "yes"], [
>  ])
>  AM_CONDITIONAL([HAVE_LIRC], [test "${have_lirc}" = "yes"])
> 
> +dnl
> +dnl  SRT plugin
> +dnl
> +AC_ARG_ENABLE(srt,
> +[AS_HELP_STRING([--disable-srt],[srt input/output plugin (default auto)])])
> +have_srt="no"
> +AS_IF([test "${enable_srt}" != "no"], [
> +    PKG_CHECK_MODULES(SRT, [srt >= 1.2.0], [
> +        have_srt="yes"
> +        AC_CHECK_HEADER([srt/srt.h], [
> +            VLC_ADD_PLUGIN([srt])
> +            VLC_ADD_CXXFLAGS([srt], [$SRT_CFLAGS])
> +            VLC_ADD_CFLAGS([srt], [$SRT_CFLAGS])
> +            VLC_ADD_LIBS([srt], [$SRT_LIBS])
> +        ], [
> +            AS_IF([test -n "${enable_srt}"],
> +                [AC_MSG_ERROR([${SRT_PKG_ERRORS} (required for srt).])],
> +                [AC_MSG_WARN([${SRT_PKG_ERRORS} (required for srt).])]
> +                )
> +        ])
> +    ], [
> +        AS_IF([test "x${enable_srt}" = "xyes"],
> +            [AC_MSG_ERROR([${SRT_PKG_ERRORS} (required for srt).])],
> +            [AC_MSG_WARN([${SRT_PKG_ERRORS} (required for srt).])]
> +            )
> +    ])

Manual header tests are mostly there for old libraries that do not, or did not 
support pkg-config. Unless SRT has had significant releases without pkg-
config, this may be overkill.


> +])
> +AM_CONDITIONAL([HAVE_SRT], [test "${have_srt}" = "yes"])
> +
>  EXTEND_HELP_STRING([Visualisations and Video filter plugins:])
>  dnl
>  dnl  goom visualization plugin
> diff --git a/modules/access/Makefile.am b/modules/access/Makefile.am
> index dd485a2767..5e765884bc 100644
> --- a/modules/access/Makefile.am
> +++ b/modules/access/Makefile.am
> @@ -417,3 +417,11 @@ libaccess_mtp_plugin_la_LIBADD = $(MTP_LIBS)
>  libaccess_mtp_plugin_la_LDFLAGS = $(AM_LDFLAGS) -rpath '$(accessdir)'
>  access_LTLIBRARIES += $(LTLIBaccess_mtp)
>  EXTRA_LTLIBRARIES += libaccess_mtp_plugin.la
> +
> +### SRT ###
> +
> +libsrt_plugin_la_SOURCES = access/srt.c
> +libsrt_plugin_la_LIBADD = $(SRT_LIBS) $(LIBPTHREAD)

Is there any reasons to pull pthread here? You don´t seem to call any pthread 
functions from within the plugin.

> +if HAVE_SRT
> +access_LTLIBRARIES += libsrt_plugin.la
> +endif
> diff --git a/modules/access/srt.c b/modules/access/srt.c
> new file mode 100644
> index 0000000000..8f37232bec
> --- /dev/null
> +++ b/modules/access/srt.c
> @@ -0,0 +1,237 @@
> +/**************************************************************************
> *** + * srt.c: SRT (Secure Reliable Transport) input module
> +
> ***************************************************************************
> ** + * Copyright (C) 2017, Collabora Ltd.
> + *
> + * Authors: Justin Kim <justin.kim at collabora.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU Lesser General Public License as published by
> + * the Free Software Foundation; either version 2.1 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public License
> + * along with this program; if not, write to the Free Software Foundation,
> + * Inc., 51 Franklin Street, Fifth Floor, Boston MA 02110-1301, USA. +
> ***************************************************************************
> **/ +
> +#ifdef HAVE_CONFIG_H
> +# include "config.h"
> +#endif
> +
> +#include <errno.h>
> +#include <vlc_common.h>
> +#include <vlc_plugin.h>
> +#include <vlc_access.h>
> +#include <vlc_interface.h>

vlc_interface.h really should not be needed in an input/output plugin.

> +
> +#include <sys/socket.h>
> +#include <netinet/in.h>
> +
> +#include <srt/srt.h>
> +
> +#define SRT_DEFAULT_CHUNK_SIZE 1316
> +
> +static int Open(vlc_object_t *);
> +static void Close(vlc_object_t *);
> +
> +static block_t *BlockSRT( stream_t *, bool * );
> +static int Control( stream_t *, int, va_list );

In new code, I would recommend ordering to avoid forward declarations, with 
the descriptor at the end (so like the kernel and gstreamer modules).

> +
> +/* Module descriptor */
> +vlc_module_begin()
> +    set_shortname(N_("SRT"))
> +    set_description(N_("SRT input"))
> +    set_category(CAT_INPUT)
> +    set_subcategory( SUBCAT_INPUT_ACCESS )
> +
> +    set_capability("access", 0)
> +    add_shortcut( "srt" )
> +
> +    set_callbacks(Open, Close)
> +vlc_module_end ()
> +
> +struct stream_sys_t
> +{
> +    SRTSOCKET sock;
> +    int i_poll_id;
> +    int i_chunk_size;

size_t ?

> +};
> +
> +static int Open(vlc_object_t *p_this)
> +{
> +    stream_t     *p_stream = (stream_t*)p_this;
> +    stream_sys_t *p_sys;
> +
> +    p_sys = vlc_malloc( p_this, sizeof( *p_sys ) );
> +    if( unlikely( p_sys == NULL ) )
> +        return VLC_ENOMEM;
> +
> +    p_sys->i_chunk_size = SRT_DEFAULT_CHUNK_SIZE;
> +    p_stream->p_sys = p_sys;
> +    p_stream->pf_block = BlockSRT;
> +    p_stream->pf_control = Control;
> +
> +    p_sys->sock = srt_socket(AF_INET, SOCK_DGRAM, 0);
> +    if ( p_sys->sock == SRT_ERROR )
> +    {
> +        msg_Err( p_stream, "Failed to open socket." );
> +        return VLC_EGENERIC;
> +    }

I suppose SRT sets the the atomic close-on-flag on all its internal 
descriptors?

> +
> +    p_sys->i_poll_id = srt_epoll_create();
> +    if ( p_sys->i_poll_id == -1 )
> +    {
> +        msg_Err( p_stream, "Failed to create poll id for SRT socket." );
> +        srt_close ( p_sys->sock );
> +        return VLC_EGENERIC;
> +    }
> +    int modes = SRT_EPOLL_OUT;
> +    srt_epoll_add_usock( p_sys->i_poll_id, p_sys->sock, &modes );
> +
> +    char *psz_name = strdup( p_stream->psz_location );
> +    char *psz_parser;
> +    const char *psz_server_addr, *psz_bind_addr = "";
> +    int  i_bind_port = 1234, i_server_port = 0;
> +
> +    if( unlikely(psz_name == NULL) )
> +        return VLC_ENOMEM;
> +
> +    /* Parse psz_name syntax (Copied from udp.c):
> +     * [serveraddr[:serverport]][@[bindaddr]:[bindport]] */
> +    psz_parser = strchr( psz_name, '@' );
> +    if( psz_parser != NULL )
> +    {
> +        /* Found bind address and/or bind port */
> +        *psz_parser++ = '\0';
> +        psz_bind_addr = psz_parser;
> +
> +        if( psz_bind_addr[0] == '[' )
> +            /* skips bracket'd IPv6 address */
> +            psz_parser = strchr( psz_parser, ']' );
> +
> +        if( psz_parser != NULL )
> +        {
> +            psz_parser = strchr( psz_parser, ':' );
> +            if( psz_parser != NULL )
> +            {
> +                *psz_parser++ = '\0';
> +                i_bind_port = atoi( psz_parser );
> +            }
> +        }
> +    }
> +
> +    psz_server_addr = psz_name;
> +    psz_parser = ( psz_server_addr[0] == '[' )
> +        ? strchr( psz_name, ']' ) /* skips bracket'd IPv6 address */
> +        : psz_name;
> +
> +    if( psz_parser != NULL )
> +    {
> +        psz_parser = strchr( psz_parser, ':' );
> +        if( psz_parser != NULL )
> +        {
> +            *psz_parser++ = '\0';
> +            i_server_port = atoi( psz_parser );
> +        }
> +    }
> +
> +    msg_Dbg( p_stream, "opening server=%s:%d local=%s:%d",
> +             psz_server_addr, i_server_port, psz_bind_addr, i_bind_port );
> +
> +    struct sockaddr_in sa;
> +    memset( &sa, 0, sizeof sa );
> +    sa.sin_family = AF_INET;
> +    sa.sin_port = htons( i_server_port );
> +
> +    if ( !inet_pton( sa.sin_family, psz_server_addr, &sa.sin_addr ) )
> +    {
> +        msg_Err( p_stream, "Failed to convert server address(%s).",
> psz_server_addr );
> +        srt_close ( p_sys->sock );
> +        return VLC_EGENERIC;
> +    }

If this really is IPv4-specific, then you should probably drop all the 
unnecessary brackets handling above. If it isn´t, then vlc_getaddrinfo() with 
the numeric flag is probably simpler.

> +
> +    free( psz_name );
> +
> +    int stat;
> +    struct sockaddr *p_sa = (struct sockaddr*)&sa;
> +    stat = srt_connect( p_sys->sock, p_sa, sizeof sa);

How long does this potentially block the calling thread?

Eventually, this probably should call vlc_interrupt_register()/
vlc_interrupt_unregister() or whatever to abort the SRT event loop.

> +
> +    if ( stat == SRT_ERROR )
> +    {
> +        msg_Err( p_stream, "Failed to connect to server." );
> +        srt_close ( p_sys->sock );
> +        return VLC_EGENERIC;
> +    }
> +
> +    return VLC_SUCCESS;
> +}
> +
> +static void Close(vlc_object_t *p_this)
> +{
> +    stream_t     *p_stream = (stream_t*)p_this;
> +    stream_sys_t *p_sys = p_stream->p_sys;
> +
> +    msg_Dbg( p_stream, "closing server" );
> +    srt_close ( p_sys->sock );

Doesn´t the epoll descriptor also need to be closed?

Also in error paths in Open().

> +}
> +
> +static int Control(stream_t *p_stream, int i_query, va_list args)
> +{
> +    VLC_UNUSED (p_stream);
> +    bool    *pb_bool;
> +
> +    switch( i_query )
> +    {
> +        case STREAM_CAN_SEEK:
> +        case STREAM_CAN_FASTSEEK:
> +        case STREAM_CAN_PAUSE:
> +        case STREAM_CAN_CONTROL_PACE:
> +            pb_bool = va_arg( args, bool * );
> +            *pb_bool = false;

In new code, we just dereference the result of va_arg()  without intermediate 
locals.

> +            break;
> +        default:
> +            return VLC_EGENERIC;

GET_DELAY should be added too.

> +    }
> +    return VLC_SUCCESS;
> +}
> +
> +static block_t *BlockSRT(stream_t *p_stream, bool *restrict eof)
> +{
> +    stream_sys_t *p_sys = p_stream->p_sys;
> +
> +    block_t *pkt = block_Alloc( p_sys->i_chunk_size );
> +
> +    if (unlikely(pkt == NULL))
> +    {
> +        return NULL;
> +    }
> +
> +    SRTSOCKET ready[2];
> +    if ( srt_epoll_wait( p_sys->i_poll_id, 0, 0, ready, &(int) { 2 }, -1,
> 0, 0, 0, 0 ) == -1 )
> +    {
> +        msg_Err( p_stream, "%s", srt_getlasterror_str() );
> +        goto skip;
> +    }
> +
> +    int stat = srt_recvmsg( p_sys->sock, (char *)pkt->p_buffer,
> p_sys->i_chunk_size );
> +
> +    if ( stat == SRT_ERROR )
> +    {
> +        msg_Err( p_stream, "%s", srt_getlasterror_str() );
> +        goto skip;
> +    }
> +
> +    pkt->i_buffer = stat;
> +    return pkt;
> +
> +skip:
> +   *eof = true;
> +    block_Release(pkt);
> +    return NULL;
> +}


-- 
雷米‧德尼-库尔蒙
https://www.remlab.net/



More information about the vlc-devel mailing list