[FFmpeg-devel] XVMC Deathmatch

[Michael Niedermayer]

Hi 

The review is below

(due to 1-10 points)
-10 points (for each case)
breaking compilation, regtests or the code in question if the other
gladiator notices the breakage and post a note to ffmpeg-cvslog

-10 points (for each case)
obstructing the others work or not spliting changes half sanely


[...]

>#include <X11/Xlib.h>
>#include <X11/Xutil.h>
>#include <X11/Xatom.h>
>#include <X11/extensions/Xv.h>
>#include <X11/extensions/Xvlib.h>
>#include <X11/extensions/XvMClib.h>

10 Points:
Get rid of these includes so that the header can be included
unconditionally that is without requireing X11 headers


>
>
>//the surface should be shown, the video driver manipulates this
>#define MP_XVMC_STATE_DISPLAY_PENDING 1

10 Point:
change all comments about defines, structs, functions & struct members to
doxygen comments


>//the surface is needed for prediction, the codec manipulates this
>#define MP_XVMC_STATE_PREDICTION 2
>//this surface is needed for subpicture rendering
>#define MP_XVMC_STATE_OSD_SOURCE 4
>//                     1337    IDCT MCo
>#define MP_XVMC_RENDER_MAGIC 0x1DC711C0
>

4 points
replace MP prefixes by AV/FF depending on public/private and ensure that
compatibility is provided until the next major ver bump but not beyond

