[FFmpeg-devel] [PATCH][VAAPI][2/6] Add common data structures and helpers (take 13A -- alternate)

[Michael Niedermayer]

On Fri, Mar 20, 2009 at 03:55:40PM +0100, Gwenole Beauchesne wrote:
> Hi,
>
> On Fri, 20 Mar 2009, Michael Niedermayer wrote:
>
>> anyway the rest of the patch looks ok, you can commit it after
>> fixing these 2, but note, iam not 100% happy about the read only
>> context vs putting local vars in each Pic
>
> OK, here is an alternate version with vaapi_context only, i.e. 
> vaapi_picture_private is nuked away. It turned out MPEG-4 was the only 
> location I needed pic_param, but I noticed AVFrame had a pict_type copy. Is 
> it guaranteed to be the old s->pict_type?
>
> Really, I don't really like that approach, but it's a matter of taste I 
> believe. ;-) Reminder: vaapi.h is a public (installed) header and exposing 
> internal data doesn't really look sane to me.
>
> Anyway, I sent you both versions, just decide.

i prefer this one, a few comments below though


[...]
> +int ff_vaapi_common_end_frame(AVCodecContext *avctx)
> +{

> +    MpegEncContext * const s = avctx->priv_data;

that is not pretty, in common code


> +    struct vaapi_context * const vactx = avctx->hwaccel_context;
> +    int ret = -1;
> +
> +    dprintf(avctx, "ff_vaapi_common_end_frame()\n");
> +
> +    if (commit_slices(vactx) < 0)
> +        goto done;
> +    if (vactx->n_slice_buf_ids > 0) {
> +        if (render_picture(vactx, ff_vaapi_get_surface(s->current_picture_ptr)) < 0)
> +            goto done;

> +        ff_draw_horiz_band(s, 0, s->avctx->height);

avctx->height


[...]
> +/**
> + * This structure is used to share data between the FFmpeg library and
> + * the client video application.

> + * This is filled in by the client application prior to calling the
> + * FFmpeg decode functions. The members are considered read-only from
> + * an FFmpeg point of view. This struct shall be available as

well


> + * AVCodecContext.hwaccel_context.
> + */
> +struct vaapi_context {
> +    void           *display;                ///< Window system dependent data
> +    uint32_t        config_id;              ///< Configuration ID
> +    uint32_t        context_id;             ///< Context ID (video decode pipeline)
> +

> +    /* All fields hereunder are private and temporary FFmpeg data used
> +       for decoding. */

i think this should be written in each doxy for each var, i mean like
its done for AVCodecContext. Up there it can be easily missed

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

> ... defining _GNU_SOURCE...
For the love of all that is holy, and some that is not, don't do that.
-- Luca & Mans
-------------- 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/20090320/1424daf0/attachment.pgp>