[vlc-devel] [PATCH] Needs 2nd Reviewer: Added RemoteOSD plugin, a VNC-Client as video-filter
smile4you at gmx.ch
Thu Apr 17 22:23:11 CEST 2008
I made most of your requested changes and have still some comments and
questions. Please see below.
Jean-Paul Saman wrote:
> line 810: check malloc return value
> if( !p_sys->pic )
> // handle error case
> line 821: remove msg_Err() because it causes a memory allocation and the
> reason for failure here is lack of memory
> 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).
> 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.
> line 1242-1249: no need to lock here.
> line 1250: lock needed here.
> line 1247, 1255, 1263:
> Use return NULL iso return 0. (It is different!!!)
> 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
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))
> line 1548 - 1557: put these in a #ifdef VNC_DEBUG .. #endif
> line 1572 - 1583: ^^^
> 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.
> Jean-Paul Saman.
More information about the vlc-devel