On Mon, Jan 25, 2021 at 9:47 AM Jan Ekström <jee...@gmail.com> wrote: > > On Thu, Jan 14, 2021 at 2:13 AM Jan Ekström <jee...@gmail.com> wrote: > > > > On Tue, Jan 12, 2021 at 9:59 AM Jan Ekström <jee...@gmail.com> wrote: > > > > > > On Mon, Jan 11, 2021 at 10:53 PM Andreas Rheinhardt > > > <andreas.rheinha...@gmail.com> wrote: > > > > > > > > Jan Ekström: > > > > > On Tue, Dec 8, 2020 at 2:54 PM Jan Ekström <jee...@gmail.com> wrote: > > > > >> > > > > >> From: Jan Ekström <jan.ekst...@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.ekst...@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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
