Hi,
2014-09-05 18:13 GMT+02:00 wm4 <[email protected]>:
> On Thu, 4 Sep 2014 16:11:15 +0300
> Rémi Denis-Courmont <[email protected]> wrote:
>
>> From: Rémi Denis-Courmont <[email protected]>
>>
>> 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. But it wull no longer needs to care
>> about any codec-specific aspects.
>>
>> This deprecates av_vdpau_alloc_context() and introduces a new
>> av_vdpau_create_context() accepting the VDPAU device (VdpDevice) and
>> VDPAU driver (VdpGetProcAddress) as explicit parameter.
>>
>> Note that av_vdpau_get_profile() is also deprecated since it becomes
>> totally useless for external use.
>>
>> [Discuss: It could be:
>> int av_vdpau_init_context(AVCodecContext *, VdpDevice,
>> VdpGetProcAddress *);
>> instead (that would enable capability checks). In that case, there
>> should probably be a deinitializer for the sake of symmetry:
>> void av_vdpau_deinit_context(AVCodecContext *);
>> instead of the current reliance on av_freep().]
>> ---
>> libavcodec/vdpau.c | 84
>> ++++++++++++++++++++++++++++++++++++++++++++-
>> libavcodec/vdpau.h | 27 +++++++++++++--
>> libavcodec/vdpau_internal.h | 16 +++++++++
>> libavcodec/version.h | 3 ++
>> 4 files changed, 127 insertions(+), 3 deletions(-)
>>
>> diff --git a/libavcodec/vdpau.c b/libavcodec/vdpau.c
>> index 5406874..c285897 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"
>>
>> @@ -38,6 +40,67 @@
>> * @{
>> */
>>
>> +static VdpStatus vdp_enosys(/* Unspecified, not void - leave empty */)
>> +{
>> + return VDP_STATUS_NO_IMPLEMENTATION;
>> +}
>> +
>> +static void *ff_vdpau_get_proc_address(AVCodecContext *avctx, VdpFuncId id)
>> +{
>> + AVVDPAUContext *hwctx = avctx->hwaccel_context;
>> + void *ret;
>> +
>> + av_assert0(hwctx->device != VDP_INVALID_HANDLE);
>> +
>> + if (hwctx->get_proc_address(hwctx->device, id, &ret) != VDP_STATUS_OK)
>> + ret = vdp_enosys;
>
> Isn't this very questionable? IMO initialization should just fail if a
> required function is missing. But if a C language lawyer can confirm
> that this use of vdp_enosys is indeed legal, why not (although I
> have my doubt).
This is indeed questionable. A pointer to function is not always a
pointer to the function start address, but it could rather be a
pointer to a function descriptor, which comprises of the function
start address and additional information. e.g. some TOC pointer on
ppc64. It's better to use the correct type: pointer-to-function. This
is assuming that get_proc_address() really returns a pointer to
function to begin with.
I don't immediately remember the chapter and verse, but probably
somewhere in section 6.2.x or 6.3.x.
i.e. casts from pointer-to-function to pointer are not guaranteed.
>> + return ret;
>> +}
>> +
>> +int ff_vdpau_common_init(AVCodecContext *avctx, VdpDecoderProfile profile,
>> + uint32_t level, uint32_t surfaces)
>> +{
>> + AVVDPAUContext *hwctx = avctx->hwaccel_context;
>> + struct vdpau_context *vdctx = avctx->internal->hwaccel_priv_data;
>> + VdpDecoderCreate *create;
>> + VdpStatus status;
>> +
>> +#if FF_API_DECODER_VDPAU
>> + if (hwctx->device == VDP_INVALID_HANDLE) {
>> + vdctx->decoder = hwctx->decoder;
>> + vdctx->render = hwctx->render;
>> + return 0; /* Decoder created by user */
>> + }
>> +#endif
>> +
>> + create = ff_vdpau_get_proc_address(avctx, VDP_FUNC_ID_DECODER_CREATE);
>> +
>> + status = create(hwctx->device, profile, avctx->width, avctx->height,
>> + surfaces, &vdctx->decoder);
>> + if (status != VDP_STATUS_OK)
>> + return AVERROR(ENODEV); /* TODO: VDPAU to AV error codes */
>> +
>> + vdctx->render = ff_vdpau_get_proc_address(avctx,
>> + VDP_FUNC_ID_DECODER_RENDER);
>> + return 0;
>> +}
>> +
>> +int ff_vdpau_common_uninit(AVCodecContext *avctx)
>> +{
>> + struct vdpau_context *vdctx = avctx->internal->hwaccel_priv_data;
>> + VdpDecoderDestroy *destroy;
>> +#if FF_API_DECODER_VDPAU
>> + AVVDPAUContext *hwctx = avctx->hwaccel_context;
>> +
>> + if (hwctx->device == VDP_INVALID_HANDLE)
>> + return 0; /* Decoder to be destroyed by user */
>> +#endif
>> +
>> + destroy = ff_vdpau_get_proc_address(avctx, VDP_FUNC_ID_DECODER_DESTROY);
>> + destroy(vdctx->decoder);
>> + return 0;
>> +}
>> +
>> int ff_vdpau_common_start_frame(struct vdpau_picture_context *pic_ctx,
>> av_unused const uint8_t *buffer,
>> av_unused uint32_t size)
>> @@ -88,6 +151,7 @@ int ff_vdpau_add_buffer(struct vdpau_picture_context
>> *pic_ctx,
>> return 0;
>> }
>>
>> +#if FF_API_DECODER_VDPAU
>> int av_vdpau_get_profile(AVCodecContext *avctx, VdpDecoderProfile *profile)
>> {
>> #define PROFILE(prof) \
>> @@ -133,7 +197,25 @@ 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;
>> +}
>> +#endif
>> +
>> +AVVDPAUContext *av_vdpau_create_context(VdpDevice device,
>> + VdpGetProcAddress *get_proc_address)
>> +{
>> + AVVDPAUContext *hwctx = av_mallocz(sizeof(AVVDPAUContext));
>> + if (!hwctx)
>> + return NULL;
>> +
>> + hwctx->device = device;
>> + hwctx->get_proc_address = get_proc_address;
>> + return hwctx;
>> }
>>
>> /* @}*/
>> diff --git a/libavcodec/vdpau.h b/libavcodec/vdpau.h
>> index 75cb1bf..4a99834 100644
>> --- a/libavcodec/vdpau.h
>> +++ b/libavcodec/vdpau.h
>> @@ -80,11 +80,13 @@ union AVVDPAUPictureInfo {
>> * AVVDPAUContext.
>> */
>> typedef struct AVVDPAUContext {
>> +#if FF_API_DECODER_VDPAU
>> /**
>> * VDPAU decoder handle
>> *
>> * Set by user.
>> */
>> + attribute_deprecated
>> VdpDecoder decoder;
>>
>> /**
>> @@ -92,8 +94,9 @@ typedef struct AVVDPAUContext {
>> *
>> * Set by the user.
>> */
>> + attribute_deprecated
>> VdpDecoderRender *render;
>> -
>> +#endif
>> #if FF_API_BUFS_VDPAU
>> /**
>> * VDPAU picture information
>> @@ -128,14 +131,32 @@ 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;
>> } AVVDPAUContext;
>>
>> /**
>> + * Create an AVVDPAUContext.
>> + *
>> + * @return Newly-allocated AVVDPAUContext or NULL on failure.
>> + */
>> +AVVDPAUContext *av_vdpau_create_context(VdpDevice device,
>> + VdpGetProcAddress
>> *get_proc_address);
>> +
>> +#if FF_API_DECODER_VDPAU
>> +/**
>> * Allocate an AVVDPAUContext.
>> *
>> * @return Newly-allocated AVVDPAUContext or NULL on failure.
>> */
>> -AVVDPAUContext *av_vdpau_alloc_context(void);
>> +attribute_deprecated AVVDPAUContext *av_vdpau_alloc_context(void);
>>
>> /**
>> * Get a decoder profile that should be used for initializing a VDPAU
>> decoder.
>> @@ -148,7 +169,9 @@ AVVDPAUContext *av_vdpau_alloc_context(void);
>> *
>> * @return 0 on success (non-negative), a negative AVERROR on failure.
>> */
>> +attribute_deprecated
>> int av_vdpau_get_profile(AVCodecContext *avctx, VdpDecoderProfile *profile);
>> +#endif
>>
>> #if FF_API_CAP_VDPAU
>> /** @brief The videoSurface is used for rendering. */
>> diff --git a/libavcodec/vdpau_internal.h b/libavcodec/vdpau_internal.h
>> index 2443e0a..379407e 100644
>> --- a/libavcodec/vdpau_internal.h
>> +++ b/libavcodec/vdpau_internal.h
>> @@ -48,6 +48,18 @@ union AVVDPAUPictureInfo {
>> #include "vdpau.h"
>> #endif
>>
>> +struct vdpau_context {
>> + /**
>> + * VDPAU decoder handle
>> + */
>> + VdpDecoder decoder;
>> +
>> + /**
>> + * VDPAU decoder render callback
>> + */
>> + VdpDecoderRender *render;
>> +};
>> +
>> struct vdpau_picture_context {
>> /**
>> * VDPAU picture information.
>> @@ -70,6 +82,10 @@ struct vdpau_picture_context {
>> VdpBitstreamBuffer *bitstream_buffers;
>> };
>>
>> +int ff_vdpau_common_init(AVCodecContext *avctx, VdpDecoderProfile profile,
>> + uint32_t level, uint32_t surfaces);
>> +int ff_vdpau_common_uninit(AVCodecContext *avctx);
>> +
>> int ff_vdpau_common_start_frame(struct vdpau_picture_context *pic,
>> const uint8_t *buffer, uint32_t size);
>> int ff_vdpau_mpeg_end_frame(AVCodecContext *avctx);
>> diff --git a/libavcodec/version.h b/libavcodec/version.h
>> index 8cc2fb0..41c7fcb 100644
>> --- a/libavcodec/version.h
>> +++ b/libavcodec/version.h
>> @@ -153,5 +153,8 @@
>> #ifndef FF_API_AFD
>> #define FF_API_AFD (LIBAVCODEC_VERSION_MAJOR < 57)
>> #endif
>> +#ifndef FF_API_DECODER_VDPAU
>> +#define FF_API_DECODER_VDPAU (LIBAVCODEC_VERSION_MAJOR < 57)
>> +#endif
>>
>> #endif /* AVCODEC_VERSION_H */
>
>
> The idea seems generally ok (and making libavcodec handle more aspects
> of hw decoding is welcome). But I have some doubts whether it will
> still be possible for me to handle preemption as well as I could, and I
> wouldn't appreciate if a small incremental improvement breaks all of my
> code.
>
> Also, think of the API users: do they have to update their vdpau code
> every month now? WTF?
>
> A solution to this might be not deprecating the "old" API. Then
> downstreams can wait it out until the API finally becomes sane after a
> bunch of more incremental improvements. This is true for the other
> hwaccels as well (since I always hear of hwaccel 1.x...).
>
> _______________________________________________
> libav-devel mailing list
> [email protected]
> https://lists.libav.org/mailman/listinfo/libav-devel
Regards,
--
Gwenole Beauchesne
Intel Corporation SAS / 2 rue de Paris, 92196 Meudon Cedex, France
Registration Number (RCS): Nanterre B 302 456 199
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
