[vlc-devel] [PATCH] GSoC: MTP Devices initial release

[Laurent Aimar]

Hi,

On Tue, Jan 27, 2009, Fabio Ritrovato wrote:
> Hello again, here's the revised version of the patch, everything
> pointed has been corrected.
> If anyone has anything to else to say...

> diff --git a/modules/access/mtp.c b/modules/access/mtp.c
> new file mode 100644
> index 0000000..9e2e244
> --- /dev/null
> +++ b/modules/access/mtp.c
> @@ -0,0 +1,336 @@
> +#include <libmtp.h>
 If it is not a vlc private header it should be <> iof ""

> +    /* Update default_pts to a suitable value for file access */
> +    var_Create( p_access, "file-caching", VLC_VAR_INTEGER|VLC_VAR_DOINHERIT );
 I think "mtp-caching" was meant here.

> +
> +    STANDARD_READ_ACCESS_INIT;
> +    p_sys->i_nb_reads = 0;
> +    int fd = p_sys->fd = -1;
 It is better to do it once the probing has been done or at least after the following scanf.

> +    if( sscanf( p_access->psz_path, "%"SCNu32":%"SCNu8":%"SCNu16":%d", &i_bus,
> +                &i_dev, &i_product_id, &i_track_id ) != 4 )
> +    {
> +        free( p_sys );
> +        return VLC_EGENERIC;
> +    }

> +    i_ret = LIBMTP_Detect_Raw_Devices( &p_rawdevices, &i_numrawdevices );
> +    if ( i_ret == 0 && i_numrawdevices > 0 && p_rawdevices != NULL )
> +    {
> +        for( i_ret = 0; i_ret < i_numrawdevices; i_ret++ )
 I think not reusing i_ret but using a new loop counter would be clearer. (You can
ignore this remark).

> +        {
> +            if( i_bus == p_rawdevices[i_ret].bus_location &&
> +                i_dev == p_rawdevices[i_ret].devnum &&
> +                i_product_id == p_rawdevices[i_ret].device_entry.product_id )
> +            {
> +                if( ( p_device = LIBMTP_Open_Raw_Device( &p_rawdevices[i_ret] )
> +                    ) != NULL )
> +                {
> +                    free( p_access->psz_path );
> +                    if( ( p_access->psz_path = tempnam( NULL, "vlc" ) ) == NULL )
 I think that changing the psz_path variable here is very ugly.
Is it needed ? If not store it in access_sys_t.
 Then about tempnam, it is a bad choice because it does not prevent a race condition
and you have to delete it afterward.
 I am not sure which one will be the most appropriate for your case.
(tmpfile, utf8_mkstemp, .. ) It depends on your library needed or not the path.

> +        /* Delay a bit to avoid consuming all the CPU. This is particularly
> +         * useful when reading from an unconnected FIFO. */
> +        msleep( INPUT_ERROR_SLEEP );
 I think a poll() could be used here or maybe net_Read (like file.c) as you used O_NONBLOCK

> +/*****************************************************************************
> + * Control:
> + *****************************************************************************/
> +static int Control( access_t *p_access, int i_query, va_list args )
> +{
> +    bool   *pb_bool;
> +    int64_t      *pi_64;
> +
> +    switch( i_query )
> +    {
> +        /* */
> +        case ACCESS_CAN_SEEK:
> +        case ACCESS_CAN_FASTSEEK:
> +            pb_bool = ( bool* )va_arg( args, bool* );
> +            *pb_bool = true;
> +            break;
 You coul replace the break by return VLC_SUCCESS;
> +
> +        case ACCESS_CAN_PAUSE:
> +        case ACCESS_CAN_CONTROL_PACE:
> +            pb_bool = ( bool* )va_arg( args, bool* );
> +            *pb_bool = true;
> +            break;
> +
> +        case ACCESS_GET_PTS_DELAY:
> +            pi_64 = ( int64_t* )va_arg( args, int64_t * );
> +            *pi_64 = var_GetInteger( p_access, "file-caching" ) * INT64_C( 1000 );
> +            break;
 I think "mtp-caching" was meant here.
> +
> +        /* */
> +        case ACCESS_SET_PAUSE_STATE:
> +            /* Nothing to do */
> +            break;
> +
> +        case ACCESS_GET_TITLE_INFO:
> +        case ACCESS_SET_TITLE:
> +        case ACCESS_SET_SEEKPOINT:
> +        case ACCESS_SET_PRIVATE_ID_STATE:
> +        case ACCESS_GET_META:
> +        case ACCESS_GET_PRIVATE_ID_STATE:
> +        case ACCESS_GET_CONTENT_TYPE:
> +            return VLC_EGENERIC;
> +
> +        default:
> +            msg_Warn( p_access, "unimplemented query %d in control", i_query );
> +            return VLC_EGENERIC;
> +
> +    }
> +    return VLC_SUCCESS;
> +}
> +

> diff --git a/modules/services_discovery/mtp.c b/modules/services_discovery/mtp.c
> new file mode 100644
> index 0000000..be9e670
> --- /dev/null
> +++ b/modules/services_discovery/mtp.c
> @@ -0,0 +1,280 @@
I will have a look at it later.

-- 
fenrir