On Tue, 16 Sep 2014 21:52:06 +0300, =?UTF-8?Q?R=C3=A9mi_Denis-Courmont?= <[email protected]> wrote: > Le 2014-09-16 20:30, Anton Khirnov a écrit : > > On Thu, 11 Sep 2014 18:08:15 +0300, > > =?UTF-8?q?R=C3=A9mi=20Denis-Courmont?= <[email protected]> wrote: > >> Using the not so new init and uninit callbacks, avcodec can now take > >> care of creating and destroying the VDPAU decoder instance. > >> > >> The application is still responsible for creating the VDPAU device > >> and allocating video surfaces - this is necessary to keep video > >> surfaces on the GPU all the way to the output. But the application > >> will no longer needs to care about any codec-specific aspects. > >> --- > >> libavcodec/vdpau.c | 69 > >> ++++++++++++++++++++++++++++++++++++++++++++- > >> libavcodec/vdpau.h | 9 ++++++ > >> libavcodec/vdpau_internal.h | 26 +++++++++++++++++ > >> 3 files changed, 103 insertions(+), 1 deletion(-) > >> > >> diff --git a/libavcodec/vdpau.c b/libavcodec/vdpau.c > >> index d96b71c..2639272 100644 > >> --- a/libavcodec/vdpau.c > >> +++ b/libavcodec/vdpau.c > >> @@ -22,7 +22,9 @@ > >> */ > >> > >> #include <limits.h> > >> +#include "libavutil/avassert.h" > >> #include "avcodec.h" > >> +#include "internal.h" > >> #include "h264.h" > >> #include "vc1.h" > >> > >> @@ -62,6 +64,66 @@ static int ff_vdpau_error(VdpStatus status) > >> } > >> } > >> > >> +static void *ff_vdpau_get_proc_address(AVCodecContext *avctx, > >> VdpFuncId id) > > > > No ff_ prefix for static functions. And I don't see what this > > function > > accomplishes anyway -- you could just as well call the > > get_proc_address() > > callback directly. > > Indeed it was more meaningful in the previous version of the patch > series. > > >> +{ > >> + struct vdpau_context *vdctx = > >> avctx->internal->hwaccel_priv_data; > >> + void *ret; > >> + > >> + av_assert0(vdctx->device != VDP_INVALID_HANDLE); > >> + > >> + if (vdctx->get_proc_address(vdctx->device, id, &ret) != > >> VDP_STATUS_OK) > >> + ret = NULL; > >> + return ret; > >> +} > >> + > >> +int ff_vdpau_common_init(AVCodecContext *avctx, VdpDecoderProfile > >> profile, > >> + int level, uint32_t surfaces) > >> +{ > >> + AVVDPAUContext *hwctx = avctx->hwaccel_context; > >> + struct vdpau_context *vdctx = > >> avctx->internal->hwaccel_priv_data; > >> + VdpDecoderCreate *create; > >> + VdpStatus status; > >> + uint32_t width = (avctx->coded_width + 1) & ~1; > >> + uint32_t height = (avctx->coded_height + 3) & ~3; > > > > Why different rounding? > > That corresponds to the VDPAU surface size requirements for a field > with 4:2:0 chroma. See <vdpau/vdpau.h> for the blah blah. > > >> @@ -170,7 +232,12 @@ do { \ > >> > >> AVVDPAUContext *av_vdpau_alloc_context(void) > >> { > >> - return av_mallocz(sizeof(AVVDPAUContext)); > >> + AVVDPAUContext *hwctx = av_mallocz(sizeof(AVVDPAUContext)); > >> + if (!hwctx) > >> + return NULL; > >> + > >> + hwctx->device = VDP_INVALID_HANDLE; > >> + return hwctx; > >> } > >> > >> /* @}*/ > >> diff --git a/libavcodec/vdpau.h b/libavcodec/vdpau.h > >> index 75cb1bf..b246768 100644 > >> --- a/libavcodec/vdpau.h > >> +++ b/libavcodec/vdpau.h > >> @@ -128,6 +128,15 @@ typedef struct AVVDPAUContext { > >> attribute_deprecated > >> VdpBitstreamBuffer *bitstream_buffers; > >> #endif > >> + > >> /***************************************************************** > >> + * No fields below this line are part of the public API. They > >> + * may not be used outside of libavcodec and can be changed and > >> + * removed at will. > >> + * No new public fields will be added to this structure. > >> + > >> ***************************************************************** > >> + */ > >> + VdpDevice device; > >> + VdpGetProcAddress *get_proc_address; > > > > Eh? Aren't those supposed to be set by the caller? > > A later patch provides the setter function. That is saner than direct > access by the caller IMNSHO. > > > And if they are really private, they have no place here and must live > > in > > vdpau_context. > > Absolutely not. The private hardware acceleration data is not allocated > at that point, so it can obviously not be used.
Hmm. We already had trouble with people not noticing/disregarding that warning, so I'd still prefer to avoid private fields in public structs if at all possible. E.g. we could have av_vdpau_alloc_context() allocate an AVVDPAUContextInternal, in the same way as is done for AVCodecContext and others (though admittedly the nesting is getting kind of ridiculous). -- Anton Khirnov _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
