Re: [FFmpeg-devel] [PATCH v5 2/3] avformat/tee: Fix leaks in tee muxer when open_slave fails

2016-04-17 Thread Marton Balint 


On Fri, 15 Apr 2016, Jan Sebechlebsky wrote:



On 04/15/2016 02:28 AM, Marton Balint wrote:


On Thu, 14 Apr 2016, Marton Balint wrote:



On Tue, 12 Apr 2016, sebechlebsky...@gmail.com wrote:


From: Jan Sebechlebsky 

Calling close_slave in case error is to be returned from open_slave
will free allocated resources.

Since failure can happen before bsfs array is initialized,
close_slave must check that bsfs is not NULL before accessing
tee_slave->bsfs[i] element.

Slave muxer expects write_trailer to be called if it's
write_header suceeded (so resources allocated in write_header
are freed). Therefore if failure happens after successfull
write_header call, we must ensure that write_trailer of
that particular slave is called.


Hmm, I guess you are right, I see no other way freeing resources allocated 
in write_header then calling write_trailer. It does make the code a bit 
more complex, but I don't really see a way to make it more simple.


So this looks good to me. Nicolas, any ideas improving this?



Actually I have given this some additional thought, and by using a new 
per-slave variable to keep the information if the header was written or 
not, I think your patch can be simplifed, also close_slave can be changed 
so it will write the trailer if necessary, in fact, the write_trailer 
function can use it as well.


I have modified your patch (see attached), could you please test/review it 
and check if I missed anything? I hope you don't mind, this kind of 
collaborative work is not that common in ffmpeg, but in this case it seemed 
easier moving those few lines around than describing what I wanted.


Thanks,
Marton


Hello Marton,
I'm ok with it, you're right it is more elegant this way. I've tested it and 
it seems allright. I've recreated the last patch on top of these changes and 
I'm sending it in attachment (and I am also cc-ing this mail to Nicolas, so 
he can review the patches).




[...]


@@ -423,8 +486,9 @@ static int tee_write_header(AVFormatContext *avf)
 return 0;

 fail:
-for (i = 0; i < nb_slaves; i++)
+for (i = 0; i < nb_slaves; i++) {
 av_freep([i]);
+}


This seems a no-op change.

[...]



 static int tee_write_trailer(AVFormatContext *avf)
 {
 TeeContext *tee = avf->priv_data;
+AVFormatContext *avf2;
 int ret_all = 0, ret;
 unsigned i;

 for (i = 0; i < tee->nb_slaves; i++) {
-if ((ret = close_slave(>slaves[i])) < 0)
-if (!ret_all)
+if (!(avf2 = tee->slaves[i].avf))
+continue;
+if ((ret = close_slave(>slaves[i])) < 0) {
+ret = tee_process_slave_failure(avf2, i, ret);
+if (!ret_all && ret < 0)
 ret_all = ret;
+}
 }
+close_slaves(avf);


I guess you don't need close_slaves here, because you already closed all 
slaves.


Regards,
Marton
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v5 2/3] avformat/tee: Fix leaks in tee muxer when open_slave fails

2016-04-15 Thread Jan Sebechlebsky 


On 04/15/2016 02:28 AM, Marton Balint wrote:


On Thu, 14 Apr 2016, Marton Balint wrote:



On Tue, 12 Apr 2016, sebechlebsky...@gmail.com wrote:


From: Jan Sebechlebsky 

Calling close_slave in case error is to be returned from open_slave
will free allocated resources.

Since failure can happen before bsfs array is initialized,
close_slave must check that bsfs is not NULL before accessing
tee_slave->bsfs[i] element.

Slave muxer expects write_trailer to be called if it's
write_header suceeded (so resources allocated in write_header
are freed). Therefore if failure happens after successfull
write_header call, we must ensure that write_trailer of
that particular slave is called.


Hmm, I guess you are right, I see no other way freeing resources 
allocated in write_header then calling write_trailer. It does make 
the code a bit more complex, but I don't really see a way to make it 
more simple.


So this looks good to me. Nicolas, any ideas improving this?



Actually I have given this some additional thought, and by using a new 
per-slave variable to keep the information if the header was written 
or not, I think your patch can be simplifed, also close_slave can be 
changed so it will write the trailer if necessary, in fact, the 
write_trailer function can use it as well.


I have modified your patch (see attached), could you please 
test/review it and check if I missed anything? I hope you don't mind, 
this kind of collaborative work is not that common in ffmpeg, but in 
this case it seemed easier moving those few lines around than 
describing what I wanted.


Thanks,
Marton


