On Tue, 2 Feb 2016 19:51:53 +0000
Mark Thompson <s...@jkqxz.net> wrote:
> On 01/02/16 15:58, wm4 wrote:
> > On Sat, 30 Jan 2016 22:11:52 +0000
> > Mark Thompson <s...@jkqxz.net> wrote:
> >
> >> ---
> >> configure | 5 +
> >> libavcodec/Makefile | 1 +
> >> libavcodec/vaapi_support.c | 710
> >> +++++++++++++++++++++++++++++++++++++++++++++
> >> libavcodec/vaapi_support.h | 122 ++++++++
> >> 4 files changed, 838 insertions(+)
> >> create mode 100644 libavcodec/vaapi_support.c
> >> create mode 100644 libavcodec/vaapi_support.h
> >>
> >> diff --git a/configure b/configure
> >> index dba8180..e7f53af 100755
> >> --- a/configure
> >> +++ b/configure
> >> @@ -2037,6 +2037,7 @@ CONFIG_EXTRA="
> >> texturedsp
> >> texturedspenc
> >> tpeldsp
> >> + vaapi_recent
> >> videodsp
> >> vp3dsp
> >> vp56dsp
> >> @@ -5768,6 +5769,10 @@ enabled vaapi &&
> >> check_lib va/va.h vaInitialize -lva ||
> >> disable vaapi
> >>
> >> +enabled vaapi &&
> >> + check_code cc va/va.h "vaCreateSurfaces(0, 0, 0, 0, 0, 0, 0, 0)" &&
> >> + enable vaapi_recent
> >> +
> >> enabled vaapi && enabled xlib &&
> >> check_lib2 "va/va.h va/va_x11.h" vaGetDisplay -lva -lva-x11 &&
> >> enable vaapi_x11
> >> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> >> index de957d8..045d118 100644
> >> --- a/libavcodec/Makefile
> >> +++ b/libavcodec/Makefile
> >> @@ -719,6 +719,7 @@ OBJS-$(CONFIG_ADPCM_YAMAHA_ENCODER) +=
> >> adpcmenc.o adpcm_data.o
> >> OBJS-$(CONFIG_D3D11VA) += dxva2.o
> >> OBJS-$(CONFIG_DXVA2) += dxva2.o
> >> OBJS-$(CONFIG_VAAPI) += vaapi.o
> >> +OBJS-$(CONFIG_VAAPI_RECENT) += vaapi_support.o
> >> OBJS-$(CONFIG_VDA) += vda.o videotoolbox.o
> >> OBJS-$(CONFIG_VIDEOTOOLBOX) += videotoolbox.o
> >> OBJS-$(CONFIG_VDPAU) += vdpau.o
> >> diff --git a/libavcodec/vaapi_support.c b/libavcodec/vaapi_support.c
> >> new file mode 100644
> >> index 0000000..2e22f98
> >> --- /dev/null
> >> +++ b/libavcodec/vaapi_support.c
> >> @@ -0,0 +1,710 @@
> >> +/*
> >> + * VAAPI helper functions.
> >> + *
> >> + * Copyright (C) 2016 Mark Thompson <m...@jkqxz.net>
> >> + *
> >> + * This file is part of FFmpeg.
> >> + *
> >> + * FFmpeg is free software; you can redistribute it and/or
> >> + * modify it under the terms of the GNU Lesser General Public
> >> + * License as published by the Free Software Foundation; either
> >> + * version 2.1 of the License, or (at your option) any later version.
> >> + *
> >> + * FFmpeg is distributed in the hope that it will be useful,
> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> >> + * Lesser General Public License for more details.
> >> + *
> >> + * You should have received a copy of the GNU Lesser General Public
> >> + * License along with FFmpeg; if not, write to the Free Software
> >> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> >> 02110-1301 USA
> >> + */
> >> +
> >> +#include "vaapi_support.h"
> >> +
> >> +#include "libavutil/avassert.h"
> >> +#include "libavutil/imgutils.h"
> >> +#include "libavutil/pixfmt.h"
> >> +
> >> +
> >> +static AVClass vaapi_class = {
> >> + .class_name = "vaapi",
> >> + .item_name = av_default_item_name,
> >> + .version = LIBAVUTIL_VERSION_INT,
> >> +};
> >> +static AVClass *vaapi_log = &vaapi_class;
> >
> > We've talked about it on IRC. So the idea was matching
> > AVVAAPIHardwareContext with vaapi_context, which is why the first
> > member of AVVAAPIHardwareContext can't be AVClass. I think trying to
> > somehow make these structs compatible should be given up, and instead
> > the struct should have a vaapi_context pointer field instead.
>
> Yes.
>
> > Also, I'm very worried about how this patch in general with combine
> > with the API Libav is planning to add:
> >
> > https://lists.libav.org/pipermail/libav-devel/2016-January/074490.html
> >
> > To make this clear, above patch series:
> > - extends the buffer pool to make it useful for the hwdec case (where
> > you need to free the hw context when the final AVFrame is unreffed)
> > - create a common API for hwaccel contexts, which overlaps with this
> > patch to some degree, but not fully
> > - adds a hwaccel context field to AVFrame
> > - the hwaccel context is refcounted
> > - adds helper for reading back data from hwaccel AVFrames in an
> > API-independent way
> >
> > I'm not sure what to make out of this mess. Any ideas? I don't really
> > want to hold this patch series back either. There is not much reason to
> > refactor the whole API 1 week after it has been added either.
>
> I'm not sure how to respond usefully to all of that, it adds a lot of new
> things all over the place.
>
> It looks like it has most of the elements needed here, though I would need to
> examine it more carefully to be sure. (I don't see a way to do the
> "enumerate all of a set of frames for something which requires all render
> targets in advance" operation, but maybe I haven't looked hard enough.)
>
The newer patches have something for this, I think. At least partially.
> ...
> >> + err = 0;
> >> + if(0) {
> >> + fail:
> >
> > This looks funny. My personal preference is avoiding such tricks, but
> > maybe it's not important.
>
> I think this unwinding was more complex at some previous iteration. The
> failure case can be moved below the return now, though I still think it's
> nice to have exactly one lock/unlock pair to be sure that that is correct.
If you think so... I still think it could be more readable, but I don't
want to force you. In general, gotos should be lined up at the end of a
function for cleanup, and "if (0)" looks like a WTF.
> >> + if(surface) {
> >> + if(surface->id)
> >> + vaDestroySurfaces(hw_ctx->display, &surface->id, 1);
> >> + av_free(surface);
> >> + }
> >> + if(frame)
> >> + av_frame_free(&frame);
> >> + for(--i; i >= 0; i--)
> >> + av_frame_free(&pool->frames[i]);
> >> + }
> >> +
> >> + av_vaapi_unlock_hardware_context(hw_ctx);
> >> + return err;
> >> +}
> >> +
> >> +int av_vaapi_surface_pool_uninit(AVVAAPISurfacePool *pool)
> >> +
> >> +{
> >> + int i;
> >> +
> >> + av_vaapi_lock_hardware_context(pool->hardware_context);
> >> +
> >> + for(i = 0; i < FF_ARRAY_ELEMS(pool->frames); i++) {
> >> + if(pool->frames[i])
> >> + av_frame_free(&pool->frames[i]);
> >> + }
> >> +
> >> + av_vaapi_unlock_hardware_context(pool->hardware_context);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +int av_vaapi_surface_pool_get(AVVAAPISurfacePool *pool, AVFrame *target)
> >> +{
> >> + AVFrame *frame = 0;
> >> + int i, err;
> >> +
> >> + av_vaapi_lock_hardware_context(pool->hardware_context);
> >> +
> >> + for(i = 0; i < FF_ARRAY_ELEMS(pool->frames); i++) {
> >> + if(!pool->frames[i])
> >> + break;
> >> +
> >> + if(av_buffer_get_ref_count(pool->frames[i]->buf[0]) == 1) {
> >> + frame = pool->frames[i];
> >> + break;
> >> + }
> >> + }
> >> +
> >> + if(frame) {
> >> + target->data[3] = frame->data[3];
> >> + target->buf[0] = av_buffer_ref(frame->buf[0]);
> >> + target->buf[1] = av_buffer_ref(frame->buf[1]);
> >> + if(!target->buf[0] || !target->buf[1])
> >> + err = AVERROR(ENOMEM);
> >> + else
> >> + err = 0;
> >> + } else {
> >> + err = AVERROR(ENOMEM);
> >> + }
> >> +
> >> + av_vaapi_unlock_hardware_context(pool->hardware_context);
> >> +
> >> + return err;
> >> +}
> >> +
> >> +int av_vaapi_map_frame(AVFrame *frame, int get)
> >> +{
> >> + AVVAAPISurface *surface = vaapi_get_surface(frame);
> >> + AVVAAPISurfaceConfig *config = vaapi_get_surface_config(frame);
> >> + AVVAAPIHardwareContext *hw_ctx = surface->hardware_context;
> >> + VAImage *image = &surface->image;
> >> + VAStatus vas;
> >> + int i, err;
> >> + void *address;
> >> + // On current Intel drivers, derive gives you memory which is very
> >> slow
> >> + // to read (uncached?). It can be better for write-only cases, but
> >> for
> >> + // now play it safe and never use derive.
> >> + int derive = 0;
> >
> > Before the patch gets applied, we also should check if we can get in a
> > "GPU memcpy", and whether this codepath can be enabled. If not, there
> > wouldn't be much of a reason to keep it.
>
> Even without it, it has use for the "drawing on frames" case (subtitles?). I
> haven't studied how that works in libavfilter at all, but I imagine we want
> the derive case there to avoid a pointless copy there and back to speed up
> changing only a few pixel values.
>
> I guess it could go for now and be added back if/when someone implements
> that, though.
>
Then it would have an option. But let's not leave commented code in
there just so it can bitrot.
> ...
> > Looks in general pretty ok to me.
> >
> > There should be allocation functions for structs which don't have one
> > yet, and a clear warning should be added to the doxygen. (Doxygen
> > should also be added.)
>
> If you're happy that this is now plausible, then I will write documentation
> for the relevant parts.
>
OK, but there's still the unsolved Libav issue, which might dice up
everything again.
> > One nit: I _think_ we usually add a whitespace between statement and
> > "(", e.g. "if (". (Not for function calls or declarations.)
>
> I can sed the spaces in, I guess.
>
> Thanks for the review!
>
> - Mark
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
