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

[Michael Niedermayer]

On Fri, Nov 13, 2009 at 04:38:42PM +0100, Gwenole Beauchesne wrote:
> Hi,
>
> On Mon, 9 Nov 2009, Michael Niedermayer wrote:
>
>> On Mon, Sep 28, 2009 at 03:19:47PM +0200, Gwenole Beauchesne wrote:
>>> On Sat, 26 Sep 2009, Gwenole Beauchesne wrote:
>>>
>>>> Le 26 sept. 09 ? 09:28, Gwenole Beauchesne a ?crit :
>>>>
>>>>> This patch updates the AVHWAccel infrastructure to make it possible to:
>>>>> - Get rid of the PixelFormat hacks
>>>>> - Get pixels back from GPU memory when the user requested it
>>>>> - Fallback implicitly to SW decoding
>>>>> - Handle multiple hardware accelerators (e.g. different chips in a 
>>>>> single
>>>>> system)
>>>>> - Enable user-applications very easily
>>
>> Can these be split into seperate patches?
>
> I think this was the smallest bits and, as you noticed hereunder, some 
> enums/structs looked incomplete because they were part of other patches.

yes

[...]
>>> +
>>> +/** Identifies the hardware accelerator entry-point */
>>> +enum HWAccelEntrypoint {
>>> +    HWACCEL_ENTRYPOINT_VLD = 1, ///< Variable-length decoding
>>> +    HWACCEL_ENTRYPOINT_IZZ,     ///< Inverse zig-zag scan decoder
>>> +    HWACCEL_ENTRYPOINT_IDCT,    ///< Inverse discrete cosine transform
>>> +    HWACCEL_ENTRYPOINT_MOCO,    ///< Motion compensation
>>> +    HWACCEL_ENTRYPOINT_NB
>>> +};
>>
>> The documentation is too terse
>
> /** Identifies the hardware accelerator entry-point. The entry-point 
> defines the point where the hardware accelerator would actually handle the 
> decoding operations */
> ?

that part is fine, what i meant was things like
 "Inverse zig-zag scan decoder", 
is quite ambigous


>
>> [...]
>>> @@ -248,9 +249,29 @@ int avcodec_default_get_buffer(AVCodecContext *s, 
>>> AVFrame *pic){
>>>          }
>>>      }
>>>
>>> +    if (s->hwaccel &&
>>> +        !ff_find_hwaccel_attribute(s, HWACCEL_ATTR_GET_PIXELS, 
>>> &alloc_pixels))
>>> +        alloc_pixels = 0; /* SW surfaces are not needed */
>>> +
>>
>> We have an existing API to let the user application choose the output
>> format (get_format())
>> your patch does this instead without explaining why the existing API is
>> not used  ...
>
> get_format() deals with pixel formats. The aim was to get rid of HW accel 
> pixel formats because some other HW accelerator will need the pixel format 
> set to e.g. NV12 and we'd still need a way to identify this HW accelerator.

If avcodec_decode_video() outputs NV12, get_format() would select NV12 and
the user application would not even need to know if that was sw or hw decoded.
if it does not output a normal pixel format then the userapp has to support
that pixel format that is support the hw decoding counterpart

thus really, a hardware accelerator that returns NV12 needs no changes at all
not even any of the previous accelerator API additions

of course a user app should be able to force sw or hw too by some means ...


>
> Besides, with this iteration of hwaccel, the pixel format for HW will have 
> the same meaning as for SW: what pixel format the user actually wants for 
> real. e.g. if he wants to get the HW decoded pixels back, pixfmt will 
> identify this format, which will be the "standard" meaning, i.e. the same 
> as in SW mode.
>
>>>      if(buf->base[0]){
>>>          pic->age= *picture_number - buf->last_pic_num;
>>>          buf->last_pic_num= *picture_number;
>>> +    }else if(s->hwaccel && !alloc_pixels){
>>> +        pic->age= 256*256*256*64;
>>> +        buf->last_pic_num= -256*256*256*64;
>>> +        memset(buf->linesize, 0, sizeof(buf->linesize));
>>> +        memset(buf->base, 0, sizeof(buf->base));
>>> +        memset(buf->data, 0, sizeof(buf->data));
>>> +        /*
>>> +         * Allocate a dummy buffer so that we don't have to worry
>>> +         * about hwaccel checks afterwards in this get_buffer() /
>>> +         * release_buffer() machinery.
>>> +         */
>>> +        buf->base[0] = av_malloc(16);
>>> +        buf->data[0] = buf->base[0];
>>> +        buf->width   = s->width;
>>> +        buf->height  = s->height;
>>> +        buf->pix_fmt = s->pix_fmt;
>>
>> i would say base/data either contain pixels or could contain some struct
>> representing a surface of the hwaccelerator
>> why is neither of these the case?
>
> data[3] does contain that struct for the HW accelerator (vaapi_context, 
> vdpau_render_state). However, for the default mode of operations, that is 
> user wants to use hwaccel for decode+display, we don't have to allocate 
> data[0-2] pixels. This saves memory in that case. However, making it NULL 
> is possible but would require further checks in 
> get_buffer()/release_buffer().

could it be that this problem arises from using [3] instead of [0] for the
struct?
(note i dont know if using [0] would not lead to more mess elsewhere ...)


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20091115/1df788d9/attachment.pgp>