Re: [FFmpeg-devel] [PATCH] libavformat/fifo: Fix initialization of underlying AVFormatContext

[Jan Sebechlebsky]

On 07/13/2017 01:15 PM, Jan Sebechlebsky wrote:


I'll apply the patch in a few days with modified commit message.

Jan

Applied...
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavformat/fifo: Fix initialization of underlying AVFormatContext

[Jan Sebechlebsky]

On 07/06/2017 11:54 PM, Jan Sebechlebsky wrote:


On 07/06/2017 04:28 PM, wm4 wrote:


On Thu, 6 Jul 2017 16:16:12 +0200
Jan Sebechlebsky <sebechlebsky...@gmail.com> wrote:


On 07/06/2017 01:15 PM, wm4 wrote:


On Thu,  6 Jul 2017 13:05:14 +0200
sebechlebsky...@gmail.com wrote:

For what reason?

For example RTSP muxer attempts to access filename (url) from
AVFormatContext to extract connection parameters and open connection.
When the filename is not set, it initializes the connection with 
default

parameters, which is the cause of bug #6308 from trac:

https://trac.ffmpeg.org/ticket/6308

This commit fixes #6308 and possibly other problems with muxers
accessing filename from AVFormatContext.

Then that should go into the commit message. Nobody likes to lookup
trac and try to make sense of the user confusion there. Also, trac
could get replaced or somehow reset in the future, like it happened in
the past.

Ok, I'll modify the commit message before pushing to:

libavformat/fifo: Fix initialization of underlying AVFormatContext

Muxers may want to directly access filename in stored in AVFormatContext.
For example in case of RTSP, the filename (url) is used by the muxer to
extract parameters of the connection. These muxers will fail when used
with fifo pseudo-muxer.

This commit fixes this issue by passing filename from AVFormatContext
of fifo pseudo-muxer to all AVFormatContext(s) of underlying muxers 
during

initialization.

I'll apply the patch in a few days with modified commit message.

Jan
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavformat/fifo: Fix initialization of underlying AVFormatContext

[Jan Sebechlebsky]

On 07/06/2017 04:28 PM, wm4 wrote:


On Thu, 6 Jul 2017 16:16:12 +0200
Jan Sebechlebsky <sebechlebsky...@gmail.com> wrote:


On 07/06/2017 01:15 PM, wm4 wrote:


On Thu,  6 Jul 2017 13:05:14 +0200
sebechlebsky...@gmail.com wrote:

For what reason?

For example RTSP muxer attempts to access filename (url) from
AVFormatContext to extract connection parameters and open connection.
When the filename is not set, it initializes the connection with default
parameters, which is the cause of bug #6308 from trac:

https://trac.ffmpeg.org/ticket/6308

This commit fixes #6308 and possibly other problems with muxers
accessing filename from AVFormatContext.

Then that should go into the commit message. Nobody likes to lookup
trac and try to make sense of the user confusion there. Also, trac
could get replaced or somehow reset in the future, like it happened in
the past.

Ok, I'll modify the commit message before pushing to:

libavformat/fifo: Fix initialization of underlying AVFormatContext

Muxers may want to directly access filename in stored in AVFormatContext.
For example in case of RTSP, the filename (url) is used by the muxer to
extract parameters of the connection. These muxers will fail when used
with fifo pseudo-muxer.

This commit fixes this issue by passing filename from AVFormatContext
of fifo pseudo-muxer to all AVFormatContext(s) of underlying muxers during
initialization.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v2] libavformat/tee: Add fifo support for tee

[Jan Sebechlebsky]

On 10/31/2016 04:56 PM, Nicolas George wrote:


+if (av_match_name(use_fifo, "true,y,yes,enable,enabled,on,1")) {
+tee_slave->use_fifo = 1;
+} else if (av_match_name(use_fifo, 
"false,n,no,disable,disabled,off,0")) {

I am not happy about the duplication of the tests from
set_string_bool(), but it cannot be avoided for now without more
unrelated work.

(I really with each option type came with the corresponding silent and
verbose parsing and dump functions. Unfortunately, this discipline was
not kept in the past.)
It may be worth refactoring just these tests setting variable to 0/1/-1 
based on boolean/"auto" string to separate function. If you agree I can 
do this in next patch

+ret = avformat_alloc_output_context2(, NULL, "fifo", filename);
+} else {
+ret = avformat_alloc_output_context2(, NULL, format, filename);

You could probably factor these two lines with
"use_fifo ? "fifo" : format".

Thanks, looks nicer that way, I've modified it.

+}
  if (ret < 0)
  goto end;
  tee_slave->avf = avf2;
@@ -394,6 +467,12 @@ static int tee_write_header(AVFormatContext *avf)
  filename++;
  }
  
