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

[Jan Ekström]

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
:) .

Jan