[FFmpeg-devel] [PATCH 5/6] avformat/mux: Don't modify packets we don't own

[Andreas Rheinhardt]

Marton Balint:
> 
> 
> On Sat, 11 Apr 2020, Andreas Rheinhardt wrote:
> 
>> The documentation of av_write_frame() explicitly states that the function
>> doesn't take ownership of the packets sent to it; while av_write_frame()
>> does not directly unreference the packets after having written them, it
>> nevertheless modifies the packet in various ways:
>> 1. The timestamps might be modified either by prepare_input_packet() or
>> compute_muxer_pkt_fields().
>> 2. If a bitstream filter gets applied, it takes ownership of the
>> reference and the side-data in the packet sent to it.
>> In case of do_packet_auto_bsf(), the end result is that the returned
>> packet
>> contains the output of the last bsf in the chain. If an error happens,
>> a blank packet will be returned; a packet may also simply not lead to
>> any output (vp9_superframe).
>> This also implies that side data needs to be really copied and can't be
>> shared with the input packet.
>> The method choosen here minimizes copying of data: When the input isn't
>> refcounted and no bitstream filter is applied, the packet's data will
>> not be copied.
>>
>> Notice that packets that contain uncoded frames are exempt from this
>> because these packets are not owned by and returned to the user. This
>> also moves unreferencing the packets containing uncoded frames to
>> av_write_frame() in the noninterleaved codepath; in the interleaved
>> codepath, these packets are already freed in
>> av_interleaved_write_frame(),
>> so that unreferencing the packets in write_uncoded_frame_internal() is
>> no longer needed. It has been removed.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
>> ---
>> I was actually surprised when I realized how freeing uncoded frames in
>> the noninterleaved codepath could be left to av_write_frame().
>>
>> libavformat/mux.c | 48 +++++++++++++++++++++++++++++++++++------------
>> 1 file changed, 36 insertions(+), 12 deletions(-)
>>
>> diff --git a/libavformat/mux.c b/libavformat/mux.c
>> index cae9f42d11..48c0d4cd5f 100644
>> --- a/libavformat/mux.c
>> +++ b/libavformat/mux.c
>> @@ -874,11 +874,12 @@ static int do_packet_auto_bsf(AVFormatContext
>> *s, AVPacket *pkt) {
>>     return 1;
>> }
>>
>> -int av_write_frame(AVFormatContext *s, AVPacket *pkt)
>> +int av_write_frame(AVFormatContext *s, AVPacket *in)
>> {
>> +    AVPacket local_pkt, *pkt = &local_pkt;
>>     int ret;
>>
>> -    if (!pkt) {
>> +    if (!in) {
>>         if (s->oformat->flags & AVFMT_ALLOW_FLUSH) {
>>             ret = s->oformat->write_packet(s, NULL);
>>             flush_if_needed(s);
>> @@ -889,22 +890,49 @@ int av_write_frame(AVFormatContext *s, AVPacket
>> *pkt)
>>         return 1;
>>     }
>>
>> +    if (in->flags & AV_PKT_FLAG_UNCODED_FRAME) {
>> +        pkt = in;
>> +    } else {
>> +        /* We don't own in, so we have to make sure not to modify it.
>> +         * The following avoids copying in's data unnecessarily.
>> +         * Copying side data is unavoidable as a bitstream filter
>> +         * may change it, e.g. free it on errors. */
>> +        pkt->buf  = NULL;
>> +        pkt->data = in->data;
>> +        pkt->size = in->size;
>> +        ret = av_packet_copy_props(pkt, in);
>> +        if (ret < 0)
>> +            return ret;
>> +        if (in->buf) {
>> +            pkt->buf = av_buffer_ref(in->buf);
>> +            if (!pkt->buf) {
>> +                ret = AVERROR(ENOMEM);
>> +                goto fail;
>> +            }
>> +        }
>> +    }
>> +
>>     ret = prepare_input_packet(s, pkt);
>>     if (ret < 0)
>> -        return ret;
>> +        goto fail;
>>
>>     ret = do_packet_auto_bsf(s, pkt);
>>     if (ret <= 0)
>> -        return ret;
>> +        goto fail;
>>
>> #if FF_API_COMPUTE_PKT_FIELDS2 && FF_API_LAVF_AVCTX
>>     ret = compute_muxer_pkt_fields(s, s->streams[pkt->stream_index],
>> pkt);
>>
>>     if (ret < 0 && !(s->oformat->flags & AVFMT_NOTIMESTAMPS))
>> -        return ret;
>> +        goto fail;
>> #endif
>>
>> -    return write_packet(s, pkt);
>> +    ret = write_packet(s, pkt);
>> +
>> +fail:
>> +    // Uncoded frames using the noninterleaved codepath are freed here
> 
> This comment does not seem accurate. Pkt must always be unrefed here,
> not only for the uncoded_frames (where it happens to be == in), or I
> miss something? If not, then I'd just simply remove this comment.

Of course ordinary packets need to be unreferenced here, too; but I
thought that someone reading and potentially changing av_write_frame()
is already aware of that. But they might not be aware of the fact that
(contrary to the usual behaviour of av_write_frame()) some packets not
created in av_write_frame() are unreferenced there, hence this comment.

- Andreas