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

[techfreak]

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.
>