[Ffmpeg-devel] [RFC #5] X11 device demuxer (synced svn 2006-11-29)
Michael Niedermayer
michaelni
Thu Nov 30 03:01:40 CET 2006
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
More information about the ffmpeg-devel
mailing list