Re: [FFmpeg-devel] [PATCH 2/2] avcodec/libdav1d: use a custom picture allocator
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
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
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
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
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(&dav1d->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(&buf); >> +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(&buf); >> +} >> > > 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
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(&dav1d->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(&buf); > +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(&buf); > +} > 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
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(&dav1d->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(&buf); +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(&buf); +} + 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); 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(&dav1d->pool); da