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

[techfreak]

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.
>
> +/* 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.
>
> +#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?

> 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?
>
> +    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.
> 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.
> +    strcpy(pv, "RFB 003.003");
> What's the size of pv ?
>
>   
The size is 13, defined in remoteosd_rfbproto.h
> +
> +    #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.
> The threads should call vlc_object_release on themselves before
> returning
>
>   
Ok, I can do this.
> ------------------------------------------------------------------------
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> http://mailman.videolan.org/listinfo/vlc-devel
>