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"?
+@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)?
+
+@item queue_size
+Specify size of the queue (number of packets)
+
+@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
