[FFmpeg-devel] [PATCH v4 3/3] avformat/tee: Handling slave failure in tee muxer
Jan Sebechlebsky
sebechlebskyjan at gmail.com
Fri Apr 8 13:18:50 CEST 2016
On 04/07/2016 12:37 AM, Marton Balint wrote:
>
> On Mon, 4 Apr 2016, sebechlebskyjan at gmail.com wrote:
>
>> From: Jan Sebechlebsky <sebechlebskyjan at gmail.com>
>>
>> Adds per slave option 'onfail' to the tee muxer allowing an output to
>> fail,so other slave outputs can continue.
>>
>> Signed-off-by: Jan Sebechlebsky <sebechlebskyjan at gmail.com>
>> ---
>> I've just added topic to commit message title as Marton Balint
>> suggested.
>>
>> doc/muxers.texi | 14 ++++++++
>> libavformat/tee.c | 96
>> +++++++++++++++++++++++++++++++++++++++++++++++++------
>> 2 files changed, 100 insertions(+), 10 deletions(-)
>>
>> diff --git a/doc/muxers.texi b/doc/muxers.texi
>> index 2aafbad..2d63083 100644
>> --- a/doc/muxers.texi
>> +++ b/doc/muxers.texi
>> @@ -1372,6 +1372,12 @@ Select the streams that should be mapped to
>> the slave output,
>> specified by a stream specifier. If not specified, this defaults to
>> all the input streams. You may use multiple stream specifiers
>> separated by commas (@code{,}) e.g.: @code{a:0,v}
>> +
>> + at item onfail
>> +Specify behaviour on output failure. This can be set to either
>> @code{abort} (which is
>> +default) or @code{ignore}. @code{abort} will cause whole process to
>> fail in case of failure
>> +on this slave output. @code{ignore} will ignore failure on this
>> output, so other outputs
>> +will continue without being affected.
>> @end table
>>
>> @subsection Examples
>> @@ -1386,6 +1392,14 @@ ffmpeg -i ... -c:v libx264 -c:a mp2 -f tee
>> -map 0:v -map 0:a
>> @end example
>>
>> @item
>> +As above, but continue streaming even if output to local file fails
>> +(for example local drive fills up):
>> + at example
>> +ffmpeg -i ... -c:v libx264 -c:a mp2 -f tee -map 0:v -map 0:a
>> + "[onfail=ignore]archive-20121107.mkv|[f=mpegts]udp://10.0.1.255:1234/"
>> + at end example
>> +
>> + at item
>> Use @command{ffmpeg} to encode the input, and send the output
>> to three different destinations. The @code{dump_extra} bitstream
>> filter is used to add extradata information to all the output video
>> diff --git a/libavformat/tee.c b/libavformat/tee.c
>> index d823805..50aaf9f 100644
>> --- a/libavformat/tee.c
>> +++ b/libavformat/tee.c
>> @@ -29,10 +29,20 @@
>>
>> #define MAX_SLAVES 16
>>
>> +typedef enum {
>> + ON_SLAVE_FAILURE_ABORT = 1,
>> + ON_SLAVE_FAILURE_IGNORE = 2
>> +} SlaveFailurePolicy;
>> +
>> +#define DEFAULT_SLAVE_FAILURE_POLICY ON_SLAVE_FAILURE_ABORT
>> +
>> typedef struct {
>> AVFormatContext *avf;
>> AVBitStreamFilterContext **bsfs; ///< bitstream filters per stream
>>
>> + SlaveFailurePolicy on_fail;
>> + unsigned char is_alive;
>> +
>> /** map from input to output streams indexes,
>> * disabled output streams are set to -1 */
>> int *stream_map;
>> @@ -41,6 +51,7 @@ typedef struct {
>> typedef struct TeeContext {
>> const AVClass *class;
>> unsigned nb_slaves;
>> + unsigned nb_alive;
>> TeeSlave slaves[MAX_SLAVES];
>> } TeeContext;
>>
>> @@ -135,6 +146,18 @@ end:
>> return ret;
>> }
>>
>> +static inline int parse_slave_failure_policy_option(const char *opt)
>> +{
>> + if (!opt) {
>> + return DEFAULT_SLAVE_FAILURE_POLICY;
>> + } else if (!av_strcasecmp("abort", opt)) {
>> + return ON_SLAVE_FAILURE_ABORT;
>> + } else if (!av_strcasecmp("ignore", opt)) {
>> + return ON_SLAVE_FAILURE_IGNORE;
>> + }
>> + return 0;
>
> Probably better, if you return AVERROR(EINVAL) in case of an invalid
> option, and check for a negative return value to detect an error, that
> is the way most functions work in ffmpeg.
I'll do that.
>
>> +}
>> +
>> static void close_slave(TeeSlave *tee_slave)
>> {
>> AVFormatContext *avf;
>> @@ -165,7 +188,8 @@ static void close_slaves(AVFormatContext *avf)
>> unsigned i;
>>
>> for (i = 0; i < tee->nb_slaves; i++) {
>> - close_slave(&tee->slaves[i]);
>> + if (tee->slaves[i].is_alive)
>> + close_slave(&tee->slaves[i]);
>> }
>> }
>>
>> @@ -175,7 +199,7 @@ static int open_slave(AVFormatContext *avf, char
>> *slave, TeeSlave *tee_slave)
>> AVDictionary *options = NULL;
>> AVDictionaryEntry *entry;
>> char *filename;
>> - char *format = NULL, *select = NULL;
>> + char *format = NULL, *select = NULL, *on_fail = NULL;
>> AVFormatContext *avf2 = NULL;
>> AVStream *st, *st2;
>> int stream_count;
>> @@ -195,6 +219,17 @@ static int open_slave(AVFormatContext *avf, char
>> *slave, TeeSlave *tee_slave)
>>
>> STEAL_OPTION("f", format);
>> STEAL_OPTION("select", select);
>> + STEAL_OPTION("onfail", on_fail);
>> +
>> + tee_slave->on_fail = parse_slave_failure_policy_option(on_fail);
>> + if (!tee_slave->on_fail) {
>> + av_log(avf, AV_LOG_ERROR,
>> + "Invalid onfail option value, valid options are
>> 'abort' and 'ignore'\n");
>> + ret = AVERROR(EINVAL);
>> + /* Set failure behaviour to abort, so invalid option error
>> will not be ignored */
>> + tee_slave->on_fail = ON_SLAVE_FAILURE_ABORT;
>> + goto end;
>> + }
>>
>> ret = avformat_alloc_output_context2(&avf2, NULL, format, filename);
>> if (ret < 0)
>> @@ -339,8 +374,11 @@ static int open_slave(AVFormatContext *avf, char
>> *slave, TeeSlave *tee_slave)
>> }
>>
>> end:
>> + if (ret < 0)
>> + close_slave(tee_slave);
>
> I may miss something, but is this necessary here? In case of an error,
> you will call tee_process_slave_failure explictly requiring not to
> close the slave... You should consider always closing the slave there,
> it would make the code easier to follow IMHO.
>
You're right, makes much more sense, I'll fix that.
>> av_free(format);
>> av_free(select);
>> + av_free(on_fail);
>> av_dict_free(&options);
>> av_freep(&tmp_select);
>> return ret;
>> @@ -370,6 +408,31 @@ static void log_slave(TeeSlave *slave, void
>> *log_ctx, int log_level)
>> }
>> }
>>
>> +static int tee_process_slave_failure(AVFormatContext *avf, unsigned
>> slave_idx,
>> + int err_n, unsigned char
>> needs_closing)
>> +{
>> + TeeContext *tee = avf->priv_data;
>> + TeeSlave *tee_slave = &tee->slaves[slave_idx];
>> +
>> + tee_slave->is_alive = 0;
>> + tee->nb_alive--;
>> +
>> + if (needs_closing)
>> + close_slave(tee_slave);
>
> If you always close the slave here, the needs_closing parameter will
> become unnecessary.
>> +
>> + if (!tee->nb_alive) {
>> + av_log(avf, AV_LOG_ERROR, "All tee outputs failed.\n");
>> + return err_n;
>> + } else if (tee_slave->on_fail == ON_SLAVE_FAILURE_ABORT) {
>> + av_log(avf, AV_LOG_ERROR, "Slave muxer #%u
>> failed,aborting.\n", slave_idx);
>
> breathing space missing in text.
>
>> + return err_n;
>> + } else {
>> + av_log(avf, AV_LOG_ERROR, "Slave muxer #%u failed,
>> continuing with %u/%u slaves.\n",
>> + slave_idx, tee->nb_alive, tee->nb_slaves);
>> + return 0;
>> + }
>> +}
>> +
>> static int tee_write_header(AVFormatContext *avf)
>> {
>> TeeContext *tee = avf->priv_data;
>> @@ -393,19 +456,25 @@ static int tee_write_header(AVFormatContext *avf)
>> filename++;
>> }
>>
>> + tee->nb_slaves = tee->nb_alive = nb_slaves;
>> +
>
> Here you already changed the order of the tee->nb_slaves assignment I
> mentioned in my comments of the second patch in the series, on the
> other hand you do not check for possible null tee_slave->avf in
> close_slave(s).
>
I'll fix that.
>> for (i = 0; i < nb_slaves; i++) {
>> - if ((ret = open_slave(avf, slaves[i], &tee->slaves[i])) < 0)
>> - goto fail;
>> - log_slave(&tee->slaves[i], avf, AV_LOG_VERBOSE);
>> + if ((ret = open_slave(avf, slaves[i], &tee->slaves[i])) < 0) {
>> + ret = tee_process_slave_failure(avf, i , ret, 0);
>
> awkward space after i variable.
>
>> + if (ret < 0)
>> + goto fail;
>> + } else {
>> + log_slave(&tee->slaves[i], avf, AV_LOG_VERBOSE);
>> + tee->slaves[i].is_alive = 1;
>> + }
>> av_freep(&slaves[i]);
>> }
>>
>> - tee->nb_slaves = nb_slaves;
>> -
>> for (i = 0; i < avf->nb_streams; i++) {
>> int j, mapped = 0;
>> for (j = 0; j < tee->nb_slaves; j++)
>> - mapped += tee->slaves[j].stream_map[i] >= 0;
>> + if (tee->slaves[j].is_alive)
>> + mapped += tee->slaves[j].stream_map[i] >= 0;
>> if (!mapped)
>> av_log(avf, AV_LOG_WARNING, "Input stream #%d is not
>> mapped "
>> "to any slave.\n", i);
>> @@ -427,6 +496,8 @@ static int tee_write_trailer(AVFormatContext *avf)
>> unsigned i;
>>
>> for (i = 0; i < tee->nb_slaves; i++) {
>> + if (!tee->slaves[i].is_alive)
>> + continue;
>> avf2 = tee->slaves[i].avf;
>> if ((ret = av_write_trailer(avf2)) < 0)
>> if (!ret_all)
>> @@ -449,6 +520,9 @@ static int tee_write_packet(AVFormatContext *avf,
>> AVPacket *pkt)
>> AVRational tb, tb2;
>>
>> for (i = 0; i < tee->nb_slaves; i++) {
>> + if (!tee->slaves[i].is_alive)
>> + continue;
>> +
>> avf2 = tee->slaves[i].avf;
>> s = pkt->stream_index;
>> s2 = tee->slaves[i].stream_map[s];
>> @@ -470,9 +544,11 @@ static int tee_write_packet(AVFormatContext
>> *avf, AVPacket *pkt)
>>
>> if ((ret =
>> av_apply_bitstream_filters(avf2->streams[s2]->codec, &pkt2,
>> tee->slaves[i].bsfs[s2])) < 0 ||
>> - (ret = av_interleaved_write_frame(avf2, &pkt2)) < 0)
>> - if (!ret_all)
>> + (ret = av_interleaved_write_frame(avf2, &pkt2)) < 0) {
>> + ret = tee_process_slave_failure(avf, i, ret, 1);
>> + if (!ret_all && ret < 0)
>> ret_all = ret;
>> + }
>> }
>> return ret_all;
>> }
>> --
>> 1.9.1
>
> Regards,
> Marton
Thanks for the review, I'm not sure if I'll be able to fix all the
issues during today or during the weekend (I'm travelling), but if not
I'll send the patch on monday. I'll also try to run several scenarios
and check it with valgrind. Hopefully on monday it will be finally
all-right and ready to be applied. (And I should probably really find
some tool to check formatting, so I don't bother you with another
missing/extra space mistakes).
Have a nice day
Jan S.
More information about the ffmpeg-devel
mailing list