[FFmpeg-devel] [PATCH]select attribute of tee pseudo demuxer may contain multiple stream specifiers

[Nicolas George]

Le decadi 10 vendémiaire, an CCXXIV, Bodecs Bela a écrit :
> thank you for your feedback, I have altered the patch accordingly. I have
> enclosed the updated patch file.

Please remember not to top-post on these mailing-lists.

See interleaved comments below.

> From bcbd5e3e1a850fef1002d3a63c06fc52b2a3d169 Mon Sep 17 00:00:00 2001
> From: Bela Bodecs <bodecsb at vivanet.hu>
> Date: Thu, 1 Oct 2015 13:00:50 +0200
> Subject: [PATCH 1/1] select attribute of tee pseudo demuxer may contain
>  multiple stream specifiers
> MIME-Version: 1.0
> Content-Type: multipart/mixed; boundary="------------1.8.3.1"
> 
> This is a multi-part message in MIME format.
> --------------1.8.3.1
> Content-Type: text/plain; charset=UTF-8; format=fixed
> Content-Transfer-Encoding: 8bit
> 
> ---
>  doc/muxers.texi   |  3 ++-
>  libavformat/tee.c | 32 +++++++++++++++++++++++---------
>  2 files changed, 25 insertions(+), 10 deletions(-)
> 
> 
> --------------1.8.3.1
> Content-Type: text/x-patch; name="0001-select-attribute-of-tee-pseudo-demuxer-may-contain-m.patch"
> Content-Transfer-Encoding: 8bit
> Content-Disposition: attachment; filename="0001-select-attribute-of-tee-pseudo-demuxer-may-contain-m.patch"

This is rather strange. I wonder how you managed to produce this patch.

> 
> diff --git a/doc/muxers.texi b/doc/muxers.texi
> index d75d7de..113b76a 100644
> --- a/doc/muxers.texi
> +++ b/doc/muxers.texi
> @@ -1224,7 +1224,8 @@ Several bitstream filters can be specified, separated by ",".
>  @item select
>  Select the streams that should be mapped to the slave output,
>  specified by a stream specifier. If not specified, this defaults to
> -all the input streams.
> +all the input streams. You may use multiple stream specifiers 
> +separated by commas (@code{,}) e.g.: @code{a:0,v}
>  @end table
>  
>  @subsection Examples
> diff --git a/libavformat/tee.c b/libavformat/tee.c
> index e3d466a..7d67652 100644
> --- a/libavformat/tee.c
> +++ b/libavformat/tee.c
> @@ -47,6 +47,7 @@ static const char *const slave_opt_open  = "[";
>  static const char *const slave_opt_close = "]";
>  static const char *const slave_opt_delim = ":]"; /* must have the close too */
>  static const char *const slave_bsfs_spec_sep = "/";
> +static const char *const slave_select_sep = ",";
>  
>  static const AVClass tee_muxer_class = {
>      .class_name = "Tee muxer",
> @@ -142,7 +143,9 @@ static int open_slave(AVFormatContext *avf, char *slave, TeeSlave *tee_slave)
>      AVFormatContext *avf2 = NULL;
>      AVStream *st, *st2;
>      int stream_count;
> -
> +    int fullret;
> +    char *subselect = NULL, *next_subselect = NULL, *first_subselect;
> +    
>      if ((ret = parse_slave_options(avf, slave, &options, &filename)) < 0)
>          return ret;
>  
> @@ -172,15 +175,26 @@ static int open_slave(AVFormatContext *avf, char *slave, TeeSlave *tee_slave)
>      for (i = 0; i < avf->nb_streams; i++) {
>          st = avf->streams[i];
>          if (select) {
> -            ret = avformat_match_stream_specifier(avf, avf->streams[i], select);
> -            if (ret < 0) {
> -                av_log(avf, AV_LOG_ERROR,
> -                       "Invalid stream specifier '%s' for output '%s'\n",
> -                       select, slave);
> -                goto end;
> -            }
> +            fullret = 0;

> +            first_subselect = select;
> +            next_subselect = NULL;
> +            while (subselect = av_strtok(first_subselect, slave_select_sep, &next_subselect)) {
> +                first_subselect = NULL;

I was about to say "LGTM", but I just noticed this: av_strtok(), just like
strtok(), is destructive: it replace the delimiters by a NUL character in
the original string. If it is called again with the same string, it will
only see the first token.

Unless I am mistaken, this is what will happen here: if the specifier is
"0,1", the stream #0 will be matched on the first round of the loop, but
then stream #1 will only see select as "0", no longer "0,1".

Fixing it can be done easily enough, though. For example with av_strdup() on
the string (but beware of the leaks).

I am a bit surprised you did not catch this during testing. Maybe I am
missing something.

> +
> +                ret = avformat_match_stream_specifier(avf, avf->streams[i], subselect);
> +                if (ret < 0) {
> +                    av_log(avf, AV_LOG_ERROR,
> +                        "Invalid stream specifier '%s' for output '%s'\n",
> +                        subselect, slave);
> +                    goto end;
> +                }
> +                if (ret != 0) {
> +                    fullret = 1; // match
> +                    break;
> +                }
>  
> -            if (ret == 0) { /* no match */
> +            }
> +            if (fullret == 0) { /* no match */
>                  tee_slave->stream_map[i] = -1;
>                  continue;
>              }
> 

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20151004/c915313d/attachment.sig>