Hello Marton,
I'm ok with it, you're right it is more elegant this way. I've tested it 
and it seems allright. I've recreated the last patch on top of these 
changes and I'm sending it in attachment (and I am also cc-ing this mail 
to Nicolas, so he can review the patches).


Have a nice day!

Jan

>From 81901087b1cda032fc9688f436974578359bfb36 Mon Sep 17 00:00:00 2001
From: Jan Sebechlebsky 
Date: Fri, 15 Apr 2016 17:33:59 +0300
Subject: [PATCH v7 3/3] avformat/tee: Handling slave failure in tee muxer

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 
---
 doc/muxers.texi   | 14 
 libavformat/tee.c | 98 ---
 2 files changed, 100 insertions(+), 12 deletions(-)

diff --git a/doc/muxers.texi b/doc/muxers.texi
index 042efce..c62d4b5 100644
--- a/doc/muxers.texi
+++ b/doc/muxers.texi
@@ -1453,6 +1453,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
@@ -1467,6 +1473,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 753f7ea..2aa3071 100644
--- a/libavformat/tee.c
+++ b/libavformat/tee.c
@@ -29,10 +29,19 @@
 
 #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;
+
 /** map from input to output streams indexes,
  * disabled output streams are set to -1 */
 int *stream_map;
@@ -42,6 +51,7 @@ typedef struct {
 typedef struct TeeContext {
 const AVClass *class;
 unsigned nb_slaves;
+unsigned nb_alive;
 TeeSlave slaves[MAX_SLAVES];
 } TeeContext;
 
@@ -136,6 +146,23 @@ end:
 return ret;
 }
 
