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

Gwenole Beauchesne gbeauchesne
Sat Nov 6 07:28:34 CET 2010


Hi,

Ping^3.2. ;-)

On Tue, 2 Nov 2010, Gwenole Beauchesne wrote:

> 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.



More information about the ffmpeg-devel mailing list