[FFmpeg-devel] [PATCH] AVHWAccel infrastructure v2 (take 6) [ping^3]

Gwenole Beauchesne gbeauchesne
Tue Nov 2 11:05:35 CET 2010


Hi,

On Sat, 30 Oct 2010, Michael Niedermayer wrote:

>> On Mon, 20 Sep 2010, Gwenole Beauchesne wrote:
>>
>>> It seems I didn't reply to this one or the message got lost because I
>>> had a v5 lying around for some time. Here is a v6 anyway, that
>>> intersects with the tidsp needs.
>>>
>>> On Mon, 22 Feb 2010, Michael Niedermayer wrote:
>>>
>>>> On Mon, Jan 04, 2010 at 04:39:02PM +0100, Gwenole Beauchesne wrote:
>>>>> Hi,
>>>>>
>>>>> On Mon, 4 Jan 2010, Michael Niedermayer wrote:
>>>>>
>>>>>>> In the new model I wanted to use, hwaccel_context was to be solely
>>>>>>> maintained by libavcodec. However, it still needed a way to get some
>>>>>>> hwaccel context initialized by the user-application. e.g. a VADisplay
>>>>>>> (void
>>>>>>> *), an LPDIRECTXVIDEODECODER, etc.
>>>>>>>
>>>>>>> I felt interesting to pass this information through hwaccel attrs along
>>>>>>> with other int attributes.
>>>>>>
>>>>>> We have existing means to pass information, that is through
>>>>>> AVCodecContext
>>>>>> and using AVOption where appropriate
>>>>>> As well as using AVFrame instead of AVCodecContext where its per frame
>>>>>> data
>>>>>
>>>>> OK, I nuked the hwaccel attrs and replaced it with
>>>>> AVCodecContext.hwaccel_{id,flags}. hwaccel_flags is handled by AVOption. I
>>>>> left hwaccel_id as is to match existing practise (pix_fmt et al.).
>>>>>
>>>>> I haven't tested with MPlayer yet but this new approach still works
>>>>> with my
>>>>> modified ffplay.
>>>>>
>>>>> So, HW accelerator selection works as is:
>>>>> - Set AVCodecContext.hwaccel_id to the desired accelerator
>>>>> - Set AVCodecContext.hwaccel_flags to HWACCEL_CAP_GET_PIXELS if the user
>>>>> wants AVFrame.data[0-2] populated with YV12 or NV12 pixels
>>>>>
>>>>> This means we can handle multiple accelerators automatically at once.
>>>>> Anyway, this lived in an ideal world without practical use since the
>>>>> user-application would still have to handle the HW accelerator
>>>>> specifically.
>>>>>
>>>>> Regards,
>>>>> Gwenole.
>>>>
>>>> ]...]
>>>>> @@ -2497,7 +2498,7 @@ typedef struct AVCodecContext {
>>>>>       * provided by the user. In that case, this holds display-dependent
>>>>>       * data FFmpeg cannot instantiate itself. Please refer to the
>>>>>       * FFmpeg HW accelerator documentation to know how to fill this
>>>>> -     * is. e.g. for VA API, this is a struct vaapi_context.
>>>>> +     * is. e.g. for VA API, this is a VADisplay.
>>>>>       * - encoding: unused
>>>>>       * - decoding: Set by user
>>>>>       */
>>>>
>>>> hunk ok probably
>>>>
>>>> [...]
>>>>> +    /**
>>>>> +     * Called in codec initialization.
>>>>> +     *
>>>>> +     * This is used to initialize the AVCodecContext.hwaccel_context
>>>>> +     * hardware accelerator context.
>>>>> +     *
>>>>> +     * @param avctx the codec context
>>>>> +     * @return zero if successful, a negative value otherwise
>>>>> +     */
>>>>> +    int (*init)(AVCodecContext *avctx);
>>>>> +
>>>>> +    /**
>>>>> +     * Called in codec finalization.
>>>>> +     *
>>>>> +     * This is used to clean-up any data from the hardware accelerator
>>>>> +     * context. Should this function be implemented, it shall reset
>>>>> +     * AVCodecContext.hwaccel_context to NULL.
>>>>> +     *
>>>>> +     * @param avctx the codec context
>>>>> +     * @return zero if successful, a negative value otherwise
>>>>> +     */
>>>>> +    int (*close)(AVCodecContext *avctx);
>>>>> +
>>>>> +    /**
>>>>> +     * Called at the beginning of each frame to allocate a HW surface.
>>>>> +     *
>>>>> +     * The returned surface will be stored in Picture.data[3].
>>>>> +     *
>>>>> +     * @param avctx the codec context
>>>>> +     * @param surface the pointer to the HW accelerator surface
>>>>> +     * @return zero if successful, a negative value otherwise
>>>>> +     */
>>>>> +    int (*alloc_surface)(AVCodecContext *avctx, uint8_t **surface);
>>>>> +
>>>>> +    /**
>>>>> +     * Called to free the HW surface that was allocated with
>>>>> alloc_surface().
>>>>> +     *
>>>>> +     * @param avctx the codec context
>>>>> +     * @param surface the HW accelerator surface
>>>>> +     * @return zero if successful, a negative value otherwise
>>>>> +     */
>>>>> +    void (*free_surface)(AVCodecContext *avctx, uint8_t *surface);
>>>>>  } AVHWAccel;
>>>>
>>>> iam not in favor of these. What are these good for?
>>>> Why are they needed?
>>>
>>> This is used to allocate the HW surfaces (VdpSurface, VASurface, etc.)
>>> from within FFmpeg. i.e. the AVFrame.data[3] part, hence the uint8_t *
>>> instead of the void *.
>>
>> Those functions are also used to help user implement
>> AVCodecContext.get_buffer() and release_buffer() if he wants to do so
>> manually.
>>
>> e.g. by default, AVCodecContext.get_buffer() will point to
>> avcodec_default_get_buffer(). If a user overrides that function, he will
>> lose automatic data[3] handling. So, two possible cases:
>> 1) He really knows how to do so, so he allocates the HW surface himself
>> 2) He can delegate that to FFmpeg that will know how to do that just fine
>> too. e.g. calling the right vaCreateSurfaces(), VdpVideoSurfaceCreate(),
>> etc.
>>
>>>>> +/** Identifies the hardware accelerator */
>>>>> +enum HWAccelID {
>>>>> +    HWACCEL_ID_NONE,
>>>>> +    HWACCEL_ID_VAAPI,           ///< Video Acceleration (VA) API
>>>>> +    HWACCEL_ID_NB
>>>>> +};
>>>>
>>>> this needs a any or auto for autodetection (which should be 0)
>>>
>>> I finally forgot about autodetection because it's too complex to handle
>>> at the FFmpeg level and delegated that to the player level. In
>>> particular, HWAccel context may depend on upper level info, like an X
>>> display...
>>>
>>>>> +
>>>>> +/** Identifies the hardware accelerator entry-point */
>>>>> +enum HWAccelEntrypoint {
>>>>> +    HWACCEL_ENTRYPOINT_VLD = 1, ///< Variable-length decoding
>>>>> +    HWACCEL_ENTRYPOINT_IDCT,    ///< Inverse discrete cosine transform
>>>>> +    HWACCEL_ENTRYPOINT_MOCO,    ///< Motion compensation
>>>>> +    HWACCEL_ENTRYPOINT_NB
>>>>> +};
>>>>
>>>> as said previously this is ambigous
>>>>
>>>> HWACCEL_ENTRYPOINT_VLD could mean the hw does just VLD and leaves IDCT/MC
>>>> to software
>>>
>>> Actually, I didn't find the exact meaning. But from a pipeline point of
>>> view, this would mean XXX and above. i.e. VLD and above, motion
>>> compensation and above. Flagging those would lead to more player work
>>> (well, just OR'ing more flags).
>>>
>>> I tried to express that in the comment.
>>>
>>>>> +/**
>>>>> + * The hardware accelerator can fetch the pixels from the decoded
>>>>> + * frames. If the user requested it, avcodec_default_get_buffer()
>>>>> + * will then allocate the data buffers.
>>>>> + */
>>>>> +#define HWACCEL_CAP_GET_PIXELS 0x00000001
>>>>
>>>> might need a AV prefix
>>>
>>> OK.
>>
>> Regards,
>> Gwenole.
>
>>  Makefile    |    2 -
>>  avcodec.h   |   87 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>>  hwaccel.h   |   55 +++++++++++++++++++++++++++++++++++++
>>  internal.h  |   14 +++++++++
>>  mpegvideo.c |    4 ++
>>  options.c   |    2 +
>>  utils.c     |   52 +++++++++++++++++++++++++++++++++++
>>  7 files changed, 213 insertions(+), 3 deletions(-)
>> e890e0f1046043c9a28d3a64d9380146b0316139  ffmpeg.hwaccel.v2.8.patch
>> commit 8277742b00c2dbe41fd3af7f25c1c903f4477b43
>> Author: Gwenole Beauchesne <gbeauchesne at splitted-desktop.com>
>> Date:   Fri Oct 29 06:16:55 2010 +0200
>>
>>     Update AVHWAccel infrastructure to v2.
>>
>> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
>> index 385ae02..1b64768 100644
>> --- a/libavcodec/Makefile
>> +++ b/libavcodec/Makefile
>> @@ -3,7 +3,7 @@ include $(SUBDIR)../config.mak
>>  NAME = avcodec
>>  FFLIBS = avutil avcore
>>
>> -HEADERS = avcodec.h avfft.h dxva2.h opt.h vaapi.h vdpau.h xvmc.h
>> +HEADERS = avcodec.h avfft.h dxva2.h hwaccel.h opt.h vaapi.h vdpau.h xvmc.h
>>
>>  OBJS = allcodecs.o                                                      \
>>         audioconvert.o                                                   \
>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>> index 705259e..137f485 100644
>> --- a/libavcodec/avcodec.h
>> +++ b/libavcodec/avcodec.h
>> @@ -29,6 +29,7 @@
>>  #include <errno.h>
>>  #include "libavutil/avutil.h"
>>  #include "libavutil/cpu.h"
>> +#include "libavcodec/hwaccel.h"
>>
>>  #define LIBAVCODEC_VERSION_MAJOR 52
>>  #define LIBAVCODEC_VERSION_MINOR 93
>> @@ -2607,7 +2608,7 @@ typedef struct AVCodecContext {
>>       * provided by the user. In that case, this holds display-dependent
>>       * data FFmpeg cannot instantiate itself. Please refer to the
>>       * FFmpeg HW accelerator documentation to know how to fill this
>> -     * is. e.g. for VA API, this is a struct vaapi_context.
>> +     * is. e.g. for VA API, this is a VADisplay.
>>       * - encoding: unused
>>       * - decoding: Set by user
>>       */
>
>> @@ -2753,6 +2754,27 @@ typedef struct AVCodecContext {
>>       * - decoding: unused
>>       */
>>      int slices;
>> +
>> +    /**
>> +     * User-requested hardware accelerator. See HWACCEL_ID_xxx.
>> +     * - encoding: unused
>> +     * - decoding: Set by user
>> +     */
>> +    enum HWAccelID hwaccel_id;
>
> could you remind me why this is needed compared to the apparently more flexible
> system with pixel formats?

Before hwaccel v1, we had: PIX_FMT_<hwaccel>_<codec>. One <hwaccel> could 
easily have ~5 codecs. If you have 2 <haccel>, you get 10 pixel formats. 
Besides, you can have <codec> supported at different entrypoint. e.g. VLD, 
MoComp, IDCT. Thus, we could have combinatorial explosion.

With hwaccel v1, we factored this with PIX_FMT_<hwaccel>_<entrypoint>, the 
<codec> information became irrelevant. <entrypoint> was reduced to 
typically 1 to 3. What does a pixel format with a <codec> name in it 
actually mean anyway?

This is better, but this was still silly. A pixel format defines... well, 
a pixel format. If the user wants to extract the decoded pixels from the 
GPU, he requires so with AV_HWACCEL_CAP_GET_PIXELS. The pixel format of 
those needs to be expressed the usual way, e.g. avctx->pix_fmt == 
PIX_FMT_YUV420P. How would this have been expressed if avctx->pix_fmt was 
PIX_FMT_<hwaccel>_<entrypoint>?

Besides, from an implementation point of view (in libavcodec), the pre-v1 
and v1 way of using pixel formats becomes tedious to handle.

We currently have hooks called through:

if (avctx->hwaccel)
   avctx->hwaccel->decode_slice(...);

this needs to become:
if (avctx->hwaccel && <is entrypoint VLD>)
   avctx->hwaccel->decode_slice(...);

so that other entry-points could be managed easily.

With the v1 way, <is entrypoint == VLD> becomes a big evil switch:
return avctx->pix_fmt == PIX_FMT_hwaccel1_VLD ||?avctx->pix_fmt == 
PIX_FMT_hwaccel2_VLD || avcxtx->pix_fmt == PIX_FMT_hwaccel3_VLD, etc.

Horrible, isn't it?

In the v2 proposal, we just need to do:
if (avctx->hwaccel && avctx->hwaccel->entrypoint == HWACCEL_ENTRYPOINT_VLD)
   avctx->hwaccel->decode_slice(...);

>> +    /**
>> +     * Hardware accelerator configuration flags. See See AV_HWACCEL_CAP_xxx.
>> +     * - encoding: unused
>> +     * - decoding: Set by user
>> +     */
>> +    unsigned int hwaccel_flags;
>
>> +
>> +    /**
>> +     * Hardware accelerator private context.
>> +     * - encoding: unused
>> +     * - decoding: Set by libavcodec
>> +     */
>> +    void *hwaccel_context_private;
>
> This looks like it can be merged with hwaccel_context

OK, the initial intent was to have private contents to a private 
struct. I reckon that, should this be needed, a priv_data field in the 
relevant HW accel *_context would be better instead, akin to 
AVCodecContext.

So, I have attached a new patch with field removed. This still works for 
me.

Thanks,
Gwenole.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ffmpeg.hwaccel.v2.9.patch
Type: text/x-diff
Size: 11004 bytes
Desc: 
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101102/4daf5a8e/attachment.patch>



More information about the ffmpeg-devel mailing list