[XviD-devel] About dev-api-4

[suxen_drol]

On Fri, 7 Mar 2003 19:11:49 +0100 Edouard Gomez <ed.gomez at free.fr> wrote:

> I always thought that the (handle, operation, param1, param2) API was
> cool because it's simple, however i think we use the wrong way for at
> least 1 case.

> 
> If we look at xvid_dec_create_t, xvid_enc_create_t and xvid_enc_frame_t,
> we can notice that  we have an [out] field in the  structure. I think we
> should not mix  in/out fields in a  same structure if we want  to have a
> nice API. In the first 2 cases, why not use the param2 parameter ? 
> 
> It would become something like:
> 
> xvid_enc_create_t xvid_enc_create;
> void *enc_handle;
> 
> xvid_encore(NULL, XVID_ENC_CREATE, &xvid_enc_create, &enc_handle);
> 
> Same thing for the xvid_dec_create. 

seems appropriate.
 
> Now  concerning the  xvid_enc_frame_t case,  i think  we should  not mix
> input  frame and  output stream  in the  same structure  again,  nor the
> out_flags that  are already available  through the stats  structure.  In
> other words  i think we  are mixing lot of  concepts/parameters together
> and  we  should not...   image  should  contain  all image  informations
> including width and  height, quant matrices and par/par_width/par_height
> should be part of vol structure, type/quant/bquant should be part of the
> RC structure (i'm guilty for this one because i'm late at my RC merge)

out_flags and the encoded bitstream length (which is now the return
value of encore) are closely related. iam not sure where else to placeit.

it could be moved into xvid_enc_stats_t. though i intended to make
xvid_enc_stats_t strictly "encoded-frame-information" only. (out_flags
related to "encoded-bitstream-chunk-information").

> Perhaps my OO courses at school  are slowly modeling my mind but i think
> the actual structures are rather badly organized and inter dependent. 

okay. i dont disagree with that, but the we must leverage simplicity
versus elegance here.
> 
> Perhaps we should do this that way:
> 
> typedef struct {
> 
>     int version;
> 
>     xvid_vol_header_t vol_header;
>     xvid_vop_header_t vop_header;
>     xvid_motion_t motion_flags;
>     xvid_image_t input;
> 
> }xvid_enc_frame_t;
> 
> typedef struct {
>     int csp;
>     int width;
>     int height;
>     void * plane[4];
>     int stride[4];
> }xvid_image_t;

^^ placing width/height inside xvid_image_t had crossed my mind. for the
encoder, do we ignore the image.width/height fields? or use them to
allocate the buffers upon the first XVID_ENC_ENCODE call?
> 
> And where  xvid_vol_header_t and xvid_vop_header_t are more  or less the
> anonymous structures  defined in xvid_dec_stats_t  + some more  stuff in
> it. 

i'd prefer we declare the xvid_vol/vop/motion structure inside the
xvid_enc_frame_t. there structs on only valid inside xvid_enc_frame_t.
> 
> The resulting code should look far more "logic" as structures will group
> all related infos to the "object" they are supposed to represent. 
> 
> Just my  first closer look at  this new API,  as we are breaking  lot of
> things, i think it's the good time to do it better (a bit more than pete
> already did) :-)

i didnt want to go drift too far away from the original api, because
that requires lots of internal changes.

alternatively we could drop encore/decore altogether, and use provide
some direct functions:

int xvid_enc_create(xvid_enc_create_t *, void *);
int xvid_enc_destroy(void *)
int xvid_enc_create(void *, xvid_enc_frame_t *, xvid_enc_stats_t *, int out_flags)

-- pete