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

[Moritz Barsnick]

Hi,
just some formal (non-functional) comments from me.

On Tue, Jun 28, 2016 at 13:35:41 +0200, sebechlebskyjan at gmail.com wrote:
> From: Jan Sebechlebsky <sebechlebskyjan at 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).

> + at item queue_size
> +Specify size of the queue (number of packets)

Do mention the defaults, please.

> + at 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.

> + at 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.

> + at 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.

> + at 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

> + at 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

> + at 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