diff mbox series

[FFmpeg-devel,6/8] avformat/mux: do not destroy packets of av_write_frame on bitstream filtering

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
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Marton Balint March 28, 2020, 6:15 p.m. UTC
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(-)

Comments

Andreas Rheinhardt March 28, 2020, 8:56 p.m. UTC | #1
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
Marton Balint March 28, 2020, 11:08 p.m. UTC | #2
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 mbox series

Patch

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;