Re: [FFmpeg-devel] [PATCH 2/2] avcodec/libdav1d: use a custom picture allocator

[James Almer]

On 3/6/2019 10:33 AM, Carl Eugen Hoyos wrote:
> 2019-03-06 14:22 GMT+01:00, James Almer :
>> On 3/6/2019 5:06 AM, Carl Eugen Hoyos wrote:
>>> 2019-03-04 22:06 GMT+01:00, James Almer :
>>>
 +static const enum AVPixelFormat pix_fmt[][3] = {
 +[DAV1D_PIXEL_LAYOUT_I400] = { AV_PIX_FMT_GRAY8,   AV_PIX_FMT_GRAY10,
 AV_PIX_FMT_GRAY12 },
 +[DAV1D_PIXEL_LAYOUT_I420] = { AV_PIX_FMT_YUV420P,
 AV_PIX_FMT_YUV420P10,
 AV_PIX_FMT_YUV420P12 },
 +[DAV1D_PIXEL_LAYOUT_I422] = { AV_PIX_FMT_YUV422P,
 AV_PIX_FMT_YUV422P10,
 AV_PIX_FMT_YUV422P12 },
 +[DAV1D_PIXEL_LAYOUT_I444] = { AV_PIX_FMT_YUV444P,
 AV_PIX_FMT_YUV444P10,
 AV_PIX_FMT_YUV444P12 },
 +};
>>>
>>> Looks like a separate change.
>>>
>>> And please use the ERROR defines.
>>>
>>> Carl Eugen
>>
>> You want me to move this array up in the file in a separate commit?
> 
> No, I just wanted to carefully remind you that smaller commits
> do have advantages.
> But this is clearly your code.
> 
> Sorry for the noise, Carl Eugen

No, i can do it. Just wanted to make sure you weren't mistakenly seeing
some change to the array which would definitely require a separate commit.
Changed locally.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/2] avcodec/libdav1d: use a custom picture allocator

[Carl Eugen Hoyos]

2019-03-06 14:22 GMT+01:00, James Almer :
> On 3/6/2019 5:06 AM, Carl Eugen Hoyos wrote:
>> 2019-03-04 22:06 GMT+01:00, James Almer :
>>
>>> +static const enum AVPixelFormat pix_fmt[][3] = {
>>> +[DAV1D_PIXEL_LAYOUT_I400] = { AV_PIX_FMT_GRAY8,   AV_PIX_FMT_GRAY10,
>>> AV_PIX_FMT_GRAY12 },
>>> +[DAV1D_PIXEL_LAYOUT_I420] = { AV_PIX_FMT_YUV420P,
>>> AV_PIX_FMT_YUV420P10,
>>> AV_PIX_FMT_YUV420P12 },
>>> +[DAV1D_PIXEL_LAYOUT_I422] = { AV_PIX_FMT_YUV422P,
>>> AV_PIX_FMT_YUV422P10,
>>> AV_PIX_FMT_YUV422P12 },
>>> +[DAV1D_PIXEL_LAYOUT_I444] = { AV_PIX_FMT_YUV444P,
>>> AV_PIX_FMT_YUV444P10,
>>> AV_PIX_FMT_YUV444P12 },
>>> +};
>>
>> Looks like a separate change.
>>
>> And please use the ERROR defines.
>>
>> Carl Eugen
>
> You want me to move this array up in the file in a separate commit?

No, I just wanted to carefully remind you that smaller commits
do have advantages.
But this is clearly your code.

Sorry for the noise, Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/2] avcodec/libdav1d: use a custom picture allocator

[James Almer]

