[vlc-devel] 2 weeks to 0.9.0 features freeze

Rafaël Carré funman at videolan.org
Sun Jun 8 22:28:37 CEST 2008


Le Sat, 07 Jun 2008 13:33:27 +0200,
Matthias Bauer <smile4you at gmx.ch> a écrit :

> Hi Rafaël
> 
> Thank you for the review. Please look my answers and changes below.
> If you agree with the changes and answers, I will create the new
> patch and post it to the list.

Ok

> Regards, Matthias
> 
> 
> Rafaël Carré schrieb:
> > Le Mon, 2 Jun 2008 21:09:25 +0200,
> > Pierre d'Herbemont <pdherbemont at free.fr> a écrit :
> >
> >   
> >> On Jun 2, 2008, at 9:06 PM, Matthias Bauer wrote:
> >>     
> >
> >   
> >>> The patch was reviewed by Funman and Jean-Paul-Saman and I
> >>> implemented most of their change requests, but didn't get any
> >>> answer to my later posts. That disencouraged me to work on. I you
> >>> say yes, I would be pleased to continue working on this plugin.
> >>>       
> >
> > I have a last few remarks:
> >
> > +#define CARD8  uint8_t  
> > +#define CARD16 uint16_t
> > +#define CARD32 uint32_t
> >   
> > What about putting them in the .h that needs them ? (after including
> > the correct header, which is inttypes.h)
> >   
> OK, I moved them to remoteosd_rfbproto.h .

ok

> > +    int           i_alpha;             /* alpha transparency value
> > */
> >
> > That should be uint8_t , or whatever unsigned type which suits the
> > maximum value i_alpha can take
> >   
> OK, done. It is uint8_t.

ok

> > +   +    bool    b_vnc_poll;         /* Activatie VNC polling ? */
> >  bool    b_vnc_poll;         /* Activatie VNC polling ? */
> >
> > Is "Activatie" german or just a typo ? ;)
> >   
> Typo, not german ;)

It's what I thought :)

> > +    p_sys->psz_host = var_CreateGetString( p_this, RMTOSD_CFG
> > "host" );
> > +    if( EMPTY_STR(p_sys->psz_host) )
> >
> > You shall free psz_host since it may be 1 byte allocated: '\0'
> >   
> It is freed in the "error:" section.

ok

> > +    p_sys->psz_passwd = var_CreateGetString( p_this, RMTOSD_CFG
> > "password" );
> > +    if( !p_sys->psz_passwd )
> >
> > You shall free psz_host & psz_passwd
> >
> >   
> I added freeing psz_passwd in the "error:" section, psz_host is
> already freed there.

ok

> > +    p_sys->p_vout = vlc_object_find( p_this, VLC_OBJECT_VOUT,
> > FIND_ANYWHERE );
> >
> > This should use FIND_PARENT, since the video_output (if any) we are
> > looking for is the creator of the remote_osd object, and not another
> > one which may be completely unrelated to us.
> >   
> OK, done.

ok

> > +    if( p_sys->psz_host ) free( p_sys->psz_host );
> > +
> > +    if( p_sys ) free( p_sys );
> >
> > You don't need to check for the pointers being != NULL before
> > free()ing them.
> > You are also leaking p_sys->psz_passwd
> >   
> OK. Removed the NULL-Check and added psz_passwd.

ok
 
> > +    FREENULL( p_sys->psz_host );
> > +    FREENULL( p_sys->psz_passwd );
> >
> > Since you are closing the module, and freeing p_sys just the line
> > after, no need to use the FREENULL macro, free() is fine.
> >   
> OK, done.

ok

> > +    if ( i_bytes == net_Read( p_filter, i_socket, NULL,
> > +                              (unsigned char*)p_readbuf,
> > +                              i_bytes, true ) )
> > +        return true;
> > +    else 
> > +        return false;
> >
> > You can use:
> >     return net_Read( p_filter, i_socket, NULL,
> >                                (unsigned char*)p_readbuf,
> >                                i_bytes, true ) );
> >
> > Any integer != 0 means true, and 0 means false.
> >   
> No. You didn't see the "i_bytes ==" comparision. But because you like 
> short forms, I will change to
> 
>     return (i_bytes == net_Read( p_filter, i_socket, NULL,
>                                (unsigned char*)p_readbuf,
>                                i_bytes, true ) ) );
> 
> 
> > (same for write_exact() )
> >   
> Yes, with the same "i_bytes ==" added.

Sorry, I forgot the "i_bytes ==" but you got what I meant ;)

> > +    rfbProtocolVersionMsg pv;
> > +    pv[sz_rfbProtocolVersionMsg] = '\0';
> >
> > It looks to me like an off-by-one overflow, shouldn't this be
> > sz_rfbProtocolVersionMsg-1 ?
> >   
> No.The programmer that definied these types already made added one
> extra byte for the ' \0', see the following lines.
> 
> typedef char rfbProtocolVersionMsg[13];    /* allow extra byte for
> null */ #define sz_rfbProtocolVersionMsg 12
> 
> I will add this comment to the line: "/* pv size is 
> sz_rfbProtocolVersionMsg+1 */"

