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

[Rafaël Carré]

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

> I hope I will get anaswer  this time.
> 
> Here a short description of the plugin:

>  * Using password protected VNC hosts is supported but not
> recommended, 
> because
>  * you need to insert the used password in the plugin configuration page of
>  * VLC configuration in plain text and it's saved in plain text.

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


+#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 );

You can use add_password(), which will hide the password from the UI, but not from the config file


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

+    if( (p_sys->psz_host == NULL) ||
+        (*p_sys->psz_host == '\0') )

if( !p_sys->psz_host || !*p_sys->p_host ) is shorter


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

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

+    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


What are read_exact() and write_exact() for exactly ? looks useless to
me.

+    strcpy(pv, "RFB 003.003");
What's the size of pv ?

+
+    #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


The threads should call vlc_object_release on themselves before
returning

-- 
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/20080315/c077b1e4/attachment.sig>