diff mbox series

[FFmpeg-devel,06/10] avformat/mux: Don't modify packets we don't own

Message ID 20200331123745.6461-7-andreas.rheinhardt@gmail.com
State Superseded
Headers show
Series libavformat/mux patches
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

Andreas Rheinhardt March 31, 2020, 12:37 p.m. UTC
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 av_bsf_send_packet.
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,
an empty 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.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
Whether a commit like this is necessary depends upon the meaning of
"ownership" (whether not having ownership disallows one from modifying a
packet) and of the user's expectations (namely if the av_write_frame()
should attempt to not modify the user-provided packet even if it were
allowed to). Furthermore, this commit has the downside of adding an
AVPacket on the stack.

 libavformat/mux.c | 34 ++++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)

Comments

Marton Balint March 31, 2020, 10:07 p.m. UTC | #1
On Tue, 31 Mar 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 av_bsf_send_packet.
> 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,
> an empty 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.
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
> Whether a commit like this is necessary depends upon the meaning of
> "ownership" (whether not having ownership disallows one from modifying a
> packet) and of the user's expectations (namely if the av_write_frame()
> should attempt to not modify the user-provided packet even if it were
> allowed to). Furthermore, this commit has the downside of adding an
> AVPacket on the stack.
>
> libavformat/mux.c | 34 ++++++++++++++++++++++++++++------
> 1 file changed, 28 insertions(+), 6 deletions(-)
>
> diff --git a/libavformat/mux.c b/libavformat/mux.c
> index 79731d3008..be7652c97b 100644
> --- a/libavformat/mux.c
> +++ b/libavformat/mux.c
> @@ -880,11 +880,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);
> @@ -895,22 +896,43 @@ int av_write_frame(AVFormatContext *s, AVPacket *pkt)
>         return 1;
>     }
> 
> +    /* 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->data = in->data;
> +    pkt->size = in->size;
> +    if (in->buf) {
> +        pkt->buf = av_buffer_ref(in->buf);
> +        if (!pkt->buf)
> +            return AVERROR(ENOMEM);
> +    } else {
> +        pkt->buf = NULL;
> +    }
> +    ret = av_packet_copy_props(pkt, in);
> +    if (ret < 0)
> +        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:
> +    av_packet_unref(pkt);
> +    return ret;
> }
>

LGTM, thanks.

Marton
diff mbox series

Patch

diff --git a/libavformat/mux.c b/libavformat/mux.c
index 79731d3008..be7652c97b 100644
--- a/libavformat/mux.c
+++ b/libavformat/mux.c
@@ -880,11 +880,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);
@@ -895,22 +896,43 @@  int av_write_frame(AVFormatContext *s, AVPacket *pkt)
         return 1;
     }
 
+    /* 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->data = in->data;
+    pkt->size = in->size;
+    if (in->buf) {
+        pkt->buf = av_buffer_ref(in->buf);
+        if (!pkt->buf)
+            return AVERROR(ENOMEM);
+    } else {
+        pkt->buf = NULL;
+    }
+    ret = av_packet_copy_props(pkt, in);
+    if (ret < 0)
+        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:
+    av_packet_unref(pkt);
+    return ret;
 }
 
 #define CHUNK_START 0x1000