[vlc-devel] [PATCH 1/2] demux/mp4: Add fragmented MP4 support

[Jean-Baptiste Kempf]

On Thu, Sep 06, 2012 at 11:38:54AM +0200, Frédéric Yhuel wrote :
> -    uint8_t codec_data_length;
> -    MP4_GET1BYTE( codec_data_length );
> -    p_stra->CodecPrivateData = malloc( codec_data_length + 1);
> +    MP4_GET1BYTE( p_stra->cpd_len );
> +    if( p_stra->cpd_len > i_read )
> +        goto error;
> +    p_stra->CodecPrivateData = malloc( p_stra->cpd_len );

Why not +1 anymore?

> +    bool         b_smooth;       /* Smooth Streaming => no moov box */
> +    bool         b_fragmented;   /* fMP4 */

If we could get rid of those...

> -#define chunk p_track->chunk[p_track->i_chunk]
> +    demux_sys_t *p_sys = p_demux->p_sys;
> +    mp4_chunk_t chunk;
> +    if( p_sys->b_fragmented )
> +        chunk = *p_track->cchunk;
> +    else
> +        chunk = p_track->chunk[p_track->i_chunk];

You should use a macro maybe?

> -    /* Now load all boxes ( except raw data ) */
> -    if( ( p_sys->p_root = MP4_BoxGetRoot( p_demux->s ) ) == NULL )
> +    /* Is it Smooth Streaming? */
> +    if( stream_Peek( p_demux->s, &p_peek, 24 ) < 24 ) return VLC_EGENERIC;
> +    if( !CmpUUID( (UUID_t *)(p_peek + 8), &SmooBoxUUID ) )

Is there not a better way to detect it? Like finding the right box?

> +    else if( false )

???

>  static int TrackCreateES( demux_t *p_demux, mp4_track_t *p_track,
>                            unsigned int i_chunk, es_out_id_t **pp_es )
>  {
> -    const unsigned i_sample_description_index =
> -        p_track->chunk[i_chunk].i_sample_description_index;
> +    demux_sys_t *p_sys = p_demux->p_sys;
> +    unsigned int i_sample_description_index;
> +
> +    if( p_sys->b_fragmented )
> +        i_sample_description_index = 1; /* XXX */

Why ?

> -    if( !TrackGotoChunkSample( p_demux, p_track, i_chunk, i_sample ) )
> -        p_track->b_selected = true;
> +    TrackGotoChunkSample( p_demux, p_track, i_chunk, i_sample );

I don't get this change.

> +    /* Unless we already put something in fmt->p_extra, like for H.264,
> +     * we put CodecPrivateData in it */

This is confusing

> +static mp4_track_t *MP4_frg_GetTrack( demux_t *p_demux, const uint16_t tid )
> +{
> +    demux_sys_t *p_sys = p_demux->p_sys;
> +
> +    mp4_track_t *ret = NULL;
> +    for( unsigned i = 0; i < p_sys->i_tracks; i++ )
> +    {
> +        ret = &p_sys->track[i];
> +        if( !ret )
> +            return NULL;
> +        if( ret->i_track_ID == tid )
> +            break;

return ret here.

> +        if( i == p_sys->i_tracks - 1 )
> +        {
> +            msg_Err( p_demux, "MP4_frg_GetTrack: track %"PRIu16" not found!", tid );
> +            return NULL;
> +        }
remove this

> +    }
> +    return ret;
return NULL here;


> +static int FreeChunk( mp4_chunk_t *ck )
> +{
> +    free( ck->p_sample_count_dts );
> +    free( ck->p_sample_delta_dts );
> +    free( ck->p_sample_count_pts );
> +    free( ck->p_sample_offset_pts );
> +    free( ck->p_sample_size );
> +    for( uint32_t i = 0; i < ck->i_sample_count; i++ )
> +        free( ck->p_sample_data[i] );
> +    free( ck->p_sample_data );
> +    memset( ck, 0, sizeof( mp4_chunk_t ) );

Are you sure about this memset?

> +    /* There is only one traf per moof in un-multiplexed fMP4 */

How does this comment help in this context?

> +    if( p_tfhd->data.p_tfhd->b_empty )
> +    {
> +        /* XXX Don't know what to do in this case actually */

skip to the next one?

Best regards,

-- 
Jean-Baptiste Kempf
http://www.jbkempf.com/ - +33 672 704 734
Sent from my Electronic Device