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

[Michael Niedermayer]

On Fri, Oct 29, 2010 at 02:52:41PM +0200, Gwenole Beauchesne wrote:
> Hi,
>
> Here is a new patch that applies to current SVN tree with some further  
> cosmetics (comments) added.
>
> Regards,
> Gwenole.
>
> 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.

the user can call the default get/release buffer. And if the user doesnt know
how to allocate the surface what is he doing with the functions thats their
purpose. Your explanations are
so incomplete and terse that i cannot really comment. Because i do not know
what problem this solves or where this problem would arrise

Also the code works currently AFAIK and you do not explain what these functions
add on top of that.
Also they are not used in your patch so one can also not see it from the code
what they would be usefull for.

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

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101029/a743d4a0/attachment.pgp>