[vlc-devel] [PATCH] demux/mp4: (asking for pre-review) add support of fMP4

[Frederic YHUEL]

Jean-Baptiste, thanks for your review!

On Thu, Jan 26, 2012 at 3:32 PM, Jean-Baptiste Kempf <jb at videolan.org> wrote:
> On Wed, Jan 25, 2012 at 03:55:18PM +0100, Frédéric Yhuel wrote :
>> This is not a patch to be merged, I'm just seeking advices before
>> continuing further.
> Ok.
>
>> This also to answer a "pix or didn't happen" on irc ;-)
> Who could have said that? :)
>

I don't remember, a random guy there...

>> @@ -311,6 +311,10 @@ static int MP4_ReadBoxContainerRaw( stream_t *p_stream, MP4_Box_t *p_container )
>>          if( !p_container->p_first ) p_container->p_first = p_box;
>>          else p_container->p_last->p_next = p_box;
>>          p_container->p_last = p_box;
>> +#ifdef fMP4
>> +        if( p_box->i_type == ATOM_moov )
>> +            break;
>> +#endif
> Why is that needed?
>

Because otherwise it parses all the file / fragments, not just the
initialization fragment.

>> +MP4_Box_t *MP4_BoxGetNextChunk( stream_t *s, unsigned i_tk_id )
>> +{
>> +    /* p_chunk is a virtual root container for the moof and mdat boxes */
>> +    MP4_Box_t *p_chunk;
>> +    MP4_Box_t *p_moof;
>> +    MP4_Box_t *p_mdat;
>> +
>> +    p_chunk = malloc( sizeof( MP4_Box_t ) );
>> +    p_mdat = malloc( sizeof( MP4_Box_t ) );
>> +    if( unlikely( p_chunk == NULL || p_mdat == NULL ) )
>> +        return NULL;
>> +
>> +    p_chunk->i_type = ATOM_root;
>> +    p_chunk->i_shortsize = 1;
>> +
>> +    p_chunk->data.p_data = NULL;
>> +    p_chunk->p_father    = NULL;
>> +    p_chunk->p_first     = NULL;
>> +    p_chunk->p_last      = NULL;
>> +    p_chunk->p_next      = NULL;
>
> Why not calloc p_chunk ?
>

Indeed... Actually I took the code of MP4_BoxGetRoot and didn't think
that much :-)

>> +
>> +    p_moof = MP4_ReadBox( s, p_chunk );
>> +    /* we need to know the size of the mdat */
>> +    MP4_ReadBoxCommon( s, p_mdat );
>> +
>> +    if( p_moof )
>> +    {
>> +        p_chunk->p_first = p_moof;
>> +        p_moof->p_next = p_mdat;
>> +        p_chunk->p_last = p_mdat;
>> +    }
>> +    else
>> +    {
>> +        msg_Warn( s, "no moof box found!");
>> +        return NULL;
>
> leaking here, right?
>

indeed

>> +    if( p_tfhd == NULL)
>> +    {
>> +        msg_Warn( s, "no tfhd box found!");
>> +        return NULL;
>
> or here?
>

yes


>> +    p_sys->pi_sample_first = calloc( p_sys->i_tracks, sizeof( uint32_t ) );
>> +    p_sys->pi_first_dts = calloc( p_sys->i_tracks, sizeof( uint64_t ) );
> callocing for uints?
>

Why not?

>> +    for( int i=1; i < 256; i++)
>> +        p_sys->map[i] = -1;
>
> Memset ?
>
I don't know how to use memset to fill an array of integers with
"-1"s. That beeing said, I don't really need an array of int, an array
of char would do :-)


Thanks again,

-- 
Frédéric