+static inline int parse_slave_failure_policy_option(const char *opt, TeeSlave *tee_slave)
+{
+if (!opt) {
+tee_slave->on_fail = DEFAULT_SLAVE_FAILURE_POLICY;
+return 0;
+} else if (!av_strcasecmp("abort", opt)) {
+tee_slave->on_fail = ON_SLAVE_FAILURE_ABORT;

Re: [FFmpeg-devel] [PATCH v5 2/3] avformat/tee: Fix leaks in tee muxer when open_slave fails

2016-04-14 Thread Marton Balint 


On Thu, 14 Apr 2016, Marton Balint wrote:



On Tue, 12 Apr 2016, sebechlebsky...@gmail.com wrote:


From: Jan Sebechlebsky 

Calling close_slave in case error is to be returned from open_slave
will free allocated resources.

Since failure can happen before bsfs array is initialized,
close_slave must check that bsfs is not NULL before accessing
tee_slave->bsfs[i] element.

Slave muxer expects write_trailer to be called if it's
write_header suceeded (so resources allocated in write_header
are freed). Therefore if failure happens after successfull
write_header call, we must ensure that write_trailer of
that particular slave is called.


Hmm, I guess you are right, I see no other way freeing 
resources allocated in write_header then calling write_trailer. It does 
make the code a bit more complex, but I don't really see a way to make it 
more simple.


So this looks good to me. Nicolas, any ideas improving this?



Actually I have given this some additional thought, and by using a new 
per-slave variable to keep the information if the header was written or 
not, I think your patch can be simplifed, also close_slave can be changed 
so it will write the trailer if necessary, in fact, the write_trailer 
function can use it as well.


I have modified your patch (see attached), could you please test/review it 
and check if I missed anything? I hope you don't mind, this kind of 
collaborative work is not that common in ffmpeg, but in this case it 
seemed easier moving those few lines around than describing what I wanted.


Thanks,
MartonFrom 1dc441476841512e97efda8eab515cce2097f382 Mon Sep 17 00:00:00 2001
From: Jan Sebechlebsky 
Date: Tue, 12 Apr 2016 20:46:28 +0300
Subject: [PATCH] avformat/tee: Fix leaks in tee muxer when open_slave fails

In open_slave failure can happen before bsfs array is initialized,
close_slave must check that bsfs is not NULL before accessing
tee_slave->bsfs[i] element.

Slave muxer expects write_trailer to be called if it's
write_header suceeded (so resources allocated in write_header
are freed). Therefore if failure happens after successfull
write_header call, we must ensure that write_trailer of
that particular slave is called.

Some cleanups are made by Marton Balint.

Signed-off-by: Jan Sebechlebsky 
Signed-off-by: Marton Balint 
---
 libavformat/tee.c | 42 +-
 1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/libavformat/tee.c b/libavformat/tee.c
index ab6cd32..753f7ea 100644
--- a/libavformat/tee.c
+++ b/libavformat/tee.c
@@ -36,6 +36,7 @@ typedef struct {
 /** map from input to output streams indexes,
  * disabled output streams are set to -1 */
 int *stream_map;
+int header_written;
 } TeeSlave;
 
 typedef struct TeeContext {
@@ -135,18 +136,27 @@ end:
 return ret;
 }
 
-static void close_slave(TeeSlave *tee_slave)
+static int close_slave(TeeSlave *tee_slave)
 {
 AVFormatContext *avf;
 unsigned i;
+int ret = 0;
 
 avf = tee_slave->avf;
-for (i = 0; i < avf->nb_streams; ++i) {
-AVBitStreamFilterContext *bsf_next, *bsf = tee_slave->bsfs[i];
-while (bsf) {
-bsf_next = bsf->next;
-av_bitstream_filter_close(bsf);
-bsf = bsf_next;
+if (!avf)
+return 0;
+
+if (tee_slave->header_written)
+ret = av_write_trailer(avf);
+
+if (tee_slave->bsfs) {
+for (i = 0; i < avf->nb_streams; ++i) {
+AVBitStreamFilterContext *bsf_next, *bsf = tee_slave->bsfs[i];
+while (bsf) {
+bsf_next = bsf->next;
+av_bitstream_filter_close(bsf);
+bsf = bsf_next;
+}
 }
 }
 av_freep(_slave->stream_map);
@@ -155,6 +165,7 @@ static void close_slave(TeeSlave *tee_slave)
 ff_format_io_close(avf, >pb);
 avformat_free_context(avf);
 tee_slave->avf = NULL;
+return ret;
 }
 
 static void close_slaves(AVFormatContext *avf)
@@ -197,6 +208,7 @@ static int open_slave(AVFormatContext *avf, char *slave, TeeSlave *tee_slave)
 ret = avformat_alloc_output_context2(, NULL, format, filename);
 if (ret < 0)
 goto end;
+tee_slave->avf = avf2;
 av_dict_copy(>metadata, avf->metadata, 0);
 avf2->opaque   = avf->opaque;
 avf2->io_open  = avf->io_open;
@@ -275,8 +287,8 @@ static int open_slave(AVFormatContext *avf, char *slave, TeeSlave *tee_slave)
slave, av_err2str(ret));
 goto end;
 }
+tee_slave->header_written = 1;
 
-tee_slave->avf = avf2;
 tee_slave->bsfs = av_calloc(avf2->nb_streams, sizeof(TeeSlave));
 if (!tee_slave->bsfs) {
 ret = AVERROR(ENOMEM);
@@ -291,7 +303,8 @@ static int open_slave(AVFormatContext *avf, char *slave, TeeSlave *tee_slave)
 av_log(avf, AV_LOG_ERROR,
"Specifier separator in '%s' is '%c', but only

Re: [FFmpeg-devel] [PATCH v5 2/3] avformat/tee: Fix leaks in tee muxer when open_slave fails

2016-04-13 Thread Marton Balint 


On Tue, 12 Apr 2016, sebechlebsky...@gmail.com wrote:


From: Jan Sebechlebsky 

Calling close_slave in case error is to be returned from open_slave
will free allocated resources.

Since failure can happen before bsfs array is initialized,
close_slave must check that bsfs is not NULL before accessing
tee_slave->bsfs[i] element.

Slave muxer expects write_trailer to be called if it's
write_header suceeded (so resources allocated in write_header
are freed). Therefore if failure happens after successfull
write_header call, we must ensure that write_trailer of
that particular slave is called.


Hmm, I guess you are right, I see no other way freeing 
resources allocated in write_header then calling write_trailer. It does 
make the code a bit more complex, but I don't really see a way to make it 
more simple.


So this looks good to me. Nicolas, any ideas improving this?

Thanks,
Marton
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH v5 2/3] avformat/tee: Fix leaks in tee muxer when open_slave fails

2016-04-12 Thread sebechlebskyjan 
From: Jan Sebechlebsky 

Calling close_slave in case error is to be returned from open_slave
will free allocated resources.

Since failure can happen before bsfs array is initialized,
close_slave must check that bsfs is not NULL before accessing
tee_slave->bsfs[i] element.

Slave muxer expects write_trailer to be called if it's
write_header suceeded (so resources allocated in write_header
are freed). Therefore if failure happens after successfull
write_header call, we must ensure that write_trailer of
that particular slave is called.

Signed-off-by: Jan Sebechlebsky 
---
 libavformat/tee.c | 44 +---
 1 file changed, 29 insertions(+), 15 deletions(-)

diff --git a/libavformat/tee.c b/libavformat/tee.c
index ab6cd32..222826a 100644
--- a/libavformat/tee.c
+++ b/libavformat/tee.c
@@ -141,12 +141,17 @@ static void close_slave(TeeSlave *tee_slave)
 unsigned i;
 
 avf = tee_slave->avf;
-for (i = 0; i < avf->nb_streams; ++i) {
-AVBitStreamFilterContext *bsf_next, *bsf = tee_slave->bsfs[i];
-while (bsf) {
-bsf_next = bsf->next;
-av_bitstream_filter_close(bsf);
-bsf = bsf_next;
+if (!avf)
+return;
+
+if (tee_slave->bsfs) {
+for (i = 0; i < avf->nb_streams; ++i) {
+AVBitStreamFilterContext *bsf_next, *bsf = tee_slave->bsfs[i];
+while (bsf) {
+bsf_next = bsf->next;
+av_bitstream_filter_close(bsf);
+bsf = bsf_next;
+}
 }
 }
 av_freep(_slave->stream_map);
@@ -197,6 +202,7 @@ static int open_slave(AVFormatContext *avf, char *slave, 
TeeSlave *tee_slave)
 ret = avformat_alloc_output_context2(, NULL, format, filename);
 if (ret < 0)
 goto end;
+tee_slave->avf = avf2;
 av_dict_copy(>metadata, avf->metadata, 0);
 avf2->opaque   = avf->opaque;
 avf2->io_open  = avf->io_open;
@@ -276,11 +282,10 @@ static int open_slave(AVFormatContext *avf, char *slave, 
TeeSlave *tee_slave)
 goto end;
 }
 
-tee_slave->avf = avf2;
 tee_slave->bsfs = av_calloc(avf2->nb_streams, sizeof(TeeSlave));
 if (!tee_slave->bsfs) {
 ret = AVERROR(ENOMEM);
-goto end;
+goto fail;
 }
 
 entry = NULL;
@@ -291,7 +296,8 @@ static int open_slave(AVFormatContext *avf, char *slave, 
TeeSlave *tee_slave)
 av_log(avf, AV_LOG_ERROR,
"Specifier separator in '%s' is '%c', but only 
characters '%s' "
"are allowed\n", entry->key, *spec, 
slave_bsfs_spec_sep);
-return AVERROR(EINVAL);
+ret = AVERROR(EINVAL);
+goto fail;
 }
 spec++; /* consume separator */
 }
@@ -302,7 +308,7 @@ static int open_slave(AVFormatContext *avf, char *slave, 
TeeSlave *tee_slave)
 av_log(avf, AV_LOG_ERROR,
"Invalid stream specifier '%s' in bsfs option '%s' for 
slave "
"output '%s'\n", spec, entry->key, filename);
-goto end;
+goto fail;
 }
 
 if (ret > 0) {
@@ -319,7 +325,7 @@ static int open_slave(AVFormatContext *avf, char *slave, 
TeeSlave *tee_slave)
 av_log(avf, AV_LOG_ERROR,
"Error parsing bitstream filter sequence '%s' 
associated to "
"stream %d of slave output '%s'\n", entry->value, 
i, filename);
-goto end;
+goto fail;
 }
 }
 }
