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

[Bodecs Bela]

See my interleaved comments below.


2015.10.04. 18:50 keltezéssel, Nicolas George írta:
> 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.
I am very sorry. I won't do it again.
>
> 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.
git add libavformat/tee.c
git add doc/muxers.texi
git commit -m "select attribute of tee pseudo demuxer may contain 
multiple stream specifiers"
git format-patch -n -o /tmp/ --attach  origin

>
>> 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.
You would be right unless I used

first_subselect = NULL;

right after the while statement, so the next round av_strtok will go 
further, because the first parameter will be null, not the same string 
and next_subselect will point the next start.
Yes it is destructive, but we never use this string (select) again in 
this function.
I swear, I have really tested it, I use it on my own  in my production 
environment.

>> +
>> +                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,
>
>
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
best regards,

Bela Bodecs