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

[Rémi Denis-Courmont]

Le samedi 24 janvier 2009 21:36:08 Fabio Ritrovato, vous avez écrit :
> Hi there, due to some changes in the code in the last months, most of
> my code is not working anymore (and to get it working i think I'll
> need to have some talk with the big guys...), but I wanted to let you
> have at least the basic part of my work before the feature freeze.
> What's there is the first part of my project, the basic read-only
> service discovery and access modules, to let VLC play files from an
> MTP device.
> If there are problems, let me know and I'll try to fix them ASAP.
>
> Best regards,
> Fabio Ritrovato

+    set_capability( "access", 50 )

Is MTP using normal file paths? I would have thought it only accepts mtp://... 
locations. In the latter case, priority should be zero, if I'm not mistaken.

+    bool b_seekable;

This was meant for the file access to handle FIFOs properly. I assume libmtp 
only creates regular files, so you should not need this.

+    bool b_pace_control;

This is always true, hence the variable should be removed completely.

+    sscanf( p_access->psz_path, "%"SCNu32":%"SCNu8":%"SCNu16":%d", &i_bus,
+            &i_dev, &i_product_id, &i_track_id );

I wouldn't hurt to check for formatting errors here.

+                    if( asprintf( &psz_to_path, "%s/mtp%d",
+                        config_GetCacheDir(), i_track_id ) == -1 )

Maybe you should use a normal temporary file, instead of going to .cache ?

+    i_ret = net_Read (p_access, fd, NULL, p_buffer, i_len, false);

Regular files don't do non-blocking I/O, so net_Read() is wasteful here. You 
could probably call read() directly.

+#ifdef HAVE_SYS_STAT_H
+    if( p_access->info.i_size != 0 &&
+        (p_sys->i_nb_reads % INPUT_FSTAT_NB_READS) == 0 )
+    {
+        struct stat st;
+
+        if ((fstat (fd, &st) == 0)
+         && (p_access->info.i_size != st.st_size))
+        {
+            p_access->info.i_size = st.st_size;
+            p_access->info.i_update |= INPUT_UPDATE_SIZE;
+        }
+    }
+#endif

That's for the file access to detect asynchronous file size changes. I assume 
you don't need it here.

Regarding the SD:

+    while( !p_sd->b_die )

Direct access to b_die is not safe outside the object lock. Anyway, you don't 
need to check b_die at all as you use vlc_cancel().


Unless LIBMTP is cancellation safe, you should add
`int canc = vlc_savecancel();` here...

+        i_ret = LIBMTP_Detect_Raw_Devices( &p_rawdevices, &i_numrawdevices );
+        if ( i_ret == 0 && i_numrawdevices > 0 && p_rawdevices != NULL &&
+             i_status == 0 )
+        {
/* ... */
+        }
+        else
+        {
+            if ( ( i_ret != 0 || i_numrawdevices == 0 || p_rawdevices == 
NULL )
+                 && i_status == 1)
+            {
/* ... */
+                CloseDevice( p_sd );
+                i_status = 0;
+            }
+        }

And `vlc_restorecancel (canc);` here.

+        free( p_rawdevices );
+        msleep( 5000 );

5 ms is waaaay toooo short. It's not only harmful for power consumption, it's 
so fast it's harmful for scheduling. If you really cannot handle events 
properly, please use something orders longer than that.


+        p_sd->p_sys->p_device = p_device;

You don't use this variable outside of the block, so there is no point in 
storing it in the plugin instance data.

+static int CountTracks( uint64_t const sent, uint64_t const total,
+                        void const * const data )
+{
+    VLC_UNUSED( sent );
+    VLC_UNUSED( total );
+    VLC_UNUSED( data );
+    p_sd_global->p_sys->i_tracks_num = total;
+    return 0;
+}

What is `data`? It looks like an opaque pointer. If it is, then you should 
really use that instead of the ugly p_sd_global.

-- 
Rémi Denis-Courmont
http://git.remlab.net/cgi-bin/gitweb.cgi?p=vlc-courmisch.git;a=summary