>struct xvmc_render_state {

1 point
rename this to something with pix_fmt in its name


>    //these are not changed by the decoder!
>    int  magic;

3 points
Give this field and related constants a descriptive name and if needed
a comment

3 points
find and get rid of or rename fields to "ununsed123" all unused fields in this
struct, dont break ABI!


>
>    short * data_blocks;
>    XvMCMacroBlock * mv_blocks;
>    int total_number_of_mv_blocks;
>    int total_number_of_data_blocks;

>    int mc_type; //XVMC_MPEG1/2/4,XVMC_H263 without XVMC_IDCT



>    int idct; //Do we use IDCT acceleration?

2 point
fix this comment to be an awnser to "what is this field?" or at
least dont write it like a question



>    int chroma_format; //420, 422, 444

2 point clarify what this contains, i suspect its not the litteral numbers
above


>    int unsigned_intra; //+-128 for intra pictures after clipping
>    XvMCSurface* p_surface; //pointer to rendered surface, never changed

5 point.
vertically align all comments and related statements, related statements are
for example:
assert(render->filled_mv_blocks_num <= render->total_number_of_mv_blocks);
assert(render->next_free_data_block_num <= render->total_number_of_data_blocks);
to
assert(render->filled_mv_blocks_num     <= render->total_number_of_mv_blocks  );
assert(render->next_free_data_block_num <= render->total_number_of_data_blocks);



>
>    //these are changed by the decoder
>    //used by the XvMCRenderSurface function
>    XvMCSurface* p_past_surface; //pointer to the past surface
>    XvMCSurface* p_future_surface; //pointer to the future prediction surface
>

>    unsigned int picture_structure; //top/bottom fields or frame!
>    unsigned int flags; //XVMC_SECOND_FIELD - 1st or 2nd field in the sequence
>    unsigned int display_flags; //1,2 or 1+2 fields for XvMCPutSurface

>
>    //these are for internal communication

2 points, communication between what and what? (yes add it to the comment)


>    int state; //0 - free, 1 - waiting to display, 2 - waiting for prediction

3 points:
This field seems to have stuff or/anded into it thus above appears
oversimplified, fix this


>    int start_mv_blocks_num; //offset in the array for the current slice, updated by vo
>    int filled_mv_blocks_num; //processed mv block in this slice, changed by decoder
>
>    int next_free_data_block_num; //used in add_mv_block, pointer to next free block
>    //extensions
>    void * p_osd_target_surface_render; //pointer to the surface where subpicture is rendered

10 points
clarify for each field if its set by decoder or vo like user vs lavc is
in AVCodecContext in avcodec.h

[...]

>//set s->block
>void XVMC_init_block(MpegEncContext *s){
>    struct xvmc_render_state * render;
>    render = (struct xvmc_render_state*)s->current_picture.data[2];

>    assert(render != NULL);
>    if( (render == NULL) || (render->magic != MP_XVMC_RENDER_MAGIC) ){
>        assert(0);
>        return;//make sure that this is a render packet
>    }

2 points
get rid of all != NULL and == NULL and replace by x / !x

3 points get rid of the double assert(x) & if(!x) checks
either its an assert or a error condition but not both

10 points, implement
av_assert_fast()
av_assert_security()
av_assert()

10 points
change each in xvmc to use the correct av_assert*()

10 points
add a AVHWAccel struct as described in the VAAPI thread with register function
and all

10 points make xvmc use that struct



>    s->block =(DCTELEM *)(render->data_blocks+(render->next_free_data_block_num)*64);
>}
>

>void XVMC_pack_pblocks(MpegEncContext *s, int cbp){

2 points
give all private global functions a ff_ prefix or make them static

5 point
give all public global functions a av_ prefix
and make sure you keep ABI compatibility until but not after the next major
bump


>int i,j;
>const int mb_block_count = 4+(1<<s->chroma_format);

2 points
indent all the function local variables like the code following them

>

>    j=0;

2 points, merge all the init & declarations where possible


>    cbp<<= 12-mb_block_count;
>    for(i=0; i<mb_block_count; i++){
>        if(cbp & (1<<11)) {
>           s->pblocks[i] = (short *)(&s->block[(j++)]);

2 points
fix all the not a multiple by 4 indention


>        }else{
>           s->pblocks[i] = NULL;
>        }
>        cbp+=cbp;

>//        printf("s->pblocks[%d]=%p ,s->block=%p cbp=%d\n",i,s->pblocks[i],s->block,cbp);

1 points
get rid of all printf


>    }
>}
>
>//These functions should be called on every new field and/or frame.
>//They should be safe if they are called a few times for the same field!
>int XVMC_field_start(MpegEncContext*s, AVCodecContext *avctx){
>    struct xvmc_render_state * render, * last, * next;
>
>    assert(avctx != NULL);
>
>    render = (struct xvmc_render_state*)s->current_picture.data[2];
>    assert(render != NULL);
>    if( (render == NULL) || (render->magic != MP_XVMC_RENDER_MAGIC) )
>        return -1;//make sure that this is render packet
>
>    render->picture_structure = s->picture_structure;

>    render->flags = (s->first_field)? 0: XVMC_SECOND_FIELD;

3 point
remove all useless () except where it helps readability

[...]
>void XVMC_field_end(MpegEncContext *s){
>    struct xvmc_render_state * render;
>    render = (struct xvmc_render_state*)s->current_picture.data[2];
>    assert(render != NULL);
>
>    if(render->filled_mv_blocks_num > 0){
>//        printf("xvmc: rendering %d left blocks after last slice!!!\n",render->filled_mv_blocks_num );
>        ff_draw_horiz_band(s,0,0);

5 points
document what ff_draw_horiz_band() that is supposed to be a call back
to render rows does in a hwaccel


[...]
>            case  MV_TYPE_DMV:
>                mv_block->motion_type = XVMC_PREDICTION_DUAL_PRIME;
>                if(s->picture_structure == PICT_FRAME){
>
>                    mv_block->PMV[0][0][0] = s->mv[0][0][0];//top from top
>                    mv_block->PMV[0][0][1] = s->mv[0][0][1]<<1;
>
>                    mv_block->PMV[0][1][0] = s->mv[0][0][0];//bottom from bottom
>                    mv_block->PMV[0][1][1] = s->mv[0][0][1]<<1;
>
>                    mv_block->PMV[1][0][0] = s->mv[0][2][0];//dmv00, top from bottom
>                    mv_block->PMV[1][0][1] = s->mv[0][2][1]<<1;//dmv01
>
>                    mv_block->PMV[1][1][0] = s->mv[0][3][0];//dmv10, bottom from top
>                    mv_block->PMV[1][1][1] = s->mv[0][3][1]<<1;//dmv11
>
>                }else{

>                    mv_block->PMV[0][1][0] = s->mv[0][2][0];//dmv00
>                    mv_block->PMV[0][1][1] = s->mv[0][2][1];//dmv01

1 point
copy both at once


>                }
>                break;
>            default:
>                assert(0);
>        }
>
>        mv_block->motion_vertical_field_select = 0;
>
>//set correct field references
>        if(s->mv_type == MV_TYPE_FIELD || s->mv_type == MV_TYPE_16X8){

>            if( s->field_select[0][0] ) mv_block->motion_vertical_field_select|=1;
>            if( s->field_select[1][0] ) mv_block->motion_vertical_field_select|=2;
>            if( s->field_select[0][1] ) mv_block->motion_vertical_field_select|=4;
>            if( s->field_select[1][1] ) mv_block->motion_vertical_field_select|=8;

2 point
get rid of the branch prediction hurting if()


>        }
>    }//!intra
>//time to handle data blocks;
>    mv_block->index = render->next_free_data_block_num;
>
>    blocks_per_mb = 6;
>    if( s->chroma_format >= 2){
>        blocks_per_mb = 4 + (1 << (s->chroma_format));
>    }
>
>//  calculate cbp
>    cbp = 0;
>    for(i=0; i<blocks_per_mb; i++) {
>        cbp+= cbp;
>        if(s->block_last_index[i] >= 0)
>            cbp++;
>    }
>
>    if(s->flags & CODEC_FLAG_GRAY){
>        if(s->mb_intra){//intra frames are always full chroma block
>            for(i=4; i<blocks_per_mb; i++){
>                memset(s->pblocks[i],0,sizeof(short)*8*8);//so we need to clear them
>                if(!render->unsigned_intra)
>                    s->pblocks[i][0] = 1<<10;
>            }
>        }else{
>            cbp&= 0xf << (blocks_per_mb - 4);
>            blocks_per_mb = 4;//luminance blocks only
>        }
>    }
>    mv_block->coded_block_pattern = cbp;
>    if(cbp == 0)
>        mv_block->macroblock_type &= ~XVMC_MB_TYPE_PATTERN;
>
>    for(i=0; i<blocks_per_mb; i++){
>        if(s->block_last_index[i] >= 0){
>            // I do not have unsigned_intra MOCO to test, hope it is OK.
>            if( (s->mb_intra) && ( render->idct || (!render->idct && !render->unsigned_intra)) )
>                s->pblocks[i][0]-=1<<10;
>            if(!render->idct){
>                s->dsp.idct(s->pblocks[i]);

>                //!!TODO!clip!!!

5 points
clarify what has to be done


>            }
>//copy blocks only if the codec doesn't support pblocks reordering
>            if(s->avctx->xvmc_acceleration == 1){

>                memcpy(&render->data_blocks[(render->next_free_data_block_num)*64],
>                        s->pblocks[i],sizeof(short)*8*8);

2 point
change all sizeof(short/int..)*123 to more robust sizeof(some_array)
where possible


>            }else{

>/*              if(s->pblocks[i] != &render->data_blocks[
>                        (render->next_free_data_block_num)*64]){
>                   printf("ERROR mb(%d,%d) s->pblocks[i]=%p data_block[]=%p\n",
>                   s->mb_x,s->mb_y, s->pblocks[i],
>                   &render->data_blocks[(render->next_free_data_block_num)*64]);
>                }*/

2 points
get rid of all outcommenetd code

>            }
>            render->next_free_data_block_num++;
>        }
>    }
>    render->filled_mv_blocks_num++;
>

>    assert(render->filled_mv_blocks_num <= render->total_number_of_mv_blocks);
>    assert(render->next_free_data_block_num <= render->total_number_of_data_blocks);

5 points
replace all assert() checks on all user settable fields by
normal if() av_log() whatever else is needed checks


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

He who knows, does not speak. He who speaks, does not know. -- Lao Tsu
-------------- 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/20090214/34a6d534/attachment.pgp>