diff mbox series

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

Message ID 20201208125433.40590-1-jeebjp@gmail.com
State New
Headers show
Series [FFmpeg-devel] avformat/fifo: utilize a clone packet for muxing
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Jan Ekström Dec. 8, 2020, 12:54 p.m. UTC
From: Jan Ekström <jan.ekstrom@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@24i.com>
---
 libavformat/fifo.c | 39 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)

Comments

Jan Ekström Jan. 11, 2021, 2:44 p.m. UTC | #1
On Tue, Dec 8, 2020 at 2:54 PM Jan Ekström <jeebjp@gmail.com> wrote:
>
> From: Jan Ekström <jan.ekstrom@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@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
Andreas Rheinhardt Jan. 11, 2021, 8:52 p.m. UTC | #2
Jan Ekström:
> On Tue, Dec 8, 2020 at 2:54 PM Jan Ekström <jeebjp@gmail.com> wrote:
>>
>> From: Jan Ekström <jan.ekstrom@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@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? (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.)

- Andreas
Jan Ekström Jan. 12, 2021, 7:59 a.m. UTC | #3
On Mon, Jan 11, 2021 at 10:53 PM Andreas Rheinhardt
<andreas.rheinhardt@gmail.com> wrote:
>
> Jan Ekström:
> > On Tue, Dec 8, 2020 at 2:54 PM Jan Ekström <jeebjp@gmail.com> wrote:
> >>
> >> From: Jan Ekström <jan.ekstrom@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@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
Jan Ekström Jan. 14, 2021, 12:13 a.m. UTC | #4
On Tue, Jan 12, 2021 at 9:59 AM Jan Ekström <jeebjp@gmail.com> wrote:
>
> On Mon, Jan 11, 2021 at 10:53 PM Andreas Rheinhardt
> <andreas.rheinhardt@gmail.com> wrote:
> >
> > Jan Ekström:
> > > On Tue, Dec 8, 2020 at 2:54 PM Jan Ekström <jeebjp@gmail.com> wrote:
> > >>
> > >> From: Jan Ekström <jan.ekstrom@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@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.

Jan


Jan
Jan Ekström Jan. 25, 2021, 7:47 a.m. UTC | #5
On Thu, Jan 14, 2021 at 2:13 AM Jan Ekström <jeebjp@gmail.com> wrote:
>
> On Tue, Jan 12, 2021 at 9:59 AM Jan Ekström <jeebjp@gmail.com> wrote:
> >
> > On Mon, Jan 11, 2021 at 10:53 PM Andreas Rheinhardt
> > <andreas.rheinhardt@gmail.com> wrote:
> > >
> > > Jan Ekström:
> > > > On Tue, Dec 8, 2020 at 2:54 PM Jan Ekström <jeebjp@gmail.com> wrote:
> > > >>
> > > >> From: Jan Ekström <jan.ekstrom@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@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 :) .

Jan
Jan Ekström Feb. 3, 2021, 7:38 a.m. UTC | #6
On Mon, Jan 25, 2021 at 9:47 AM Jan Ekström <jeebjp@gmail.com> wrote:
>
> On Thu, Jan 14, 2021 at 2:13 AM Jan Ekström <jeebjp@gmail.com> wrote:
> >
> > On Tue, Jan 12, 2021 at 9:59 AM Jan Ekström <jeebjp@gmail.com> wrote:
> > >
> > > On Mon, Jan 11, 2021 at 10:53 PM Andreas Rheinhardt
> > > <andreas.rheinhardt@gmail.com> wrote:
> > > >
> > > > Jan Ekström:
> > > > > On Tue, Dec 8, 2020 at 2:54 PM Jan Ekström <jeebjp@gmail.com> wrote:
> > > > >>
> > > > >> From: Jan Ekström <jan.ekstrom@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@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
Andreas Rheinhardt Feb. 3, 2021, 8:17 a.m. UTC | #7
Jan Ekström:
> On Mon, Jan 25, 2021 at 9:47 AM Jan Ekström <jeebjp@gmail.com> wrote:
>>
>> On Thu, Jan 14, 2021 at 2:13 AM Jan Ekström <jeebjp@gmail.com> wrote:
>>>
>>> On Tue, Jan 12, 2021 at 9:59 AM Jan Ekström <jeebjp@gmail.com> wrote:
>>>>
>>>> On Mon, Jan 11, 2021 at 10:53 PM Andreas Rheinhardt
>>>> <andreas.rheinhardt@gmail.com> wrote:
>>>>>
>>>>> Jan Ekström:
>>>>>> On Tue, Dec 8, 2020 at 2:54 PM Jan Ekström <jeebjp@gmail.com> wrote:
>>>>>>>
>>>>>>> From: Jan Ekström <jan.ekstrom@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@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.
> 
1. It can't be relied upon that the underlying muxer already sets the
desired timebase during init. So making the user deliver the packets in
the correct timebase is not always possible.
2. The fifo-muxer does something weird: Typically, one allocates an
AVFormatContext, initializes it, calls the init/write header function,
writes packets, writes the trailer and frees it. Yet in case of errors
the FIFO muxer writes the trailer and then writes the header again,
using the same AVFormatContext. This is not in accordance with the
documentation in avformat.h. av_write_trailer actually has some freeing
code that duplicates stuff from avformat_free_context. I always wondered
why it is there, but it seems to be there for exactly this purpose.
Nevertheless this is very fragile.
3. I really don't know why you opt for the overkill solution that adds
another error path to be checked.
4. There is another bug: If the trailer is never written, the
FIFO-thread never gets deleted. Yet av_thread_message_queue_free gets
called.

