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

[Michael Niedermayer]

On Wed, Mar 11, 2009 at 06:45:28AM +0100, Gwenole Beauchesne wrote:
> 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...

yes but why?
also the function name is simply too long

"vaapi_render_state_fini"

and fini is not a english word, not that i mind personally, i understand it
but maybe others do not.


> 
> >> +/**
> >> + * 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. ;-)

with your kind of proof math would be funny.


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

if its "global" and not surface specific then it also doesnt belong
in AVFrame
a field in AVCodecContext would make more sense


[...]
> > [...]
> >> +/** 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...

it would be a matter of an automatic search and replace

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

I know you won't believe me, but the highest form of Human Excellence is
to question oneself and others. -- Socrates
-------------- 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/20090311/765d3f0c/attachment.pgp>