[vlc-devel] [PATCH] srt: parse srt parameters from url

[Hugo Beauzée-Luyssen]

Hi,

On Tue, Mar 5, 2019, at 10:42 PM, Aaron Boxer wrote:
> Parse pass phrase and latency from url for incoming srt stream
> 
> 
> diff --git a/modules/access/srt.c b/modules/access/srt.c
> index d8d8cbe659..55c0efc201 100644
> --- a/modules/access/srt.c
> +++ b/modules/access/srt.c
> @@ -37,6 +37,9 @@
> 
>  #include <srt/srt.h>
> 
> +const char* SRT_PARAM_PASSPHRASE = "passphrase";
> +const char* SRT_PARAM_LATENCY = "latency";
> +
>  /* libsrt defines default packet size as 1316 internally
>  * so srt module takes same value. */
>  #define SRT_DEFAULT_CHUNK_SIZE 1316
> @@ -70,6 +73,85 @@ typedef struct
>  int i_chunks; /* Number of chunks to allocate in the next read */
>  } stream_sys_t;
> 
> +
> +
> +struct parsed_param {
> + char *key;
> + char *val;
> +};
> +
> +static inline char *
> +find(char *str, char find)
> +{
> + str = strchr(str, find);
> + if (NULL == str) {
> + return NULL;
> + }
> + return str + 1;

This helper is only called from find_query, why not inline it there?
And that being said, I'm not convinced this helper simplifies anything compared to calling strchr directly

> +}
> +
> +static inline char *
> +find_query(char *str)
> +{
> + return find(str, '?');
> +}
> +
> +/**
> + * Parse a query string into an array of key/value structs.
> + *
> + * The query string should be a null terminated string of parameters 
> separated by
> + * a delimiter. Each parameter are checked for the equal sign 
> character. If it
> + * appears in the parameter, it will be used as a null terminator and 
> the part
> + * that comes after it will be the value of the parameter.
> + *
> + *
> + * param: query: the query string to parse. The string will be 
> modified.
> + * param: delimiter: the character that separates the key/value pairs 
> from each other.
> + * param: params: an array of parsed_param structs to hold the result.
> + * param: max_params: maximum number of parameters to parse.
> + *
> + * Return: the number of parsed items. -1 if there was an error.
> + */
> +static int
> +srt_url_parse_query(char *query, char delimiter, struct parsed_param 
> *params, int max_params)
> +{
> + int i = 0;
> +
> + if (!query || *query == '\0')
> + {
> + return -1;
> + }
> + if (!params || max_params == 0)
> + {
> + return 0;
> + }

I'm not sure this check is needed since the array is declared in the call site, you might want to add an assert instead though

> +
> + params[i++].key = query;
> + while (i < max_params && NULL != (query = strchr(query, delimiter)))
> + {
> + *query = '\0';
> + params[i].key = ++query;
> + params[i].val = NULL;
> +
> + /* Go back and split previous param if one exists */
> + if (i > 0)
> + {
> + if ((params[i - 1].val = strchr(params[i - 1].key, '=')) != NULL)
> + {
> + *(params[i - 1].val)++ = '\0';
> + }
> + }
> + i++;
> + }
> + /* Go back and split last param */
> + if ((params[i - 1].val = strchr(params[i - 1].key, '=')) != NULL)
> + {
> + *(params[i - 1].val)++ = '\0';
> + }

I think this could be implemented using strtok_r to split per '&', and strchr to split using '=' as you did. I'm pretty sure it would be clearer.
That being said you are only creating the array to extract SRT_PARAM_PASSPHRASE or SRT_PARAM_LATENCY, why not parse it directly from the url directly?

> + return i;
> +}
> +
> +
>  static void srt_wait_interrupted(void *p_data)
>  {
>  stream_t *p_stream = p_data;
> @@ -115,9 +197,11 @@ static int Control(stream_t *p_stream, int 
> i_query, va_list args)
> 
>  static bool srt_schedule_reconnect(stream_t *p_stream)
>  {
> - int i_latency;
> + int i_latency=-1;
>  int stat;
>  char *psz_passphrase = NULL;
> + bool parsed_passphrase = false;
> + char *url = NULL;
> 
>  struct addrinfo hints = {
>  .ai_socktype = SOCK_DGRAM,
> @@ -153,6 +237,38 @@ static bool srt_schedule_reconnect(stream_t *p_stream)
>  goto out;
>  }
> 
> + /* Parse URL parameters */
> + if (p_stream->psz_url && strlen(p_stream->psz_url) < 512)
> + {
> + char* query = NULL;
> + struct parsed_param params[32];
> + int num_params = 0;
> + int i=0;
> + size_t url_size = strlen(p_stream->psz_url)+1;
> +
> + url = malloc(url_size);
> + url[url_size-1] = 0;
> + strcpy(url, p_stream->psz_url);

You probably should use strdup instead.

> + query = find_query(url);
> + if (query)
> + {
> + num_params = srt_url_parse_query(query,'&', params, 
> sizeof(params)/sizeof(struct parsed_param) );

You could use ARRAY_SIZE here

> + if (num_params > 0) {
> + for (i = 0; i < num_params; ++i)
> + {
> + if (strcmp(params[i].key, SRT_PARAM_PASSPHRASE) == 0)
> + {
> + psz_passphrase = params[i].val;
> + parsed_passphrase = true;
> + }
> + else if (strcmp(params[i].key, SRT_PARAM_LATENCY) == 0)
> + i_latency = atoi(params[i].val);
> + }
> + }
> + }
> + }
> +
> +
>  /* Make SRT non-blocking */
>  srt_setsockopt( p_sys->sock, 0, SRTO_SNDSYN,
>  &(bool) { false }, sizeof( bool ) );
> @@ -168,11 +284,13 @@ static bool srt_schedule_reconnect(stream_t 
> *p_stream)
>  &(int) { 0 }, sizeof( int ) );
> 
>  /* Set latency */
> - i_latency = var_InheritInteger( p_stream, "latency" );
> + if (i_latency == -1)
> + i_latency = var_InheritInteger( p_stream, SRT_PARAM_LATENCY );
>  srt_setsockopt( p_sys->sock, 0, SRTO_TSBPDDELAY,
>  &i_latency, sizeof( int ) );
> 
> - psz_passphrase = var_InheritString( p_stream, "passphrase" );
> + if (!psz_passphrase)
> + psz_passphrase = var_InheritString( p_stream, SRT_PARAM_PASSPHRASE );
>  if ( psz_passphrase != NULL && psz_passphrase[0] != '\0')
>  {
>  int i_key_length = var_InheritInteger( p_stream, "key-length" );
> @@ -211,7 +329,11 @@ out:
>  }
> 
>  freeaddrinfo( res );
> - free( psz_passphrase );
> + if (!parsed_passphrase)
> + {
> + free( psz_passphrase );
> + }
> + free(url);
> 
>  return !failed;
>  }
> @@ -422,8 +544,8 @@ vlc_module_begin ()
>  N_("SRT chunk size (bytes)"), NULL, true )
>  add_integer( "poll-timeout", SRT_DEFAULT_POLL_TIMEOUT,
>  N_("Return poll wait after timeout milliseconds (-1 = infinite)"), 
> NULL, true )
> - add_integer( "latency", SRT_DEFAULT_LATENCY, N_("SRT latency (ms)"), 
> NULL, true )
> - add_password("passphrase", "", N_("Password for stream encryption"), 
> NULL)
> + add_integer( SRT_PARAM_LATENCY, SRT_DEFAULT_LATENCY, N_("SRT latency 
> (ms)"), NULL, true )
> + add_password(SRT_PARAM_PASSPHRASE, "", N_("Password for stream 
> encryption"), NULL)
>  add_integer( "key-length", SRT_DEFAULT_KEY_LENGTH,
>  SRT_KEY_LENGTH_TEXT, SRT_KEY_LENGTH_TEXT, false )
>  change_integer_list( srt_key_lengths, srt_key_length_names )
> 

It seems you patch was misformated, patchwork didn't detect it properly (https://patches.videolan.org/patch/22712/) did you use git-send-email?

Regards,

-- 
  Hugo Beauzée-Luyssen
  hugo at beauzee.fr