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

[Frédéric Yhuel]

On Wed, Sep 12, 2012 at 7:12 PM, Jean-Baptiste Kempf <jb at videolan.org> wrote:
> On Wed, Sep 12, 2012 at 10:41:39AM +0200, Frédéric Yhuel wrote :
>> -    p_root->i_size = stream_Size( s );
>> +    p_root->i_size = INT64_MAX; /* could be fragmented MP4 */
>
> This seems weird.
> Can't you set it when you know the size?
>

No, I don't, we're talking about the size of the entire media, and of
course, if it is DASH ot Smooth Streaming, the size is unknown.

>> --- a/modules/demux/mp4/mp4.c
>> +++ b/modules/demux/mp4/mp4.c
>
>
>> +#include <vlc_modules.h>
>
> Why do you need this?
>

I don't remember why I did it, but it any case, it's not necessary anymore.

>> @@ -443,6 +570,7 @@ static int Open( vlc_object_t * p_this )
>>      {
>> +        p_sys->track[i].b_end_of_chunk = true;
>
> Why?
>

Maybe I should rename this boolean "b_need_new_chunk"

>> -    if( !TrackGotoChunkSample( p_demux, p_track, i_chunk, i_sample ) )
>> +    if( !TrackGotoChunkSample( p_demux, p_track, i_chunk, i_sample ) );
>
> Are you sure? THis seems like a typo.
>

Argh sorry...

>> +        if( ((*((uint32_t *)(CodecPrivateData + index))) ^ mark) == 0 )
>
> You mistook VLC for a LISP program :) :)
>

hehe :-) I could use less parentheses, but I'd get warnings.

>> +#define MP4_SET_CODEC_EXTRA_DATA do { \
>> +    fmt->i_extra = p_data->cpd_len; \
>> +    fmt->p_extra = malloc( p_data->cpd_len ); \
>> +    assert( fmt->p_extra ); \
>> +    memcpy( fmt->p_extra, p_data->CodecPrivateData, p_data->cpd_len ); \
>> +  } while(0)
>
> Use a static inline function with p_data and fmt ?
>

Ok

>
>> +    p_track->b_ok       = true;
>> +    p_track->b_selected = false;
>> +    p_track->i_sample_count = UINT32_MAX;
>> +
>> +    p_track->i_timescale = p_sys->i_timescale;
>> +    p_track->i_width = p_data->MaxWidth;
>> +    p_track->i_height = p_data->MaxHeight;
>> +    p_track->i_track_ID = p_data->i_track_ID;
> Vertical alignment should be nice.
>

Ok

>> +    if( b_smooth )
>> +        MP4_frg_TrackCreate( p_demux, p_track, p_stra );
>> +    else /* DASH */
>> +        MP4_TrackCreate( p_demux, p_track, p_trak, true );
>
> What about normal fragmented mp4 ?
>

I think I can process DASH mp4 and "normal" fMP4 the same way, but I
should at least remove that comment.

>> +#if 0
> ifndef NDEBUG ?
>

Ok

Thanks for the review!

-- 
Frédéric