On 3/6/2019 5:06 AM, Carl Eugen Hoyos wrote:
> 2019-03-04 22:06 GMT+01:00, James Almer :
> 
>> +static const enum AVPixelFormat pix_fmt[][3] = {
>> +[DAV1D_PIXEL_LAYOUT_I400] = { AV_PIX_FMT_GRAY8,   AV_PIX_FMT_GRAY10,
>> AV_PIX_FMT_GRAY12 },
>> +[DAV1D_PIXEL_LAYOUT_I420] = { AV_PIX_FMT_YUV420P, AV_PIX_FMT_YUV420P10,
>> AV_PIX_FMT_YUV420P12 },
>> +[DAV1D_PIXEL_LAYOUT_I422] = { AV_PIX_FMT_YUV422P, AV_PIX_FMT_YUV422P10,
>> AV_PIX_FMT_YUV422P12 },
>> +[DAV1D_PIXEL_LAYOUT_I444] = { AV_PIX_FMT_YUV444P, AV_PIX_FMT_YUV444P10,
>> AV_PIX_FMT_YUV444P12 },
>> +};
> 
> Looks like a separate change.
> 
> And please use the ERROR defines.
> 
> Carl Eugen

You want me to move this array up in the file in a separate commit?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/2] avcodec/libdav1d: use a custom picture allocator

[Carl Eugen Hoyos]

2019-03-04 22:06 GMT+01:00, James Almer :

> +static const enum AVPixelFormat pix_fmt[][3] = {
> +[DAV1D_PIXEL_LAYOUT_I400] = { AV_PIX_FMT_GRAY8,   AV_PIX_FMT_GRAY10,
> AV_PIX_FMT_GRAY12 },
> +[DAV1D_PIXEL_LAYOUT_I420] = { AV_PIX_FMT_YUV420P, AV_PIX_FMT_YUV420P10,
> AV_PIX_FMT_YUV420P12 },
> +[DAV1D_PIXEL_LAYOUT_I422] = { AV_PIX_FMT_YUV422P, AV_PIX_FMT_YUV422P10,
> AV_PIX_FMT_YUV422P12 },
> +[DAV1D_PIXEL_LAYOUT_I444] = { AV_PIX_FMT_YUV444P, AV_PIX_FMT_YUV444P10,
> AV_PIX_FMT_YUV444P12 },
> +};

Looks like a separate change.

And please use the ERROR defines.

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/2] avcodec/libdav1d: use a custom picture allocator

[James Almer]

