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

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

Otherwise looks good. I checked the other patches in the series they all 
look good as well.

Thanks,
Marton

> +    av_packet_unref(pkt);
> +    return ret;
> }
> 
> #define CHUNK_START 0x1000
> @@ -1319,7 +1347,6 @@ static int write_uncoded_frame_internal(AVFormatContext *s, int stream_index,
>                                         AVFrame *frame, int interleaved)
> {
>     AVPacket pkt, *pktp;
> -    int ret;
>
>     av_assert0(s->oformat);
>     if (!s->oformat->write_uncoded_frame) {
> @@ -1349,11 +1376,8 @@ static int write_uncoded_frame_internal(AVFormatContext *s, int stream_index,
>         pkt.flags |= AV_PKT_FLAG_UNCODED_FRAME;
>     }
> 
> -    ret = interleaved ? av_interleaved_write_frame(s, pktp) :
> -                        av_write_frame(s, pktp);
> -    if (pktp)
> -        av_packet_unref(pktp);
> -    return ret;
> +    return interleaved ? av_interleaved_write_frame(s, pktp) :
> +                         av_write_frame(s, pktp);
> }
> 
> int av_write_uncoded_frame(AVFormatContext *s, int stream_index,
> -- 
> 2.20.1
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".