Hi,
just some formal (non-functional) comments from me.
On Tue, Jun 28, 2016 at 13:35:41 +0200, sebechlebsky...@gmail.com wrote:
> From: Jan Sebechlebsky <sebechlebsky...@gmail.com>
>
> FIFO pseudo-muxer allows to separate decoder from the
> actual output by using first-in-first-out queue and
> running actual muxer asynchronously in separate thread.
This and the documentation could use some grammar-cleaning, especially
articles. BTW, it's the encoder, not the decoder. Let me attempt:
> The fifo pseudo-muxer allows to separate the encoder from the actual
> output by using a first-in-first-out queue and running the actual
> muxer asynchronously in separate thread.
>
> It can be configured to attempt transparent recovery
> of output on failure.
More fixes by me here:
> @section fifo
>
> The fifo pseudo-muxer allows to separate encoding from any other
> muxer by using a first-in-first-out queue and running the actual
> muxer in a separate thread. This is especially useful in combination
> with the @ref{tee} muxer and output to several destinations with
> different reliability/writing speed/latency.
(Note the @ref{}.)
> The fifo muxer can operate in regular or fully non-blocking mode.
> This determines the behaviour in case the queue fills up. In regular
> mode the encoding is blocked until the muxer processes some of the
> packets from the queue; in non-blocking mode the packets are thrown
> away rather than blocking the encoder (this might be useful in
> real-time streaming scenarios).
> +@item queue_size
> +Specify size of the queue (number of packets)
Do mention the defaults, please.
> +@item block_on_overflow 0|1
I think this is usually the doc syntax:
@item block_on_overflow @var{bool}
> If set to 0 (false), 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 (true), so in case of queue overflow the encoder will be
> block until the muxer processes some of the packets.
> +@item attempt_recovery 0|1
Same here, and for all other boolean options
> +If failure happens, attempt to recover the output. This is especially useful
^^^^^^^ "occurs" seems better.
> +when used with network output, allows to restart streaming transparently.
> +By default this option is turned off.
0, false, off. So many ways of expressing it. ;-) Yet okay, I guess.
> +@item max_recovery_attempts
> +Sets maximal number of successive unsucessfull recovery attempts after which
> +the output fails permanently. Unlimited if set to zero.
- the maximal ("maximum"?) number
- unsuccessful (one 'l' only)
And please document the default.
> +@item recovery_wait_time
> +Waiting time before the next recovery attempt after previous unsuccessfull
> +recovery attempt.
- unsuccessful (one 'l' only)
- please add that it's in seconds
> +@item recovery_wait_streamtime 0|1
> +If set to 0 (default), the real time is used when waiting for the recovery
> attempt
> +(i.e. the recovery will be attempted after at least recovery_wait_time
> seconds).
> +If set to 1, the time of the processed stream is taken into account instead
> +(i.e. the recovery will be attempted after at least recovery_wait_time
> seconds
> +of the stream is ommited).
- "omitted"
- please state the default
> +@item recover_any_error 0|1
> +If set to 1, recovery will be attempted regardless of type of the error
> causing
> +the failure (by default, in case of certain errors the recovery is not
> attempted
> +even when attempt_recovery is on).
Make this two sentences instead of an appended bracket case.
> +#define FIFO_DEFAULT_RECOVERY_WAIT_TIME 1000000 // 1 second
???? You're assigning it as a default to an option which claims to be
in seconds:
> + {"recovery_wait_time", "Waiting time between recovery attempts
> (seconds)", OFFSET(recovery_wait_time),
> + AV_OPT_TYPE_DURATION, {.i64 = FIFO_DEFAULT_RECOVERY_WAIT_TIME}, 0,
> INT64_MAX, AV_OPT_FLAG_ENCODING_PARAM},
So the default you are setting happens to be 1000000 seconds. Or am I
missing something? (I could check by applying your patch. Sorry, didn't
do so.)
> + for (i = 0;i < avf2->nb_streams; i++)
Nit: spaces
> + switch (err_no) {
> + case AVERROR(EINVAL):
> + case AVERROR(ENOSYS):
> + case AVERROR_EOF:
> + case AVERROR_EXIT:
> + case AVERROR_PATCHWELCOME:
> + return 0;
> + default:
> + return 1;
> + }
I believe switch/case is indented differently in ffmpeg.
> + } while(ret == AVERROR(EAGAIN) && fifo->block_on_overflow);
Nit: space
> + if (ret < 0) {
> + return ret;
> + }
You should probably drop the brackets, according to ffmpeg style.
> + }else if (ret < 0) {
Nit: bracket
> +static const AVOption options[] = {
> + {"fifo_format", "Target muxer", OFFSET(format),
> + AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0,
> AV_OPT_FLAG_ENCODING_PARAM},
> +
> + {"queue_size", "Size of fifo queue", OFFSET(queue_size),
> + AV_OPT_TYPE_INT, {.i64 = FIFO_DEFAULT_QUEUE_SIZE}, 1, 1024,
> AV_OPT_FLAG_ENCODING_PARAM},
> +
> + {"format_opts", "Options to be passed to underlying muxer",
> OFFSET(format_options_str),
> + AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0,
> AV_OPT_FLAG_ENCODING_PARAM},
> +
> + {"block_on_overflow", "Block output on FIFO queue overflow until
> queue frees up.", OFFSET(block_on_overflow),
> + AV_OPT_TYPE_BOOL, {.i64 = 1}, 0, 1, AV_OPT_FLAG_ENCODING_PARAM},
This text ends in a dot, the others not.
> +
> + {"restart_with_keyframe", "Wait for keyframe when restarting
> output", OFFSET(restart_with_keyframe),
> + AV_OPT_TYPE_BOOL, {.i64 = 1}, 0, 1, AV_OPT_FLAG_ENCODING_PARAM},
> +
> + {"attempt_recovery", "Attempt recovery in case of failure",
> OFFSET(attempt_recovery),
> + AV_OPT_TYPE_BOOL, {.i64 = 0}, 0, 1, AV_OPT_FLAG_ENCODING_PARAM},
> +
> + {"max_recovery_attempts", "Maximal number of recovery attempts",
> OFFSET(max_recovery_attempts),
> + AV_OPT_TYPE_INT, {.i64 = FIFO_DEFAULT_MAX_RECOVERY_ATTEMPTS}, 0,
> INT_MAX, AV_OPT_FLAG_ENCODING_PARAM},
> +
> + {"recovery_wait_time", "Waiting time between recovery attempts
> (seconds)", OFFSET(recovery_wait_time),
> + AV_OPT_TYPE_DURATION, {.i64 = FIFO_DEFAULT_RECOVERY_WAIT_TIME}, 0,
> INT64_MAX, AV_OPT_FLAG_ENCODING_PARAM},
> +
> + {"recovery_wait_streamtime", "Use stream time instead of real time
> while waiting for recovery",
> + OFFSET(recovery_wait_streamtime), AV_OPT_TYPE_BOOL, {.i64 = 0}, 0,
> 1, AV_OPT_FLAG_ENCODING_PARAM},
> +
> + {"recover_any_error", "Attempt recovery regardless of type of the
> error", OFFSET(recover_any_error),
> + AV_OPT_TYPE_BOOL, {.i64 = 0}, 0, 1, AV_OPT_FLAG_ENCODING_PARAM},
> +
> + {NULL},
I *think* long lines, and no empty lines in between, are accepted for
readability here.
Moritz
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
