[FFmpeg-devel] [PATCH v2] libavformat/tee: Add fifo support for tee

[Jan Sebechlebsky]

On 10/31/2016 04:56 PM, Nicolas George wrote:

>> +        if (av_match_name(use_fifo, "true,y,yes,enable,enabled,on,1")) {
>> +            tee_slave->use_fifo = 1;
>> +        } else if (av_match_name(use_fifo, "false,n,no,disable,disabled,off,0")) {
> I am not happy about the duplication of the tests from
> set_string_bool(), but it cannot be avoided for now without more
> unrelated work.
>
> (I really with each option type came with the corresponding silent and
> verbose parsing and dump functions. Unfortunately, this discipline was
> not kept in the past.)
It may be worth refactoring just these tests setting variable to 0/1/-1 
based on boolean/"auto" string to separate function. If you agree I can 
do this in next patch
>> +        ret = avformat_alloc_output_context2(&avf2, NULL, "fifo", filename);
>> +    } else {
>> +        ret = avformat_alloc_output_context2(&avf2, NULL, format, filename);
> You could probably factor these two lines with
> "use_fifo ? "fifo" : format".
Thanks, looks nicer that way, I've modified it.
>> +    }
>>       if (ret < 0)
>>           goto end;
>>       tee_slave->avf = avf2;
>> @@ -394,6 +467,12 @@ static int tee_write_header(AVFormatContext *avf)
>>               filename++;
>>       }
>>   
>> +    if (tee->fifo_options_str) {
>> +        ret = av_dict_parse_string(&tee->fifo_options, tee->fifo_options_str, "=", ":", 0);
>> +        if (ret < 0)
>> +            goto fail;
>> +    }
>> +
>>       if (!(tee->slaves = av_mallocz_array(nb_slaves, sizeof(*tee->slaves)))) {
>>           ret = AVERROR(ENOMEM);
>>           goto fail;
>> @@ -401,6 +480,12 @@ static int tee_write_header(AVFormatContext *avf)
>>       tee->nb_slaves = tee->nb_alive = nb_slaves;
>>   
>>       for (i = 0; i < nb_slaves; i++) {
>> +
>> +        tee->slaves[i].use_fifo = tee->use_fifo;
>> +        ret = av_dict_copy(&tee->slaves[i].fifo_options, tee->fifo_options, 0);
> Unless I am mistaken, if there are keys present in both dicts, the
> entries in tee->fifo_options will overwrite the ones in
> slave[i].fifo_options: in other words, global overrides local. Usually,
> people want it the other way around.
>
This is executed before open_slave() function is run for that slave. So 
the options "inherited" from global tee->fifo_options will be 
overwritten by per-slave options in open_slave(). I think this is OK.
>> +        if (ret < 0)
>> +            goto fail;
>> +
>>           if ((ret = open_slave(avf, slaves[i], &tee->slaves[i])) < 0) {
>>               ret = tee_process_slave_failure(avf, i, ret);
>>               if (ret < 0)
> In short, there are a few points that could be IMHO slightly better, but
> I think the patch can go in as is if you want to move forward, possibly
> with a few /* TODO */ (but no need to send the patch again if you add
> them).
>
> Hum. Now I realize I have some doubts about the way the options work.
> But with the current state of the options parsing, I am not sure we can
> do much better.
>
> Maybe a small suggestion: instead of stealing the fifo_options option,
> steal all options starting with "fifo." (av_dict_get() can do that).
> That would avoid one level of escaping. But it can also be added later
> if you prefer.
I like this idea and have implemented it, now I am wondering if I should 
keep both possibilities of how to pass options to fifo muxer... What do 
you think?

Also, I am sorry for such late response to your review...

Thank you,

Regards,
Jan S.