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

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@gmail.com>
---
 libavformat/movenc.c | 40 ++++++++++++++++++++++++++++------------
 1 file changed, 28 insertions(+), 12 deletions(-)

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@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;
>          }
>      }
>  
>

On 23/06/2020 16:59, Andreas Rheinhardt wrote:
> 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).

OK, will do and send a v2.

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

I guess my question would be: Why do we leave the public AVIO API with this
function we try to even avoid internally?

- Derek