[vlc-devel] [PATCH] Added video-filter RemoteOSD
techfreak
smile4you at gmx.ch
Sat Mar 22 17:38:23 CET 2008
Hello Rafaël
Can you please take a look again on my patched patch? I hope I changed
all as you wated me to change.
I could not yet use libgcrypt to encrypt the password because I get a
linker error then. May be you can give me an idea on how to modify
configure.ac.
The mail with attached patch was too large for the mailing-list. You can download it from here:
http://www.digitalhome.ch/tmp_vlc/patches_RemoteOSD.tar.gz
Regarda, Matthias
Rafaël Carré schrieb:
> 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 ;)
>
>
> ------------------------------------------------------------------------
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> http://mailman.videolan.org/listinfo/vlc-devel
>
More information about the vlc-devel
mailing list