[FFmpeg-devel] [PATCH] libavformat: Add FIFO pseudo-muxer

[Jan Sebechlebsky]

Hello Nicolas,

On 06/28/2016 03:42 PM, Nicolas George wrote:
> +The fifo pseudo-muxer allows to separate encoding from any other muxer
> +by using first-in-first-out queue and running the actual muxer in separate
> +thread. This is especially useful in combination with tee muxer
> +and output to several destinations with different reliability/writing speed/latency.
> +
> +The fifo muxer can operate in regular or fully non-blocking mode. This determines
> Not true: the current code is non-blocking for packets, but not for the
> trailer. It can not be really non-blocking but as is it can block
> indefinitely.
>
>> +the behaviour in case the queue fills up. In regular mode the encoding is blocked
>> +until muxer processes some of the packets from queue, in non-blocking mode the packets
>> +are thrown away rather than blocking the encoder (this might be useful in real-time
>> +streaming scenario).
Would it be ok to call it "asynchronous"?
>> + at item fifo_format
>> +Specify the format name. Useful if it cannot be guessed from the
>> +output name suffix.
> This option is defined but never used.
I don't understand this note - the fifo_format option is used (and seems 
to work)?
>> +
>> + at item queue_size
>> +Specify size of the queue (number of packets)
>> +
>> + at item format_opts
>> +Specify format options for the underlying muxer. Muxer options can be specified
>> +as a list of @var{key}=@var{value} pairs separated by ':'.
> Yay, another trip to escaping hell!
Unfortunately :( Do you think cmd muxer initialization could be easily 
modified in a way that muxer would also get access to option dictionary?
>
>> +    if (!(avf2->oformat->flags & AVFMT_NOFILE)) {
>> +        ret = avf2->io_open(avf2, &avf2->pb, avf->filename, AVIO_FLAG_WRITE, &format_options);
>> +        if (ret < 0) {
>> +            av_log(avf, AV_LOG_ERROR, "Error opening %s: %s\n",
>> +                   avf->filename, av_err2str(ret));
>> +            goto end;
>> +        }
>> +    }
> Why do we not have a utility function for that?
>
I'll create ff_format_io_open and, this can be refactored in other 
muxer, too.

>> +    s_idx = pkt->stream_index;
>> +    src_tb = avf->streams[s_idx]->time_base;
>> +    dst_tb = avf2->streams[s_idx]->time_base;
>> +
>> +    pkt->pts = av_rescale_q(pkt->pts, src_tb, dst_tb);
>> +    pkt->dts = av_rescale_q(pkt->dts, src_tb, dst_tb);
>> +    pkt->duration = av_rescale_q(pkt->duration, src_tb, dst_tb);
> This looks suspicious.
>
> For starters, it does not handle the NOPTS value, but that is an easy fix.
>
> More worryingly, it looks that the application does not see the time base
> selected by the real muxer. It could be a problem.
I'll change it to handle NOPTS (the same should be done for tee muxer, 
so I'll patch that too). Can you specify what could be the problem when 
the application does not see the time base of real muxer? If it was 
really neccessary, I can make write_header call before actually 
executing the thread (so only write_packet calls would be non-blocking) 
and copy the selected timebase, however, I prefer write_header to be 
executed in consumer thread. Also, this situation is impossible to 
handle in tee muxer, since every slave muxer can select different time base.
>
>> +        /* Check and solve overflow */
>> +        pthread_mutex_lock(&fifo->overflow_flag_lock);
>> +        if (fifo->overflow_flag) {
>> +            av_thread_message_flush(queue);
>> +            if (fifo->restart_with_keyframe)
>> +                fifo->drop_until_keyframe = 1;
>> +            fifo->overflow_flag = 0;
>> +            just_flushed = 1;
>> +        }
>> +        pthread_mutex_unlock(&fifo->overflow_flag_lock);
> I think the communication logic here needs an extensive comment here.
>
> And I am a bit worried about the extra lock: mixing several communication
> mechanisms between threads is a recipe for headache.
I'll add the comment. I've tried to do this without the extra lock at 
first by setting error to the thread message queue and adding 
threadmessage queue flag which allows the error to be returned 
immediately, but I think using this single extra lock is really cleaner 
solution, I would prefer that.
>
>> +    avf2->io_close = avf->io_close;
>> +    avf2->io_open = avf->io_open;
> This could be dangerous too, I am not sure these functions are guaranteed to
> be thread-safe. It needs at least documentation.
I'll try to check existing functions for dangerous operations - I guess 
we could add a note to documentation saying that the io_{open,close} 
should be thread-safe.
>> +        st2->r_frame_rate        = st->r_frame_rate;
>> +        st2->time_base           = st->time_base;
>> +        st2->start_time          = st->start_time;
>> +        st2->duration            = st->duration;
>> +        st2->nb_frames           = st->nb_frames;
>> +        st2->disposition         = st->disposition;
>> +        st2->sample_aspect_ratio = st->sample_aspect_ratio;
>> +        st2->avg_frame_rate      = st->avg_frame_rate;
> Why do we not have an utility function for that (bis)?
I've actually created the function av_stream_encode_params_copy:

https://github.com/jsebechlebsky/FFmpeg/commit/c46a35b19daa0f58efb70254e30bbd767f5d62ef

but not submitted the patch yet. I was not completely sure what fields 
actually have to be copied. In segment muxer some of them are ommited. 
Tee muxer copies all of those, but've I checked documentation for all 
the fields which might be used during encoding and there is one more - 
AVPacketSideData* side_data field (together with nb_side_data) which 
might be used during muxing. So I guess I'll stick to documentation and 
copy everything which might be set during encoding according to 
documentation.

I agree with all other suggested changes and will implement them.

Thanks for feedback!

Regards,
Jan