[FFmpeg-devel] [PATCH] avformat/fifo: utilize a clone packet for muxing

[Jan Ekström]

On Mon, Jan 25, 2021 at 9:47 AM Jan Ekström <jeebjp at gmail.com> wrote:
>
> On Thu, Jan 14, 2021 at 2:13 AM Jan Ekström <jeebjp at gmail.com> wrote:
> >
> > On Tue, Jan 12, 2021 at 9:59 AM Jan Ekström <jeebjp at gmail.com> wrote:
> > >
> > > On Mon, Jan 11, 2021 at 10:53 PM Andreas Rheinhardt
> > > <andreas.rheinhardt at gmail.com> wrote:
> > > >
> > > > Jan Ekström:
> > > > > On Tue, Dec 8, 2020 at 2:54 PM Jan Ekström <jeebjp at gmail.com> wrote:
> > > > >>
> > > > >> From: Jan Ekström <jan.ekstrom at 24i.com>
> > > > >>
> > > > >> This way the timestamp adjustments do not have to be manually
> > > > >> undone in case of failure and need to recover/retry.
> > > > >>
> > > > >> Fixes an issue where the timestamp adjustment would be re-done over
> > > > >> and over again when recovery by muxing the same AVPacket again is
> > > > >> attempted. Would become visible if the fifo muxer's time base and
> > > > >> the output muxer's time base do not match (by the value either
> > > > >> becoming smaller and smaller, or larger and larger).
> > > > >>
> > > > >> Signed-off-by: Jan Ekström <jan.ekstrom at 24i.com>
> > > > >
> > > > > Ping.
> > > > >
> > > > > Unless someone finds some disgruntling points in this patch, I will
> > > > > soon move towards applying this.
> > > > >
> > > > > My initial plan was to make a simplified v2 where the output AVPacket
> > > > > was on stack limited to the fifo_thread_write_packet context, but
> > > > > apparently gradual removal of stack usage of AVPackets is being
> > > > > planned, so I decided to not to.
> > > > >
> > > > > Best regards,
> > > > > Jan
> > > >
> > > > Can't you just record (in FifoMessage) whether the timestamps have
> > > > already been converted to the desired timebase?
> > >
> > > Back when I found this issue in this function that aligns the time
> > > bases and writes the packet on the underlying avformat context, I did
> > > not think of that, but I did think of reversing the time base scaling
> > > in case of failure. That I then opted not to do due to possibly being
> > > inaccurate unless I saved all of those values from the AVPacket that
> > > av_packet_rescale_ts touches. Thus, I settled on just utilizing a
> > > reference/copy for the actual muxing, so that it could be easily
> > > discarded and the original AVPacket's values were not lost.
> > >
> > > > (Or why not just copy
> > > > the timebase chosen by the internal muxer to the user-visible stream so
> > > > that we don't even have to convert it? This is how muxers always operate.)
> > >
> > > This one sounds more like the correct way in the end, if possible. My
> > > attempt for now was to just fix an issue within the current logic
> > > (time base handling in case of failure was forgotten). I am trying to
> > > remind myself at which point the AVStreams should no longer be
> > > poked/modified as far as time base is concerned... init or header?
> > > init seems to call init_pts in libavformat/mux.c, so my initial
> > > guesstimate is that where fifo.c is currently doing its things
> > > (fifo_mux_init), you could just add the time base sync. Most API
> > > clients tend to call only avformat_write_header so in that sense with
> > > various API clients you probably wouldn't even notice the difference
> > > :) .
> >
> > So I just double-checked the docs in avformat.h, and the point of
> > update given to the API user is the header writing, which in the fifo
> > muxer is being done asynchronously in its own thread. We would have to
> > basically make fifo_write_header wait until the first time
> > avformat_write_header gets successfully called in
> > fifo_thread_write_header.
> >
> > While possible - and could make the time base juggling unnecessary -
> > not sure if it would be my first attempt. :) I'd probably first want
> > to get the time base logic fixed since that would be limited to
> > fifo_thread_write_packet.
> >
> > If there were question marks on why I utilized a separate reference
> > AVPacket, it was mostly to keep the time related values original in
> > the fed AVPacket, thus backing up all of the time values that
> > av_packet_rescale_ts might touch (if new time fields were to be added
> > to AVPacket, and modified by av_packet_rescale_ts, then the code would
> > have to be updated if I had just backed up the values manually and
> > then passed them back). In addition to possibly causing a new buffer
> > to be allocated in case of a non-refcounted buffer (although the fifo
> > muxer does do an av_packet_ref for all input packets, so everything
> > fed in through the worker thread should be reference counted), It does
> > cause the side data etc to be copied as well, which is of course
> > unfortunate. In the testing that was done this did not seem to cause
> > major issues, though.
> >
> > If someone feels heavily about this, I can of course write
> > ff_packet_copy_ts(AVPacket *dst, const AVPacket *src), which could
> > then be utilized against the dummy AVPacket.
> >
>
> Ping on this case.
>
> 1. Currently since fifo muxer seems to already generate a refcounted
> AVPacket when taking in an AVPacket, doing another av_packet_ref is
> indeed overkill, but does not seem to be a too big of an overhead
> (esp. since done asynchronously), and makes sure all of the relevant
> timestamp fields are backed up (since they were actually not touched
> at all in the original reference). Additionally, there is no need to
> specifically "reset" or "clean up" the original reference. But the
> timestamp copying logic would also be valid, if someone feels like we
> should have such a helper (to make sure that when new timestamp
> entries such as time base appear, they wouldn't have to be remembered
> by each API user).
> 2. Header writing is done in the async part of the code, so making
> sure we actually succeed at it before letting the API client get away
> from avformat_write_header means quite a bit of logic would have to be
> rewritten.
>
> I'd just like to get this bug fixed, since I've actually hit it with
> HTTP, where a timeout was hit on the receiving side, and thus when the
> time case for the next packet to be written, the timestamps would be
> going into the future in case of the actual underlying muxer having a
> larger time base :) .

As there has been no responses, and this - while being a bit overkill
(but this is also the only official way of backing up the timestamps
of an AVPacket) - does fix an actual bug that I have come up and hit,
I will be pushing this later today unless there are further comments.

Jan