[vlc-devel] [PATCH] Add Smooth Streaming module

[Frederic YHUEL]

On Wed, Apr 11, 2012 at 12:47 PM, Jean-Baptiste Kempf <jb at videolan.org> wrote:
> On Wed, Apr 11, 2012 at 11:51:36AM +0200, Frédéric Yhuel wrote :
>> +SOURCES_stream_filter_smooth = smooth.c
>
> mp4/mp4.h too, no?
>

Ok

>> +#include <vlc_xml.h>
>> +#include <vlc_charset.h>
>> +#include <vlc_threads.h>
>> +#include <vlc_arrays.h>
>> +#include <vlc_stream.h>
>> +#include <vlc_url.h>
>> +#include <vlc_memory.h>
>
> You need all of those?
>

No I don't, I can remove 4 of them... sorry  :)

>> +    add_bool( "smooth-disable-audio", false, DA_TEXT, DA_LONGTEXT, false )
>> +        change_safe()
>
> Sorry, but why ?
>

Audio doesn't work with the Microsoft demo stream for example, so for
me this option is very useful... but should get removed in the future.

>> +enum FourCC {
>> +    NULL_CC=0,
>> +    H264 = VLC_FOURCC( 'H', '2', '6', '4' ),
>> +    AVC1 = VLC_FOURCC( 'A', 'V', 'C', '1' ),
>> +    AACL = VLC_FOURCC( 'A', 'A', 'C', 'L' ),
>> +    AACH = VLC_FOURCC( 'A', 'A', 'C', 'H' ),
>> +    WVC1 = VLC_FOURCC( 'W', 'V', 'C', '1' ),
>> +    WMAP = VLC_FOURCC( 'W', 'M', 'A', 'P' ),
>> +    TTML = VLC_FOURCC( 'T', 'T', 'M', 'L' ),
>> +};
>
> Why not using VLC_CODEC_ID ?
>

Indeed... !

>> +    unsigned long   SamplingRate;
>> +    unsigned long   Channels;
>> +    unsigned long   BitsPerSample;
>
> Why long?
>

I don't remember... seems silly indeed.

>> +#define SMS_GET4BYTES( dst ) do { \
>> +    dst = U32_AT( slice ); \
>> +    slice += 4; \
>> +  } while(0)
>> +
>> +#define SMS_GET1BYTE( dst ) do { \
>> +    dst = *slice; \
>> +    slice += 1; \
>> +  } while(0)
>> +
>> +#define SMS_GET3BYTES( dst ) do { \
>> +    dst = Get24bBE( slice ); \
>> +    slice += 3; \
>> +  } while(0)
>> +
>> +#define SMS_GET8BYTES( dst ) do { \
>> +    dst = U64_AT( slice ); \
>> +    slice += 8; \
>> +  } while(0)
>> +
>> +#define SMS_GET4or8BYTES( dst ) \
>> +    if( (version) == 0 ) \
>> +        SMS_GET4BYTES( dst ); \
>> +    else \
>> +        SMS_GET8BYTES( dst ); \
>> +
>> +#define SMS_GETFOURCC( dst ) do { \
>> +    memcpy( &dst, slice, 4 ); \
>> +    slice += 4; \
>> +  } while(0)
>
> Really look like mp4 ones, no?
>

Yes and that break my heart, but I don't see how to use libmp4 ones.

>> +quality_level_t *get_qlevel( sms_stream_t *, uint16_t );
>> +sms_stream_t    *get_sms_stream( stream_t *, uint16_t );
>> +uint16_t        set_track_id( chunk_t *, uint16_t, uint16_t );
>> +static int      sms_TrackCreate( stream_t *, uint16_t, mp4_track_t * );
>> +static int      sms_FmtCreate( stream_t *, uint16_t, uint16_t, es_format_t * );
>> +int             build_raw_avcC( uint8_t **, const char * );
>> +int             build_raw_esds( uint8_t **, const char * );
>> +static int      chunk_index_add( uint32_t *, int32_t );
>> +static          chunk_t *get_chunk( stream_t *, uint32_t, bool );
>> +static int      sms_Download( stream_t *, chunk_t *, char *);
>> +static int      Download( stream_t *, sms_stream_t *, uint64_t *);
>> +static void     *sms_Thread( void *);
>> +static void     *sms_Reload( void *);
>
> No way to reorder to get a bit less of those?
>

Ok :-)

>> +    if(  i_size < 1 )
> Are you sure 1 is enough?
>

should be 1024... don't know what happened :-s

>> +    if( ql == NULL )
> unlikely
>

Ok

>
> Shouldn't sms_Free/new, ql_Free/New be in another file or so?
>

Ah? Ok.

>> +    if( !isSmoothStreaming( s ) )
>> +    {
>> +        msg_Dbg(p_this, "Smooth Streaming: this is not a valid manifest");
>
> Are you sure this cannot happen when opening other valid streams? If so,
> remove this.
>

Ok

>> +    if( p_sys == NULL )
>> +        return VLC_ENOMEM;
> unlikely
>

Ok

>> +    /* remove the last part of the url */
>> +    char *base_url = strdup( uri );
>> +    char *pos = strrchr( base_url, '/');
>> +    *pos = '\0';
>> +    p_sys->base_url = base_url;
> Don't we have functions for that?
>

I don't think so, do we?

>> +    bool disable_audio = var_CreateGetBool( s, "smooth-disable-audio" );
> inherit
>

Ok

>> +    /* */
>> +    s->pf_read = Read;
>> +    s->pf_peek = Peek;
>> +    s->pf_control = Control;
> Move that down.
>

Ok

>> +    /* Parse SMS ismc content. */
>> +    if( parse_Manifest( s, false ) != VLC_SUCCESS )
>> +        return VLC_EGENERIC;
>
> No memleak here?
>

Oops...

>> +    /* Choose first audio stream available (TO FIX) */
> FIXME
>
>> +/******************************************************************************
>> + *  Download thread
>> + *****************************************************************************/
>
> Split it in another file? Maybe?
>

Ok

>> +    qual = strtok( url_template, "{" );
>> +    strtok( NULL, "}" );
>> +    frag = strtok( NULL, "{" );
>> +    strtok( NULL, "}" );
>> +    end = strtok( NULL, "" );
>> +    char *url = NULL;
>
> strtok? I think strtok_r
>

Ok

>> +static int sms_FmtCreate( stream_t *s, uint16_t tid, uint16_t qid,
>
>> +/* Luc Saillard code */
>
> Seriously?
>

I don't know how I am supposed to give credit. What should I do?

Thanks for this review Jean-Baptiste!

-- 
Frédéric