mbox series

[FFmpeg-devel,0/5] FIFO meta muxer related improvements

Message ID 20201207100845.17520-1-jeebjp@gmail.com
Headers show
Series FIFO meta muxer related improvements
Related show

Message

Jan Ekström Dec. 7, 2020, 10:08 a.m. UTC
The primary parts of this are patches 1,4,5. 2 and 3 were just noticed when
poking at the recovery timestamp logic, where the stream-time comparison logic
seemed somewhat weird (such as comparing the pts to last_recovery_ts only if
last_recovery_ts == AV_NOPTS_VALUE). If they seem incorrectly understood,
they can be left out.

As for the rest of the patches, they address the following issues:
1. Even if the header and the first packet get written out, the muxer might
   still fail at writing to a server when it actually decides to do I/O.
   Currently (for example with the MP4 muxer) this leads to a constant retry
   loop as each recovery is actually "successful". This patch makes sure that
   even if a recovery is "successful", the following recovery will only happen
   after the configured time, easing the load on any receiving server.
4. In case of avformat_write_header failing, the fifo muxer until now would not
   close the avformat context and its underlying I/O. This is now added, so
   that file sockets do not keep creeping up.
5. Unset a configured codec_tag if it is not supported by the underlying muxer,
   as the API client has no visibility regarding whether a codec tag is
   acceptable in cases of meta-muxers such as fifo.

Jan

Bernard Boulay (1):
  avformat/fifo: always wait recovery_wait_time between recoveries

Jan Ekström (4):
  avformat/fifo: fix handling of stream-time non-NOPTS recovery
  avformat/fifo: cause immediate stream-time recovery if time went
    backwards
  avformat/fifo: close IO in case header writing fails
  avformat/fifo: unset codec tag if unsupported by underlying muxer

 libavformat/fifo.c | 77 ++++++++++++++++++++++++++++++----------------
 1 file changed, 51 insertions(+), 26 deletions(-)

Comments

Jan Ekström Jan. 11, 2021, 2:46 p.m. UTC | #1
On Mon, Dec 7, 2020 at 12:08 PM Jan Ekström <jeebjp@gmail.com> wrote:
>
> The primary parts of this are patches 1,4,5. 2 and 3 were just noticed when
> poking at the recovery timestamp logic, where the stream-time comparison logic
> seemed somewhat weird (such as comparing the pts to last_recovery_ts only if
> last_recovery_ts == AV_NOPTS_VALUE). If they seem incorrectly understood,
> they can be left out.
>
> As for the rest of the patches, they address the following issues:
> 1. Even if the header and the first packet get written out, the muxer might
>    still fail at writing to a server when it actually decides to do I/O.
>    Currently (for example with the MP4 muxer) this leads to a constant retry
>    loop as each recovery is actually "successful". This patch makes sure that
>    even if a recovery is "successful", the following recovery will only happen
>    after the configured time, easing the load on any receiving server.
> 4. In case of avformat_write_header failing, the fifo muxer until now would not
>    close the avformat context and its underlying I/O. This is now added, so
>    that file sockets do not keep creeping up.
> 5. Unset a configured codec_tag if it is not supported by the underlying muxer,
>    as the API client has no visibility regarding whether a codec tag is
>    acceptable in cases of meta-muxers such as fifo.
>
> Jan
>
> Bernard Boulay (1):
>   avformat/fifo: always wait recovery_wait_time between recoveries
>
> Jan Ekström (4):
>   avformat/fifo: fix handling of stream-time non-NOPTS recovery
>   avformat/fifo: cause immediate stream-time recovery if time went
>     backwards
>   avformat/fifo: close IO in case header writing fails
>   avformat/fifo: unset codec tag if unsupported by underlying muxer
>
>  libavformat/fifo.c | 77 ++++++++++++++++++++++++++++++----------------
>  1 file changed, 51 insertions(+), 26 deletions(-)
>

Ping for this patch set.

