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

[Jean-Paul Saman]

techfreak wrote:
> 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: 
> http://www.digitalhome.ch/tmp_vlc/0001-Added-video-filter-plugin-RemoteOSD.patch

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 
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 
include/vlc_chroma.h)

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)

Overall comment: code looks well written.

Gtz
Jean-Paul Saman.