[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