[Ffmpeg-devel] [RFC #4] X11 device demuxer (synced svn 2006-11-28)

[Michael Niedermayer]

Hi

On Tue, Nov 28, 2006 at 10:22:46PM +0100, Edouard Gomez wrote:
> Problems addressed in this #4th review:
>  - configure style, boolean operators alignment
>  - untabified/tab widthified=4/line ending cleaned
>  - License is defintely GPL according to primary code author
>    He sees no benefit for the work he does on xvidcap to license the
>    code under the LGPL. I'm just a messenger and respects his choice
>    I mark this item as Done in the Todo list.
>  - DEMUXER registration rigthly placed in allformats.c
> 
> So please fire up your next comments for code inclusion approval.


[...]

> +#define DRAW_CURSOR_TEMPLATE(type_t)									\
> +    do {                                                                \
> +        type_t *cursor;                                                 \
> +        int width_cursor;                                               \
> +        uint16_t bm_b, bm_w, mask;                                      \
> +																		\
> +        for (line = 0; line < min(20, (y_off + height) - *y); line++ ) { \
> +            if (s->mouse_wanted == 1) {                                 \
> +                bm_b = mousePointerBlack[line];                         \
> +                bm_w = mousePointerWhite[line];                         \
> +            } else {                                                    \
> +                bm_b = mousePointerWhite[line];                         \
> +                bm_w = mousePointerBlack[line];                         \
> +            }                                                           \
> +            mask = ( 0x0001 << 15 );                                    \
> +																		\
> +            for (cursor = (type_t*) im_data, width_cursor = 0;          \
> +                 ((width_cursor + *x) < (width + x_off) && width_cursor < 16); \
> +                 cursor++, width_cursor++) {                            \
> +                if ( ( bm_b & mask ) > 0 ) {                            \
> +                    *cursor &= ((~ image->red_mask) & (~ image->green_mask) & (~image->blue_mask )); \
> +                } else if ( ( bm_w & mask ) > 0 ) {                     \
> +                    *cursor |= (image->red_mask | image->green_mask | image->blue_mask ); \
> +                }                                                       \
> +                mask >>= 1;                                             \
> +            }                                                           \
> +            im_data += image->bytes_per_line;                           \
> +        }                                                               \
> +    } while (0)

the code contains tabs, which are forbidden in svn, also it duplicates code
(after preprocessing) and this is not speedcritical stuff ...
something like the following would be better IMHO:

apply_masks(void *dst, int and, int or, int bits_per_pixel){
    switch(bits_per_pixel){
    case 16:
        *(uint16_t*)dst = *(uint16_t*)dst & and | or; break;
    ...
    }
}

masks= image->red_mask | image->green_mask | image->blue_mask;
for
    ...
    for
        ...
        apply_masks(cursor, ~(masks*(bm_b&1)), masks*(bm_w&1), bits_per_pixel);
        bm_b>>=1;
        bm_w>>=1;




[...]
> +    if (av_new_packet(pkt, s->frame_size) < 0) {
> +        return AVERROR_IO;
> +    }
> +
> +    pkt->pts = curtime & ((1LL << 48) - 1);

what is the & ((1LL << 48) - 1) good for?

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

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is