On 3/5/2019 3:19 PM, Vittorio Giovara wrote:
> On Mon, Mar 4, 2019 at 4:08 PM James Almer  wrote:
> 
>> Replaces the libdav1d internal allocator. It uses an AVBufferPool to
>> reduce the
>> amount of allocated buffers.
>> About 5% speed up when decoding 720p or higher streams.
>>
>> Signed-off-by: James Almer 
>> ---
>> get_buffer2() can't be used for this decoder, as there's no guarantee the
>> buffers
>> it returns will respect the constrains specified by libdav1d.
>>
>>  libavcodec/libdav1d.c | 72 ++-
>>  1 file changed, 65 insertions(+), 7 deletions(-)
>>
>> diff --git a/libavcodec/libdav1d.c b/libavcodec/libdav1d.c
>> index 459bbae687..855c27db7f 100644
>> --- a/libavcodec/libdav1d.c
>> +++ b/libavcodec/libdav1d.c
>> @@ -22,6 +22,7 @@
>>  #include 
>>
>>  #include "libavutil/avassert.h"
>> +#include "libavutil/imgutils.h"
>>  #include "libavutil/opt.h"
>>
>>  #include "avcodec.h"
>> @@ -31,12 +32,21 @@
>>  typedef struct Libdav1dContext {
>>  AVClass *class;
>>  Dav1dContext *c;
>> +AVBufferPool *pool;
>> +int pool_size;
>>
>>  Dav1dData data;
>>  int tile_threads;
>>  int apply_grain;
>>  } Libdav1dContext;
>>
>> +static const enum AVPixelFormat pix_fmt[][3] = {
>> +[DAV1D_PIXEL_LAYOUT_I400] = { AV_PIX_FMT_GRAY8,   AV_PIX_FMT_GRAY10,
>>   AV_PIX_FMT_GRAY12 },
>> +[DAV1D_PIXEL_LAYOUT_I420] = { AV_PIX_FMT_YUV420P,
>> AV_PIX_FMT_YUV420P10, AV_PIX_FMT_YUV420P12 },
>> +[DAV1D_PIXEL_LAYOUT_I422] = { AV_PIX_FMT_YUV422P,
>> AV_PIX_FMT_YUV422P10, AV_PIX_FMT_YUV422P12 },
>> +[DAV1D_PIXEL_LAYOUT_I444] = { AV_PIX_FMT_YUV444P,
>> AV_PIX_FMT_YUV444P10, AV_PIX_FMT_YUV444P12 },
>> +};
>> +
>>  static void libdav1d_log_callback(void *opaque, const char *fmt, va_list
>> vl)
>>  {
>>  AVCodecContext *c = opaque;
>> @@ -44,6 +54,57 @@ static void libdav1d_log_callback(void *opaque, const
>> char *fmt, va_list vl)
>>  av_vlog(c, AV_LOG_ERROR, fmt, vl);
>>  }
>>
>> +static int libdav1d_picture_allocator(Dav1dPicture *p, void *cookie) {
>> +Libdav1dContext *dav1d = cookie;
>> +enum AVPixelFormat format = pix_fmt[p->p.layout][p->seq_hdr->hbd];
>> +int ret, linesize[4], h = FFALIGN(p->p.h, 128);
>> +uint8_t *aligned_ptr, *data[4];
>> +AVBufferRef *buf;
>> +
>> +ret = av_image_fill_arrays(data, linesize, NULL, format,
>> FFALIGN(p->p.w, 128),
>> +   h, DAV1D_PICTURE_ALIGNMENT);
>> +if (ret < 0)
>> +return ret;
>> +
>> +if (ret != dav1d->pool_size) {
>> +av_buffer_pool_uninit(>pool);
>> +// Use twice the amount of required padding bytes for aligned_ptr
>> below.
>> +dav1d->pool = av_buffer_pool_init(ret + DAV1D_PICTURE_ALIGNMENT *
>> 2, NULL);
>> +if (!dav1d->pool)
>> +return -ENOMEM;
>>
> 
> AVERROR(ENOMEM) ?

AVERROR(x) will expand to -x on every target libdav1d supports, so
there's no difference. And since this is a callback function, i figured
following the doxy to the letter was proper.

I can replace it if you prefer in any case.

> 
> +dav1d->pool_size = ret;
>> +}
>> +buf = av_buffer_pool_get(dav1d->pool);
>> +if (!buf)
>> +return -ENOMEM;
>> +
>> +// libdav1d requires DAV1D_PICTURE_ALIGNMENT aligned buffers, which
>> av_malloc()
>> +// doesn't guarantee for example when AVX is disabled at configure
>> time.
>> +// Use the extra DAV1D_PICTURE_ALIGNMENT padding bytes in the buffer
>> to align it
>> +// if required.
>> +aligned_ptr = (uint8_t *)FFALIGN((uintptr_t)buf->data,
>> DAV1D_PICTURE_ALIGNMENT);
>> +ret = av_image_fill_pointers(data, format, h, aligned_ptr, linesize);
>> +if (ret < 0) {
>> +av_buffer_unref();
>> +return ret;
>> +}
>> +
>> +p->data[0] = data[0];
>> +p->data[1] = data[1];
>> +p->data[2] = data[2];
>> +p->stride[0] = linesize[0];
>> +p->stride[1] = linesize[1];
>> +p->allocator_data = buf;
>> +
>> +return 0;
>> +}
>> +
>> +static void libdav1d_picture_release(Dav1dPicture *p, void *cookie) {
>> +AVBufferRef *buf = p->allocator_data;
>> +
>> +av_buffer_unref();
>> +}
>>
> 
> nit: keep { on a new line for functions

Fixed locally. And i see a couple others that also need to be changed in
a separate commit.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/2] avcodec/libdav1d: use a custom picture allocator

[Vittorio Giovara]

On Mon, Mar 4, 2019 at 4:08 PM James Almer  wrote:

