<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
  <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
  <meta http-equiv="Content-Style-Type" content="text/css" />
  <meta name="generator" content="pandoc" />
  <title></title>
  <style type="text/css">code{white-space: pre;}</style>
</head>
<body>
<p>Hi Gonzague,</p>
<p>These changes seems to be an iteration of a patch posted by you back in June 2012, is that correct?</p>
<ul>
<li>https://patches.videolan.org/patch/413/</li>
</ul>
<p>On 2017-02-07 15:14, Gonzague Defos du Rau wrote:</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> 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" )</code></pre>
</blockquote>
<p>Could be rephrased so easier explain the intent of the code.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><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;</code></pre>
</blockquote>
<p>After the socket has been created there is no need for <code>filter_sys_t::port</code>, as such you can remove the <em>data-member</em> from the above struct.</p>
<p>Instead of using <code>p_sys->port > 0</code>, you can rely on the fact that <em>file-descriptors</em> cannot be negative, and replace the former usage with <code>p_sys->fd != -1</code> (where <code>-1</code> denotes no connection, or similar).</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code>  };

  /*****************************************************************************
 @@ -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" );</code></pre>
</blockquote>
<p>Why not use <code>p_sys->port</code> all the way through, instead of having both <code>i_port</code> and <code>p_sys->port</code>?</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +    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] = "";</code></pre>
</blockquote>
<p>Is the above guaranteed not to overflow when using <code>sprintf</code>?</p>
<p>At least use <code>snprintf</code> if it is not 100% clear that it will not overflow, or make use of the functionality in <code>vlc_memstream.h</code> to easily create strings (though beware that they will be heap-allocated).</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code>      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);</code></pre>
</blockquote>
<p>To second <em>RĂ©mi</em> in the previously linked discussion; where is this format specified?</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +        }
 +        
          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 */</code></pre>
</blockquote>
<p>The below does not look correct as there is nothing stating that the entire data in <code>sizeof( buffer )</code> contains valid data, unless the (not mentioned) protocol specifies that trailing null-bytes are to be sent.</p>
<p>It looks like you want to store the length of the generated data somewhere, and only send that.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +    if ( send( p_sys->fd, buffer, sizeof( buffer ), 0 ) <= 0 )</code></pre>
</blockquote>
<p>You should treat <code>send( p_sys->fd, ..., data_size ) != data_size</code> as an error, as send might not be able to send the full packet (<code>data_size</code> is an imaginary variable used as a replacement for <code>sizeof( buffer )</code> given the previous remark).</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +    {
 +        msg_Dbg( p_filter, "Can not send to socket");</code></pre>
</blockquote>
<p>An error such as the above is worthy of <em>at least</em> <code>msg_Warn</code>.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +    }
  }</code></pre>
</blockquote>
</body>
</html>