Jan
Jan Ekström Jan. 25, 2021, 8:01 a.m. UTC | #2
On Mon, Jan 11, 2021 at 4:46 PM Jan Ekström <jeebjp@gmail.com> wrote:
>
> On Mon, Dec 7, 2020 at 12:08 PM Jan Ekström <jeebjp@gmail.com> wrote:
> >
> > The primary parts of this are patches 1,4,5. 2 and 3 were just noticed when
> > poking at the recovery timestamp logic, where the stream-time comparison logic
> > seemed somewhat weird (such as comparing the pts to last_recovery_ts only if
> > last_recovery_ts == AV_NOPTS_VALUE). If they seem incorrectly understood,
> > they can be left out.
> >
> > As for the rest of the patches, they address the following issues:
> > 1. Even if the header and the first packet get written out, the muxer might
> >    still fail at writing to a server when it actually decides to do I/O.
> >    Currently (for example with the MP4 muxer) this leads to a constant retry
> >    loop as each recovery is actually "successful". This patch makes sure that
> >    even if a recovery is "successful", the following recovery will only happen
> >    after the configured time, easing the load on any receiving server.
> > 4. In case of avformat_write_header failing, the fifo muxer until now would not
> >    close the avformat context and its underlying I/O. This is now added, so
> >    that file sockets do not keep creeping up.
> > 5. Unset a configured codec_tag if it is not supported by the underlying muxer,
> >    as the API client has no visibility regarding whether a codec tag is
> >    acceptable in cases of meta-muxers such as fifo.
> >
> > Jan
> >
> > Bernard Boulay (1):
> >   avformat/fifo: always wait recovery_wait_time between recoveries
> >
> > Jan Ekström (4):
> >   avformat/fifo: fix handling of stream-time non-NOPTS recovery
> >   avformat/fifo: cause immediate stream-time recovery if time went
> >     backwards
> >   avformat/fifo: close IO in case header writing fails
> >   avformat/fifo: unset codec tag if unsupported by underlying muxer
> >
> >  libavformat/fifo.c | 77 ++++++++++++++++++++++++++++++----------------
> >  1 file changed, 51 insertions(+), 26 deletions(-)
> >
>
> Ping for this patch set.

Ping for this patch set.

Jan
Jan Ekström Feb. 3, 2021, 7:50 a.m. UTC | #3
On Mon, Jan 25, 2021 at 10:01 AM Jan Ekström <jeebjp@gmail.com> wrote:
>
> On Mon, Jan 11, 2021 at 4:46 PM Jan Ekström <jeebjp@gmail.com> wrote:
> >
> > On Mon, Dec 7, 2020 at 12:08 PM Jan Ekström <jeebjp@gmail.com> wrote:
> > >
> > > The primary parts of this are patches 1,4,5. 2 and 3 were just noticed when
> > > poking at the recovery timestamp logic, where the stream-time comparison logic
> > > seemed somewhat weird (such as comparing the pts to last_recovery_ts only if
> > > last_recovery_ts == AV_NOPTS_VALUE). If they seem incorrectly understood,
> > > they can be left out.
> > >
> > > As for the rest of the patches, they address the following issues:
> > > 1. Even if the header and the first packet get written out, the muxer might
> > >    still fail at writing to a server when it actually decides to do I/O.
> > >    Currently (for example with the MP4 muxer) this leads to a constant retry
> > >    loop as each recovery is actually "successful". This patch makes sure that
> > >    even if a recovery is "successful", the following recovery will only happen
> > >    after the configured time, easing the load on any receiving server.
> > > 4. In case of avformat_write_header failing, the fifo muxer until now would not
> > >    close the avformat context and its underlying I/O. This is now added, so
> > >    that file sockets do not keep creeping up.
> > > 5. Unset a configured codec_tag if it is not supported by the underlying muxer,
> > >    as the API client has no visibility regarding whether a codec tag is
> > >    acceptable in cases of meta-muxers such as fifo.
> > >
> > > Jan
> > >
> > > Bernard Boulay (1):
> > >   avformat/fifo: always wait recovery_wait_time between recoveries
> > >
> > > Jan Ekström (4):
> > >   avformat/fifo: fix handling of stream-time non-NOPTS recovery
> > >   avformat/fifo: cause immediate stream-time recovery if time went
> > >     backwards
> > >   avformat/fifo: close IO in case header writing fails
> > >   avformat/fifo: unset codec tag if unsupported by underlying muxer
> > >
> > >  libavformat/fifo.c | 77 ++++++++++++++++++++++++++++++----------------
> > >  1 file changed, 51 insertions(+), 26 deletions(-)
> > >
> >
> > Ping for this patch set.
>
> Ping for this patch set.

As there were no comments...

I think that while this is a behavioral change (you no longer get an
immediate retry in case you have recovery_wait_time set with the first
attempt):

1. the current way of doing recovery waits can cause DoS with muxers
such as movenc - which can return success initially (and thus the
retry counter is reset), but then fail to actually write actual data
out
2. I do not think we can ask from the muxer whether it actually sent
data out through the I/O or not.

Thus, just making the fifo meta muxer always wait for the
recovery_wait_time makes sense.

Additionally, there are some bug fixes that I found, such as the
forgotten closing of I/O, or unsetting the codec tag if it is not
supported by the actual underlying format (since the API client has no
visibility of the underlying format).

Jan