@@ -332,7 +338,7 @@ static int open_slave(AVFormatContext *avf, char *slave, 
TeeSlave *tee_slave)
 while ((entry = av_dict_get(options, "", entry, 
AV_DICT_IGNORE_SUFFIX)))
 av_log(avf2, AV_LOG_ERROR, "Unknown option '%s'\n", entry->key);
 ret = AVERROR_OPTION_NOT_FOUND;
-goto end;
+goto fail;
 }
 
 end:
@@ -341,6 +347,9 @@ end:
 av_dict_free();
 av_freep(_select);
 return ret;
+fail:
+av_write_trailer(avf2);
+goto end;
 }
 
 static void log_slave(TeeSlave *slave, void *log_ctx, int log_level)
@@ -390,15 +399,18 @@ static int tee_write_header(AVFormatContext *avf)
 filename++;
 }
 
+tee->nb_slaves = 0;
+
 for (i = 0; i < nb_slaves; i++) {
-if ((ret = open_slave(avf, slaves[i], >slaves[i])) < 0)
+if ((ret = open_slave(avf, slaves[i], >slaves[i])) < 0) {
+close_slave(>slaves[i]);
 goto fail;
+}
 log_slave(>slaves[i], avf, AV_LOG_VERBOSE);
 av_freep([i]);
+tee->nb_slaves++;
 }
 
-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++)
@@ -412,6 +424,8 @@ static int tee_write_header(AVFormatContext *avf)
 fail:
 for (i = 0; i <