[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...
> 
> Best regards,
> Fabio Ritrovato
> diff --git a/modules/services_discovery/Modules.am b/modules/services_discovery/Modules.am
> index 7aa24b8..055500e 100644
> --- a/modules/services_discovery/Modules.am
> +++ b/modules/services_discovery/Modules.am
> @@ -5,3 +5,4 @@ SOURCES_upnp_cc = upnp_cc.cpp
>  SOURCES_upnp_intel = upnp_intel.cpp upnp_intel.hpp
>  SOURCES_bonjour = bonjour.c
>  SOURCES_podcast = podcast.c
> +SOURCES_mtp = mtp.c
> 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 @@
> +/*****************************************************************************
> + * mtp.c :  MTP interface module
> + *****************************************************************************
> + * Copyright (C) 2009 the VideoLAN team
> + *
> + * Authors: Fabio Ritrovato <exsephiroth87 at gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston MA 02110-1301, USA.
> + *****************************************************************************/
> +
> +#ifdef HAVE_CONFIG_H
> +# include "config.h"
> +#endif
> +
> +#include <vlc_common.h>
> +#include <vlc_playlist.h>
> +#include <vlc_plugin.h>
> +#include <errno.h>
> +#include <vlc_charset.h>
> +#include <vlc_interface.h>
> +#include <vlc_services_discovery.h>
> +
> +#ifdef HAVE_SYS_STAT_H
> +#include <sys/stat.h>
> +#endif
> +
> +#include <libmtp.h>
> +
> +/*****************************************************************************
> + * Local prototypes
> + *****************************************************************************/
> +
> +static void *Run( void * );
> +static int Open( vlc_object_t * );
> +static void Close( vlc_object_t * );
> +
> +static int AddDevice( services_discovery_t *, LIBMTP_raw_device_t * );
> +static void AddTrack( services_discovery_t *, LIBMTP_track_t *);
> +static void CloseDevice( services_discovery_t * );
> +static int CountTracks( uint64_t const, uint64_t const, void const * const );
> +
> +services_discovery_t *p_sd_global;
Is that one mandatory ? It seems unused outside Open().

> +/*****************************************************************************
> + * Open: initialize and create stuff
> + *****************************************************************************/
> +static int Open( vlc_object_t *p_this )
> +{
> +    services_discovery_t *p_sd = ( services_discovery_t * )p_this;
> +    services_discovery_sys_t *p_sys;
> +
> +    if( !( p_sys = malloc( sizeof( services_discovery_sys_t ) ) ) )
> +        return VLC_ENOMEM;
> +    p_sd->p_sys = p_sys;
> +    p_sd_global = p_sd;
Used ?
> +    p_sys->psz_name = NULL;
> +    LIBMTP_Init();
 From libmtp doc:
 "Initialize the library. You are only supposed to call this one, before
 using the library for the first time in a program. Never re-initialize libmtp!"
So something is needed here.

> +/*****************************************************************************
> + * Close: cleanup
> + *****************************************************************************/
> +static void Close( vlc_object_t *p_this )
> +{
> +    services_discovery_t *p_sd = ( services_discovery_t * )p_this;
> +
> +    if( p_sd->p_sys->psz_name != NULL )
> +        free( p_sd->p_sys->psz_name );
> +    vlc_cancel (p_sd->p_sys->thread);
> +    vlc_join (p_sd->p_sys->thread, NULL);
> +    free( p_sd->p_sys );
> +}

> +/*****************************************************************************
> + * Run: main thread
> + *****************************************************************************/
> +static void *Run( void *data )
> +{
> +    LIBMTP_raw_device_t *p_rawdevices;
> +    int i_numrawdevices;
> +    int i_ret;
> +    int i_status = 0;
> +    services_discovery_t *p_sd = data;
> +
> +    for(;;)
> +    {
> +        int canc = vlc_savecancel();
> +        i_ret = LIBMTP_Detect_Raw_Devices( &p_rawdevices, &i_numrawdevices );
> +        if ( i_ret == 0 && i_numrawdevices > 0 && p_rawdevices != NULL &&
> +             i_status == 0 )
> +        {
> +            /* Found a new device, add it */
> +            msg_Info( p_sd, "New device found" );
> +            if( AddDevice( p_sd, &p_rawdevices[0] ) == VLC_SUCCESS )
> +                i_status = 1;
> +        }
> +        else
> +        {
> +            if ( ( i_ret != 0 || i_numrawdevices == 0 || p_rawdevices == NULL )
> +                 && i_status == 1)
> +            {
> +                /* The device is not connected anymore, delete it */
> +                msg_Info( p_sd, "Device disconnected" );
> +                CloseDevice( p_sd );
> +                i_status = 0;
> +            }
> +            vlc_restorecancel(canc);
> +        }
> +        free( p_rawdevices );
> +        vlc_restorecancel(canc);
> +        msleep( 50000 );
 If this loop is used to detect new hardware, I think that a sleep of 500 ms would be
perfectly acceptable and will use less CPU.
 It seems that libmtp does not provide a clean API based on event :( but if I am wrong
that should be used instead of polling.

> +/*****************************************************************************
> + * Everything else
> + *****************************************************************************/
> +static int AddDevice( services_discovery_t *p_sd,
> +                      LIBMTP_raw_device_t *p_raw_device )
> +{
> +    char *psz_name = NULL;
> +    LIBMTP_mtpdevice_t *p_device;
> +    LIBMTP_track_t *p_track, *p_tmp;
> +
> +    if( ( p_device = LIBMTP_Open_Raw_Device( p_raw_device ) ) != NULL )
> +    {
> +        if( !( psz_name = LIBMTP_Get_Friendlyname( p_device ) ) )
> +            if( !( psz_name = LIBMTP_Get_Modelname( p_device ) ) )
> +                if( !( psz_name = strdup( N_( "MTP Device" ) ) ) )
> +                    return VLC_ENOMEM;
> +        msg_Info( p_sd, "Found device: %s", psz_name );
> +        p_sd->p_sys->i_bus = p_raw_device->bus_location;
> +        p_sd->p_sys->i_dev = p_raw_device->devnum;
> +        p_sd->p_sys->i_product_id = p_raw_device->device_entry.product_id;
> +        if( ( p_track = LIBMTP_Get_Tracklisting_With_Callback( p_device,
> +                            CountTracks, p_sd ) ) == NULL )
> +            msg_Info( p_sd, "No tracks on the device" );
> +        else
> +        {
> +            if( !( p_sd->p_sys->pp_items = malloc( p_sd->p_sys->i_tracks_num *
> +                                                   sizeof( input_item_t * ) ) ) )
> +                return VLC_ENOMEM;
 calloc is better to avoid overflow in computation and cleaner.
> +            p_sd->p_sys->i_count = 0;
> +            while( p_track != NULL )
> +            {
> +                msg_Dbg( p_sd, "Track found: %s - %s", p_track->artist,
> +                         p_track->title );
> +                AddTrack( p_sd, p_track );
> +                p_tmp = p_track;
> +                p_track = p_track->next;
> +                LIBMTP_destroy_track_t( p_tmp );
> +            }
> +        }
> +        p_sd->p_sys->psz_name = psz_name;
> +        LIBMTP_Release_Device( p_device );
> +        return VLC_SUCCESS;
> +    }
> +    else
> +    {
> +        msg_Info( p_sd, "No device found, after all" );
> +        return VLC_EGENERIC;
> +    }
> +}
> +
> +static void AddTrack( services_discovery_t *p_sd, LIBMTP_track_t *p_track )
> +{
> +    input_item_t *p_input;
> +    char *psz_string;
> +    char *extension;
> +
> +    extension = rindex( p_track->filename, '.' );
> +    if( asprintf( &psz_string, "mtp://%"PRIu32":%"PRIu8":%"PRIu16":%d%s",
> +                  p_sd->p_sys->i_bus, p_sd->p_sys->i_dev,
> +                  p_sd->p_sys->i_product_id, p_track->item_id,
> +                  extension ) == -1 )
> +    {
> +        msg_Err( p_sd, "Error adding %s, skipping it", p_track->filename );
> +        return;
> +    }
> +    if( ( p_input = input_item_New( p_sd, psz_string,
> +                                    p_track->title ) ) == NULL )
> +    {
> +        msg_Err( p_sd, "Error adding %s, skipping it", p_track->filename );
> +        return;
> +    }
> +    input_item_SetArtist( p_input, p_track->artist );
> +    input_item_SetGenre( p_input, p_track->genre );
> +    input_item_SetAlbum( p_input, p_track->album );
> +    free( psz_string );
> +    if( asprintf( &psz_string, "%d", p_track->tracknumber ) != -1 )
> +    {
> +        input_item_SetTrackNum( p_input, psz_string );
> +        free( psz_string );
> +    }
> +    if( asprintf( &psz_string, "%d", p_track->rating ) != -1 )
> +    {
> +        input_item_SetRating( p_input, psz_string );
> +        free( psz_string );
> +    }
> +    input_item_SetDate( p_input, p_track->date );
> +    p_input->i_duration = p_track->duration * 1000;
input_item_SetDuration would be prefered.

> +    services_discovery_AddItem( p_sd, p_input, NULL );
> +    p_sd->p_sys->pp_items[p_sd->p_sys->i_count++] = p_input;
> +    vlc_gc_decref( p_input );
 Are you sure about this vlc_gc_decref ? As you store the p_input you probably need
to keep a reference and release it only when no used anymore.

> +}
> +
> +static void CloseDevice( services_discovery_t *p_sd )
> +{
> +    input_item_t **pp_items = p_sd->p_sys->pp_items;
> +    int i_i;
> +
> +    if( pp_items != NULL )
> +    {
> +        for( i_i = 0; i_i < p_sd->p_sys->i_count; i_i++ )
> +        {
> +            if( pp_items[i_i] != NULL )
> +                services_discovery_RemoveItem( p_sd, pp_items[i_i] );
 I think the decref should be done here instead (after services_discovery_RemoveItem).
> +        }
> +        free( pp_items );
> +    }
> +}
> +
> +static int CountTracks( uint64_t const sent, uint64_t const total,
> +                        void const * const data )
> +{
> +    VLC_UNUSED( sent );
> +    VLC_UNUSED( total );
 It should be removed as you actually use total.
> +    services_discovery_t *p_sd = (services_discovery_t *)data;
> +    p_sd->p_sys->i_tracks_num = total;
> +    return 0;
> +}

-- 
fenrir