[FFmpeg-devel,6/6] avformat/av1: Improve filtering AV1 OBUs

Both ISOBMFF as well as Matroska require certain OBUs to be stripped
before muxing them. There are two functions for this purpose; one writes
directly into an AVIOContext, the other returns a freshly allocated
buffer with the undesired units stripped away.

The latter one actually relies on the former by means of a dynamic
buffer. This has several drawbacks: The underlying buffer might have to
be reallocated multiple times; the buffer will eventually be
overallocated; the data will not be directly copied into the final
buffer, but rather first in the write buffer (in chunks of 1024 byte)
and then written in these chunks. Moreover, the API for dynamic buffers
is defective wrt error checking and as a consequence, the earlier code
would indicate a length of -AV_INPUT_BUFFER_PADDING_SIZE on allocation
failure, but it would not return an error; there would also be no error
in case the arbitrary limit of INT_MAX/2 that is currently imposed on
dynamic buffers is hit.

This commit changes this: The buffer is now parsed twice, once to get
the precise length which will then be allocated; and once to actually
write the data.

For a 22.7mb/s file with average framesize 113 kB this improved the time
for the calls to ff_av1_filter_obus_buf() when writing Matroska from
753662 decicycles to 313319 decicycles (based upon 50 runs a 2048 frames
each); for another 1.5mb/s file (with average framesize of 7.3 kB) it
improved from 79270 decicycles to 34539 decicycles (based upon 50 runs a
4096 frames).

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavformat/av1.c | 32 +++++++++++++++++++++-----------
 libavformat/av1.h |  4 ++--
 2 files changed, 23 insertions(+), 13 deletions(-)

