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

[Jean-Baptiste Kempf]

Hello,

On Wed, 9 Aug 2017, at 20:40, Justin Kim wrote:
>  Access:
> + * New SRT access module using libsrt
>   * New NFS access module using libnfs
>   * New SMB access module using libdsm
>   * Rewrite MPEG-DASH (Dynamic Adaptive Streaming over HTTP) support,

Please add at the end.

> +dnl
> +dnl  SRT plugin
> +dnl
> +PKG_ENABLE_MODULES_VLC([SRT], [], [srt >= 1.2.0], [srt input/output
> plugin], [auto])
> +AM_CONDITIONAL([HAVE_SRT], [test "${enable_srt}" = "yes"])

You do not need to define HAVE_SRT.

> +if HAVE_SRT
> +access_LTLIBRARIES += libsrt_plugin.la
> +endif

access_LTLIBRARIES += $(LTLIBsrt)
EXTRA_LTLIBRARIES += libsrt_plugin.la

Should replace your HAVE_SRT part, if I am not missing something.

> +#include <sys/socket.h>

sys/socket.h is not present everywhere.

> +static int Open(vlc_object_t *p_this)

> +    p_sys = vlc_malloc( p_this, sizeof( *p_sys ) );
> +    if( unlikely( p_sys == NULL ) )
> +        return VLC_ENOMEM;
> +
> +    p_sys->i_chunk_size = var_InheritInteger( p_stream, "chunk-size" );
> +    p_sys->i_poll_id = -1;
> +    p_stream->p_sys = p_sys;
> +    p_stream->pf_block = BlockSRT;
> +    p_stream->pf_control = Control;
> +
> +    if ( vlc_UrlParse( &parsed_url, p_stream->psz_url ) == -1 )
> +    {
> +      msg_Err( p_stream, "Failed to parse a given URL (%s)",
> p_stream->psz_url );
> +      return VLC_EGENERIC;
> +    }

Maybe this failing block should appear higher in the code, before
var_Inheriting or even allocing p_sys.

> +
> +    msg_Dbg( p_stream, "Parsed URL (scheme: %s, host:port=%s:%u)",
> +             parsed_url.psz_protocol,
> +             parsed_url.psz_host,
> +             parsed_url.i_port );

I'm not sure this brings anything to the user, tbh.

> +    stat = vlc_getaddrinfo( parsed_url.psz_host, parsed_url.i_port,
> &hints, &res );
> +    if ( stat )
> +    {
> +      msg_Err( p_stream, "Cannot resolve [%s]:%d (reason: %s)",
> +               parsed_url.psz_host,
> +               parsed_url.i_port,
> +               gai_strerror( stat ) );
> +            
> +      goto failed;
> +    }
> +
> +    p_sys->sock = srt_socket(res->ai_family, SOCK_DGRAM, 0);
> +    if ( p_sys->sock == SRT_ERROR )
> +    {
> +        msg_Err( p_stream, "Failed to open socket." );
> +        return VLC_EGENERIC;
> +    }
> +
> +    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." );
> +        goto failed;
> +    }
> +    srt_epoll_add_usock( p_sys->i_poll_id, p_sys->sock, &(int) {
> SRT_EPOLL_OUT } );
> +
> +    stat = srt_connect( p_sys->sock, res->ai_addr, sizeof (struct
> sockaddr));
> +
> +    if ( stat == SRT_ERROR )
> +    {
> +        msg_Err( p_stream, "Failed to connect to server." );
> +        goto failed;
> +    } 
> +
> +    vlc_UrlClean( &parsed_url );
> +    freeaddrinfo( res );
> +
> +    return VLC_SUCCESS;
> +
> +failed:
> +    vlc_UrlClean( &parsed_url );
> +    if ( res != NULL )
> +    {
> +       freeaddrinfo( res );
> +    }
> +
> +    if ( p_sys->i_poll_id != -1 )
> +    {
> +       srt_epoll_release( p_sys->i_poll_id );
> +       p_sys->i_poll_id = -1;
> +    }
> +    srt_close( p_sys->sock );
> +    return VLC_EGENERIC;

missing free( p_sys) in error case?

> +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;
> +
> +    srt_epoll_release( p_sys->i_poll_id );
> +    msg_Dbg( p_stream, "closing server" );
> +    srt_close( p_sys->sock );
> +}

same?

> +### SRT ###
> +libaccess_output_srt_plugin_la_SOURCES = access_output/srt.c
> +libaccess_output_srt_plugin_la_LIBADD = $(SRT_LIBS) $(LIBPTHREAD)
> +if HAVE_SRT
> +access_out_LTLIBRARIES += libaccess_output_srt_plugin.la
> +endif

As above.

> +#define GROUP_TEXT N_("Group packets")
> +#define GROUP_LONGTEXT N_("Packets can be sent one by one at the right
> time " \
> +                          "or by groups. You can choose the number " \
> +                          "of packets that will be sent at a time. It "
> \
> +                          "helps reducing the scheduling load on " \
> +                          "heavily-loaded systems." )

This help is a too long and hard to translate.

Good, for the rest, as far as I am concerned.

-- 
Jean-Baptiste Kempf -  President
+33 672 704 734