Message ID | 20200328181515.5333-6-cus@passwd.hu |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel,1/8] fftools/ffmpeg: also flush encoders which have a variable frame size | expand |
Context | Check | Description |
---|---|---|
andriy/ffmpeg-patchwork | success | Make fate finished |
Marton Balint: > av_write_frame() does not take ownership of the packet. > > Signed-off-by: Marton Balint <cus@passwd.hu> > --- > libavformat/mux.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/libavformat/mux.c b/libavformat/mux.c > index 3054ab8644..706fdcbbf4 100644 > --- a/libavformat/mux.c > +++ b/libavformat/mux.c > @@ -935,7 +935,16 @@ static int write_packets_common(AVFormatContext *s, AVStream *st, AVPacket *pkt, > if (ret < 0) { > if (ret == AVERROR(EAGAIN) && !consumed_packet) { > av_assert2(sti->bsfcs_idx == 0); > - ret = av_bsf_send_packet(sti->bsfcs[0], pkt); > + if (!interleaved && pkt) { > + AVPacket tmp; > + ret = av_packet_ref(&tmp, pkt); > + if (ret < 0) > + goto fail; > + ret = av_bsf_send_packet(sti->bsfcs[0], &tmp); > + av_packet_unref(&tmp); > + } else { > + ret = av_bsf_send_packet(sti->bsfcs[0], pkt); > + } > av_assert2(ret != AVERROR(EAGAIN)); > if (ret >= 0) { > consumed_packet = 1; > When I proposed something similar in [1] (based upon exactly the same thinking as you with your patch), I was told that the owner of a packet just has the obligation to free it; and not the right to expect others not to modify it. I changed my mind on this: Given that av_write_frame() does not take a const AVPacket * as parameter, the caller has no right to believe that the packets are returned untouched. Furthermore, you are using an AVPacket on the stack, yet I thought that this should be avoided, because sizeof(AVPacket) should eventually no longer be part of the public API. - Andreas [1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2019-August/248153.html
On Sat, 28 Mar 2020, Andreas Rheinhardt wrote: > Marton Balint: >> av_write_frame() does not take ownership of the packet. >> >> Signed-off-by: Marton Balint <cus@passwd.hu> >> --- >> libavformat/mux.c | 11 ++++++++++- >> 1 file changed, 10 insertions(+), 1 deletion(-) >> >> diff --git a/libavformat/mux.c b/libavformat/mux.c >> index 3054ab8644..706fdcbbf4 100644 >> --- a/libavformat/mux.c >> +++ b/libavformat/mux.c >> @@ -935,7 +935,16 @@ static int write_packets_common(AVFormatContext *s, AVStream *st, AVPacket *pkt, >> if (ret < 0) { >> if (ret == AVERROR(EAGAIN) && !consumed_packet) { >> av_assert2(sti->bsfcs_idx == 0); >> - ret = av_bsf_send_packet(sti->bsfcs[0], pkt); >> + if (!interleaved && pkt) { >> + AVPacket tmp; >> + ret = av_packet_ref(&tmp, pkt); >> + if (ret < 0) >> + goto fail; >> + ret = av_bsf_send_packet(sti->bsfcs[0], &tmp); >> + av_packet_unref(&tmp); >> + } else { >> + ret = av_bsf_send_packet(sti->bsfcs[0], pkt); >> + } >> av_assert2(ret != AVERROR(EAGAIN)); >> if (ret >= 0) { >> consumed_packet = 1; >> > When I proposed something similar in [1] (based upon exactly the same > thinking as you with your patch), I was told that the owner of a packet > just has the obligation to free it; and not the right to expect others > not to modify it. pts/dts/duration is one thing. But if the user must free the data (or the buffer) then the function must NOT. Is it a valid expectation from the user to have the data available after the av_write_frame() call? I think so. > I changed my mind on this: Given that av_write_frame() > does not take a const AVPacket * as parameter, the caller has no right > to believe that the packets are returned untouched. IMHO we should make reasonable effort to not change the behaviour of the API, even if it is not explictly documented. Freeing data/buf passed to av_write_frame() seems like a breaking change to me. If you overwrite pkt->data to NULL in av_write_frame then fate-fifo-muxer-tst will fail. > > Furthermore, you are using an AVPacket on the stack, yet I thought that > this should be avoided, because sizeof(AVPacket) should eventually no > longer be part of the public API. Is there still interest to do this? We have tons of static AVPacket allocations all over the codebase because it is simple. Even in new code. I don't think the added complexity actually worth removing it. Regards, Marton
diff --git a/libavformat/mux.c b/libavformat/mux.c index 3054ab8644..706fdcbbf4 100644 --- a/libavformat/mux.c +++ b/libavformat/mux.c @@ -935,7 +935,16 @@ static int write_packets_common(AVFormatContext *s, AVStream *st, AVPacket *pkt, if (ret < 0) { if (ret == AVERROR(EAGAIN) && !consumed_packet) { av_assert2(sti->bsfcs_idx == 0); - ret = av_bsf_send_packet(sti->bsfcs[0], pkt); + if (!interleaved && pkt) { + AVPacket tmp; + ret = av_packet_ref(&tmp, pkt); + if (ret < 0) + goto fail; + ret = av_bsf_send_packet(sti->bsfcs[0], &tmp); + av_packet_unref(&tmp); + } else { + ret = av_bsf_send_packet(sti->bsfcs[0], pkt); + } av_assert2(ret != AVERROR(EAGAIN)); if (ret >= 0) { consumed_packet = 1;
av_write_frame() does not take ownership of the packet. Signed-off-by: Marton Balint <cus@passwd.hu> --- libavformat/mux.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)