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

[Marton Balint]

On Thu, 16 Apr 2020, Andreas Rheinhardt wrote:

> 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.

Wow, that really confused me :)

Then write something like: uncoded frames are *also* freed here.

Thanks,
Marton