[vlc-devel] [PATCH] Added video-filter RemoteOSD

[Rafaël Carré]

On Sun, 2008-03-16 at 23:31 +0100, techfreak wrote:
> Hello Rafaël
> 
> Thank you for the review.  I answered your comments and added some 
> questions below.
> 
> I will fix what you don't  like and post the better patch again.
> 
> Regards, Matthias (techfreak) 
> 
> 
> Rafaël Carré schrieb:
> > On Sat, 2008-03-15 at 23:11 +0100, techfreak wrote:
> >   
> >> Hello all
> >>
> >> 10 days ago I already sent a patch with my new RemoteOSD video-filter 
> >> plugin and I didn't get any answer.
> >>
> >>     
> >
> > I don't remember seeing your patch Oo
> >
> >   
> No problem. Now you saw it. But I just began to think I am not welcome here.

Looking in the archives it is there :
http://mailman.videolan.org/pipermail/vlc-devel/2008-March/040334.html
But understand we are a few and there is a lot of mail coming here ..
> > +/* some defines needed in the file rfbproto.h included below
> > + * rfbproto.h is the original headerfile of the RFB protocol from VNC */
> > +#define CARD8  uint8_t  
> > +#define CARD16 uint16_t
> > +#define CARD32 uint32_t  
> > +#define INT8   int8_t  
> > +#define INT16  int16_t
> > +#define INT32  int32_t
> >
> > I don't see them being used anywhere ?
> >   
> They are used in the structures in remoteosd_rfbproto.h. I use these 
> structures to decode and encode messages from RFB protocol.

CARD* are here, but not INT*

> > +#include "remoteosd_d3des.h"    /* DES encryption library for password */ 
> >
> > We already use libgcrypt, no need to reinvent the wheel ^^
> >
> > +    add_string( RMTOSD_CFG "password", "", NULL, RMTOSD_PASSWORD_TEXT,
> > +        RMTOSD_PASSWORD_LONGTEXT, VLC_FALSE );
> >
> >   
> Where can I find a sample where libgcrypt is used to encode a string?

I think libgcrypt works on arbitrary data, so strings are the same.
However I couldn't find any tutorial when working with it :(
There is the reference manual :
http://66.102.9.104/search?q=cache:pEc94euYsq8J:www.cse.psu.edu/~tjaeger/cse497b-s07/projects/gcrypt.pdf+des+libgcrypt+api&hl=nl&ct=clnk&cd=4&gl=nl&client=firefox-a
You can push this for another time, and comment some code while putting
something like:
/* FIXME: implement DES coding of the password */


> > You can use add_password(), which will hide the password from the UI, but not from the config file
> >
> >   
> Ok, I will do this.
> 
> > +    p_sys->b_worker_thread_created = VLC_FALSE;
> > +    p_sys->b_update_request_thread_created = VLC_FALSE;
> > +    p_sys->b_worker_thread_running = VLC_FALSE;
> > +    p_sys->b_update_request_thread_running = VLC_FALSE;
> > +    p_sys->b_connection_active = VLC_FALSE;
> > ..
> > +    p_sys->i_vncWidth = 0; 
> > +    p_sys->i_vncHeight = 0; 
> >
> > and others
> >
> > they were already set to VLC_FALSE / 0 / NULL by the
> > memset(p_sys,0,sizeof(p_sys));
> >   
> Ok, I will remove  these lines  initializing  to  VLC_FALSE,  0 or NULL.
> > +    if( (p_sys->psz_host == NULL) ||
> > +        (*p_sys->psz_host == '\0') )
> >
> > if( !p_sys->psz_host || !*p_sys->p_host ) is shorter
> >   
> Yes it is shorter, but is it better readable?

Well for me at least ^^ , I became used to this form of check.
There is even a macro for it: EMPTY_STR() in vlc_common.h

> > +    if( p_sys->psz_host ) free( p_sys->psz_host );
> > +
> > +    if( p_sys ) free( p_sys );
> >
> > Do you want ivoire to kill you ? :)
> > if( xx ) free ( xx ); is useless, free() does check already (try
> > free(NULL));
> >   
> Ok, sometimes I am too careful. I really don't like nullpointer 
> exceptions, but if free checks it first, then it's not needed.
> > and you are leaking psz_host, psz_passwd, and do not kill/detach/release
> > vnc_worker_thread()
> > plus you don't detach the callbacks, and release p_vout
> >   
> I will fix this.
> > +    int i_countdown_wait_thread_end = 100;
> > +    while ( i_countdown_wait_thread_end > 0 && 
> > +            ( p_sys->b_worker_thread_running == VLC_TRUE ||
> > +              p_sys->b_update_request_thread_running == VLC_TRUE )
> > +          )
> > +    {
> > +        i_countdown_wait_thread_end--;
> > +        Sleep(100);
> > +    }
> >
> > This looks ugly !
> > You should use vlc_thread_join() your threads and wait for proper
> > completion, and I don't know what Sleep() is (and don't want to know)
> >
> > Please use lowercase for variable names, to be consistent with VLC's
> > code
> >
> >   
> I will look what vlc_thread_join does and will try to use it. The loop 
> with Sleep() does something similar but join should be cleaner.

vlc_thread_join() blocks its execution until the joined thread has
finished its execution.
If you really need to sleep, there is msleep() which takes a mtime_t
argument (int64_t as microseconds).

> > What are read_exact() and write_exact() for exactly ? looks useless to
> > me.
> > 
> Read_exact and write_exact do check if the return-value of net_read and 
> net_send match the expected size and return a boolean value.

Ok

> > +    strcpy(pv, "RFB 003.003");
> > What's the size of pv ?
> >
> >   
> The size is 13, defined in remoteosd_rfbproto.h

Please use strncpy(), even with a known size.

> > +    #define rfbEncSpecialUseAlpha 0xFFFFF000
> > +    msg_Dbg( p_filter, "Writing SetEncodings rfbEncSpecialUseAlpha" );
> > +    i_encoding = Swap32(rfbEncSpecialUseAlpha);
> > You don't need to use Swap32() if you already know the result
> >
> >   
> I think this is more readable than define it directly inverted. I don't 
> want to mixup the OSI layers. The network wants the bytes swapped, but I 
> want to fill it in real order first.

Maybe you can use something like:
i_encoding = 0x00F0FFFF; /* rbEncSpecialUseAlpha is 0xFFFFF000
                          * before we swap it. */

> > The threads should call vlc_object_release on themselves before
> > returning
> >
> >   
> Ok, I can do this.

Good luck ;)

-- 
Rafaël Carré <funman at videolan.org>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20080317/411a79c6/attachment.sig>