[vlc-devel] [PATCH] Video_filter motiondetect to send shapes over UDP.
Filip Roséen
filip at atch.se
Tue Feb 7 15:49:09 CET 2017
Hi Gonzague,
These changes seems to be an iteration of a patch posted by you back
in June 2012, is that correct?
- https://patches.videolan.org/patch/413/
On 2017-02-07 15:14, Gonzague Defos du Rau wrote:
> If the option --motiondetect-port is set, detected shapes are sent on localhost over UDP.
> Packet looks like "ShapeNumber x_min x_max y_min y_max ;\n..."
>
> ---
> modules/video_filter/motiondetect.c | 63 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 63 insertions(+)
>
> diff --git a/modules/video_filter/motiondetect.c b/modules/video_filter/motiondetect.c
> index fee7e60..a13d6b8 100644
> --- a/modules/video_filter/motiondetect.c
> +++ b/modules/video_filter/motiondetect.c
> @@ -36,6 +36,7 @@
> #include <vlc_picture.h>
> #include "filter_picture.h"
>
> +#include <vlc_network.h>
> /*****************************************************************************
> * Module descriptor
> *****************************************************************************/
> @@ -44,6 +45,11 @@ static void Destroy ( vlc_object_t * );
>
> #define FILTER_PREFIX "motiondetect-"
>
> +#define PORT_TEXT N_( "motiondetect resend udp port" )
> +#define PORT_LONGTEXT N_( \
> + "resend detected boxes values over udp, "\
> + "default port is 0, that means resending is not active" )
Could be rephrased so easier explain the intent of the code.
> +
> vlc_module_begin ()
> set_description( N_("Motion detect video filter") )
> set_shortname( N_( "Motion Detect" ))
> @@ -51,6 +57,9 @@ vlc_module_begin ()
> set_subcategory( SUBCAT_VIDEO_VFILTER )
> set_capability( "video filter", 0 )
>
> + add_integer(FILTER_PREFIX "port", 0,
> + PORT_TEXT, PORT_LONGTEXT, false)
> +
> add_shortcut( "motion" )
> set_callbacks( Create, Destroy )
> vlc_module_end ()
> @@ -66,6 +75,11 @@ static int FindShapes( uint32_t *, uint32_t *, int, int, int,
> static void Draw( filter_t *p_filter, uint8_t *p_pix, int i_pix_pitch, int i_pix_size );
> #define NUM_COLORS (5000)
>
> +/* Boxes UDP resend, options */
> +static const char *const ppsz_filter_options[] = {
> + "port", NULL
> +};
> +
> struct filter_sys_t
> {
> bool is_yuv_planar;
> @@ -81,6 +95,10 @@ struct filter_sys_t
> int color_x_max[NUM_COLORS];
> int color_y_min[NUM_COLORS];
> int color_y_max[NUM_COLORS];
> +
> + /* Boxes UDP resend , socket */
> + int fd;
> + int port;
After the socket has been created there is no need for
`filter_sys_t::port`, as such you can remove the *data-member* from the
above struct.
Instead of using `p_sys->port > 0`, you can rely on the fact that
*file-descriptors* cannot be negative, and replace the former usage
with `p_sys->fd != -1` (where `-1` denotes no connection, or similar).
> };
>
> /*****************************************************************************
> @@ -115,6 +133,26 @@ static int Create( vlc_object_t *p_this )
> if( p_filter->p_sys == NULL )
> return VLC_ENOMEM;
>
> + /* Boxes UDP resend , open socket */
> + int fd;
> + config_ChainParse( p_filter, FILTER_PREFIX, ppsz_filter_options, p_filter->p_cfg );
> + int i_port = var_CreateGetIntegerCommand( p_filter, FILTER_PREFIX "port" );
Why not use `p_sys->port` all the way through, instead of having both
`i_port` and `p_sys->port`?
>
> + if( i_port > 0 )
> + {
> + fd = net_ConnectUDP( VLC_OBJECT( p_filter ), "127.0.0.1", i_port, -1 );
>
> + if (fd == -1)
> + {
> + msg_Err(p_filter, "Can not open socket");
> + i_port = 0 ;
> + p_sys->port = 0 ;
> + }
> + else
> + {
> + p_sys->fd = fd;
> + p_sys->port = i_port ;
> + }
> + }
> +
> p_sys->is_yuv_planar = is_yuv_planar;
> p_sys->b_old = false;
> p_sys->p_old = picture_NewFromFormat( p_fmt );
> @@ -144,6 +182,11 @@ static void Destroy( vlc_object_t *p_this )
> free( p_sys->p_buf2 );
> free( p_sys->p_buf );
> picture_Release( p_sys->p_old );
> +
> + /* Boxes UDP resend , close socket */
> + if( p_sys->port > 0 )
> + net_Close( p_sys->fd );
> +
> free( p_sys );
> }
>
> @@ -522,6 +565,8 @@ static void Draw( filter_t *p_filter, uint8_t *p_pix, int i_pix_pitch, int i_pix
> filter_sys_t *p_sys = p_filter->p_sys;
>
> int j = 0;
> +
> + char buffer[NUM_COLORS] = "";
Is the above guaranteed not to overflow when using `sprintf`?
At least use `snprintf` if it is not 100% clear that it will not
overflow, or make use of the functionality in `vlc_memstream.h` to
easily create strings (though beware that they will be
heap-allocated).
>
> for( int i = 1; i < p_sys->i_colors; i++ )
> {
> @@ -535,6 +580,17 @@ static void Draw( filter_t *p_filter, uint8_t *p_pix, int i_pix_pitch, int i_pix
> const int color_y_min = p_sys->color_y_min[i];
> const int color_y_max = p_sys->color_y_max[i];
>
> + /**
> + * Boxes UDP resend, building datas packet.
> + * The packet concatenate lines of boxe's number,
> + * x_min, x_max, y_min and y_max separated by a space,
> + * and ending with "space semi-colon new-line" (" ;\n")
> + */
> + if( p_sys->port > 0)
> + {
> + sprintf(buffer, "%s%d %d %d %d %d ;\n", buffer, j, color_x_min, color_x_max, color_y_min, color_y_max);
To second *Rémi* in the previously linked discussion; where is this
format specified?
> + }
> +
> if( color_x_min == -1 )
> continue;
> if( ( color_y_max - color_y_min ) * ( color_x_max - color_x_min ) < 16 )
> @@ -558,5 +614,12 @@ static void Draw( filter_t *p_filter, uint8_t *p_pix, int i_pix_pitch, int i_pix
> for( y = color_y_min; y <= color_y_max; y++ )
> p_pix[y*i_pix_pitch+x*i_pix_size] = 0xff;
> }
> +
> msg_Dbg( p_filter, "Counted %d moving shapes.", j );
> +
> + /* Boxes UDP resend, sending buffer */
The below does not look correct as there is nothing stating that the entire
data in `sizeof( buffer )` contains valid data, unless the (not
mentioned) protocol specifies that trailing null-bytes are to be sent.
It looks like you want to store the length of the generated data
somewhere, and only send that.
> + if ( send( p_sys->fd, buffer, sizeof( buffer ), 0 ) <= 0 )
You should treat `send( p_sys->fd, ..., data_size ) != data_size` as
an error, as send might not be able to send the full packet
(`data_size` is an imaginary variable used as a replacement for
`sizeof( buffer )` given the previous remark).
> + {
> + msg_Dbg( p_filter, "Can not send to socket");
An error such as the above is worthy of *at least* `msg_Warn`.
> + }
> }
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20170207/f50eb855/attachment.html>
More information about the vlc-devel
mailing list