On 1/23/2020 1:08 PM, Andreas Rheinhardt wrote:
> Both ISOBMFF as well as Matroska require certain OBUs to be stripped
> before muxing them. There are two functions for this purpose; one writes
> directly into an AVIOContext, the other returns a freshly allocated
> buffer with the undesired units stripped away.
> 
> The latter one actually relies on the former by means of a dynamic
> buffer. This has several drawbacks: The underlying buffer might have to
> be reallocated multiple times; the buffer will eventually be
> overallocated; the data will not be directly copied into the final
> buffer, but rather first in the write buffer (in chunks of 1024 byte)
> and then written in these chunks. Moreover, the API for dynamic buffers
> is defective wrt error checking and as a consequence, the earlier code
> would indicate a length of -AV_INPUT_BUFFER_PADDING_SIZE on allocation
> failure, but it would not return an error; there would also be no error
> in case the arbitrary limit of INT_MAX/2 that is currently imposed on
> dynamic buffers is hit.
> 
> This commit changes this: The buffer is now parsed twice, once to get
> the precise length which will then be allocated; and once to actually
> write the data.
> 
> For a 22.7mb/s file with average framesize 113 kB this improved the time
> for the calls to ff_av1_filter_obus_buf() when writing Matroska from
> 753662 decicycles to 313319 decicycles (based upon 50 runs a 2048 frames
> each); for another 1.5mb/s file (with average framesize of 7.3 kB) it
> improved from 79270 decicycles to 34539 decicycles (based upon 50 runs a
> 4096 frames).
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavformat/av1.c | 32 +++++++++++++++++++++-----------
>  libavformat/av1.h |  4 ++--
>  2 files changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/libavformat/av1.c b/libavformat/av1.c
> index 4ff4bffddf..80c049f62f 100644
> --- a/libavformat/av1.c
> +++ b/libavformat/av1.c
> @@ -19,6 +19,7 @@
>   * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>   */
>  
> +#include "libavutil/avassert.h"
>  #include "libavutil/mem.h"
>  #include "libavcodec/av1.h"
>  #include "libavcodec/av1_parse.h"
> @@ -48,7 +49,8 @@ int ff_av1_filter_obus(AVIOContext *pb, const uint8_t *buf, int size)
>          case AV1_OBU_PADDING:
>              break;
>          default:
> -            avio_write(pb, buf, len);
> +            if (pb)
> +                avio_write(pb, buf, len);
>              size += len;
>              break;
>          }
> @@ -58,23 +60,31 @@ int ff_av1_filter_obus(AVIOContext *pb, const uint8_t *buf, int size)
>      return size;
>  }
>  
> -int ff_av1_filter_obus_buf(const uint8_t *buf, uint8_t **out, int *size)
> +int ff_av1_filter_obus_buf(const uint8_t *in, uint8_t **out, int *size)
>  {
> -    AVIOContext *pb;
> -    int ret;
> +    AVIOContext pb;
> +    uint8_t *buf;
> +    int len, ret;
>  
> -    ret = avio_open_dyn_buf(&pb);
> -    if (ret < 0)
> -        return ret;
> -
> -    ret = ff_av1_filter_obus(pb, buf, *size);
> +    len = ret = ff_av1_filter_obus(NULL, in, *size);
>      if (ret < 0) {
> -        ffio_free_dyn_buf(&pb);
>          return ret;
>      }
>  
> +    buf = av_malloc((size_t)len + AV_INPUT_BUFFER_PADDING_SIZE);

We never cast the arguments to av_malloc() to size_t, and len here is
guaranteed to be <= size by parse_obu_header(), so it's unnecessary.

Removed it and pushed the patch. Thanks.

> +    if (!buf)
> +        return AVERROR(ENOMEM);
> +
> +    ffio_init_context(&pb, buf, len, 1, NULL, NULL, NULL, NULL);
> +
> +    ret = ff_av1_filter_obus(&pb, in, *size);
> +    av_assert1(ret == len);
> +
> +    memset(buf + len, 0, AV_INPUT_BUFFER_PADDING_SIZE);
> +
>      av_freep(out);
> -    *size = avio_close_dyn_buf(pb, out);
> +    *out  = buf;
> +    *size = len;
>  
>      return 0;
>  }
> diff --git a/libavformat/av1.h b/libavformat/av1.h
> index 0578435376..acba12612c 100644
> --- a/libavformat/av1.h
> +++ b/libavformat/av1.h
> @@ -61,7 +61,7 @@ int ff_av1_filter_obus(AVIOContext *pb, const uint8_t *buf, int size);
>   *
>   * @param pb pointer to the AVIOContext where the filtered bitstream shall be
>   *           written
> - * @param buf input data buffer
> + * @param in input data buffer
>   * @param out pointer to pointer that will hold the allocated data buffer
>   * @param size size of the input data buffer. The size of the resulting output
>                 data buffer will be written here
> @@ -69,7 +69,7 @@ int ff_av1_filter_obus(AVIOContext *pb, const uint8_t *buf, int size);
>   * @return 0 in case of success, a negative AVERROR code in case of failure.
>   *         On failure, out and size are unchanged
>   */
> -int ff_av1_filter_obus_buf(const uint8_t *buf, uint8_t **out, int *size);
> +int ff_av1_filter_obus_buf(const uint8_t *in, uint8_t **out, int *size);
>  
>  /**
>   * Parses a Sequence Header from the the provided buffer.
>

On Sun, Jan 26, 2020 at 4:54 PM James Almer <jamrial@gmail.com> wrote:

> On 1/23/2020 1:08 PM, Andreas Rheinhardt wrote:
> >
> > +    buf = av_malloc((size_t)len + AV_INPUT_BUFFER_PADDING_SIZE);
>
> We never cast the arguments to av_malloc() to size_t, and len here is
> guaranteed to be <= size by parse_obu_header(), so it's unnecessary.
>
> Removed it and pushed the patch. Thanks.
>

The input data currently always comes from a packet, so that its size is <=
INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE, but there is no requirement that
this always has to be that way. So I added the cast in order to make sure
that no overflow happens during the addition. But I guess it's not
important.

- Andreas

PS: None of the current callers needs the output to be padded, but I
nevertheless retained it.