> Replaces the libdav1d internal allocator. It uses an AVBufferPool to
> reduce the
> amount of allocated buffers.
> About 5% speed up when decoding 720p or higher streams.
>
> Signed-off-by: James Almer 
> ---
> get_buffer2() can't be used for this decoder, as there's no guarantee the
> buffers
> it returns will respect the constrains specified by libdav1d.
>
>  libavcodec/libdav1d.c | 72 ++-
>  1 file changed, 65 insertions(+), 7 deletions(-)
>
> diff --git a/libavcodec/libdav1d.c b/libavcodec/libdav1d.c
> index 459bbae687..855c27db7f 100644
> --- a/libavcodec/libdav1d.c
> +++ b/libavcodec/libdav1d.c
> @@ -22,6 +22,7 @@
>  #include 
>
>  #include "libavutil/avassert.h"
> +#include "libavutil/imgutils.h"
>  #include "libavutil/opt.h"
>
>  #include "avcodec.h"
> @@ -31,12 +32,21 @@
>  typedef struct Libdav1dContext {
>  AVClass *class;
>  Dav1dContext *c;
> +AVBufferPool *pool;
> +int pool_size;
>
>  Dav1dData data;
>  int tile_threads;
>  int apply_grain;
>  } Libdav1dContext;
>
> +static const enum AVPixelFormat pix_fmt[][3] = {
> +[DAV1D_PIXEL_LAYOUT_I400] = { AV_PIX_FMT_GRAY8,   AV_PIX_FMT_GRAY10,
>   AV_PIX_FMT_GRAY12 },
> +[DAV1D_PIXEL_LAYOUT_I420] = { AV_PIX_FMT_YUV420P,
> AV_PIX_FMT_YUV420P10, AV_PIX_FMT_YUV420P12 },
> +[DAV1D_PIXEL_LAYOUT_I422] = { AV_PIX_FMT_YUV422P,
> AV_PIX_FMT_YUV422P10, AV_PIX_FMT_YUV422P12 },
> +[DAV1D_PIXEL_LAYOUT_I444] = { AV_PIX_FMT_YUV444P,
> AV_PIX_FMT_YUV444P10, AV_PIX_FMT_YUV444P12 },
> +};
> +
>  static void libdav1d_log_callback(void *opaque, const char *fmt, va_list
> vl)
>  {
>  AVCodecContext *c = opaque;
> @@ -44,6 +54,57 @@ static void libdav1d_log_callback(void *opaque, const
> char *fmt, va_list vl)
>  av_vlog(c, AV_LOG_ERROR, fmt, vl);
>  }
>
> +static int libdav1d_picture_allocator(Dav1dPicture *p, void *cookie) {
> +Libdav1dContext *dav1d = cookie;
> +enum AVPixelFormat format = pix_fmt[p->p.layout][p->seq_hdr->hbd];
> +int ret, linesize[4], h = FFALIGN(p->p.h, 128);
> +uint8_t *aligned_ptr, *data[4];
> +AVBufferRef *buf;
> +
> +ret = av_image_fill_arrays(data, linesize, NULL, format,
> FFALIGN(p->p.w, 128),
> +   h, DAV1D_PICTURE_ALIGNMENT);
> +if (ret < 0)
> +return ret;
> +
> +if (ret != dav1d->pool_size) {
> +av_buffer_pool_uninit(>pool);
> +// Use twice the amount of required padding bytes for aligned_ptr
> below.
> +dav1d->pool = av_buffer_pool_init(ret + DAV1D_PICTURE_ALIGNMENT *
> 2, NULL);
> +if (!dav1d->pool)
> +return -ENOMEM;
>

AVERROR(ENOMEM) ?

+dav1d->pool_size = ret;
> +}
> +buf = av_buffer_pool_get(dav1d->pool);
> +if (!buf)
> +return -ENOMEM;
> +
> +// libdav1d requires DAV1D_PICTURE_ALIGNMENT aligned buffers, which
> av_malloc()
> +// doesn't guarantee for example when AVX is disabled at configure
> time.
> +// Use the extra DAV1D_PICTURE_ALIGNMENT padding bytes in the buffer
> to align it
> +// if required.
> +aligned_ptr = (uint8_t *)FFALIGN((uintptr_t)buf->data,
> DAV1D_PICTURE_ALIGNMENT);
> +ret = av_image_fill_pointers(data, format, h, aligned_ptr, linesize);
> +if (ret < 0) {
> +av_buffer_unref();
> +return ret;
> +}
> +
> +p->data[0] = data[0];
> +p->data[1] = data[1];
> +p->data[2] = data[2];
> +p->stride[0] = linesize[0];
> +p->stride[1] = linesize[1];
> +p->allocator_data = buf;
> +
> +return 0;
> +}
> +
> +static void libdav1d_picture_release(Dav1dPicture *p, void *cookie) {
> +AVBufferRef *buf = p->allocator_data;
> +
> +av_buffer_unref();
> +}
>

