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

[Frédéric Yhuel]

On Thu, Sep 6, 2012 at 7:43 PM, Jean-Baptiste Kempf <jb at videolan.org> wrote:
> 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?
>

Because it's not a null-terminated string anymore?

>> +    bool         b_smooth;       /* Smooth Streaming => no moov box */
>> +    bool         b_fragmented;   /* fMP4 */
>
> If we could get rid of those...
>

Grmmpffff... ok, ok, I'll try  :-)

>> -#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?
>

Ok

>> -    /* 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?
>

Er? I think this a very reliable and efficient way to detect it. Do
you mean I break encapsulation somehow?

>> +    else if( false )
>
> ???
>

I'll remove it.

>>  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 ?
>

Good question, it seems to me it's always 1, but I'm not entirely sure.

For *live* streaming, of course, it's not possible to have a "stsc"
box which maps chunks to sample_description_index, since you have a
potentially infinite number of chunks.

Thus, I guess you have always one and only one stsd per moov in DASH,
and that you have to parse a new init fragment if necessary. This is
consitent with the fact that in DASH, a init fragment is provided for
each quality level.

>> -    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.
>

Sorry, it is just a simplification of the code bit it should be a
separate patch.

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

Ok

>> +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.
>

Ok

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

Ok

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

Ok

>
>> +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?
>

No :-)

>> +    /* There is only one traf per moof in un-multiplexed fMP4 */
>
> How does this comment help in this context?
>

Well, you could have more than one traf per moof, but since
multiplexed fragmented MP4 is not yet supported anyway, this comment
is probably not necessary.

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

Ok

Thanks a lot for your review.


-- 
Frédéric