Hello Nicolas,
I am sending the second version of patch, I have some notes why certain
things from your feedback are not implemented (yet?) and additional
questions :)
On 06/28/2016 03:42 PM, Nicolas George wrote:
+@item block_on_overflow 0|1
+If set to 0, fully non-blocking mode will be used and in case the queue
+fills up, packets will be dropped. By default this option is set to 1,
+so in case of queue overflow the encoder will be block until muxer
+processes some of the packets.
IMHO, this should use AVFMT_FLAG_NONBLOCK.
This is not yet exposed to the user via cmd options, do you think I
should add this flag to options in option_table.h (for encoding only)?
I had to look twice to be sure it was only used in the thread.
Suggestions:
- make sure function names describe exactly where they can be called:
fifo_thread_description() for functions called in the thread,
fifo_description() for functions called from the main thread,
description() for utility functions that are used in both but do nothing
dangerous.
- move fields that are only useful in the thread to a different struct,
allocate it on the stack in the main function of the thread;
- try to make the FifoContext pointer const in the thread.
I've renamed the functions and also moved the fields accessed only from
the thread to separate structure, however I didn't make the FifoContext
const in thread - it would require creating another structure for the
fields accessed both by "main" thread and fifo thread (or some other
solution which seemed more messy to me).
+ avf2->interrupt_callback = avf->interrupt_callback;
This is wrong. First, we do not know that the application-provided callback
is thread-safe. Second, the thread needs an interrupt_callback of its own to
interrupt the actual I/O when asked to abort.
Can you please explain little bit more why is this wrong (appart from
the undocumented requirement for the interrupt callback to be thread-safe)?
+ FifoMessage message = {.type = FIFO_WRITE_HEADER};
+
+ ret = av_thread_message_queue_send(fifo->queue, &message, 0);
+ if (ret < 0)
+ return ret;
This message is sent only once. Maybe take it out of the loop to make things
simpler.
I decided not to take it out of the loop - it would be a special case to
handle in case the call would fail.
Regards,
Jan
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