nit: keep { on a new line for functions

-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 2/2] avcodec/libdav1d: use a custom picture allocator

[James Almer]

Replaces the libdav1d internal allocator. It uses an AVBufferPool to reduce the
amount of allocated buffers.
About 5% speed up when decoding 720p or higher streams.

Signed-off-by: James Almer 
---
get_buffer2() can't be used for this decoder, as there's no guarantee the 
buffers
it returns will respect the constrains specified by libdav1d.

 libavcodec/libdav1d.c | 72 ++-
 1 file changed, 65 insertions(+), 7 deletions(-)

diff --git a/libavcodec/libdav1d.c b/libavcodec/libdav1d.c
index 459bbae687..855c27db7f 100644
--- a/libavcodec/libdav1d.c
+++ b/libavcodec/libdav1d.c
@@ -22,6 +22,7 @@
 #include 
 
 #include "libavutil/avassert.h"
+#include "libavutil/imgutils.h"
 #include "libavutil/opt.h"
 
 #include "avcodec.h"
@@ -31,12 +32,21 @@
 typedef struct Libdav1dContext {
 AVClass *class;
 Dav1dContext *c;
+AVBufferPool *pool;
+int pool_size;
 
 Dav1dData data;
 int tile_threads;
 int apply_grain;
 } Libdav1dContext;
 
+static const enum AVPixelFormat pix_fmt[][3] = {
+[DAV1D_PIXEL_LAYOUT_I400] = { AV_PIX_FMT_GRAY8,   AV_PIX_FMT_GRAY10,
AV_PIX_FMT_GRAY12 },
+[DAV1D_PIXEL_LAYOUT_I420] = { AV_PIX_FMT_YUV420P, AV_PIX_FMT_YUV420P10, 
AV_PIX_FMT_YUV420P12 },
+[DAV1D_PIXEL_LAYOUT_I422] = { AV_PIX_FMT_YUV422P, AV_PIX_FMT_YUV422P10, 
AV_PIX_FMT_YUV422P12 },
+[DAV1D_PIXEL_LAYOUT_I444] = { AV_PIX_FMT_YUV444P, AV_PIX_FMT_YUV444P10, 
AV_PIX_FMT_YUV444P12 },
+};
+
 static void libdav1d_log_callback(void *opaque, const char *fmt, va_list vl)
 {
 AVCodecContext *c = opaque;
@@ -44,6 +54,57 @@ static void libdav1d_log_callback(void *opaque, const char 
*fmt, va_list vl)
 av_vlog(c, AV_LOG_ERROR, fmt, vl);
 }
 
