[vlc-devel] 2 weeks to 0.9.0 features freeze
Matthias Bauer
smile4you at gmx.ch
Sat Jun 7 13:33:27 CEST 2008
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.
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 .
> + 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.
> + + bool b_vnc_poll; /* Activatie VNC polling ? */
> bool b_vnc_poll; /* Activatie VNC polling ? */
>
> Is "Activatie" german or just a typo ? ;)
>
Typo, not german ;)
> + 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.
> + 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.
> + 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.
> + 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.
> + 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.
> + 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.
> + 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 */"
>
> + 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
.
> + 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.
> + 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.
>
> + 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.
> + 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.
> 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.
> ------------------------------------------------------------------------
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> http://mailman.videolan.org/listinfo/vlc-devel
>
More information about the vlc-devel
mailing list