+if (tee->fifo_options_str) {

+ret = av_dict_parse_string(>fifo_options, tee->fifo_options_str, "=", 
":", 0);
+if (ret < 0)
+goto fail;
+}
+
  if (!(tee->slaves = av_mallocz_array(nb_slaves, sizeof(*tee->slaves {
  ret = AVERROR(ENOMEM);
  goto fail;
@@ -401,6 +480,12 @@ static int tee_write_header(AVFormatContext *avf)
  tee->nb_slaves = tee->nb_alive = nb_slaves;
  
  for (i = 0; i < nb_slaves; i++) {

+
+tee->slaves[i].use_fifo = tee->use_fifo;
+ret = av_dict_copy(>slaves[i].fifo_options, tee->fifo_options, 0);

Unless I am mistaken, if there are keys present in both dicts, the
entries in tee->fifo_options will overwrite the ones in
slave[i].fifo_options: in other words, global overrides local. Usually,
people want it the other way around.

This is executed before open_slave() function is run for that slave. So 
the options "inherited" from global tee->fifo_options will be 
overwritten by per-slave options in open_slave(). I think this is OK.

+if (ret < 0)
+goto fail;
+
  if ((ret = open_slave(avf, slaves[i], >slaves[i])) < 0) {
  ret = tee_process_slave_failure(avf, i, ret);
  if (ret < 0)

In short, there are a few points that could be IMHO slightly better, but
I think the patch can go in as is if you want to move forward, possibly
with a few /* TODO */ (but no need to send the patch again if you add
them).

Hum. Now I realize I have some doubts about the way the options work.
But with the current state of the options parsing, I am not sure we can
do much better.

Maybe a small suggestion: instead of stealing the fifo_options option,
steal all options starting with "fifo." (av_dict_get() can do that).
That would avoid one level of escaping. But it can also be added later
if you prefer.
I like this idea and have implemented it, now I am wondering if I should 
keep both possibilities of how to pass options to fifo muxer... What do 
you think?


Also, I am sorry for such late response to your review...

Thank you,

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

Re: [FFmpeg-devel] [PATCH 2/2] lavf/fifo: fix undefined behaviour in deinit when destroying mutex

[Jan Sebechlebsky]

On 11/12/2016 02:23 AM, Marton Balint wrote:


Signed-off-by: Marton Balint 
---
  libavformat/fifo.c | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/libavformat/fifo.c b/libavformat/fifo.c
index 15435fe..8f525e5 100644
--- a/libavformat/fifo.c
+++ b/libavformat/fifo.c
@@ -73,6 +73,7 @@ typedef struct FifoContext {
  int restart_with_keyframe;
  
  pthread_mutex_t overflow_flag_lock;

+int overflow_flag_lock_initialized;
  /* Value > 0 signals queue overflow */
  volatile uint8_t overflow_flag;
  
@@ -515,6 +516,7 @@ static int fifo_init(AVFormatContext *avf)

  ret = pthread_mutex_init(>overflow_flag_lock, NULL);
  if (ret < 0)
  return AVERROR(ret);
+fifo->overflow_flag_lock_initialized = 1;
  
  return 0;

  }
@@ -601,7 +603,8 @@ static void fifo_deinit(AVFormatContext *avf)
  av_dict_free(>format_options);
  avformat_free_context(fifo->avf);
  av_thread_message_queue_free(>queue);
-pthread_mutex_destroy(>overflow_flag_lock);
+if (fifo->overflow_flag_lock_initialized)
+pthread_mutex_destroy(>overflow_flag_lock);
  }
  
  #define OFFSET(x) offsetof(FifoContext, x)

LGTM, thanks! :)

Reviewed-by: jsebechlebsky

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

Re: [FFmpeg-devel] [PATCH v2] libavformat/tee: Add fifo support for tee

[Jan Sebechlebsky]

On 10/17/2016 01:13 PM, sebechlebsky...@gmail.com wrote:


From: Jan Sebechlebsky <sebechlebsky...@gmail.com>

Signed-off-by: Jan Sebechlebsky <sebechlebsky...@gmail.com>
---
  Thanks for noticing, I've fixed the patch
  (also some minor formatting issues I've noticed).

  doc/muxers.texi   | 20 +
  libavformat/tee.c | 87 ++-
  2 files changed, 106 insertions(+), 1 deletion(-)



[...]

Can I push this? - I am afraid I accidentally didn't send it as a reply 
to the Michael's review:

http://ffmpeg.org/pipermail/ffmpeg-devel/2016-October/200763.html

Double-free is fixed in this version.

Best regards,
J. S.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/tests/gitignore: add fifo_muxer entry

[Jan Sebechlebsky]

On 10/15/2016 06:32 PM, Zhao Zhili wrote:


 From a3ee9afc37331315e4ec57f1ebf881779b62bf60 Mon Sep 17 00:00:00 2001
From: Zhao Zhili 
Date: Sun, 16 Oct 2016 00:09:25 +0800
Subject: [PATCH] avformat/tests/gitignore: add fifo_muxer entry

---
  libavformat/tests/.gitignore | 1 +
  1 file changed, 1 insertion(+)

diff --git a/libavformat/tests/.gitignore b/libavformat/tests/.gitignore
index c8adb86..7ceb7a3 100644
--- a/libavformat/tests/.gitignore
+++ b/libavformat/tests/.gitignore
@@ -1,3 +1,4 @@
+/fifo_muxer
  /movenc
  /noproxy
  /rtmpdh

LGTM, thanks!
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/2] avformat/tee: Add FATE tests for tee

[Jan Sebechlebsky]

On 09/29/2016 12:49 AM, sebechlebsky...@gmail.com wrote:


From: Jan Sebechlebsky <sebechlebsky...@gmail.com>

This commit also adds new diff option for fate tests allowing do compare
multiple tuples of files.

Signed-off-by: Jan Sebechlebsky <sebechlebsky...@gmail.com>
---
  tests/Makefile|  1 +
  tests/fate-run.sh |  7 
  tests/fate/tee-muxer.mak  | 22 ++
  tests/ref/fate/tee-muxer-h264 |  2 +
  tests/ref/fate/tee-muxer-h264-audio   | 30 +
  tests/ref/fate/tee-muxer-h264-copy| 47 +
  tests/ref/fate/tee-muxer-ignorefail   | 79 +++
  tests/ref/fate/tee-muxer-tstsrc   |  2 +
  tests/ref/fate/tee-muxer-tstsrc-audio | 49 ++
  9 files changed, 239 insertions(+)
  create mode 100644 tests/fate/tee-muxer.mak
  create mode 100644 tests/ref/fate/tee-muxer-h264
  create mode 100644 tests/ref/fate/tee-muxer-h264-audio
  create mode 100644 tests/ref/fate/tee-muxer-h264-copy
  create mode 100644 tests/ref/fate/tee-muxer-ignorefail
  create mode 100644 tests/ref/fate/tee-muxer-tstsrc
  create mode 100644 tests/ref/fate/tee-muxer-tstsrc-audio

[...]

Ping :)
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] avformat/tee: Copy interrupt callback and flags to slave

[Jan Sebechlebsky]

On 09/29/2016 09:03 AM, Nicolas George wrote:


L'octidi 8 vendémiaire, an CCXXV, sebechlebsky...@gmail.com a écrit :

From: Jan Sebechlebsky <sebechlebsky...@gmail.com>

Copy interrupt callback to slave format context to allow
user to interrupt IO. Copy format flags as well.

Signed-off-by: Jan Sebechlebsky <sebechlebsky...@gmail.com>
---
  libavformat/tee.c | 2 ++
  1 file changed, 2 insertions(+)

LGTM, thanks.

Regards,


Applied

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

Re: [FFmpeg-devel] [PATCH v5 5/5] avformat/tee: Use BSF list API

[Jan Sebechlebsky]

On 09/10/2016 10:48 AM, Nicolas George wrote:

Le sextidi 16 fructidor, an CCXXIV, sebechlebsky...@gmail.com a écrit :

From: Jan Sebechlebsky <sebechlebsky...@gmail.com>

Signed-off-by: Jan Sebechlebsky <sebechlebsky...@gmail.com>
---
  Changes since the last version of the patch:
  - added check for  avcodec_parameters_copy return value
  - fixed stray space
  - rewritten cycle receiving packets from bsf so case when 
av_interleaved_write_frame
returns EAGAIN is treated as error.

There is a stray empty line at the end. LGTM, thanks.

I've removed stray empty line localy and applied the patch.

Thanks

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

Re: [FFmpeg-devel] [PATCH v5 5/5] avformat/tee: Use BSF list API

[Jan Sebechlebsky]

On 09/05/2016 05:42 PM, Jan Sebechlebsky wrote:


On 09/01/2016 09:42 PM, sebechlebsky...@gmail.com wrote:


From: Jan Sebechlebsky <sebechlebsky...@gmail.com>

Signed-off-by: Jan Sebechlebsky <sebechlebsky...@gmail.com>
---
  Changes since the last version of the patch:
  - added check for  avcodec_parameters_copy return value
  - fixed stray space
  - rewritten cycle receiving packets from bsf so case when 
av_interleaved_write_frame

returns EAGAIN is treated as error.

  libavformat/tee.c | 137 
--

  1 file changed, 70 insertions(+), 67 deletions(-)


Can I push this?

Regards,
Jan

If noone objects, I'll push this in few days.

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

Re: [FFmpeg-devel] [PATCH v5 5/5] avformat/tee: Use BSF list API

[Jan Sebechlebsky]

On 09/01/2016 09:42 PM, sebechlebsky...@gmail.com wrote:


From: Jan Sebechlebsky <sebechlebsky...@gmail.com>

Signed-off-by: Jan Sebechlebsky <sebechlebsky...@gmail.com>
---
  Changes since the last version of the patch:
  - added check for  avcodec_parameters_copy return value
  - fixed stray space
  - rewritten cycle receiving packets from bsf so case when 
av_interleaved_write_frame
returns EAGAIN is treated as error.

  libavformat/tee.c | 137 --
  1 file changed, 70 insertions(+), 67 deletions(-)


Can I push this?

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

Re: [FFmpeg-devel] [PATCH v2 04/11] avformat/muxers: Add non-blocking mode support for av_write_trailer

[Jan Sebechlebsky]

On 08/26/2016 12:59 AM, Jan Sebechlebsky wrote:

On 08/22/2016 04:49 PM, Michael Niedermayer wrote:


On Mon, Aug 22, 2016 at 04:27:16PM +0200, Jan Sebechlebsky wrote:


On 08/22/2016 09:51 AM, Michael Niedermayer wrote:
On Thu, Aug 11, 2016 at 02:38:29PM +0200, sebechlebsky...@gmail.com 
wrote:

From: Jan Sebechlebsky <sebechlebsky...@gmail.com>

This makes av_write_trailer not to free the resources if 
write_trailer
call returns AVERROR(EAGAIN) allowing repeated calls of 
write_trailer of

non-blocking muxer.

Signed-off-by: Jan Sebechlebsky <sebechlebsky...@gmail.com>
---
  Changes since the last version of the patch:
  - Added assert to the part of the code dealing with flushing
interleaved packets which should not be entered if
muxer in non-blocking mode is used.
(also there is assert for the same condition added into
 av_interleaved_write_packet in one of the following
 patches).

  libavformat/avformat.h |  6 +-
  libavformat/mux.c  | 10 --
  2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/libavformat/avformat.h b/libavformat/avformat.h
index d8a6cf3..2cc3156 100644
--- a/libavformat/avformat.h
+++ b/libavformat/avformat.h
@@ -2510,8 +2510,12 @@ int 
av_write_uncoded_frame_query(AVFormatContext *s, int stream_index);

   *
   * May only be called after a successful call to 
avformat_write_header.

   *
+ * If AVFMT_FLAG_NONBLOCK is set, this call may return 
AVERROR(EAGAIN)

+ * meaning the operation is pending and the call should be repeated.
+ *
   * @param s media file handle
- * @return 0 if OK, AVERROR_xxx on error
+ * @return 0 if OK, AVERROR(EAGAIN) in case call should be repeated,
+ * other AVERROR on error
   */
  int av_write_trailer(AVFormatContext *s);
diff --git a/libavformat/mux.c b/libavformat/mux.c
index e9973ed..3ae924c 100644
--- a/libavformat/mux.c
+++ b/libavformat/mux.c
@@ -1204,11 +1204,14 @@ int av_write_trailer(AVFormatContext *s)
  for (;; ) {
  AVPacket pkt;
  ret = interleave_packet(s, , NULL, 1);
-if (ret < 0)
-goto fail;
  if (!ret)
  break;
+av_assert0(!(s->flags & AVFMT_FLAG_NONBLOCK));

this would abort on any error not just EAGAIN

I think it will abort in case interleave_packets does not return 0
from the first call in loop, which means that interleaving was used
(because there are some packets to be flushed) and that situation
cannot happen with AVFMT_FLAG_NONBLOCK set when interleaving is
forbidded. The next patch also adds assert to
av_interleaved_write_packet. But I think the assert here is on the
right place, or have I misunderstood the problem you're pointing
out?

I thought interleave_packet can return AVERROR(ENOMEM)
maybe this is not possible, still it seems non-robust to assert if
it errors out

I think it cannot return AVERROR(ENOMEM) in case interleaving was not 
used. But maybe I can remove assert from this function since there 
will be assert in av_interleaved_write_frame?


Regards,
Jan
Should I remove the assert from the patch (since it will be in 
av_interleaved_write_frame)?


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

Re: [FFmpeg-devel] [PATCH v4 5/5] avformat/tee: Use BSF list API

[Jan Sebechlebsky]

On 08/26/2016 12:53 AM, sebechlebsky...@gmail.com wrote:


From: Jan Sebechlebsky <sebechlebsky...@gmail.com>

Signed-off-by: Jan Sebechlebsky <sebechlebsky...@gmail.com>
---
  I believe I have fixed handling input / output timebase and input parameters
  to bitstream filters list.

  libavformat/tee.c | 131 ++
  1 file changed, 64 insertions(+), 67 deletions(-)

[...]

ping :)
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v2 04/11] avformat/muxers: Add non-blocking mode support for av_write_trailer

[Jan Sebechlebsky]

On 08/22/2016 04:49 PM, Michael Niedermayer wrote:


On Mon, Aug 22, 2016 at 04:27:16PM +0200, Jan Sebechlebsky wrote:


On 08/22/2016 09:51 AM, Michael Niedermayer wrote:

On Thu, Aug 11, 2016 at 02:38:29PM +0200, sebechlebsky...@gmail.com wrote:

From: Jan Sebechlebsky <sebechlebsky...@gmail.com>

This makes av_write_trailer not to free the resources if write_trailer
call returns AVERROR(EAGAIN) allowing repeated calls of write_trailer of
non-blocking muxer.

Signed-off-by: Jan Sebechlebsky <sebechlebsky...@gmail.com>
---
  Changes since the last version of the patch:
  - Added assert to the part of the code dealing with flushing
interleaved packets which should not be entered if
muxer in non-blocking mode is used.
(also there is assert for the same condition added into
 av_interleaved_write_packet in one of the following
 patches).

  libavformat/avformat.h |  6 +-
  libavformat/mux.c  | 10 --
  2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/libavformat/avformat.h b/libavformat/avformat.h
index d8a6cf3..2cc3156 100644
--- a/libavformat/avformat.h
+++ b/libavformat/avformat.h
@@ -2510,8 +2510,12 @@ int av_write_uncoded_frame_query(AVFormatContext *s, int 
stream_index);
   *
   * May only be called after a successful call to avformat_write_header.
   *
+ * If AVFMT_FLAG_NONBLOCK is set, this call may return AVERROR(EAGAIN)
+ * meaning the operation is pending and the call should be repeated.
+ *
   * @param s media file handle
- * @return 0 if OK, AVERROR_xxx on error
+ * @return 0 if OK, AVERROR(EAGAIN) in case call should be repeated,
+ * other AVERROR on error
   */
  int av_write_trailer(AVFormatContext *s);
diff --git a/libavformat/mux.c b/libavformat/mux.c
index e9973ed..3ae924c 100644
--- a/libavformat/mux.c
+++ b/libavformat/mux.c
@@ -1204,11 +1204,14 @@ int av_write_trailer(AVFormatContext *s)
  for (;; ) {
  AVPacket pkt;
  ret = interleave_packet(s, , NULL, 1);
-if (ret < 0)
-goto fail;
  if (!ret)
  break;
+av_assert0(!(s->flags & AVFMT_FLAG_NONBLOCK));

this would abort on any error not just EAGAIN

I think it will abort in case interleave_packets does not return 0
from the first call in loop, which means that interleaving was used
(because there are some packets to be flushed) and that situation
cannot happen with AVFMT_FLAG_NONBLOCK set when interleaving is
forbidded. The next patch also adds assert to
av_interleaved_write_packet. But I think the assert here is on the
right place, or have I misunderstood the problem you're pointing
out?

I thought interleave_packet can return AVERROR(ENOMEM)
maybe this is not possible, still it seems non-robust to assert if
it errors out

I think it cannot return AVERROR(ENOMEM) in case interleaving was not 
used. But maybe I can remove assert from this function since there will 
be assert in av_interleaved_write_frame?


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

[FFmpeg-devel] [GSoC] Tee muxer improvement project (fifo muxer)

[Jan Sebechlebsky]

Hello,

GSoC 2016 is almost over, so I am sending brief summary of the project 
(links to my work are at the end of this e-mail).


Original plan was to improve tee muxer so that:

1. outputs will not block each other (occasional long I/O operation on 
single output will not delay processing of other outputs)


2. failure of single output will not necessarily cause failure of whole 
process


3. recovery can be transparently attempted on failed output(s)

After initial discussion where Nicolas pointed out that proposed changes 
are too specific to be part of tee muxer, Marton (my mentor) suggested 
to implement new "fifo" pseudo-muxer which would allow to run actual 
muxer in separate thread (satisfying 1. requirement) and would be 
cabable of transparently restarting output in case of error (satisfying 
3. requirement).


I've implemented on_fail per-slave option (dealing with 2. requirement) 
to tee as qualification task. Currently patches implementing 
functionality mentioned above are already part of the codebase (changes 
in tee muxer and new fifo pseudo-muxer).


See documentation http://ffmpeg.org/ffmpeg-formats.html#tee
and http://ffmpeg.org/ffmpeg-formats.html#fifo for usage info.

   After initial version of fifo muxer was implemented, Nicolas 
insisted on implementation of nonblocking mode support 
(AVFMT_FLAG_NONBLOCK), which would allow developers to use this muxer to 
get nonblocking write_header/write_packet/write_trailer calls with 
muxers not naturally supporting nonblocking mode. This required some 
small changes in API and internal functions (av_write_packet, 
write_packet, av_write_trailer) and addition of av_write_abort() 
function which can be used to force-stop nonblocking muxer operation. 
Patches implementing these changes and adding AVFMT_FLAG_NONBLOCK 
support for fifo muxer are already being reviewed in mailing list.


Tee muxer was using deprecated bitstream filtering API, and there was no 
support for processing list of bitstream filters in new BSF API. 	I've 
decided to implement this as a side project based on Nicolases and 
Martons suggestions. Patches implementing this new API are already 
accepted, patch switching tee to this new API is currently reviewed in 
mailing list.


Probably the only drawback of implementing "fifo" features in separate 
muxer is that setting up tee with fifo for slaves is uncomfortable - it 
includes 3 muxers on top of each other which means trouble with lots of 
escaping of arguments for "inner" muxers (fifo and actual muxer).
To eliminate this I've decided to add options and per-slave options to 
the tee muxer which will allow to turn on/off and configure fifo 
functionality for the slaves (globaly for all slaves or override global 
behaviour for individual slaves). This way usage of fifo does not add 
another "muxer level" from the user point of view. This patch depends on 
patch changing bsfs API in tee, so I will send it to mailing list as 
soon as that patch is applied.


Apart from what is mentioned above, I've also fixed several bugs I've 
encountered, done some minor refactoring and implemented FATE tests for 
the newly added code (and tee).


You can find all my accepted patches in FFmpeg codebase:

https://github.com/FFmpeg/FFmpeg/commits/master?author=jsebechlebsky

all my patches which are currently being reviewed can be viewed on 
patchwork:


https://patchwork.ffmpeg.org/project/ffmpeg/list/?submitter=3

and all my commits, including those which were not yet submitted to the 
mailing list, can be found in my FFmpeg fork on github:


https://github.com/jsebechlebsky/FFmpeg/commits/master?author=jsebechlebsky

I would like to thank everyone who helped me during the GSoC by 
participating in reviews, discussion and making constructive 
suggestions, especially to my project mentor Marton Balint and also to 
Nicolas George who often reviewed my work and noticed what I've missed.


I will try to participate in FFmpeg development also after end of GSoC.

Regards,

Jan

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

Re: [FFmpeg-devel] [PATCH v2 04/11] avformat/muxers: Add non-blocking mode support for av_write_trailer

[Jan Sebechlebsky]

On 08/22/2016 09:51 AM, Michael Niedermayer wrote:

On Thu, Aug 11, 2016 at 02:38:29PM +0200, sebechlebsky...@gmail.com wrote:

From: Jan Sebechlebsky <sebechlebsky...@gmail.com>

This makes av_write_trailer not to free the resources if write_trailer
call returns AVERROR(EAGAIN) allowing repeated calls of write_trailer of
non-blocking muxer.

Signed-off-by: Jan Sebechlebsky <sebechlebsky...@gmail.com>
---
  Changes since the last version of the patch:
  - Added assert to the part of the code dealing with flushing
interleaved packets which should not be entered if
muxer in non-blocking mode is used.
(also there is assert for the same condition added into
 av_interleaved_write_packet in one of the following
 patches).

  libavformat/avformat.h |  6 +-
  libavformat/mux.c  | 10 --
  2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/libavformat/avformat.h b/libavformat/avformat.h
index d8a6cf3..2cc3156 100644
--- a/libavformat/avformat.h
+++ b/libavformat/avformat.h
@@ -2510,8 +2510,12 @@ int av_write_uncoded_frame_query(AVFormatContext *s, int 
stream_index);
   *
   * May only be called after a successful call to avformat_write_header.
   *
+ * If AVFMT_FLAG_NONBLOCK is set, this call may return AVERROR(EAGAIN)
+ * meaning the operation is pending and the call should be repeated.
+ *
   * @param s media file handle
- * @return 0 if OK, AVERROR_xxx on error
+ * @return 0 if OK, AVERROR(EAGAIN) in case call should be repeated,
+ * other AVERROR on error
   */
  int av_write_trailer(AVFormatContext *s);
  
diff --git a/libavformat/mux.c b/libavformat/mux.c

index e9973ed..3ae924c 100644
--- a/libavformat/mux.c
+++ b/libavformat/mux.c
@@ -1204,11 +1204,14 @@ int av_write_trailer(AVFormatContext *s)
  for (;; ) {
  AVPacket pkt;
  ret = interleave_packet(s, , NULL, 1);
-if (ret < 0)
-goto fail;
  if (!ret)
  break;
  
+av_assert0(!(s->flags & AVFMT_FLAG_NONBLOCK));

this would abort on any error not just EAGAIN
I think it will abort in case interleave_packets does not return 0 from 
the first call in loop, which means that interleaving was used (because 
there are some packets to be flushed) and that situation cannot happen 
with AVFMT_FLAG_NONBLOCK set when interleaving is forbidded. The next 
patch also adds assert to av_interleaved_write_packet. But I think the 
assert here is on the right place, or have I misunderstood the problem 
you're pointing out?


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

Re: [FFmpeg-devel] [PATCH v3 5/5] avformat/tee: Use BSF list API

[Jan Sebechlebsky]

On 08/21/2016 02:57 PM, Michael Niedermayer wrote:

On Tue, Aug 09, 2016 at 02:00:24PM +0200, sebechlebsky...@gmail.com wrote:
[...]
  
@@ -506,13 +481,32 @@ static int tee_write_packet(AVFormatContext *avf, AVPacket *pkt)

  av_packet_rescale_ts(, tb, tb2);
  pkt2.stream_index = s2;
  
-if ((ret = av_apply_bitstream_filters(avf2->streams[s2]->codec, ,

-  tee->slaves[i].bsfs[s2])) < 0 ||
-(ret = av_interleaved_write_frame(avf2, )) < 0) {
+ret = av_bsf_send_packet(tee->slaves[i].bsfs[s2], );
+if (ret < 0) {
+av_log(avf, AV_LOG_ERROR, "Error while sending packet to bitstream 
filter: %s\n",
+   av_err2str(ret));
  ret = tee_process_slave_failure(avf, i, ret);
  if (!ret_all && ret < 0)
  ret_all = ret;
  }
+
+do {
+ret = av_bsf_receive_packet(tee->slaves[i].bsfs[s2], );
+if (ret < 0)
+break;
+
+av_packet_rescale_ts(, tb, tb2);

are the timestamps rescaled twice ?
is that intended ?

This is a mistake, sorry for that and thanks for noticing. I'll send 
fixed patch.


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

Re: [FFmpeg-devel] [PATCH v11 01/11] avformat: Add fifo pseudo-muxer

[Jan Sebechlebsky]

On 08/18/2016 09:05 PM, Moritz Barsnick wrote:


On Thu, Aug 18, 2016 at 01:25:01 +0200, sebechlebsky...@gmail.com wrote:

+@item attempt_recovery @var{bool}
+If failure occurs, attempt to recover the output. This is especially useful
+when used with network output, allows to restart streaming transparently.
+By default this option set to 0 (false).

  ^ is


+Sets maximum number of successive unsucessful recovery attempts after which

  ^ unsuccessful


+Waiting time before the next recovery attempt after previous unsuccessfull

 ^ unsuccessful


+if (ret < 0) {
+return ret;
+}

You can skip the curly brackets here.


+static int fifo_write_header(AVFormatContext *avf)
+{
+FifoContext * fifo = avf->priv_data;
+int ret;
+
+ret = pthread_create(>writer_thread, NULL, fifo_consumer_thread, 
avf);
+if (ret) {
+av_log(avf, AV_LOG_ERROR, "Failed to start thread: %s\n",
+   av_err2str(AVERROR(ret)));
+ret = AVERROR(ret);
+}
+
+return 0;
+}

You're re-assigning ret in the error case but not using it. I think you
meant to return it?

Yes, that's right - thanks for noticing that. I'll fix that and also the 
issues above and resend the patch.


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

Re: [FFmpeg-devel] [PATCH] avformat: Document thread-safety requirement for interrupt callback

[Jan Sebechlebsky]

On 08/16/2016 06:42 PM, Nicolas George wrote:


Le decadi 30 thermidor, an CCXXIV, Marton Balint a écrit :

I think the idea was to document this limitation to the fifo muxer only.

That would be acceptable, although I still think it would be preferable to
avoid that requirement altogether.

Regards,

Is it OK, if I leave the comment only in avformat.h in AVFormatContext 
and change it to

"Callback must be thread safe when fifo muxer is used"?

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

Re: [FFmpeg-devel] [PATCH v8 01/11] avformat: Add fifo pseudo-muxer

[Jan Sebechlebsky]

On 08/16/2016 01:05 AM, Jan Sebechlebsky wrote:

On 08/15/2016 11:50 PM, Nicolas George wrote:



+
+if (just_flushed)
+av_log(avf, AV_LOG_INFO, "FIFO queue flushed\n");
+
+ret = av_thread_message_queue_recv(queue, , 0);

[...]

+if (ret < 0) {
+av_thread_message_queue_set_err_send(queue, ret);
+break;
I do not think this is necessary: the only error possible here is the 
one

you sent yourself using av_thread_message_queue_set_err_recv().

You're right. I'll remove it.
Actually on second thought I think it I'll leave it there - it ensures 
that all subsequent calls to write_frame will return error code.


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

Re: [FFmpeg-devel] [PATCH v8 01/11] avformat: Add fifo pseudo-muxer

[Jan Sebechlebsky]

On 08/15/2016 11:50 PM, Nicolas George wrote:


L'octidi 28 thermidor, an CCXXIV, sebechlebsky...@gmail.com a écrit :

[...]

+s@item recovery_wait_streamtime @var{bool}

^
Strange.

Sorry, that is obviously a typo. Strange thing is it had not produced 
any kind of warning/error.

[...]

+#define FIFO_DEFAULT_QUEUE_SIZE  60
+#define FIFO_DEFAULT_MAX_RECOVERY_ATTEMPTS   0
+#define FIFO_DEFAULT_RECOVERY_WAIT_TIME_USEC 500 // 5 second

Not very useful, but not harmful either.

Nit: plural for seconds.


+
+typedef struct FifoContext {
+const AVClass *class;
+AVFormatContext *avf;
+
+char *format;
+AVOutputFormat *oformat;

Does not seem to be needed in the context, could be a local variable.

You're probably right, I'll check it and make it local variable if so.

+// Check for options unrecognized by underlying muxer
+if (format_options) {
+AVDictionaryEntry *entry = NULL;
+while ((entry = av_dict_get(format_options, "", entry, 
AV_DICT_IGNORE_SUFFIX)))
+av_log(avf2, AV_LOG_ERROR, "Unknown option '%s'\n", entry->key);
+ret = AVERROR(EINVAL);
+}

This snippet seems to pop at quite a few places, it could become a helper
function. But do not consider it necessary for now.

I'll try to keep it in mind and add helper function later.

[...]

+ctx->last_recovery_ts = pkt->pts;
+} else {
+ctx->last_recovery_ts = av_gettime_relative();

In my experience, this kind of construct is slightly simpler if you compute
the next timestamp immediately instead of keeping the last one. I mean,
instead of this:

last_ts = now();
...
sleep(last_ts + delay - now());

you can write this:

next_ts = now() + delay;
...
sleep(next_ts - now());

I'll give it a try and see if it is more cleaner in context of this code.

+}
+
+if (fifo->max_recovery_attempts &&
+ctx->recovery_nr >= fifo->max_recovery_attempts) {
+av_log(avf, AV_LOG_ERROR,
+   "Maximal number of %d recovery attempts reached.\n",
+   fifo->max_recovery_attempts);
+ret = err_no;
+} else {
+ret = AVERROR(EAGAIN);
+}
+
+return ret;
+}
+
+static int fifo_thread_attempt_recovery(FifoThreadContext *ctx, FifoMessage 
*msg, int err_no)
+{
+AVFormatContext *avf = ctx->avf;
+FifoContext *fifo = avf->priv_data;
+AVPacket *pkt = >pkt;
+int64_t time_since_recovery;
+int ret;
+
+if (!is_recoverable(fifo, err_no)) {
+ret = err_no;
+goto fail;
+}
+
+if (ctx->header_written) {
+fifo->write_trailer_ret = fifo_thread_write_trailer(ctx);
+ctx->header_written = 0;
+}
+
+if (!ctx->recovery_nr) {
+ctx->last_recovery_ts = 0;

AV_NOPTS_VALUE? And maybe an av_assert1() when you use it to be sure it was
actually set.
I'll take a look at it. Using zero will delay the initial recovery 
attempt if failure happens at

the pts = 0, so treating it specially seems like a good idea.

[...]

+pthread_mutex_lock(>overflow_flag_lock);
+if (fifo->overflow_flag) {
+av_thread_message_flush(queue);
+if (fifo->restart_with_keyframe)
+fifo_thread_ctx.drop_until_keyframe = 1;
+fifo->overflow_flag = 0;
+just_flushed = 1;
+}
+pthread_mutex_unlock(>overflow_flag_lock);

I wonder if this whole thing could not be made simpler. This is just a
suggestion, so do not feel obligated to act on it if you lack time.

This has the feel of an out-of-band message. This is a rather useful
feature. This could be added to the thread message queue rather easily.
I am not sure if I understand this. Do you mean thread queue function 
which would set
certain flag that the queue should be flushed and flushed the queue at 
certain point in time (next receive call?)?

But in fact, I think it could be made even simpler. See below where
overflow_flag is set.


+
+if (just_flushed)
+av_log(avf, AV_LOG_INFO, "FIFO queue flushed\n");
+
+ret = av_thread_message_queue_recv(queue, , 0);

[...]

+if (ret < 0) {
+av_thread_message_queue_set_err_send(queue, ret);
+break;

I do not think this is necessary: the only error possible here is the one
you sent yourself using av_thread_message_queue_set_err_recv().

You're right. I'll remove it.

+}
+}
+
+fifo->write_trailer_ret = fifo_thread_write_trailer(_thread_ctx);
+
+return NULL;
+}
+
+static int fifo_mux_init(AVFormatContext *avf)
+{
+FifoContext *fifo = avf->priv_data;
+AVFormatContext *avf2;
+int ret = 0, i;
+
+ret = avformat_alloc_output_context2(, fifo->oformat, NULL, NULL);
+if (ret < 0) {
+return ret;
+}
+
+fifo->avf = avf2;
+
+avf2->interrupt_callback = avf->interrupt_callback;

You have not documented the fact that the interrupt callback needs to be
thread-safe. And if users can 

Re: [FFmpeg-devel] [PATCH v3 2/5] avcodec/bsf: Add list BSF API

[Jan Sebechlebsky]

On 08/15/2016 02:08 PM, Michael Niedermayer wrote:

On Mon, Aug 15, 2016 at 02:05:43PM +0200, Michael Niedermayer wrote:

On Mon, Aug 15, 2016 at 12:38:35PM +0200, sebechlebsky...@gmail.com wrote:

From: Jan Sebechlebsky <sebechlebsky...@gmail.com>

---
  Changes since the last version of the patch:
  - removed av_freep(>item_name) from bsf_list_close since it belongs to
next commit

  libavcodec/avcodec.h |  85 
  libavcodec/bsf.c | 278 +++
  2 files changed, 363 insertions(+)

applied

please also submit a patch that adds this to APIChanges

thx

[...]

Sure, I'll do that, have you already pushed the changes (I can't see it 
being applied)?
I am just asking because I wanted to check commit hash - I am not sure 
if it is guaranteed to be the same as in my repo, is it?


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

Re: [FFmpeg-devel] [PATCH v2 3/5] avcodec/bsf: Add list BSF API

[Jan Sebechlebsky]

On 08/15/2016 03:40 AM, Michael Niedermayer wrote:

On Fri, Aug 05, 2016 at 02:00:25PM +0200, sebechlebsky...@gmail.com wrote:

From: Jan Sebechlebsky <sebechlebsky...@gmail.com>

---
  Changes from last version:
  - fixed doxygen comments
  - added av_bsf_list_append2() function
  - changed names of array and length fields to follow naming convention
  - idx and flushed_idx is now unsigned
  - merged bsf_list_flush to bsf_list_filter to reduce code duplication
  - aligned structure fields initialization
  - added check for NULL to av_bsf_free()
  - fixed memleak in av_bsf_finalize in case there is only 1 filter

  libavcodec/avcodec.h |  85 
  libavcodec/bsf.c | 279 +++
  2 files changed, 364 insertions(+)

seems to break build or i did something silly

CC  libavcodec/bsf.o
libavcodec/bsf.c: In function ‘bsf_list_close’:
libavcodec/bsf.c:348:18: error: ‘BSFListContext’ has no member named ‘item_name’
make: *** [libavcodec/bsf.o] Error 1


That is my mistake, sorry for that - when I was splitting custom item 
name functionality to separate patch I left line freeing item name in 
the first patch. I will move it to the correct patch and resend.


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

Re: [FFmpeg-devel] [PATCH v7 01/11] avformat: Add fifo pseudo-muxer

[Jan Sebechlebsky]

On 08/14/2016 08:12 PM, Marton Balint wrote:




[...]


+@item max_recovery_attempts
+Sets maximum number of successive unsucessful recovery attempts 
after which
+the output fails permanently. Unlimited if set to zero. Default 
value is 16.


Maybe it's just me, but I'd set this to unlimited by default, since 
attempt_recovery is not enabled by default anyway.



That seems reasonable, I'll change that.

[...]


[...]


[...]

Otherwise it looks good to me, I don't think there is a reason to 
delay this any longer, so if you rebase the patch against the current 
git HEAD and fix my few comments, I will apply this in a day or two.


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

I'll fix the issues you mentioned and resend the patch.

Thank you!

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

Re: [FFmpeg-devel] [PATCH v7 01/11] avformat: Add fifo pseudo-muxer

[Jan Sebechlebsky]

On 08/14/2016 08:19 PM, Timothy Gu wrote:


On Sun, Aug 14, 2016 at 11:12 AM Marton Balint  wrote:


On Thu, 11 Aug 2016, sebechlebsky...@gmail.com wrote:

+@anchor tee

I still get error when building the docs:

doc/muxers.texi:1531: @anchor expected braces
doc/muxers.texi:1443: @ref reference to nonexistent node `tee'
make: *** [doc/ffmpeg-all.html] Error 1

@anchor{tee} should work.
Timothy
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Thank you!

Jan
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v2 03/11] avformat/fifo: Add fate test

[Jan Sebechlebsky]

On 08/11/2016 05:00 PM, Michael Niedermayer wrote:

make: *** No rule to make target `libavformat/tests/fifo_muxer.exe', needed by 
`fate-fifo-muxer-tst'.
make: Target `fate-fifo-muxer-tst' not remade because of errors.

I was not able to reproduce the exactly same error you got, however
I've fixed the whitespace and also there was $(EXESUF) missing at

fate-fifo-muxer-tst: CMD = run libavformat/tests/fifo_muxer

line - I hope that was the cause the test refused to built. I've
setup windows build and found also some other issues, now it passes
fate on linux 32/64 bit build and also mingw32/64 bit build.

Thank you for testing it - I will try to run windows tests as well
in future to avoid such mistakes.
I'll resend fixed patches soon - can you please re-run the
fate-fifo-muxer test as well in your environment?

it appears after closer inspection the fifo muxer was not enabled
with mingw32 here

you can get the same failure on linux with
make distclean ; ./configure --disable-pthreads --samples=.../ && make -j12 fate



Sorry for that, I've missed that. I'll resend the patch.

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

Re: [FFmpeg-devel] [PATCH v2 03/11] avformat/fifo: Add fate test

[Jan Sebechlebsky]

On 08/09/2016 07:42 PM, Michael Niedermayer wrote:

On Tue, Aug 09, 2016 at 01:26:04PM +0200, sebechlebsky...@gmail.com wrote:

From: Jan Sebechlebsky <sebechlebsky...@gmail.com>

Signed-off-by: Jan Sebechlebsky <sebechlebsky...@gmail.com>
---
  Changes since the last version:
  - Removed empty lines at the end of fifo_muxer.c file


[...]

diff --git a/tests/Makefile b/tests/Makefile
index 895944d..0848766 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -128,6 +128,7 @@ include $(SRC_PATH)/tests/fate/exif.mak
  include $(SRC_PATH)/tests/fate/ffmpeg.mak
  include $(SRC_PATH)/tests/fate/ffprobe.mak
  include $(SRC_PATH)/tests/fate/fft.mak
+include $(SRC_PATH)/tests/fate/fifo-muxer.mak
  include $(SRC_PATH)/tests/fate/filter-audio.mak
  include $(SRC_PATH)/tests/fate/filter-video.mak
  include $(SRC_PATH)/tests/fate/flac.mak
diff --git a/tests/fate/fifo-muxer.mak b/tests/fate/fifo-muxer.mak
new file mode 100644
index 000..a064069
--- /dev/null
+++ b/tests/fate/fifo-muxer.mak
@@ -0,0 +1,20 @@
+fate-fifo-muxer-h264: CMD = ffmpeg -i $(TARGET_SAMPLES)/mkv/1242-small.mkv 
-vframes 11\
+-c:v copy -c:a copy -map v:0 -map a:0 -flags 
+bitexact\
+-fflags +bitexact -f fifo -fifo_format framecrc -
+fate-fifo-muxer-h264: REF = $(SRC_PATH)/tests/ref/fate/mkv-1242
+FATE_FIFO_MUXER-$(call ALLYES, FIFO_MUXER, MATROSKA_DEMUXER, H264_DECODER) += 
fate-fifo-muxer-h264
+
+fate-fifo-muxer-wav: CMD = ffmpeg -i 
$(TARGET_SAMPLES)/audio-reference/chorusnoise_2ch_44kHz_s16.wav\
+   -c:a copy -map a:0 -flags +bitexact\
+  -fflags +bitexact -f fifo -fifo_format wav md5:

inconsistent mix of space and tab



+fate-fifo-muxer-wav: CMP = oneline
+fate-fifo-muxer-wav: REF = 4dda5dcc7ecdc2218b0739a152ada802
+FATE_FIFO_MUXER-$(call ALLYES, FIFO_MUXER, WAV_DEMUXER) += fate-fifo-muxer-wav
+
+fate-fifo-muxer-tst: libavformat/tests/fifo_muxer$(EXESUF)
+fate-fifo-muxer-tst: CMD = run libavformat/tests/fifo_muxer
+FATE_FIFO_MUXER-$(CONFIG_FIFO_MUXER) += fate-fifo-muxer-tst
+
+FATE_SAMPLES_FFMPEG += fate-fifo-muxer-h264 fate-fifo-muxer-wav
+FATE_FFMPEG += fate-fifo-muxer-tst
+fate-fifo-muxer: $(FATE_FIFO_MUXER-yes)

works on linux 32 & 64bit

fails on mingw32 & 64

make: *** No rule to make target `libavformat/tests/fifo_muxer.exe', needed by 
`fate-fifo-muxer-tst'.
make: Target `fate-fifo-muxer-tst' not remade because of errors.
I was not able to reproduce the exactly same error you got, however I've 
fixed the whitespace and also there was $(EXESUF) missing at


fate-fifo-muxer-tst: CMD = run libavformat/tests/fifo_muxer

line - I hope that was the cause the test refused to built. I've setup 
windows build and found also some other issues, now it passes fate on 
linux 32/64 bit build and also mingw32/64 bit build.


Thank you for testing it - I will try to run windows tests as well in 
future to avoid such mistakes.
I'll resend fixed patches soon - can you please re-run the 
fate-fifo-muxer test as well in your environment?


Regards,
Jan

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

Re: [FFmpeg-devel] [PATCH v5 01/11] avformat: Add fifo pseudo-muxer

[Jan Sebechlebsky]

On 08/08/2016 11:55 PM, Marton Balint wrote:



Thanks for your work, and sorry for the delay in the review.

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

Thanks for review! Hopefully I've fixed all the mentioned issues.
I'm resending the patch and also affected patches.

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

Re: [FFmpeg-devel] [PATCH 3/5] avcodec/bsf: Add list BSF API

[Jan Sebechlebsky]

On 08/02/2016 05:26 PM, James Almer wrote:

On 8/2/2016 12:14 PM, Nicolas George wrote:

+AVBSFList *av_bsf_list_alloc(void);

This is personal, but for new APIs, I like "int foo_alloc(Foo **rfoo)"
better than "Foo *foo_alloc(void)": that way, the caller can forward the
error code instead of guessing it is ENOMEM.

There is no other error this could generate. It's literally an
av_mallocz wrapper.
Every other similar alloc() function in avcodec.h (with no extra
parameters that could generate EINVAL errors and such) also have
this signature, so lets keep try to keep it consistent.
___

I'll keep it this way then :)

Regards,
Jan

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

Re: [FFmpeg-devel] [PATCH 3/5] avcodec/bsf: Add list BSF API

[Jan Sebechlebsky]

Hello Nicolas,

On 08/02/2016 05:14 PM, Nicolas George wrote:



+AVBSFList *av_bsf_list_alloc(void);

This is personal, but for new APIs, I like "int foo_alloc(Foo **rfoo)"
better than "Foo *foo_alloc(void)": that way, the caller can forward the
error code instead of guessing it is ENOMEM.

I left that as it is, since James commented this is more common way.



+
+typedef struct AVBSFList {
+AVBSFContext **bsf_lst;
+int ctx_nr;
+} AVBSFList;

I think it would be possible to use the same structure for AVBSFList and
BSFListContext. Sure, AVBSFList would have extra fields, but that is no
matter, since its only use is to be eventually upgraded into a
BSFListContext.

The benefit would be to avoid a malloc()/copy/free() cycle: just declare
.priv_data_size = 0 to prevent lavc from allocating the private structure,
and set priv_data directly.
I think it is nicer the way it is - I do not like an idea of manually 
assigning private data when priv_data_size will be set to 0, it seems 
like something which can potentially create problem in future. Also the 
overhead is negligible here, since bsf list finalization will be done 
only once or few times per whole transcoding.


I've fixed all other issues you suggested.

Thanks for review!

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

Re: [FFmpeg-devel] [PATCH 03/11] avformat/fifo: Add fate test

[Jan Sebechlebsky]

On 08/02/2016 10:08 PM, Michael Niedermayer wrote:

On Tue, Aug 02, 2016 at 03:24:14PM +0200, sebechlebsky...@gmail.com wrote:
segfaults  on x86-32


Thanks for testing,
I wrongly used uint8_t for boolean AVOptions instead of int.
I'll send fixed patch soon.

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

Re: [FFmpeg-devel] [PATCH v3 1/2] avcodec/bsf: Set EOF flag only in pkt == NULL

[Jan Sebechlebsky]

On 07/26/2016 12:40 PM, sebechlebsky...@gmail.com wrote:


From: Jan Sebechlebsky <sebechlebsky...@gmail.com>

Set BSF EOF flag only if pkt == NULL in av_bsf_send_packet().

Signed-off-by: Jan Sebechlebsky <sebechlebsky...@gmail.com>
---
  libavcodec/bsf.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c
index 88b7f29..9b9ada7 100644
--- a/libavcodec/bsf.c
+++ b/libavcodec/bsf.c
@@ -172,7 +172,7 @@ int av_bsf_init(AVBSFContext *ctx)
  
  int av_bsf_send_packet(AVBSFContext *ctx, AVPacket *pkt)

  {
-if (!pkt || !pkt->data) {
+if (!pkt) {
  ctx->internal->eof = 1;
  return 0;
  }

Ping for this and the next patch :)


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

Re: [FFmpeg-devel] [PATCH 06/11] avformat: add av_abort_output() function

[Jan Sebechlebsky]

On 08/03/2016 03:15 PM, Nicolas George wrote:

Le sextidi 16 thermidor, an CCXXIV, sebechlebsky...@gmail.com a écrit :

From: Jan Sebechlebsky <sebechlebsky...@gmail.com>

Signed-off-by: Jan Sebechlebsky <sebechlebsky...@gmail.com>
---
  libavformat/avformat.h | 14 ++
  libavformat/mux.c  | 16 
  2 files changed, 30 insertions(+)

diff --git a/libavformat/avformat.h b/libavformat/avformat.h
index 9191c69..9173908 100644
--- a/libavformat/avformat.h
+++ b/libavformat/avformat.h
@@ -2510,6 +2510,8 @@ int av_write_uncoded_frame_query(AVFormatContext *s, int 
stream_index);
   *
   * If AVFMT_FLAG_NONBLOCK is set, this call may return AVERROR(EAGAIN)
   * meaning the operation is pending and the call should be repeated.
+ * If caller decides to abort operation (after too many calls have returned
+ * AVERROR(EAGAIN)), it can be done by calling @ref av_abort_output().
   *
   * @param s media file handle
   * @return 0 if OK, AVERROR(EAGAIN) in case call should be repeated,
@@ -2518,6 +2520,18 @@ int av_write_uncoded_frame_query(AVFormatContext *s, int 
stream_index);
  int av_write_trailer(AVFormatContext *s);
  
  /**

+ * Abort non-blocking muxer operation and free private data.
+ *
+ * May only be called after a successful call to avformat_write_header,
+ * and used only with muxer operating in non-blocking mode 
(AVFMT_FLAG_NONBLOCK)
+ * must be set.
+ *
+ * @param s media file handle
+ * return >= 0 on success, negative AVERROR on error
+ */
+int av_abort_output(AVFormatContext *s);

The other functions are called av_write_something() or
avformat_write_something(): maybe avformat_write_abort()?

I'll change to avformat_write_abort.

Also, it could call avformat_free_context() and set s to NULL, unless there
is some use in reusing the context?
I thought of this function as about an alternative to av_write_trailer - 
so you can replace one call for another and get the same behaviour (from 
the "outside" point of view). But it's not big deal, if you wish I can 
change this.

+
+/**
   * Return the output format in the list of registered output formats
   * which best matches the provided parameters, or return NULL if
   * there is no match.
diff --git a/libavformat/mux.c b/libavformat/mux.c
index bc9c98f..888a9f1 100644
--- a/libavformat/mux.c
+++ b/libavformat/mux.c
@@ -1267,6 +1267,22 @@ fail:
  return ret;
  }
  
+int av_abort_output(AVFormatContext *s)

+{
+int ret;
+
+if (!(s->flags & AVFMT_FLAG_NONBLOCK))
+return AVERROR(EINVAL);

Since the application should be able to know whether the muxer is in
blocking mode, it could be an assert.

But is it really necessary? Applications in blocking mode may want to exit
immediately too.
I think there is no reason for blocking muxer to call this, it shouldn't 
avoid calling av_write_trailer to free resources. But I can leave here 
the av_write_trailer call below and remove the check above, in that case 
this would became just av_write_trailer wrapper for blocking muxer.


Anyway, any of these changes would allow to make the function void.


+
+ret = av_write_trailer(s);
+if (ret == AVERROR(EAGAIN)) {

Is it useful? The application could try and write the trailer itself,
possibly allowing a little more time to finish. And only call
av_abort_output() when it really wants to stop now.
I thought it might be good idea to call write_trailer before actually 
calling deinit. User can still do as many av_write_trailer attempts as 
he wants before, but if he does none (for example decides to abort 
output not because of write_trailer timeout, but because of some other 
error or signal caught before all packets are send to muxer) this will 
at first attempt to finish in a graceful way.
If you're ok with that I would keep that here and I'll remove the 
AVFMT_FLAG_NONBLOCK flag check since then it's safe to call this instead 
of av_write_trailer regardless of whether muxer is operating in 
blocking/nonblocking mode.

+deinit_muxer(s);

There is a subtle pitfall here: if the muxer does not have a deinit function
and av_write_trailer() returned EAGAIN (or was not called like I suggest),
then the resources leak.

Fortunately, we currently do not have any muxer that both supports
non-blocking mode and has a potentially blocking write_trailer(). We can
decide that any future muxer that would must have a deinit() function. In
that case, we can get away with the following steps:

   1. If the muxer has a deinit() method, just call deinit_muxer().

   2. Else, sabotage the context's AVIO stream so that it returns an error
  for all operations and call av_write_trailer().

The "sabotage" part of 2 is not very robust, but I think it is fine enough
for our use case. In fact, I would not object if that detail was left as a
TODO comment.
Maybe a good solution would be to document that nonblocking muxer must 
have deinit function and assert that during initialization in 
avf

Re: [FFmpeg-devel] [PATCH 1/2] avformat/tee: Factor parse_slave_options() out

[Jan Sebechlebsky]

On 08/02/2016 04:42 PM, Nicolas George wrote:


LGTM apart from that, but maybe ask Jan if it will not interfere with his
work.
It will interfere with patch "avformat/tee: Use BSF list API" I've send 
to ML, but it's not a problem at all, I'll just regenerate and resend 
the patch after this is applied.

I am ok with the patch.

Regards,
Jan

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

Re: [FFmpeg-devel] [PATCH v2] avcodec/bsf: Set EOF flag only if pkt == NULL

[Jan Sebechlebsky]

On 07/25/2016 05:14 PM, Nicolas George wrote:


Le septidi 7 thermidor, an CCXXIV, Jan Sebechlebsky a écrit :

I gave this a second thought, wouldn't it be better to simply ignore pkt
without payload? So after caller would send empty packet using
av_bsf_send_packet, he would get AVERROR(EAGAIN) from the next
av_bsf_receive_packet call (from the definition in documentation
AVERROR(EAGAIN) means "more data needed" when returned by
av_bsf_receive_packet).
However, if you think it is better to reserve packet without payload for
some future use, I won't object.

Both are possible, it is a matter of balance between convenience for us and
convenience for the applications.

If at some point someone finds a way to use that kind of packet. If they
used to be ignored, it makes an incompatible API change. If they used to be
forbidden, it works.

The same logic applies for functions that return 0 for success or an AVERROR
code: the documentation usually states ">= 0 for success", that allows to
extend their return value later.

In this particular case, I do not see how an application can accidentally
send a completely empty packet, and therefore I think forbidding them is
better.


OK, Let's make it that way then :)

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

Re: [FFmpeg-devel] [PATCH v2] avcodec/bsf: Set EOF flag only if pkt == NULL

[Jan Sebechlebsky]

On 07/22/2016 10:14 PM, Nicolas George wrote:

Le quintidi 5 thermidor, an CCXXIV, sebechlebsky...@gmail.com a écrit :

From: Jan Sebechlebsky <sebechlebsky...@gmail.com>

Set BSF EOF flag only if pkt == NULL in av_bsf_send_packet().

Signed-off-by: Jan Sebechlebsky <sebechlebsky...@gmail.com>
---
  I agree, it seems cleaner that way.

  Thanks,
  please apply this version of patch then and ignore
  the patch changing the comment.

  Regards,
  Jan

  libavcodec/bsf.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c
index 88b7f29..9b9ada7 100644
--- a/libavcodec/bsf.c
+++ b/libavcodec/bsf.c
@@ -172,7 +172,7 @@ int av_bsf_init(AVBSFContext *ctx)
  
  int av_bsf_send_packet(AVBSFContext *ctx, AVPacket *pkt)

  {
-if (!pkt || !pkt->data) {
+if (!pkt) {

It we make the case where pkt != NULL but no data nor side data forbidden, I
would suggest to detect it.

I know that others disagree, but I think an assert is the best solution for
that: if the caller pass a forbidden value, it can expect an undefined
behaviour, and an assert failure is the most sympathetic undefined
behaviours for developers.
I gave this a second thought, wouldn't it be better to simply ignore pkt 
without payload? So after caller would send empty packet using 
av_bsf_send_packet, he would get AVERROR(EAGAIN) from the next 
av_bsf_receive_packet call (from the definition in documentation 
AVERROR(EAGAIN) means "more data needed" when returned by 
av_bsf_receive_packet).
However, if you think it is better to reserve packet without payload for 
some future use, I won't object.


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

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

[Jan Sebechlebsky]

Hello Nicolas,

On 07/15/2016 02:17 PM, Nicolas George wrote:



Therefore I would like to keep the fifo muxer as Jan submitted it, without
EAGAIN support. If there is a use case for non-blockingingness in a sense
you use the phrase, then it can be added later.

I am against it. As it is, I consider it is way too specific to be added to
the code base.
We've talked with Marton and decided that I will try to implement the 
non-blocking mode as described in my previous e-mail as a separate patch 
at the top of current fifo muxer (also with necessary API patches as 
separate patches).
However, I don't see a reason why the fifo muxer couldn't be 
accepted to codebase before the patch adding AVFMT_FLAG_NONBLOCK support 
is completed - non-blocking mode will not be used in FFmpeg anyway. 
Although developers using libavformat can benefit from nonblocking fifo 
muxer, there is no reason not to provide functionality it adds to ffmpeg 
users (and adding AVFMT_FLAG_NONBLOCK support will not change the 
functionality it provides for users).
So, I would like to send current version of fifo muxer to the ML 
and I hope we can deal separately with the support for 
AVFMT_FLAG_NONBLOCK later. I think this way it will also simplify the 
review process. Can we do it this way? :)


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

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

[Jan Sebechlebsky]

On 07/12/2016 10:28 AM, Nicolas George wrote:

Le quintidi 25 messidor, an CCXXIV, Marton Balint a écrit :

The fifo muxer never returns EAGAIN. It silently drops the packets in
non-blocking mode on a full queue. This behaviour is useful for the tee
muxer case, when you don't want one slow/unreliable (network) output to
block reading the input, therefore blocking fast outputs (disk) as well.

Wait a minute. This is way to specific to be the default behaviour, let
alone the only one.


As far as I know, in the current API, if the user gets a negative return
value from av_write_frame(), it should be handled as a fatal error. EAGAIN
is not handled/interpreted specially. The same is true for
av_write_trailer(), and calling av_write_trailer - regardless of the return
value - frees all private resources for the context as well, so you cannot
change the semantics of av_write_trailer to not free private data in case of
EAGAIN, because it would cause unfreed data for legacy users.

You are wrong. Returning EAGAIN so that the caller try again later is the
normal behaviour for muxers that support non-blocking operation. I grant you
that there are only between one and three of them, but still, that is how
they work.

And that is also how they are supposed to work, because that is how
non-blocking protocols work, and also how non-blocking works outside FFmpeg.
If you take a look at av_write_trailer it really frees all the resources 
in case
of any error. There is also no other way in API to free the resources 
associated

with AVFormatContext, than through
av_write_trailer.
I was able to find two muxers which support AVFMT_FLAG_NONBLOCK - 
v4l2enc.c
and pulse_audio_enc.c. Neither of these two can return EAGAIN from 
write_trailer_call.


Write trailer of v4l2enc.c contains simple close() call and always 
returns 0.


The pulse audio muxer is however similar to FIFO muxer, the 
mainloop of the pulse audio is
being run in separate thread. If you take a look at the write_trailer of 
pulse audio muxer (also always returns 0):


static av_cold int pulse_write_trailer(AVFormatContext *h)
{
PulseData *s = h->priv_data;

if (s->mainloop) {
...
pa_threaded_mainloop_unlock(s->mainloop);
pa_threaded_mainloop_stop(s->mainloop);
pa_threaded_mainloop_free(s->mainloop);
s->mainloop = NULL;
}

return 0;
}

pa_threaded_mainloop_stop() contains:

void pa_threaded_mainloop_stop(pa_threaded_mainloop *m) {
...

pa_thread_join(m->thread);
}

So I guess the write_trailer call of pulse audio muxer should be
considered blocking too - even when AVFMT_FLAG_NONBLOCK flag is set.

I guess I could implement non-blocking calls in FIFO muxer:

- block_on_overflow should be renamed to something like 
drop_packets_on_overflow

  to prevent confusion...
- av_write_trailer would have to be modified not to free resources on 
AVERROR(EAGAIN)
- separate API function for freeing AVOuputContext resources would have 
to be created,

  let's say av_format_deinit(AVFormatContext *).
- FIFO muxer would behave the same way as currently in case 
AVFMT_FLAG_NONBLOCK

  would be unset.
- In case AVFMT_FLAG_NONBLOCK would be set:
- write_packet would return AVERROR(EAGAIN) in case the queue is 
full if
  drop_packets_on_overflow was not set. If it was, it would drop 
packet, request queue flushing

  and return 0
- write_trailer would check if the thread is still running. If yes 
write_trailer request would be send

  (if it was not yet send) and AVERROR(EAGAIN) returned.
- original interrupt callback would be wrapped by custom one which 
would check terminating condition
  (flag) of FIFO first and if it would be not set, it would call 
the original callback.
- deinit function would be modified so it would check if the thread 
is still running, if it was, terminating
  condition would be set, and pthread_join called, then resources 
would be freed. This can be also implemented
  in with use of condition variable which would be set at the 
moment when the thread is exiting and checked
  with pthread_cond_wait_timeout before pthread_join. If the time 
out would be reached, thread would be
  canceled - but canceling the thread by force can introduce many 
problems.


I can implement it, but I'm really in doubt if it is worth increased code
complexity and more risky situations which could happen, so I would 
prefer not to :)


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

Re: [FFmpeg-devel] [PATCH 2/7] avformat/tee: Use ff_stream_encode_params_copy()

[Jan Sebechlebsky]

On 07/13/2016 12:56 PM, Jan Sebechlebsky wrote:



On 07/12/2016 10:23 AM, Nicolas George wrote:

Le quartidi 24 messidor, an CCXXIV, Jan Sebechlebsky a écrit :
I think it is used in decoding only - the documentation of AVStream 
mentions
only decoding and I also tried to search references to that field if 
it is
not used in some of the muxers. If you think it should be copied too 
I will

add it.
If you have checked it was never used in muxers, then ok. But you may 
want
to have a look at the "MKV Header: Writing duration early" mail that 
arrived

a few minutes ago.

Regards,


I think we can safely omit it, it's not used in any current muxers.
Soft Works contributors may use duration field in muxing context, I 
think they (or we) can then add copying of this field to 
ff_stream_encode_params_copy with their patch . The documentation of 
the field should be then modified as well.


Regards,
Jan
Resending this to mailing list - sorry I've accidentally replied 
privately again.


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

Re: [FFmpeg-devel] [PATCH 1/7] avformat/utils: Add ff_stream_encode_params_copy()

[Jan Sebechlebsky]

On 07/11/2016 02:23 AM, Marton Balint wrote:



+ret = av_dict_copy(>metadata, src->metadata, 0);
+if (ret < 0)
+return ret;


Since you are resetting every existing field in dst, it might make 
sense to free the metadata dictionary as well, before copying items to 
it.


Thanks for noticing, I've fixed that, I will wait for Nicolas's reply 
regarding duration field and then send the modified patch.


Regards,
Jan

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

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

[Jan Sebechlebsky]

On 07/08/2016 10:39 PM, Marton Balint wrote:


On Fri, 8 Jul 2016, Nicolas George wrote:


Le primidi 21 messidor, an CCXXIV, Jan Sebechlebsky a écrit :
I actually thought about this and maybe I am still missing 
something, but

how is this different from the situation without FIFO muxer?


It is not, which is exactly the answer you do not want when people 
ask what

your program is good for.

That question was related to usage of interrupt_callback, not the 
functionality of FIFO muxer :)

When the FIFO is not used, let's say an I/O operation somewhere inside
write_packet call blocks for a long time - actually it means it is 
spinning

in a cycle "wait for io with short timeout"<->"check return value of
interrupt callback". When the application decides to interrupt the
operation, it sets some interrupt condition (ffmpeg increments
received_nb_signals when receives SIGINT). The next call to interrupt
callback will cause the IO function to return AVERROR_EXIT causing
write_packet to fail with same error.

When the FIFO is used the write_packet call will return immediately. 
The I/O

blocking will happen in consumer thread, in some of the
fifo_thread_write_packet calls. Let's say that during that time all 
packets

are send to queue and write_trailer is called. The main thread
is blocked by pthread_join(). However when the interrupt condition 
is set
(for example by receiving SIGINT as in ffmpeg), this is still 
handled by the

blocking IO function which will return AVERROR_EXIT causing
fifo_thread_write_packet to fail with the same error, causing the 
thread to

terminate which unblocks write_trailer.

My point is that the same asynchronous mechanism which is supposed 
to work
in case of blocking I/O operation should work also in case 
pthread_join is
blocking, so the I/O operation should be terminated the same way 
returning

AVERROR_EXIT which will cause consumer thread to terminate and unblock
pthread_join call...

Am I missing something?


I think you are missing (not in the sense of your question) a clear 
mission
statement for the FIFO muxer. Or, to put another way, a detailed 
explanation

of what it is useful for and more importantly, how to benefit from it.


The basic goals are the ones which are set in the GSOC trac page under 
the tee muxer improvement project:


Description: FFmpeg contains a tee muxer, which is capable of writing 
the same coded packets to multiple outputs. However, if one of the 
outputs blocks or fails for any reason, the other outputs will block 
or fail too. Also there is no built-in support for gracefully 
restarting an output in case of a failure. Lacking these two features 
makes the tee muxer unsuitable for redundancy or high availability 
purposes especially on networked outputs.


Expected results:
•Add a non-blocking mode with a configurable maximum packet queue size 
where one output does not block the others
•Add a graceful restart mode where a failed output can transparently 
automatically restart its operation


We decided to implement these features in a separate muxer, instead of 
hard coding it to tee, but the goals are the same. In order to reach 
them, one will have to specify fifo muxers in the output of the tee 
muxer, or Jan can work on some syntactic sugar which makes this more 
convenient for the user, but the result is the same.


If I understand things correctly, it is meant to be used by 
applications (or
libraries, including the tee muxer), not directly by users. But how 
are the

applications supposed to do exactly, and what can they expect.


It also can be used by users. For example in blocking mode the fifo 
muxer can work as a pipeline between the encoder and the output, 
hiding disk latencies. In this scenario the user don't need to use the 
tee muxer to grab the benefits of the fifo muxer. In this case, it 
works similarly as the async protocol for input, only for output.




One of the features seems to be to turn a blocking muxer into a 
non-blocking
muxer, which is indeed useful. But if the muxer falls back to 
blocking on
close and the application needs to set up a thread-synchronized 
interrupt

callback to handle it, then I feel we are missing a serious opportunity.


The reason why neither me (and I guess nor Jan) sees this as an issue, 
is that this is not needed for the goals set in the project. We simply 
don't care if closeing a stream blocks, that is not what we are aiming 
for here.


The way I see it, in non-blocking mode, the most logical approach 
would be

something like this: the first call to write_header() causes the closing
process to start an returns EAGAIN immediately, subsequent calls return
EAGAIN until the closing process is done, then 0 for success, or an 
error

code after a configurable timeout.


Sure, this can be implemented (although an API change, so dificcult to 
pull through), but in GSOC this was simply not a goal of ours.


Regards,
Marton
__

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

[Jan Sebechlebsky]

Hello Nicolas,

On 07/07/2016 08:00 PM, Nicolas George wrote:


If your worker thread is blocked on an I/O operation when the application
tries to close the muxer, it will send the corresponding messages and call
pthread_join(). Since the worker thread is still blocked in the I/O
operation, it will be stuck like that forever. To avoid that, the main
thread needs to arrange for the I/O operation to be cancelled
asynchronously, and that is what the interrupt callbeck is for.

On the other hand, we do not want to cancel writing the trailer if it is
just a little slow, so there must be some kind of timeout.
I actually thought about this and maybe I am still missing something, 
but how is this different from the situation without FIFO muxer? Please, 
correct me if I am wrong:


When the FIFO is not used, let's say an I/O operation somewhere inside 
write_packet call blocks for a long time - actually it means it is 
spinning in a cycle "wait for io with short timeout"<->"check return 
value of interrupt callback". When the application decides to interrupt 
the operation, it sets some interrupt condition (ffmpeg increments 
received_nb_signals when receives SIGINT). The next call to interrupt 
callback will cause the IO function to return AVERROR_EXIT causing 
write_packet to fail with same error.


When the FIFO is used the write_packet call will return immediately. The 
I/O blocking will happen in consumer thread, in some of the 
fifo_thread_write_packet calls. Let's say that during that time all 
packets are send to queue and write_trailer is called. The main thread
is blocked by pthread_join(). However when the interrupt condition is 
set (for example by receiving SIGINT as in ffmpeg), this is still 
handled by the blocking IO function which will return AVERROR_EXIT 
causing fifo_thread_write_packet to fail with the same error, causing 
the thread to terminate which unblocks write_trailer.


My point is that the same asynchronous mechanism which is supposed to 
work in case of blocking I/O operation should work also in case 
pthread_join is blocking, so the I/O operation should be terminated the 
same way returning AVERROR_EXIT which will cause consumer thread to 
terminate and unblock pthread_join call...


Am I missing something?

Thank you for your help

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

Re: [FFmpeg-devel] [PATCH 3/7] avformat/tee: Handle AV_NOPTS_VALUE correctly

[Jan Sebechlebsky]

On 07/04/2016 05:07 PM, Hendrik Leppkes wrote:

On Mon, Jul 4, 2016 at 4:45 PM,   wrote:

+if (pkt->pts != AV_NOPTS_VALUE)
+pkt2.pts = av_rescale_q(pkt->pts, tb, tb2);
+if (pkt->dts != AV_NOPTS_VALUE)
+pkt2.dts = av_rescale_q(pkt->dts, tb, tb2);

Maybe this entire thing could use av_packet_rescale_ts?

Sure! I wasn't aware there is such function, I'll wait if there are some 
more comments to the patchset and change this.


Thank you,
Jan
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] Fix memory leaks in segment muxer

[Jan Sebechlebsky]

Ping...

On 06/20/2016 05:24 PM, sebechlebsky...@gmail.com wrote:

From: Jan Sebechlebsky <sebechlebsky...@gmail.com>

Hello,
I've observed several memory leaks in segment muxer in case the
failure happens in seg_init (write_header call).
I'm sending patchset to fix those.

Regards,
Jan

Jan Sebechlebsky (3):
   avformat/segment: Check write_header return value immediately
   avformat/segment: Ensure write_trailer is called
   avformat/segment: Use deinit function to deinitialize muxer

  libavformat/segment.c | 68 ---
  1 file changed, 37 insertions(+), 31 deletions(-)



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

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

[Jan Sebechlebsky]

Hello Moritz,
Thanks for feedback and help with grammar! I'll include your fixes in 
the next version of patch, which I'm planning to send today.


On 07/02/2016 08:49 PM, Moritz Barsnick wrote:

+#define FIFO_DEFAULT_RECOVERY_WAIT_TIME100 // 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 100 seconds. Or am I
missing something? (I could check by applying your patch. Sorry, didn't
do so.)
I agree this is confusing - The type of the recovery_wait_time option is 
"duration" which allows you to enter duration in several formats and 
converts it to the microseconds. So, when you enter an integer it 
actually takes this number for a number of seconds and converts it to 
the microseconds. The default value is therefore really just 1 second.
I will change the constant name to FIFO_DEFAULT_RECOVERY_WAIT_TIME_USEC 
and take a look at how "duration" options are usually documented.


Thank you for your help

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

Re: [FFmpeg-devel] [PATCH 02/13] lavf: update auto-bsf to new BSF API

[Jan Sebechlebsky]

Hello Michael,

On 06/29/2016 04:53 AM, Michael Niedermayer wrote:

On Tue, Jun 28, 2016 at 01:33:13PM +0200, Nicolas George wrote:

Le primidi 11 messidor, an CCXXIV, Nicolas George a écrit :

Well, looking at the code, I am thinking that the current design is flawed:
the extra alloc in ff_bsf_get_packet() seems completely useless, and could
be removed as is without any other change in the current code, because all
current filters are 1:1, it would be different for future 1:N filters. Maybe
implementing ff_bsf_peek_packet() and using it to replace
ff_bsf_get_packet() in 1:1 cases would do the trick.

I have checked that the following quick-and-dirty changes pass FATE. Making
it in shape (testing filters not covered by FATE, making future-proof) would
take time that I would like to spend on lavfi right now. But it proves the
overhead can be reduced easily.

applying only the attached patch causes
./ffmpeg -i ~/videos/matrixbench_mpeg2.mpg  -vbsf noise file.avi
to

...

Segmentation fault

valgrind is also unhappy
I think Nicolas has not meant the patch to be really used - he suggested 
to create separate ff_bsf_peek_packet function which would just pass 
pointer to packet from internal bsf structure and do not allocate new 
packet structure each time the pointer to existing one is passed. In 
this case caller must not free the packet - just dereference it.

Try attached patch(es) - valgrind seems quite happy.

Regards,
Jan
>From 05755a7f36fd8d0b135288d47dd51e11f93ca488 Mon Sep 17 00:00:00 2001
From: Jan Sebechlebsky <sebechlebsky...@gmail.com>
Date: Wed, 29 Jun 2016 19:41:37 +0200
Subject: [PATCH 1/2] avcodec/bsf: Add ff_bsf_peek_packet function

This functions allows to reuse packet in internal bsf
structure and saves extra allocation.

Signed-off-by: Jan Sebechlebsky <sebechlebsky...@gmail.com>
---
 libavcodec/bsf.c | 16 
 libavcodec/bsf.h |  8 
 2 files changed, 24 insertions(+)

diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c
index 88b7f29..1c2e5b6 100644
--- a/libavcodec/bsf.c
+++ b/libavcodec/bsf.c
@@ -217,3 +217,19 @@ int ff_bsf_get_packet(AVBSFContext *ctx, AVPacket **pkt)
 
 return 0;
 }
+
+int ff_bsf_peek_packet(AVBSFContext *ctx, AVPacket **pkt)
+{
+AVBSFInternal *in = ctx->internal;
+
+if (in->eof)
+return AVERROR_EOF;
+
+if (!ctx->internal->buffer_pkt->data &&
+!ctx->internal->buffer_pkt->side_data_elems)
+return AVERROR(EAGAIN);
+
+*pkt = ctx->internal->buffer_pkt;
+
+return 0;
+}
diff --git a/libavcodec/bsf.h b/libavcodec/bsf.h
index 3435df5..ecad3a6 100644
--- a/libavcodec/bsf.h
+++ b/libavcodec/bsf.h
@@ -28,6 +28,14 @@
  */
 int ff_bsf_get_packet(AVBSFContext *ctx, AVPacket **pkt);
 
+/**
+ * Called by bitstream filters to get packet for filtering.
+ * This function should be preferred in 1:1 bitstream filters.
+ * Caller must not free the packet, but is responsible for
+ * dereferencing it.
+ */
+int ff_bsf_peek_packet(AVBSFContext *ctx, AVPacket **pkt);
+
 const AVClass *ff_bsf_child_class_next(const AVClass *prev);
 
 #endif /* AVCODEC_BSF_H */
-- 
1.9.1

>From 3f917440555aad67d3c213e94499812f225c79d1 Mon Sep 17 00:00:00 2001
From: Jan Sebechlebsky <sebechlebsky...@gmail.com>
Date: Wed, 29 Jun 2016 19:46:43 +0200
Subject: [PATCH 2/2] avcodec/noise_bsf: Use ff_bsf_peek_packet function

Use of ff_bsf_peek_packet saves one malloc operation
compared to ff_bsf_get_packet.

Signed-off-by: Jan Sebechlebsky <sebechlebsky...@gmail.com>
---
 libavcodec/noise_bsf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libavcodec/noise_bsf.c b/libavcodec/noise_bsf.c
index 0aebee1..9c21f7b 100644
--- a/libavcodec/noise_bsf.c
+++ b/libavcodec/noise_bsf.c
@@ -44,7 +44,7 @@ static int noise(AVBSFContext *ctx, AVPacket *out)
 if (amount <= 0)
 return AVERROR(EINVAL);
 
-ret = ff_bsf_get_packet(ctx, );
+ret = ff_bsf_peek_packet(ctx, );
 if (ret < 0)
 return ret;
 
@@ -66,7 +66,7 @@ static int noise(AVBSFContext *ctx, AVPacket *out)
 fail:
 if (ret < 0)
 av_packet_unref(out);
-av_packet_free();
+av_packet_unref(in);
 return ret;
 }
 
-- 
1.9.1

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

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

[Jan Sebechlebsky]

Hello Nicolas,

On 06/28/2016 03:42 PM, Nicolas George wrote:

+The fifo pseudo-muxer allows to separate encoding from any other muxer
+by using first-in-first-out queue and running the actual muxer in separate
+thread. This is especially useful in combination with tee muxer
+and output to several destinations with different reliability/writing 
speed/latency.
+
+The fifo muxer can operate in regular or fully non-blocking mode. This 
determines
Not true: the current code is non-blocking for packets, but not for the
trailer. It can not be really non-blocking but as is it can block
indefinitely.


+the behaviour in case the queue fills up. In regular mode the encoding is 
blocked
+until muxer processes some of the packets from queue, in non-blocking mode the 
packets
+are thrown away rather than blocking the encoder (this might be useful in 
real-time
+streaming scenario).

Would it be ok to call it "asynchronous"?

+@item fifo_format
+Specify the format name. Useful if it cannot be guessed from the
+output name suffix.

This option is defined but never used.
I don't understand this note - the fifo_format option is used (and seems 
to work)?

+
+@item queue_size
+Specify size of the queue (number of packets)
+
+@item format_opts
+Specify format options for the underlying muxer. Muxer options can be specified
+as a list of @var{key}=@var{value} pairs separated by ':'.

Yay, another trip to escaping hell!
Unfortunately :( Do you think cmd muxer initialization could be easily 
modified in a way that muxer would also get access to option dictionary?



+if (!(avf2->oformat->flags & AVFMT_NOFILE)) {
+ret = avf2->io_open(avf2, >pb, avf->filename, AVIO_FLAG_WRITE, 
_options);
+if (ret < 0) {
+av_log(avf, AV_LOG_ERROR, "Error opening %s: %s\n",
+   avf->filename, av_err2str(ret));
+goto end;
+}
+}

Why do we not have a utility function for that?

I'll create ff_format_io_open and, this can be refactored in other 
muxer, too.



+s_idx = pkt->stream_index;
+src_tb = avf->streams[s_idx]->time_base;
+dst_tb = avf2->streams[s_idx]->time_base;
+
+pkt->pts = av_rescale_q(pkt->pts, src_tb, dst_tb);
+pkt->dts = av_rescale_q(pkt->dts, src_tb, dst_tb);
+pkt->duration = av_rescale_q(pkt->duration, src_tb, dst_tb);

This looks suspicious.

For starters, it does not handle the NOPTS value, but that is an easy fix.

More worryingly, it looks that the application does not see the time base
selected by the real muxer. It could be a problem.
I'll change it to handle NOPTS (the same should be done for tee muxer, 
so I'll patch that too). Can you specify what could be the problem when 
the application does not see the time base of real muxer? If it was 
really neccessary, I can make write_header call before actually 
executing the thread (so only write_packet calls would be non-blocking) 
and copy the selected timebase, however, I prefer write_header to be 
executed in consumer thread. Also, this situation is impossible to 
handle in tee muxer, since every slave muxer can select different time base.



+/* Check and solve overflow */
+pthread_mutex_lock(>overflow_flag_lock);
+if (fifo->overflow_flag) {
+av_thread_message_flush(queue);
+if (fifo->restart_with_keyframe)
+fifo->drop_until_keyframe = 1;
+fifo->overflow_flag = 0;
+just_flushed = 1;
+}
+pthread_mutex_unlock(>overflow_flag_lock);

I think the communication logic here needs an extensive comment here.

And I am a bit worried about the extra lock: mixing several communication
mechanisms between threads is a recipe for headache.
I'll add the comment. I've tried to do this without the extra lock at 
first by setting error to the thread message queue and adding 
threadmessage queue flag which allows the error to be returned 
immediately, but I think using this single extra lock is really cleaner 
solution, I would prefer that.



+avf2->io_close = avf->io_close;
+avf2->io_open = avf->io_open;

This could be dangerous too, I am not sure these functions are guaranteed to
be thread-safe. It needs at least documentation.
I'll try to check existing functions for dangerous operations - I guess 
we could add a note to documentation saying that the io_{open,close} 
should be thread-safe.

+st2->r_frame_rate= st->r_frame_rate;
+st2->time_base   = st->time_base;
+st2->start_time  = st->start_time;
+st2->duration= st->duration;
+st2->nb_frames   = st->nb_frames;
+st2->disposition = st->disposition;
+st2->sample_aspect_ratio = st->sample_aspect_ratio;
+st2->avg_frame_rate  = st->avg_frame_rate;

Why do we not have an utility function for that (bis)?

I've actually created the function av_stream_encode_params_copy:

Re: [FFmpeg-devel] [PATCH 02/13] lavf: update auto-bsf to new BSF API

[Jan Sebechlebsky]

On 06/28/2016 12:10 PM, Nicolas George wrote:
In summary: I am ok with this version IFF it works without malloc() 
overhead for empty lists and is reasonably simple. 
I think I can start to work on this. I think it can be done without 
dynamic allocation.
Apart from that - do you think it would be worth a try to implement 
memory pool for AVPacket allocation ?


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

Re: [FFmpeg-devel] [PATCH 02/13] lavf: update auto-bsf to new BSF API

[Jan Sebechlebsky]

On 06/20/2016 02:04 PM, Michael Niedermayer wrote:

On Sun, Jun 12, 2016 at 04:30:50PM -0500, Rodger Combs wrote:

---
  libavformat/internal.h |  5 +++--
  libavformat/mux.c  | 45 +-
  libavformat/segment.c  |  6 +++--
  libavformat/utils.c| 59 +-
  4 files changed, 91 insertions(+), 24 deletions(-)

This fixes the memleak in fate-matroska-remux
is there a reason why this is not pushed ?

[...]


___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
There is a discussion about creating new API for list of BSFs 
processing, so maybe the author decided to wait for that, but I guess 
there is no reason not to use this patch until the API is ready.


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

Re: [FFmpeg-devel] [PATCH v2] avformat/tee: Support arbitrary number of slaves

[Jan Sebechlebsky]

Ping :)

On 06/12/2016 08:17 PM, sebechlebsky...@gmail.com wrote:

From: Jan Sebechlebsky <sebechlebsky...@gmail.com>

Signed-off-by: Jan Sebechlebsky <sebechlebsky...@gmail.com>
---
  libavformat/tee.c | 24 +++-
  1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/libavformat/tee.c b/libavformat/tee.c
index 806beaa..bf7438c 100644
--- a/libavformat/tee.c
+++ b/libavformat/tee.c
@@ -27,8 +27,6 @@
  #include "avformat.h"
  #include "avio_internal.h"
  
-#define MAX_SLAVES 16

-
  typedef enum {
  ON_SLAVE_FAILURE_ABORT  = 1,
  ON_SLAVE_FAILURE_IGNORE = 2
@@ -52,7 +50,7 @@ typedef struct TeeContext {
  const AVClass *class;
  unsigned nb_slaves;
  unsigned nb_alive;
-TeeSlave slaves[MAX_SLAVES];
+TeeSlave *slaves;
  } TeeContext;
  
  static const char *const slave_delim = "|";

@@ -203,6 +201,7 @@ static void close_slaves(AVFormatContext *avf)
  for (i = 0; i < tee->nb_slaves; i++) {
  close_slave(>slaves[i]);
  }
+av_freep(>slaves);
  }
  
  static int open_slave(AVFormatContext *avf, char *slave, TeeSlave *tee_slave)

@@ -443,17 +442,17 @@ static int tee_write_header(AVFormatContext *avf)
  TeeContext *tee = avf->priv_data;
  unsigned nb_slaves = 0, i;
  const char *filename = avf->filename;
-char *slaves[MAX_SLAVES];
+char **slaves = NULL;
  int ret;
  
  while (*filename) {

-if (nb_slaves == MAX_SLAVES) {
-av_log(avf, AV_LOG_ERROR, "Maximum %d slave muxers reached.\n",
-   MAX_SLAVES);
-ret = AVERROR_PATCHWELCOME;
+char *slave = av_get_token(, slave_delim);
+if (!slave) {
+ret = AVERROR(ENOMEM);
  goto fail;
  }
-if (!(slaves[nb_slaves++] = av_get_token(, slave_delim))) {
+dynarray_add(, _slaves, slave);
+if (!slaves) {
  ret = AVERROR(ENOMEM);
  goto fail;
  }
@@ -461,6 +460,10 @@ static int tee_write_header(AVFormatContext *avf)
  filename++;
  }
  
+if (!(tee->slaves = av_mallocz_array(nb_slaves, sizeof(*tee->slaves {

+ret = AVERROR(ENOMEM);
+goto fail;
+}
  tee->nb_slaves = tee->nb_alive = nb_slaves;
  
  for (i = 0; i < nb_slaves; i++) {

@@ -483,12 +486,14 @@ static int tee_write_header(AVFormatContext *avf)
  av_log(avf, AV_LOG_WARNING, "Input stream #%d is not mapped "
 "to any slave.\n", i);
  }
+av_free(slaves);
  return 0;
  
  fail:

  for (i = 0; i < nb_slaves; i++)
  av_freep([i]);
  close_slaves(avf);
+av_free(slaves);
  return ret;
  }
  
@@ -505,6 +510,7 @@ static int tee_write_trailer(AVFormatContext *avf)

  ret_all = ret;
  }
  }
+av_freep(>slaves);
  return ret_all;
  }
  


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

Re: [FFmpeg-devel] [PATCH] avutil/threadmessage.h: Fix swapped comments

[Jan Sebechlebsky]

Ping :)

On 06/10/2016 07:11 PM, sebechlebsky...@gmail.com wrote:

From: Jan Sebechlebsky <sebechlebsky...@gmail.com>

Fix swapped descriptions of av_thread_message_queue_set_err_send
and av_thread_message_queue_set_err_recv.

Signed-off-by: Jan Sebechlebsky <sebechlebsky...@gmail.com>
---
  libavutil/threadmessage.h | 16 
  1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/libavutil/threadmessage.h b/libavutil/threadmessage.h
index e256cae..8480a0a 100644
--- a/libavutil/threadmessage.h
+++ b/libavutil/threadmessage.h
@@ -69,10 +69,10 @@ int av_thread_message_queue_recv(AVThreadMessageQueue *mq,
  /**
   * Set the sending error code.
   *
- * If the error code is set to non-zero, av_thread_message_queue_recv() will
- * return it immediately when there are no longer available messages.
- * Conventional values, such as AVERROR_EOF or AVERROR(EAGAIN), can be used
- * to cause the receiving thread to stop or suspend its operation.
+ * If the error code is set to non-zero, av_thread_message_queue_send() will
+ * return it immediately. Conventional values, such as AVERROR_EOF or
+ * AVERROR(EAGAIN), can be used to cause the sending thread to stop or
+ * suspend its operation.
   */
  void av_thread_message_queue_set_err_send(AVThreadMessageQueue *mq,
int err);
@@ -80,10 +80,10 @@ void 
av_thread_message_queue_set_err_send(AVThreadMessageQueue *mq,
  /**
   * Set the receiving error code.
   *
- * If the error code is set to non-zero, av_thread_message_queue_send() will
- * return it immediately. Conventional values, such as AVERROR_EOF or
- * AVERROR(EAGAIN), can be used to cause the sending thread to stop or
- * suspend its operation.
+ * If the error code is set to non-zero, av_thread_message_queue_recv() will
+ * return it immediately when there are no longer available messages.
+ * Conventional values, such as AVERROR_EOF or AVERROR(EAGAIN), can be used
+ * to cause the receiving thread to stop or suspend its operation.
   */
  void av_thread_message_queue_set_err_recv(AVThreadMessageQueue *mq,
int err);


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

Re: [FFmpeg-devel] [PATCH] avformat/tee: Support arbitrary number of slaves

[Jan Sebechlebsky]

On 06/12/2016 04:26 PM, Nicolas George wrote:

Le tridi 23 prairial, an CCXXIV, sebechlebsky...@gmail.com a écrit :

From: Jan Sebechlebsky <sebechlebsky...@gmail.com>

Signed-off-by: Jan Sebechlebsky <sebechlebsky...@gmail.com>
---
  libavformat/tee.c | 27 +--
  1 file changed, 17 insertions(+), 10 deletions(-)


Out of curiosity, in what situation is this a problem?

I don't think someone will ever need more than 16 slaves,
but why not :)

Thanks for review!

Jan
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] Tee improvement - discussion

[Jan Sebechlebsky]

On 06/03/2016 03:53 PM, Nicolas George wrote:

Le nonidi 9 prairial, an CCXXIV, Jan Sebechlebsky a écrit :

If I understand it correctly, I think this will be handled quite smoothy -
you don't have to kill the blocked write, the application has to call
write_trailer anyway before closing the muxer, so I guess this is the place
where tee will wait for the muxer's pending write operation to finish. So
application will know, that after call to write_trailer all operations are
finished.

There is no "after call to write_trailer": the real muxer is blocked
writing, it will not terminate. By pushing it in a thread, you may have
achieved non-blocking writes for the application, but that just pushed the
issue a bit further.

To achieve real non-blocking operation, you need to have all the muxers and
protocols stack working in non-blocking mode, or you need asynchronous
killing of I/O operations. Asynchronous killing of I/O operations exists,
that is pthread_cancel(), but it is quite tricky to use and we had no end of
portability issues with it too.

I guess then, that implementing it without killing blocked threads 
should not introduce any new problems - the process will hang as it does 
now in this kind of situation.
I can try to take a look if canceling the thread could be used. How 
do you imagine this (like that there will be a timeout enforced by 
killing the thread?). Do you know where are I/O operations which can 
block forever without timeouting used, so I can try to take a look at 
some particular case?


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

Re: [FFmpeg-devel] Tee improvement - discussion

[Jan Sebechlebsky]

On 06/03/2016 03:46 PM, Nicolas George wrote:

Sorry for forgetting to reply.

L'octidi 8 prairial, an CCXXIV, Marton Balint a écrit :

What if we decouple the non-blocking queue and the retry on failure logic to
a separate "buffer" or "fifo" muxer?

This seems like what you are trying to do, and by using the exisiting muxer
interface, we don't have to design new API around it.

Although I don't yet see if using three levels of muxing (E.g. the tee muxer
invokes the fifo muxer, which will invoke the underlying real muxer) can
cause any unseen problems or not.

Implementing the non-blocking logic in a separate muxer would indeed avoid
most of my objections. It would still be very far from a real solution to
the non-blocking problem, but it can certainly do for now.

Great! I guess I can do it that way then :)

Regards,



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

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

Re: [FFmpeg-devel] Tee improvement - discussion

[Jan Sebechlebsky]

Hi Nicolas,

On 05/26/2016 12:09 PM, Nicolas George wrote:
But the "stick everything in threads" approach is flawed. Just think 
about what will happen if the actual muxer is blocking on a write 
while the application, seeing non-blocking behaviour, wants to close 
it: how do you kill the blocked write? Regards 
If I understand it correctly, I think this will be handled quite smoothy 
- you don't have to kill the blocked write, the application has to call 
write_trailer anyway before closing the muxer, so I guess this is the 
place where tee will wait for the muxer's pending write operation to 
finish. So application will know, that after call to write_trailer all 
operations are finished.


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

Re: [FFmpeg-devel] Tee improvement - discussion

[Jan Sebechlebsky]

Hello Nicolas,

On 05/19/2016 12:44 PM, Nicolas George wrote:

Le tridi 23 floréal, an CCXXIV, Jan Sebechlebsky a écrit :

My current idea is to create queue for each output (as Marton suggested) and
process each output in separate thread. I was also considering using just
single queue, but since the AVPackets are referenced counted, the memory
overhead is negligible and using multiple queues will simplify the code.
Apart from getting advantage of non-blocking processing with multiple slave
muxers, error handling will also be improved.



Another question is what to do when some of the queues becomes full,
discussed options were so far:
 - Block write_packet call until the queue frees - this might be useful
when producer is faster than consumer, and we don't want to drop any packets
when recording to file.
 - Drop some yet unprocessed packets (until next keyframe, or free some
portion of queue) to free the queue - this might be useful for network
outputs.

I must say, I am not very happy with the direction this project takes.

Non-blocking muxers (and demuxers, and protocols) is a white whale for our
API: we really really want it, but it huge and very hard to catch.
Does this objection apply to the non-blocking full queue handling stated 
above or the whole project (using threads in tee)?

It is something that needs to be implemented globally with a very careful
design. Definitely not something that can be added in a corner of an obscure
muxer. We already went down that path twice, with the thread for the UDP
protocol and with running demuxer in threads in FFmpeg. Both did lead to
endless complications: bugs, inconsistencies, portability trouble.

If you want to work on that (in order to bring these enhancements to the tee
muxer, fitting the topic of the GSoC project), you probably can. But I
expect this to be very hard. It has been rejected as a proposal for a GSoC
project in the past on the basis that it is probably too hard for a random
student. Also, I am not sure that Marton, your official mentor, would want
such a huge shift in the project's goals.
I must accept that for now I may lack the complete overview over 
the project to see the potential problems which this could introduce.
However in the proposed project there is enough time to do proper 
testing and eliminate the found issues and the solution described in 
proposal could still be a benefit (if the ideal solution is too hard). 
Also I thing the proposed changes will not bring too much complexity to 
the project regarding thread handling.
I don't mind changing the plans and implementing it in other way if 
it is better and achievable - but you might be right this may be too hard.
Anyway if you give me some directions, (what are the requirements for 
the more general solution, what are the particular problems in the 
proposed one) I can at least try to dive more into it (getting more 
familiar with the project will be only a benefit).
I don't know how such changes are handled in GSoC (since I probably 
wouldn't be able meet milestone if the solution would require the 
complex global changes first).



Regards,



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

Thanks,

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

Re: [FFmpeg-devel] [PATCH] avformat/tee: Move to new BSF API

[Jan Sebechlebsky]

Hello Nicolas,
I am sorry for a delayed reply,

On 05/12/2016 12:17 PM, Nicolas George wrote:

Le quartidi 24 floréal, an CCXXIV, Jan Sebechlebsky a écrit :

I can change it to array, the advantage of using linked list was that number
of bitstream
filters used is not known before arguments are parsed, this way the filters
were initialized and as the the filters were parsed. With array the argument
will be parsed twice - first pass just to count and allocate the array,
second to initialize the filters. But it's not big deal to do it that way.

There are several instances of similar code, the array is often reallocated.
We have macros to reallocate an array with double size efficiently. But a
quick pre-parsing to count the number of filters is also a legitimate idea.


I'll rewrite that to use array + reallocation.

Should I wait with this patch until your changes are pushed?

You can apply it on a branch of your working tree to continue your task.


Can you explain this little bit more?

Recursive filtering is annoying to debug (lots of nested stack frames, hard
to put break points) and does not play well with threads.


Should I rewrite it it non-recursive
way (process packets with first filter, accumulare resulting packets,
process with next one and so on)?

The code in avconv.c has a nice way of dealing with the issue, I encourage
to look at it.
Could you point me to that code? I tried to search for it, but it seems 
there is old BSF API used in that file.


We briefly had an API to apply several bitstream filters sequentially, until
it was NIHed by the fork. I would support re-adding one.
Maybe after rewriting this in tee I can pull that code out to serve this 
purpose.


Regards,


Thanks for feedback

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

Re: [FFmpeg-devel] Tee improvement - discussion

[Jan Sebechlebsky]

Hello Marton,
sorry for a delayed reply.

On 05/16/2016 01:20 AM, Marton Balint wrote:


On Wed, 11 May 2016, Jan Sebechlebsky wrote:


Hi,
I'll be working on tee muxer improvement during GSoC 2016 and I 
thought maybe it is a good idea to ask about ideas which any of you 
might have regarding what could be done in avformat/tee.


Currently, the tee muxer works in a simple way, incoming packets are 
just iteratively fed to several output muxers (each muxer blocking 
the next). Also there is no possibility to reset muxer on error.


My current idea is to create queue for each output (as Marton 
suggested) and process each output in separate thread. I was also 
considering using just single queue, but since the AVPackets are 
referenced counted, the memory overhead is negligible and using 
multiple queues will simplify the code. Apart from getting advantage 
of non-blocking processing with multiple slave muxers, error handling 
will also be improved.


The option allowing to ignore failure on certain outputs is already 
implemented (this allows for example network streaming to continue 
even after disk fills up, or recording to file to continue when 
network error occurs). In the final solution the tee muxer will also 
support restarting failed output. There is a question how to deal 
with restart, there are several options what to do and this could be 
also configurable for the user (with reasonable default set):

(Does these options make sense to you? Do you have ideas for more? )
- Attempt restart immediately after failure, if it doesn't 
succeed attempt with the
next packet (keyframe). Repeat  (argument) times before giving up 
on that output.

- Attempt restart after certain time (argument).
- Combination of two options above. Attempt to recover with next 
keyframes, after several failures wait for some amount of time and 
attempt again.


I think a per-output restart_delay with an 1 second default, and a 
per-output restart_with_keyframe boolean flag defaulting to true would 
be simple yet powerful enough.


Another question is what to do when some of the queues becomes full, 
discussed options were so far:
- Block write_packet call until the queue frees - this might be 
useful when producer is faster than consumer, and we don't want to 
drop any packets when recording to file.
- Drop some yet unprocessed packets (until next keyframe, or free 
some portion of queue) to free the queue - this might be useful for 
network outputs.


I suggest a per-output "blocking" boolean flag to specify outputs, 
where - in case of a full queue - writing is blocked. A per-output 
queue_size setting might be useful as well.


If a queue is full, but non-blocking, then the producer should set an 
overflow flag of that queue, and not push any further data to the 
queue, unless the overflow flag is cleared.


The consumer should be responsible to clear the overflow flag. If the 
consumer feels itself ready to receive packets, but an overflow 
happened, then it should probably want to drop all existing packets in 
its queue and clear the flag after that. This way if the consumer is 
only slightly slower than the producer, the packet losses will be 
bursty, and this also reduce the chance to operate with an always 
almost-full queue causing unnecessary latency and memory usage.


Once the consumer is receiving packets again, it should wait for the 
first keyframe (if restart_with_keyframe is set) and start processing 
packets from that.



I think I like these ideas, it seems really like an elegant way to do it.


I'm thinking of implementing this queue by wrapping up AVFifoBuffer 
(similarily than AVThreadMessageQueue does but with the configurable 
behaviour as described above).


Exactly what behaviour is missing from AVThreadMessageQueue? Isn't 
there a chance to extend that, or implement all additional logic on 
top of it?


What is missing is basically just the discussed configurable behaviour 
how to deal with overfilled queue. I originally thought that the queue 
would flush old packets automatically (from the point of view of 
consumer / producer it would be transparent). But if the consumer will 
be responsible for flushing the packets in non-blocking mode I guess the 
AVThreadMessageQueue will do the work. This really simplifies the whole 
task.


I just wonder, is simply flushing the whole buffer good solution? 
Shouldn't keyframe flag be considered (either flush until next 
keyframe(s)), or ignore packet which arrived after flush until new 
keyframe arrives? If so this wouldrequire some additional functionality 
to be added to AVThreadMessageQueue (since it would be no longer related 
to general message queue it should be probably implemented separately).


If you have any ideas or notes regarding what would be good to do in 
libavformat/tee and want to join discussion, I'll be glad to take 
them into account and improve the proposed project.




Great, thanks.

Regar

[FFmpeg-devel] Tee improvement - discussion

[Jan Sebechlebsky]

Hi,
I'll be working on tee muxer improvement during GSoC 2016 and I thought 
maybe it is a good idea to ask about ideas which any of you might have 
regarding what could be done in avformat/tee.


Currently, the tee muxer works in a simple way, incoming packets are 
just iteratively fed to several output muxers (each muxer blocking the 
next). Also there is no possibility to reset muxer on error.


My current idea is to create queue for each output (as Marton suggested) 
and process each output in separate thread. I was also considering using 
just single queue, but since the AVPackets are referenced counted, the 
memory overhead is negligible and using multiple queues will simplify 
the code. Apart from getting advantage of non-blocking processing with 
multiple slave muxers, error handling will also be improved.


The option allowing to ignore failure on certain outputs is already 
implemented (this allows for example network streaming to continue even 
after disk fills up, or recording to file to continue when network error 
occurs). In the final solution the tee muxer will also support 
restarting failed output. There is a question how to deal with restart, 
there are several options what to do and this could be also configurable 
for the user (with reasonable default set):

(Does these options make sense to you? Do you have ideas for more? )
- Attempt restart immediately after failure, if it doesn't succeed 
attempt with the
next packet (keyframe). Repeat  (argument) times before giving up on 
that output.

- Attempt restart after certain time (argument).
- Combination of two options above. Attempt to recover with next 
keyframes, after several failures wait for some amount of time and 
attempt again.


Another question is what to do when some of the queues becomes full, 
discussed options were so far:
- Block write_packet call until the queue frees - this might be 
useful when producer is faster than consumer, and we don't want to drop 
any packets when recording to file.
- Drop some yet unprocessed packets (until next keyframe, or free 
some portion of queue) to free the queue - this might be useful for 
network outputs.


I'm thinking of implementing this queue by wrapping up AVFifoBuffer 
(similarily than AVThreadMessageQueue does but with the configurable 
behaviour as described above).


If you have any ideas or notes regarding what would be good to do in 
libavformat/tee and want to join discussion, I'll be glad to take them 
into account and improve the proposed project.


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

Re: [FFmpeg-devel] [PATCH v10 3/3][GSOC] avformat/tee: Handling slave failure in tee muxer

[Jan Sebechlebsky]

On 04/21/2016 12:14 AM, Nicolas George wrote:

Le duodi 2 floréal, an CCXXIV, sebechlebsky...@gmail.com a écrit :


+if (av_strerror(err_n, errbuf, sizeof(errbuf)) < 0)
+errbuf_ptr = strerror(AVUNERROR(err_n));

strerror() should not be used, even though a few places still use it.
Furthermore, there is no reasonable reason for av_strerror() to fail and
strerror() to succeed.

I suggest you use av_err2str(), like the rest of the modern FFmpeg code.

I did not look at the rest of the patch, under the assumption that only this
bit changed. If not, please tell me (and in the future write it in the
comment part of the Git mail).

Also, no need to Cc me, I am subscribed to the list; just following the
reply-to as is normally done by default should be good.

I will look at the other patch soon.

Regards,


Thanks for feedback,
I've checked how the error string is retrieved in the use case you've 
posted (print_error in cmdutils.c which is called from ffmpeg.c) so 
that's why I used av_strerror together with strerror.


I'll send modified patch with av_err2str.

Only thing changed in the patch apart from the discussed change was, 
that I've removed forgotten unused variable avf2 declaration in 
tee_write_trailer function from patch.

I'll remeber to note changes next time in the comment.

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

Re: [FFmpeg-devel] [PATCH v9 3/3][GSOC] avformat/tee: Handling slave failure in tee muxer

[Jan Sebechlebsky]

Hi Nicolas,

On 04/20/2016 04:31 PM, Nicolas George wrote:


  
+static int tee_process_slave_failure(AVFormatContext *avf, unsigned slave_idx, int err_n)

+{
+TeeContext *tee = avf->priv_data;
+TeeSlave *tee_slave = >slaves[slave_idx];
+
+tee->nb_alive--;
+
+close_slave(tee_slave);
+
+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);
+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);

This is minor: it would probably be a good idea to print the error message
here, since the error is being ignored afterwards.

I am not sure, I've understood this. If you meant the error of slave 
muxer, it should be already printed (since it is handled by slave muxer 
itself).

Apart from the above comment, I see nothing wrong with the patch.

Regards,


Regards,

Jan S.
___
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

[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 <sebechlebsky...@gmail.com>

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 <sebechlebsky...@gmail.com>
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 <sebechlebsky...@gmail.com>
---
 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)) {
+ 

Re: [FFmpeg-devel] [PATCH v4 3/3] avformat/tee: Handling slave failure in tee muxer

[Jan Sebechlebsky]

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(>slaves[i]);
+if (tee->slaves[i].is_alive)
+close_slave(>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(, 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)
+

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

[Jan Sebechlebsky]

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


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


From: Jan Sebechlebsky <sebechlebsky...@gmail.com>

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.


I noticed a problem. tee->nb_slaves is only assigned after opening all 
slaves, so if any of the open_slave call fails, tee->nb_slaves will 
still be 0, so no resources will get freed.


On the other hand, if you move the assignment before the open_slave 
loop, you will also have to handle the case in close_slaves when 
tee_slave->avf is null, or you have to increase tee->nb_slaves 
incrementally in the loop...


Regards,
Marton
Thanks, this is another thing I overlooked when I was splitting the 
changes in 3 commits. Actually in third patch tee->nb_slaves is assigned 
before open_slave loop. I'll fix it to have stable version after each 
single patch.


Have a nice day

Jan S.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v4 3/3] avformat/tee: Handling slave failure in tee muxer

[Jan Sebechlebsky]

Hello,
can the patchset be applied as it is now? ( I don't want to hurry you, 
just reminding :) )


Jan S.
On 04/04/2016 12:06 AM, 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;
+}
+
  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(>slaves[i]);
+if (tee->slaves[i].is_alive)
+close_slave(>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(, 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)

Re: [FFmpeg-devel] [PATCH v2 3/3] Tee muxer improvement (handling slave failure)

[Jan Sebechlebsky]

On 03/29/2016 09:01 PM, Nicolas George wrote:

Le decadi 10 germinal, an CCXXIV, Nicolas George a écrit :

Le decadi 10 germinal, an CCXXIV, Jan Sebechlebsky a écrit :

+@item onfail
+Specify behaviour on output failure. This can be set to either 'abort' (which 
is

I believe @code{...} is recommended for explicit words.

Line containing @code{...} is not added by patch (Fix of documentation
should be probably separate commit).

The patch adds lines containing "'something'", and I was suggesting that it
should have been "@code{something}".

... and please remember to send your mail to the mailing-list, not
privately.

Regards,



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

Sorry for that, I'll change it.

Jan S.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Jan Sebechlebsky]

On 03/24/2016 09:51 PM, Marton Balint wrote:


On Thu, 24 Mar 2016, Jan Sebechlebsky wrote:


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.

Signed-off-by: Jan Sebechlebsky <sebechlebsky...@gmail.com>
---
libavformat/tee.c | 22 ++
1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/libavformat/tee.c b/libavformat/tee.c
index 09551b3..e43ef08 100644
--- a/libavformat/tee.c
+++ b/libavformat/tee.c
@@ -141,12 +141,14 @@ 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 (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);
@@ -198,6 +200,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;
@@ -277,7 +280,6 @@ 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);
@@ -292,7 +294,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 end;
}
spec++; /* consume separator */
}
@@ -337,6 +340,9 @@ static int open_slave(AVFormatContext *avf, char 
*slave, TeeSlave *tee_slave)

}

end:
+if ( ret < 0 ){
+close_slave(tee_slave);
+}


Do you really need to call close_slave here? As far as I see if 
open_slave fails then the failure path of tee_write_header will call 
close_slaves, so the streams will be closed, therefore no need to do 
it here.
You're right, actually at this point I don't need to call close_slave 
here. But it will be needed in the next commit, since close_slaves 
function skips already dead slaves and then in case of failure of a 
slave which is allowed to fail, this one would not be freed.


I'll move this to the next commit to have consistent changes in each one.


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


Have a nice day
Jan

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

[FFmpeg-devel] [PATCH v2 1/3] Refactor close_slaves function in tee muxer

[Jan Sebechlebsky]

Closing single slave operation is pulled out into separate
function close_slave(TeeSlave*).
Both close_slave and close_slaves function are moved before
open_slave function.

Signed-off-by: Jan Sebechlebsky <sebechlebsky...@gmail.com>
---
 libavformat/tee.c | 59 +++
 1 file changed, 33 insertions(+), 26 deletions(-)

diff --git a/libavformat/tee.c b/libavformat/tee.c
index 1390705..09551b3 100644
--- a/libavformat/tee.c
+++ b/libavformat/tee.c
@@ -135,6 +135,39 @@ end:
 return ret;
 }
 
+static void close_slave(TeeSlave* tee_slave)
+{
+AVFormatContext * avf;
+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;
+}
+}
+av_freep(_slave->stream_map);
+av_freep(_slave->bsfs);
+
+ff_format_io_close(avf,>pb);
+avformat_free_context(avf);
+tee_slave->avf = NULL;
+}
+
+static void close_slaves(AVFormatContext *avf)
+{
+TeeContext *tee = avf->priv_data;
+unsigned i;
+
+for (i = 0; i < tee->nb_slaves; i++) {
+if (tee->slaves[i].is_alive)
+close_slave(>slaves[i]);
+}
+}
+
 static int open_slave(AVFormatContext *avf, char *slave, TeeSlave *tee_slave)
 {
 int i, ret;
@@ -311,32 +344,6 @@ end:
 return ret;
 }
 
-static void close_slaves(AVFormatContext *avf)
-{
-TeeContext *tee = avf->priv_data;
-AVFormatContext *avf2;
-unsigned i, j;
-
-for (i = 0; i < tee->nb_slaves; i++) {
-avf2 = tee->slaves[i].avf;
-
-for (j = 0; j < avf2->nb_streams; j++) {
-AVBitStreamFilterContext *bsf_next, *bsf = tee->slaves[i].bsfs[j];
-while (bsf) {
-bsf_next = bsf->next;
-av_bitstream_filter_close(bsf);
-bsf = bsf_next;
-}
-}
-av_freep(>slaves[i].stream_map);
-av_freep(>slaves[i].bsfs);
-
-ff_format_io_close(avf2, >pb);
-avformat_free_context(avf2);
-tee->slaves[i].avf = NULL;
-}
-}
-
 static void log_slave(TeeSlave *slave, void *log_ctx, int log_level)
 {
 int i;
-- 
1.9.1

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

[FFmpeg-devel] [PATCH v2 3/3] Tee muxer improvement (handling slave failure)

[Jan Sebechlebsky]

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

diff --git a/doc/muxers.texi b/doc/muxers.texi
index c36c72c..6fa9054 100644
--- a/doc/muxers.texi
+++ b/doc/muxers.texi
@@ -1367,6 +1367,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 'abort' (which 
is
+default) or 'ignore'. 'abort' will cause whole process to fail in case of 
failure
+on this slave output. 'ignore' will ignore failure on this output, so other 
outputs
+will continue without being affected.
 @end table
 
 @subsection Examples
@@ -1381,6 +1387,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 e43ef08..a937efc 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;
+}
+
 static void close_slave(TeeSlave* tee_slave)
 {
 AVFormatContext * avf;
@@ -176,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;
@@ -196,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 = (SlaveFailurePolicy) 
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(, NULL, format, filename);
 if (ret < 0)
@@ -345,6 +379,7 @@ end:
 }
 av_free(format);
 av_free(select);
+av_free(on_fail);
 av_dict_free();
 av_freep(_select);
 return ret;
@@ -374,6 +409,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 = >slaves[slave_idx];
+
+tee_slave->is_alive = 0;
+tee->nb_alive--;
+
+if (needs_closing)
+close_slave(tee_slave);
+
+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 ) {
+

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

[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.

Signed-off-by: Jan Sebechlebsky <sebechlebsky...@gmail.com>
---
 libavformat/tee.c | 22 ++
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/libavformat/tee.c b/libavformat/tee.c
index 09551b3..e43ef08 100644
--- a/libavformat/tee.c
+++ b/libavformat/tee.c
@@ -141,12 +141,14 @@ 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 (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);
@@ -198,6 +200,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;
@@ -277,7 +280,6 @@ 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);
@@ -292,7 +294,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 end;
 }
 spec++; /* consume separator */
 }
@@ -337,6 +340,9 @@ static int open_slave(AVFormatContext *avf, char *slave, 
TeeSlave *tee_slave)
 }
 
 end:
+if ( ret < 0 ){
+close_slave(tee_slave);
+}
 av_free(format);
 av_free(select);
 av_dict_free();
-- 
1.9.1

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

[FFmpeg-devel] [PATCH] Tee muxer improvement (handling slave failure)

[Jan Sebechlebsky]

Adds per slave option 'onfail' to the tee muxer allowing an output to
fail, allowing other slaves output to continue.

Certain situations, when it does not make sense to continue are still
fatal (like allocation errors, or invalid options).

Update of tee muxer documentation.

Signed-off-by: Jan Sebechlebsky <sebechlebsky...@gmail.com>
---
 Solution to GSOC 2016 qualification task.

 doc/muxers.texi   |  11 +++-
 libavformat/tee.c | 150 ++
 2 files changed, 127 insertions(+), 34 deletions(-)

diff --git a/doc/muxers.texi b/doc/muxers.texi
index c36c72c..70f2c48 100644
--- a/doc/muxers.texi
+++ b/doc/muxers.texi
@@ -1354,6 +1354,12 @@ output name suffix.
 Specify a list of bitstream filters to apply to the specified
 output.
 
+@item onfail
+Specify behaviour on output failure. This can be set to either 'abort' (which 
is
+default) or 'ignore'. 'abort' will cause whole process to fail in case of 
failure
+on this slave output. 'ignore' will ignore failure on this output, so other 
outputs
+will continue without being affected.
+
 It is possible to specify to which streams a given bitstream filter
 applies, by appending a stream specifier to the option separated by
 @code{/}. @var{spec} must be a stream specifier (see @ref{Format
@@ -1374,10 +1380,11 @@ separated by commas (@code{,}) e.g.: @code{a:0,v}
 @itemize
 @item
 Encode something and both archive it in a WebM file and stream it
-as MPEG-TS over UDP (the streams need to be explicitly mapped):
+as MPEG-TS over UDP (the streams need to be explicitly mapped).
+Continue streaming even if output to file fails:
 @example
 ffmpeg -i ... -c:v libx264 -c:a mp2 -f tee -map 0:v -map 0:a
-  "archive-20121107.mkv|[f=mpegts]udp://10.0.1.255:1234/"
+  "[onfail=ignore]archive-20121107.mkv|[f=mpegts]udp://10.0.1.255:1234/"
 @end example
 
 @item
diff --git a/libavformat/tee.c b/libavformat/tee.c
index 1390705..c72bc7d 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,13 +146,25 @@ 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;
+}
+
 static int open_slave(AVFormatContext *avf, char *slave, TeeSlave *tee_slave)
 {
 int i, ret;
 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;
@@ -161,6 +184,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 = (SlaveFailurePolicy) 
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 on abort, so invalid option error will not 
be ignored
+tee_slave->on_fail = ON_SLAVE_FAILURE_ABORT;
+goto end;
+}
 
 ret = avformat_alloc_output_context2(, NULL, format, filename);
 if (ret < 0)
@@ -234,14 +268,14 @@ static int open_slave(AVFormatContext *avf, char *slave, 
TeeSlave *tee_slave)
 if ((ret = avf2->io_open(avf2, >pb, filename, AVIO_FLAG_WRITE, 
NULL)) < 0) {
 av_log(avf, AV_LOG_ERROR, "Slave '%s': error opening: %s\n",
slave, av_err2str(ret));
-goto end;
+goto open_failure;
 }
 }
 
 if ((ret = avformat_write_header(avf2, )) < 0) {
 av_log(avf, AV_LOG_ERROR, "Slave '%s': error writing header: %s\n",
slave, av_err2str(ret));
-goto end;
+goto open_failure;
 }
 
 tee_slave->av