On 04/07/2016 12:37 AM, Marton Balint wrote:

On Mon, 4 Apr 2016, sebechlebsky...@gmail.com wrote:

From: Jan Sebechlebsky <sebechlebsky...@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 <sebechlebsky...@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}
+
+@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):
+@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/"
+@end example
+
+@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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Reply via email to