- Andreas
diff mbox series

Patch

diff --git a/libavformat/fifo.c b/libavformat/fifo.c
index 17748e94ce..aa8bea6d5a 100644
--- a/libavformat/fifo.c
+++ b/libavformat/fifo.c
@@ -43,6 +43,8 @@  typedef struct FifoContext {
     int queue_size;
     AVThreadMessageQueue *queue;
 
+    AVPacket *out_pkt;
+
     pthread_t writer_thread;
 
     /* Return value of last write_trailer_call */
@@ -181,6 +183,7 @@  static int fifo_thread_write_packet(FifoThreadContext *ctx, AVPacket *pkt)
     AVFormatContext *avf = ctx->avf;
     FifoContext *fifo = avf->priv_data;
     AVFormatContext *avf2 = fifo->avf;
+    AVPacket *out_pkt = fifo->out_pkt;
     AVRational src_tb, dst_tb;
     int ret, s_idx;
 
@@ -198,14 +201,34 @@  static int fifo_thread_write_packet(FifoThreadContext *ctx, AVPacket *pkt)
         }
     }
 
-    s_idx = pkt->stream_index;
+    // We will be muxing a packet, so clone it by utilizing references.
+    // This way we do not have to undo any of the tweaking for timestamps etc
+    // that we are doing in this function in case another attempt through
+    // recovery is required.
+    if ((ret = av_packet_ref(out_pkt, pkt)) < 0) {
+        av_log(avf, AV_LOG_ERROR,
+               "Error creating a new reference for output packet (%s)!\n",
+               av_err2str(ret));
+        return ret;
+    }
+
+    s_idx = out_pkt->stream_index;
     src_tb = avf->streams[s_idx]->time_base;
     dst_tb = avf2->streams[s_idx]->time_base;
-    av_packet_rescale_ts(pkt, src_tb, dst_tb);
 
-    ret = av_write_frame(avf2, pkt);
-    if (ret >= 0)
-        av_packet_unref(pkt);
+    av_packet_rescale_ts(out_pkt, src_tb, dst_tb);
+
+    ret = av_write_frame(avf2, out_pkt);
+
+    // Always clear the output packet, as we have no more use for it.
+    av_packet_unref(out_pkt);
+
+    if (ret < 0)
+        return ret;
+
+    // We hit success, unref the actual source packet.
+    av_packet_unref(pkt);
+
     return ret;
 }
 
@@ -525,6 +548,11 @@  static int fifo_init(AVFormatContext *avf)
         return ret;
     }
 
+    if (!(fifo->out_pkt = av_packet_alloc())) {
+        av_log(avf, AV_LOG_ERROR, "Failed to allocate output packet!\n");
+        return AVERROR(ENOMEM);
+    }
+
     ret = fifo_mux_init(avf, oformat, avf->url);
     if (ret < 0)
         return ret;
@@ -650,6 +678,7 @@  static void fifo_deinit(AVFormatContext *avf)
     av_thread_message_queue_free(&fifo->queue);
     if (fifo->overflow_flag_lock_initialized)
         pthread_mutex_destroy(&fifo->overflow_flag_lock);
+    av_packet_free(&fifo->out_pkt);
 }
 
 #define OFFSET(x) offsetof(FifoContext, x)