[FFmpeg-devel] [PATCH] lavf/movenc: Use a dynamic buffer when writing the mfra box

[Andreas Rheinhardt]

Derek Buitenhuis:
> When doing streamed output, with e.g. +dash, if the mfra box ended
> up being larger than the AVIOContext write buffer, the (unchecked)
> seeking back to update the box size would silently fail and produce
> an invalid mfra box.
> 
> This is similar to how other boxes are written in fragmented mode.
> 
> Signed-off-by: Derek Buitenhuis <derek.buitenhuis at gmail.com>
> ---
>  libavformat/movenc.c | 40 ++++++++++++++++++++++++++++------------
>  1 file changed, 28 insertions(+), 12 deletions(-)
> 
> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
> index b8e45760ee..2927865cb4 100644
> --- a/libavformat/movenc.c
> +++ b/libavformat/movenc.c
> @@ -4798,28 +4798,42 @@ static int mov_write_tfra_tag(AVIOContext *pb, MOVTrack *track)
>  
>  static int mov_write_mfra_tag(AVIOContext *pb, MOVMuxContext *mov)
>  {
> -    int64_t pos = avio_tell(pb);
> -    int i;
> +    AVIOContext *mfra_pb;
> +    int i, ret, sz;
> +    uint8_t *buf;
>  
> -    avio_wb32(pb, 0); /* size placeholder */
> -    ffio_wfourcc(pb, "mfra");
> +    ret = avio_open_dyn_buf(&mfra_pb);
> +    if (ret < 0)
> +        return ret;
> +
> +    avio_wb32(mfra_pb, 0); /* size placeholder */
> +    ffio_wfourcc(mfra_pb, "mfra");
>      /* An empty mfra atom is enough to indicate to the publishing point that
>       * the stream has ended. */
>      if (mov->flags & FF_MOV_FLAG_ISML)
> -        return update_size(pb, pos);
> +        goto done_mfra;
>  
>      for (i = 0; i < mov->nb_streams; i++) {
>          MOVTrack *track = &mov->tracks[i];
>          if (track->nb_frag_info)
> -            mov_write_tfra_tag(pb, track);
> +            mov_write_tfra_tag(mfra_pb, track);
>      }
>  
> -    avio_wb32(pb, 16);
> -    ffio_wfourcc(pb, "mfro");
> -    avio_wb32(pb, 0); /* version + flags */
> -    avio_wb32(pb, avio_tell(pb) + 4 - pos);
> +    avio_wb32(mfra_pb, 16);
> +    ffio_wfourcc(mfra_pb, "mfro");
> +    avio_wb32(mfra_pb, 0); /* version + flags */
> +    avio_wb32(mfra_pb, avio_tell(mfra_pb) + 4);
>  
> -    return update_size(pb, pos);
> +done_mfra:
> +
> +    sz  = update_size(mfra_pb, 0);
> +    ret = avio_close_dyn_buf(mfra_pb, &buf);

Use avio_get_dyn_buf in combination with ffio_free_dyn_buf. That way you
save an allocation+copy in case the data fits into the dynamic buffer's
write buffer (it has a size of 1024 bytes; I don't know how common it is
for this box to be smaller).

> +    if (ret < 0)
> +        return ret;

Furthermore, avio_close_dyn_buf doesn't even provide a way to check for
errors; e.g. it does not allow you to distinguish between truncation
errors and non-truncation errors (due to returning an int, the internal
buffer is capped to INT_MAX). avio_close_dyn_buf actually may not return
anything negative (it is documented to just return the length of the
byte buffer), yet due to a bug it returns -AV_INPUT_BUFFER_PADDING_SIZE
on allocation error.

In contrast there is a way to check for errors with avio_get_dyn_buf:
Check the AVIOContext's error flag after avio_get_dyn_buf.

> +    avio_write(pb, buf, ret);
> +    av_free(buf);
> +
> +    return sz;
>  }
>  
>  static int mov_write_mdat_tag(AVIOContext *pb, MOVMuxContext *mov)
> @@ -6987,7 +7001,9 @@ static int mov_write_trailer(AVFormatContext *s)
>          }
>          if (!(mov->flags & FF_MOV_FLAG_SKIP_TRAILER)) {
>              avio_write_marker(s->pb, AV_NOPTS_VALUE, AVIO_DATA_MARKER_TRAILER);
> -            mov_write_mfra_tag(pb, mov);
> +            res = mov_write_mfra_tag(pb, mov);
> +            if (res < 0)
> +                return res;
>          }
>      }
>  
>