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

[Gwenole Beauchesne]

Le 11 mars 09 ? 02:10, Michael Niedermayer a ?crit :

>> +/** Destroy VA API render state buffers */
>> +static void vaapi_render_state_fini(struct  
>> vaapi_render_state_private *rds)
>
> the vaapi_ prefixes are still useless

vaapi_render_state is the name of the struct...

>> +/**
>> + * This structure is used as a callback between the FFmpeg
>
> callback?

Hmm, this is now proof that you did not review the VDPAU code. Either  
you did not care at all about that hwaccel or you want to delay VA API  
integration forever or you like it so much instead to be the unique  
reference HWAccel implementation. ;-)

>> + * FFmpeg decode functions. The members are considered read-only  
>> from
>> + * an FFmpeg point of view.
>> + */
>> +struct vaapi_context {
>> +    void       *display;                ///< Window system  
>> dependent data
>> +    uint32_t    config_id;              ///< Configuration ID
>> +    uint32_t    context_id;             ///< Context ID (video  
>> decode pipeline)
>> +};
>> +


>> + */
>> +struct vaapi_render_state {
>
>> +    struct vaapi_context *va_context;   ///< Pointer to display- 
>> dependent data
>
> why this pointer? why are the fields not added to this struct?

Simplification for the user to avoid copies since those don't change  
for a specific surface.

> [...]
>> +/** This structure holds all temporary data for VA API decoding */
>> +struct vaapi_render_state_private {
>
>> +    VADisplay       display;                ///< Window system  
>> dependent data (copied from vaapi_render_state->va_context)
>> +    VAContextID     context_id;             ///< Context ID (video  
>> decode pipeline, copied from vaapi_render_state->va_context)
>> +    VASurfaceID     surface;                ///< Rendering surface  
>> (copied from vaapi_render_state->va_context)
>
> copying data is generally not a good idea, its a receipe for ending up
> with different data and bugs

It's not if it's controlled from a single place, early.

> [...]
>> +/** Extract the vaapi_render_state struct from a Picture */
>> +static inline struct vaapi_render_state  
>> *ff_get_vaapi_render_state(Picture *pic)
>> +{
>> +    struct vaapi_render_state *rds = (struct vaapi_render_state  
>> *)pic->data[3];
>> +    assert(rds);
>> +    return rds;
>> +}
>> +
>> +/** Extract the vaapi_render_state_private struct from a Picture */
>> +static inline struct vaapi_render_state_private  
>> *ff_get_vaapi_render_state_private(Picture *pic)
>> +{
>> +    struct vaapi_render_state_private *rds = pic- 
>> >hwaccel_data_private;
>> +    assert(rds);
>> +    return rds;
>> +}
>
> useless wraper functions

Not if pic->data[3] is changed to another thing later...