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

[Michael Niedermayer]

Hi

On Wed, Nov 29, 2006 at 11:53:45PM +0100, Edouard Gomez wrote:
> Hello,
> 
> Today's patch adresses comments from Diego and Michael
>  - License is GPL (pasted from gnu.org to make sure :))
>  - No tabs even between the last statement and eol.
>  - Mouse painting is done a new way matching Michael's wish.
> 
> Any other showstopper left ?

not really just more nitpicking and bikeshed issues (= iam fine with the
patch after fixing the issues below)

btw, if you do have the whole revision history of this in an easy
accessibel way (somehow i think you do ...) then checking that all in
with a script would be cool (just an idea ...)


[...]
> +    int frame_format;

unused?


[...]
> +    int frame_rate;
> +    int frame_rate_base;

the code could be simplifed if this would be a AVRational


[...]

> +    int mouse_wanted;

always 1, and the name is uhm somehow not so ideal, calling it 
inverted_pointer or inverted_mouse_pointer would match its actual use
more closely



[...]
> +    dpy = XOpenDisplay(NULL);
> +    if(!dpy) {
> +        goto fail;

theres just 1 way to reach fail: and thats this one so please remove that
ugly goto


[...]
> +    av_log(s1, AV_LOG_INFO, "shared memory extension %s\n", use_shm ? "found" : "not found");

%sfound\n", use_shm ? "" : "not ");

needs a few bytes less space :)


[...]
> +        if ( image->red_mask == 0xF800 && image->green_mask == 0x07E0
> +             && image->blue_mask == 0x1F ) {
> +            av_log (s1, AV_LOG_DEBUG, "16 bit RGB565\n");
> +            input_pixfmt = PIX_FMT_RGB565;
> +        } else if ( image->red_mask == 0x7C00 &&
> +                    image->green_mask == 0x03E0 &&
> +                    image->blue_mask == 0x1F ) {

could you align all these so they are more readable, something like:

        if (        image->  red_mask == 0xF800 &&
                    image->green_mask == 0x07E0 &&
                    image-> blue_mask == 0x001F ) {
            av_log (s1, AV_LOG_DEBUG, "16 bit RGB565\n");
            input_pixfmt = PIX_FMT_RGB565;
        } else if ( image->  red_mask == 0x7C00 &&
                    image->green_mask == 0x03E0 &&
                    image-> blue_mask == 0x001F ) {


[...]
> +        *dst = (or) ? 1 : 0;

!!or



[...]
> +    static const uint16_t const mousePointerBlack[] =
> +        {
> +            0, 49152, 40960, 36864, 34816,
> +            33792, 33280, 33024, 32896, 32832,
> +            33728, 37376, 43264, 51456, 1152,
> +            1152, 576, 576, 448, 0
> +        };
> +
> +    static const uint16_t const mousePointerWhite[] =
> +        {
> +            0, 0, 16384, 24576, 28672,
> +            30720, 31744, 32256, 32512, 32640,
> +            31744, 27648, 17920, 1536, 768,
> +            768, 384, 384, 0, 0

i do think this would look nicer if they where in hex and aligned
(yes thats just an idea, patch wont be rejected because of this)


> +        };
> +
> +    int x_off = s->x_off;
> +    int y_off = s->y_off;
> +    int width = s->width;
> +    int height = s->height;
> +
> +    if (   (*x - x_off) >= 0 && *x < (width + x_off)
> +           && (*y - y_off) >= 0 && *y < (height + y_off) ) {

if (   *x - x_off >= 0 && *x < width  + x_off
    && *y - y_off >= 0 && *y < height + y_off ) {

is more readable


> +        int line;
> +        uint8_t *im_data = (uint8_t*)image->data;

if data is void then the cast isnt needed


[...]
> +        /* Select correct masks and pixel size */
> +        switch (image->bits_per_pixel) {
> +        case 32:
> +            masks = (image->red_mask|image->green_mask|image->blue_mask);
> +            onepixel = 4;
> +            break;
> +        case 24:
> +            /* XXX: Though the code seems to support 24bit images, the
> +             * apply_masks lacks support for 24bit */
> +            masks = (image->red_mask|image->green_mask|image->blue_mask);
> +            onepixel = 3;
> +            break;
> +        case 16:
> +            masks = (image->red_mask|image->green_mask|image->blue_mask);
> +            onepixel = 2;
> +            break;
> +        case 8:
> +            masks = 1;
> +            onepixel = 1;
> +            break;
> +        default:
> +            /* Shut up gcc */
> +            masks = 0;
> +            onepixel = 0;
> +        }

if(image->bits_per_pixel==8)
    masks=1;
else
    masks= image->red_mask|image->green_mask|image->blue_mask;
onepixel= image->bits_per_pixel/8;




> +
> +        /* Shift to right line */
> +        im_data += (image->bytes_per_line * (*y - y_off));
> +        /* Shift to right pixel */
> +        im_data += (image->bits_per_pixel / 8 * (*x - x_off));

s#image->bits_per_pixel / 8#onepixel#
and s/onepixel/bytes_per_pixel/
and optioanlly remove the superflous () everywhere


> +
> +        /* Draw the cursor - proper loop */
> +        for (line = 0; line < min(20, (y_off + height) - *y); line++) {

s/min/FFMIN/


> +            uint8_t *cursor = im_data;
> +            int width_cursor;
> +            uint16_t bm_b;
> +            uint16_t bm_w;
> +
> +            bm_b = black[line];
> +            bm_w = white[line];
> +
> +            for (width_cursor=0;
> +                 width_cursor < 16 && (width_cursor + *x) < (width + x_off);

width_cursor < FFMIN(16, x_off + width - *x);

this also matches the line loop above
and maybe replace width_cursor by some more sane name



[...]
> +/*
> + * just read new data in the image structure, the image
> + * structure inclusive the data area must be allocated before
> + */

not doxygen compatible (this is not acceptable, all comments for functions,
structs and fields of structs must be doxygen compatible, and more such
comments are also always welcome ...)


> +static int
> +XGetZPixmap(Display *dpy, Drawable d, XImage *image, int x, int y)
[...]
> +
> +    if (!image) {
> +        return False;

please replace False & True with 0 and 1 unless the first are needed by some
API


[...]
> +    pkt->pts = curtime & ((1LL << 48) - 1);

s/ & ((1LL << 48) - 1)//


[...]
-- 
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