[vlc-devel] [PATCH] Needs 2nd Reviewer: Added RemoteOSD plugin, a VNC-Client as video-filter

[Matthias Bauer]

Hello @all

I just updated my patch containing the changes below as requested from Jean-Paul Saman.

http://www.digitalhome.ch/tmp_vlc/0001-Added-plugin-RemoteOSD-a-VNC-client-as-video-filter.patch.gz

I would be grateful for any comments.

Regards

"techfreak" Matthias Bauer
 


techfreak wrote:
> Hi Jean-Paul
>
> I made most of your requested changes and have still some comments and 
> questions. Please see below.
>
> Regards, Matthias
>
>
> Jean-Paul Saman wrote:
>
>   
>> line 810: check malloc return value
>> if( !p_sys->pic )
>> {
>>     // handle error case
>> }
>>   
>>     
> Done.
>   
>> line 821: remove msg_Err() because it causes a memory allocation and the 
>> reason for failure here is lack of memory
>>   
>>     
> Done.
>   
>> line 827: why launch update thread from vnc_worker_thread()? Since you 
>> only attach it to p_sys,p_filter ? Please rewrite to launch it from 
>> open() function instead.
>> In fact the first part of the vnc_worker_thread() function can be done 
>> in Open(). It is run in that context anyway.
>>   
>>     
> I launch the vnc_update_thread from within vnc_worker_thread because it 
> should not start before the connection to VNC host is active and the 
> whole handshaking is done.
> And I do the first part of the vnc_worker_thread not in CreateFilter() 
> because the video output doesn't start  before CreateFilter() returns 
> and I don't want to block the video output while connecting to VNC host 
> and while handshaking with it. I even plan to try reconnecting to VNC 
> host in this vnc_worker_thread if the conection was lost or could not be 
> established (instead of terminating the thread in these situations).
>
>   
>> line 830: explicitly check VLC_EGENERIC (or <0) here.
>>   
>>     
> I don't understand. I cannot find any  vlc_thread_create in VLC source 
> where the return value is checked other than I do.
>   
>> line 860: remove msg_Dbg() here it gets printed way too often. A good 
>> hit is to only use msg_* in failure paths or in setup routines (printed 
>> one time).
>>   
>>     
> Done.
>   
>> line 786: vnc_worker_thread()
>> line 915: update_request_thread()
>> check in the while loops for object termination signals issued by vlc 
>> core: (p_thread_obj->b_die || p_thread_obj->b_error)W
>> Otherwise you'll loop continuously here.
>>   
>>     
> Done.
>   
>> line 1242-1249: no need to lock here.
>> line 1250: lock needed here.
>>   
>>     
> Done.
>   
>> line 1247, 1255, 1263:
>> Use return NULL iso return 0. (It is different!!!)
>>   
>>     
> Done.
>   
>> line 1310: why duplicate rgb_to_yuv() from 
>> modules/video_filter/blend.c:342 ? Maybe it is smart to move 
>> rgb_to_yuv() and yuv_to_rgb() into include/vlc_filter.h (or 
>> include/vlc_chroma.h)
>>   
>>     
> Should I really make changes on blend.c?.
>   
>> line 1380: swap16()
>> line 1385: swap32()
>> Why write your own swap functions? when arpa/inet.h defines them for you 
>> already(they even take care of big/little endianness for you). Consider 
>> using uint16_t htons(uint16_t) and uint32_t htonl(uint32_t) instead. 
>> (and ntohl(uint32_t), ntohs(uint16_t))
>>   
>>     
> Done..
>   
>> line 1548 - 1557: put these in a #ifdef VNC_DEBUG .. #endif
>> line 1572 - 1583: ^^^
>>   
>>     
> Done.
>   
>> 3DES files/functions should be replaced by gcrypt functionality 
>> (funman's comment)
>>   
>>     
> I will do this but I couldn't get it running until now. The result of 
> gcrypt is not the same as with the 3DES files/functions. Can you or 
> anybody in the list see whats wrong in my lines using gcrypt?
>   
>> Overall comment: code looks well written.
>>   
>>     
> Thanks, nice to hear that.
>   
>> Gtz
>> Jean-Paul Saman.
>>   
>>     
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> http://mailman.videolan.org/listinfo/vlc-devel
>
>