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

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(-)

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