+static int libdav1d_picture_allocator(Dav1dPicture *p, void *cookie) {
+Libdav1dContext *dav1d = cookie;
+enum AVPixelFormat format = pix_fmt[p->p.layout][p->seq_hdr->hbd];
+int ret, linesize[4], h = FFALIGN(p->p.h, 128);
+uint8_t *aligned_ptr, *data[4];
+AVBufferRef *buf;
+
+ret = av_image_fill_arrays(data, linesize, NULL, format, FFALIGN(p->p.w, 
128),
+   h, DAV1D_PICTURE_ALIGNMENT);
+if (ret < 0)
+return ret;
+
+if (ret != dav1d->pool_size) {
+av_buffer_pool_uninit(>pool);
+// Use twice the amount of required padding bytes for aligned_ptr 
below.
+dav1d->pool = av_buffer_pool_init(ret + DAV1D_PICTURE_ALIGNMENT * 2, 
NULL);
+if (!dav1d->pool)
+return -ENOMEM;
+dav1d->pool_size = ret;
+}
+buf = av_buffer_pool_get(dav1d->pool);
+if (!buf)
+return -ENOMEM;
+
+// libdav1d requires DAV1D_PICTURE_ALIGNMENT aligned buffers, which 
av_malloc()
+// doesn't guarantee for example when AVX is disabled at configure time.
+// Use the extra DAV1D_PICTURE_ALIGNMENT padding bytes in the buffer to 
align it
+// if required.
+aligned_ptr = (uint8_t *)FFALIGN((uintptr_t)buf->data, 
DAV1D_PICTURE_ALIGNMENT);
+ret = av_image_fill_pointers(data, format, h, aligned_ptr, linesize);
+if (ret < 0) {
+av_buffer_unref();
+return ret;
+}
+
+p->data[0] = data[0];
+p->data[1] = data[1];
+p->data[2] = data[2];
+p->stride[0] = linesize[0];
+p->stride[1] = linesize[1];
+p->allocator_data = buf;
+
+return 0;
+}
+
+static void libdav1d_picture_release(Dav1dPicture *p, void *cookie) {
+AVBufferRef *buf = p->allocator_data;
+
+av_buffer_unref();
+}
+
 static av_cold int libdav1d_init(AVCodecContext *c)
 {
 Libdav1dContext *dav1d = c->priv_data;
@@ -55,6 +116,9 @@ static av_cold int libdav1d_init(AVCodecContext *c)
 dav1d_default_settings();
 s.logger.cookie = c;
 s.logger.callback = libdav1d_log_callback;
+s.allocator.cookie = dav1d;
+s.allocator.alloc_picture_callback = libdav1d_picture_allocator;
+s.allocator.release_picture_callback = libdav1d_picture_release;
 s.n_tile_threads = dav1d->tile_threads;
 s.apply_grain = dav1d->apply_grain;
 s.n_frame_threads = FFMIN(c->thread_count ? c->thread_count : 
av_cpu_count(), DAV1D_MAX_FRAME_THREADS);
@@ -87,13 +151,6 @@ static void libdav1d_frame_free(void *opaque, uint8_t 
*data) {
 av_free(p);
 }
 
-static const enum AVPixelFormat pix_fmt[][3] = {
-[DAV1D_PIXEL_LAYOUT_I400] = { AV_PIX_FMT_GRAY8,   AV_PIX_FMT_GRAY10,
AV_PIX_FMT_GRAY12 },
-[DAV1D_PIXEL_LAYOUT_I420] = { AV_PIX_FMT_YUV420P, AV_PIX_FMT_YUV420P10, 
AV_PIX_FMT_YUV420P12 },
-[DAV1D_PIXEL_LAYOUT_I422] = { AV_PIX_FMT_YUV422P, AV_PIX_FMT_YUV422P10, 
AV_PIX_FMT_YUV422P12 },
-[DAV1D_PIXEL_LAYOUT_I444] = { AV_PIX_FMT_YUV444P, AV_PIX_FMT_YUV444P10, 
AV_PIX_FMT_YUV444P12 },
-};
-
 static int libdav1d_receive_frame(AVCodecContext *c, AVFrame *frame)
 {
 Libdav1dContext *dav1d = c->priv_data;
@@ -222,6 +279,7 @@ static av_cold int libdav1d_close(AVCodecContext *c)
 {
 Libdav1dContext *dav1d = c->priv_data;
 
+av_buffer_pool_uninit(>pool);
 dav1d_data_unref(>data);