Ok

> > +    if( i_nameLength > 100 )
> > +    {
> > +        msg_Err( p_filter, "Server name too long" );
> > +        return false;
> > +    }
> >
> > Is 100 a limit fixed by yourself ?
> > If yes, you should use a #define for it, so people can change it
> > easily, without looking at all places where it is used.
> >   
> Yes, 100 was a limit fixed by myself. I made a define 
> MAX_VNC_SERVER_NAME_LENGTH now and set the limit to 255.
> 
> By protocol the length is limited to a 32-bit unsigned value what 
> doesn't make sense to me

I think 255 limit is fine
.
> > +    while (p_sys->b_continue)
> > +    {
> > +       msg.type = 0;
> > +       if ( !read_exact(p_filter, p_sys->i_socket, (char*)&msg,
> > 1 ) )
> >
> > You rely on p_sys->b_continue to stop execution of other threads,
> > but in the close function you use:
> >
> > +    free( p_sys );
> >
> >
> > That means in the while loop of a thread, such an execution could
> > occur:
> >
> > Check p_sys->b_continue: true, then execute the whole while loop
> > * Another thread is executed, which will call the close function
> > 	p_sys is freed by the close function
> > * The while loop continues to be executed:
> > 	if ( !read_exact(p_filter, p_sys->i_socket, (char*)&msg,
> > 1 ) )
> >
> > But now you are accessing the p_sys content which has been freed,
> > and is now invalid.
> >
> > Proper synchronisation between all threads should be achieved with
> > mutexes.
> >   
> The function "stop_osdvnc" called in DestroyFilter sets the variable 
> p_sys->b_continue to true and then waits with "vlc_thread_join" until 
> both threads are finished. The free( p_sys ) is not called before the 
> stop_vnc returns and vnc_stop doesn't return before the threads are 
> terminated. This should work fine.

Ok, I don't have the patch handy, but I trust you.

> > +               i_msgSize--;
> > +               if (i_msgSize > 0)
> >
> > You can use if( --i_msgSize > 0 ), that just makes code shorter ;)
> >   
> OK, code is no one line shorter ;)

:)

> > +    filter_sys_t *p_sys = p_filter->p_sys;
> > +    subpicture_t *p_spu;
> > +    subpicture_region_t *p_region;
> > +    video_format_t fmt;
> > +    picture_t *p_pic;
> >
> > If you declare the variables just before using them, it makes it a
> > lot better for people looking at your code in the future.
> >   
> This is copied from the "logo" videofilter that I took as sample code.
> 
> I agree with you to define variables just befor using them, if they
> are used locally in a short block of code. But I prefer to define
> important variables used many times in the methode at begin of the
> methode to give an overview of the data needed in the methode.

Fine

> > +    int i_line_offset = i_y * i_pitch + i_x;
> > +
> > +    for( int i_column = 0; i_column < i_w; i_column++ )
> > +    {
> > +        int i_offset = i_line_offset + i_column;
> > +        uint8_t i_color = p_sys->read_buffer[i_column]; 
> > +        p_outY->p_pixels[ i_offset ] =
> > p_sys->ar_color_table_yuv[i_color][0];
> >
> > Are you sure i_offset can not overflow p_outY->p_pixels' size here ?
> >
> >   
> I added an additional check:
> 
>     int i_lines = p_outY->i_lines;
>     if ( i_y > i_lines)
>         return false;
> 
> Now, I'm sure.

eheh

> > +    if( i_y < 0 || i_x < 0 || i_y >= v_h || i_x >= v_w )
> > +        return VLC_SUCCESS;
> >
> > Might need a msg_Dbg() here, no ?
> >   
> Yes, good idea.
> > +    /* msg_Dbg( p_this, "mouse event x=%d y=%d btn=%x", i_x, i_y,
> > i_v ); */
> >
> > You should use #ifdef VNC_DEBUG, no ?
> >   
> Yes, good idea.
> > +    /* Fixme: remove this "handmade" DES crypt algoritm */
> >
> > When you send the next patch, I will have a look at the problem with
> > libgcrypt, so keep it like it for now, and focus on the threads
> > synchronization ;)
> >
> > If I can not find the cause, I will commit it as is (with the
> > "handmade" DES algo), so it can be tested by the community.
> >
> >   
> Thank you in advance to help debug this.

You're welcome

> > Sorry for the delay, the patch is quite long to review.
> > We should be near the end now, thanks for your patience.
> >
> >   
> No problem. Thank you for reviewing.

Now waiting for the final round :)

-- 
Rafaël Carré
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20080608/b93bdd6c/attachment.sig>


More information about the vlc-devel mailing list