| Message ID | 20190304210656.2412-2-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Mar 4, 2019 at 4:08 PM James Almer <jamrial@gmail.com> 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 <jamrial@gmail.com> > --- > 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 <dav1d/dav1d.h> > > #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
On 3/5/2019 3:19 PM, Vittorio Giovara wrote: > On Mon, Mar 4, 2019 at 4:08 PM James Almer <jamrial@gmail.com> 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 <jamrial@gmail.com> >> --- >> 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 <dav1d/dav1d.h> >> >> #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.
2019-03-04 22:06 GMT+01:00, James Almer <jamrial@gmail.com>: > +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
On 3/6/2019 5:06 AM, Carl Eugen Hoyos wrote: > 2019-03-04 22:06 GMT+01:00, James Almer <jamrial@gmail.com>: > >> +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?
2019-03-06 14:22 GMT+01:00, James Almer <jamrial@gmail.com>: > On 3/6/2019 5:06 AM, Carl Eugen Hoyos wrote: >> 2019-03-04 22:06 GMT+01:00, James Almer <jamrial@gmail.com>: >> >>> +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
On 3/6/2019 10:33 AM, Carl Eugen Hoyos wrote: > 2019-03-06 14:22 GMT+01:00, James Almer <jamrial@gmail.com>: >> On 3/6/2019 5:06 AM, Carl Eugen Hoyos wrote: >>> 2019-03-04 22:06 GMT+01:00, James Almer <jamrial@gmail.com>: >>> >>>> +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.
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 <dav1d/dav1d.h> #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); dav1d_data_unref(&dav1d->data); dav1d_close(&dav1d->c);
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 <jamrial@gmail.com> --- 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(-)