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

[Matthias Bauer]

Hello Jean-Paul

Did you miss this mail from me? I would like to hear if you agree with 
the changes I made on your request.

Regards, Matthias


Matthias Bauer schrieb:
> 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
>>
>>   
>
>