[vlc-devel] [PATCH] Needs 2nd Reviewer: Added RemoteOSD plugin, a VNC-Client as video-filter
jean-paul.saman at planet.nl
Mon Apr 7 14:44:59 CEST 2008
> Hello @all
> Funman already reviewed my plugin and I made all the changes he asked for.
> Now I am waiting for a 2nd reviewer who is familar with video-filters,
> because funman won't commit the patch without second reviewer.
> Please take a look at it:
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.
line 830: explicitly check VLC_EGENERIC (or <0) here.
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
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
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
Overall comment: code looks well written.
More information about the vlc-devel