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

[Edouard Gomez]

On Thu, 30 Nov 2006 03:01:40 +0100, Michael Niedermayer wrote:
> 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 ...)

Not fine grained. You see traces of revision history tool Mercurial
because i use Mercurial Queues to keep the patch up to date against
vanilla ffmpeg. Think of mercurial queues like quilt on top of a mercurial
repository.

But i can provide a series of files getting all public post to this list
and my initial "takeover" i sent to clemens. This would be good for
x11grab.c. Let's just commit the final state for configure and
allformats.c.

> [...]
>> +    int frame_format;
> 
> unused?

Yep, cleaned away with the fix for the next bit.

> [...]
>> +    int frame_rate;
>> +    int frame_rate_base;
> 
> the code could be simplifed if this would be a AVRational

Done

>> +    int mouse_wanted;

Removed

> [...]
>> +    dpy = XOpenDisplay(NULL);
>> +    if(!dpy) {
>> +        goto fail;
> 
> theres just 1 way to reach fail: and thats this one so please remove
> that ugly goto

Damn quite ugly, i didn't even notice that. Moved code.

> %sfound\n", use_shm ? "" : "not ");
> 
> needs a few bytes less space :)

Done

> could you align all these so (if conditions) 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 ) {
> 

Done.

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

Done
 
>> +    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)

Done.

> [...] Other stuff pointed by M.N

Done.

>> +        uint8_t *im_data = (uint8_t*)image->data;
> 
> if data is void then the cast isnt needed

Nope, i have the keep this cast, pointers differ in signedness.

> [...] Other stuff pointed by M.N

Done

> 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 ...)

Doxyfied the complete file. Hope you don't mind me using Javadoc style.
Doxygen handles it correctly when doing at ffmpeg root:
$ doxygen Doxyfile

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

It was to look like X11 code, but anyway, i replaced them with 0 and !0
 
> [...]
>> +    pkt->pts = curtime & ((1LL << 48) - 1);
> 
> s/ & ((1LL << 48) - 1)//

Fixed.

See the result in the RFC #6. If RFC #6 doesn't bring any new comment
against approval, i'll send a set of patches to import a not so fine
grained history of the whole RFC process.

Have fun.

-- 
Edouard Gomez