[vlc-devel] [PATCH]Improved nuv demuxer and new rtjpeg decoder

[Laurent Aimar]

Hi,
> +    if( !fh.i_keyframe && !p_sys->b_index )
> +        demux_IndexAppend( &p_sys->idx, p_data->i_dts, stream_Tell(p_demux->s) - sizeof( frame_header_t ) );
sizeof(frame_header_t) is not acceptable. It is compiler dependant.
Now a simple define to hide the value 12 would probably be cleaner :)

> +        if( fh.i_compression >='0' && fh.i_compression <='3' )
> +        {
> +            /* for rtjpeg data, the header is also needed */
> +            p_data = block_Realloc( p_data, sizeof( frame_header_t ), fh.i_length );
> +            memcpy( p_data->p_buffer, &fh, sizeof( frame_header_t ) );
Same remarks about  sizeof( frame_header_t ).

> -            if( p_sys->idx.i_idx > 0 )
> +            /* if we can seek in the stream and the stream isn't being recorded (size changing) */
Why cannot we seek in stream being recorded ?

> +                if( ( i_tell = stream_Tell( p_demux->s ) - sizeof( frame_header_t ) ) >= i_pos )
sizeof( frame_header_t ).

> +                    int64_t i_tell = stream_Tell( p_demux->s ) - sizeof( frame_header_t );
sizeof( frame_header_t ).

It seems to me that SET_POSITION and SET_TIME could benefit from a common
function to avoid a lot of code duplication.

> +        p_seek_table = (uint8_t*)malloc( fh.i_length );
Casting malloc is useless.

> +            p_kfa_table = (uint8_t*)malloc( fh.i_length );
Same.

A lot of code inside SeekTableLoad seems duplicated (when i_kfa_elements == 0 or not)

Also you should test the return values of
 - stream_Seek/stream_Read.
 - at least all malloc that use a size comming from the stream (like fh.i_length)

There is also missing free() in the error paths.

Regards,

-- 
fenrir