[vlc-devel] [PATCH] Adding caf file demuxer module.

Rafaël Carré funman at videolan.org
Thu Sep 19 02:53:38 CEST 2013


Hello Matthias, thanks for the patch.

Hope you're not scared by the review ^_^

Le 19/09/2013 01:56, Matthias Keiser a écrit :
> From: Matthias Keiser <info at tristan-inc.com>
> 
> ---
>  modules/demux/Modules.am |   2 +
>  modules/demux/caf.c      | 887 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 889 insertions(+)
>  create mode 100644 modules/demux/caf.c
> 
> diff --git a/modules/demux/Modules.am b/modules/demux/Modules.am
> index 8aed5d2..491fb5d 100644
> --- a/modules/demux/Modules.am
> +++ b/modules/demux/Modules.am
> @@ -30,6 +30,7 @@ SOURCES_sid = sid.cpp
>  SOURCES_dirac = dirac.c
>  SOURCES_image = image.c mxpeg_helper.h
>  SOURCES_demux_stl = stl.c
> +SOURCES_caf = caf.c
>  
>  libasf_plugin_la_SOURCES = asf/asf.c asf/libasf.c asf/libasf.h asf/libasf_guid.h
>  libasf_plugin_la_CFLAGS = $(AM_CFLAGS)
> @@ -199,6 +200,7 @@ libvlc_LTLIBRARIES += \
>  	libxa_plugin.la \
>  	libimage_plugin.la \
>  	libdemux_stl_plugin.la \
> +	libcaf_plugin.la \
>  	$(NULL)
>  
>  BUILT_SOURCES += dummy.cpp

This part doesn't apply anymore, IIUC you should modify Makefile.am instead.

> diff --git a/modules/demux/caf.c b/modules/demux/caf.c
> new file mode 100644
> index 0000000..7a371f6
> --- /dev/null
> +++ b/modules/demux/caf.c
> @@ -0,0 +1,887 @@
> +/*****************************************************************************
> + * caf.c: Core Audio File Format demuxer
> + *****************************************************************************
> + * Copyright (C) 2013 VLC authors and VideoLAN
> + * $Id$
> + *
> + * Authors: Matthias Keiser <matthias at tristan-inc.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU Lesser General Public License as published by
> + * the Free Software Foundation; either version 2.1 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public License
> + * along with this program; if not, write to the Free Software Foundation,
> + * Inc., 51 Franklin Street, Fifth Floor, Boston MA 02110-1301, USA.
> + *****************************************************************************/
> +
> +/*****************************************************************************
> + * Preamble
> + *****************************************************************************/
> +
> +#ifdef HAVE_CONFIG_H
> +# include "config.h"
> +#endif
> +
> +#include <vlc_common.h>
> +#include <vlc_plugin.h>
> +#include <vlc_demux.h>
> +#include <vlc_codecs.h>
> +
> +/*****************************************************************************
> + * Module descriptor
> + *****************************************************************************/
> +static int  Open    ( vlc_object_t * );
> +static void Close  ( vlc_object_t * );
> +
> +vlc_module_begin ()
> +set_category( CAT_INPUT )
> +set_subcategory( SUBCAT_INPUT_DEMUX )
> +set_description( N_("CAF demuxer" ) )
> +set_capability( "demux", 1000 )

Why 1000 ?

> +set_callbacks( Open, Close )
> +add_shortcut( "caf" )
> +vlc_module_end ()
> +
> +/*****************************************************************************
> + * Local prototypes
> + *****************************************************************************/
> +static int Demux  ( demux_t * );
> +static int Control( demux_t *, int i_query, va_list args );
> +
> +typedef struct frame_span_t
> +{
> +    int64_t i_frames;
> +    int64_t i_samples;
> +    int64_t i_bytes;
> +    int64_t i_desc_bytes;
> +} frame_span_t;
> +
> +typedef struct packet_table_t
> +{
> +    int64_t i_num_packets;
> +    int64_t i_num_valid_frames;
> +    int32_t i_num_priming_frames;
> +    int32_t i_num_remainder_frames;
> +    int64_t i_descriptions_start;
> +} packet_table_t;
> +
> +struct demux_sys_t
> +{
> +    es_format_t  fmt;

You can only keep the audio_format_t here, since the rest is not used.

> +    es_out_id_t *es;
> +
> +    int64_t i_data_offset;
> +    int64_t i_data_size;
> +
> +    uint32_t i_unpacked_bits;
> +
> +    frame_span_t position;
> +    packet_table_t packet_table;
> +};
> +
> +static inline void FrameSpanAddSpan(frame_span_t *span1, frame_span_t *span2);
> +static int FrameSpanAddDescription( demux_t *p_demux, uint64_t i_desc_offset, frame_span_t *span );
> +static inline mtime_t FrameSpanGetTime( frame_span_t *span, uint32_t i_sample_rate);
> +static int SetSpanWithSample(demux_t *p_demux, frame_span_t *span, int64_t i_target_sample);
> +
> +static int NextChunk( demux_t *p_demux, vlc_fourcc_t *p_fcc, int64_t *pi_size );
> +static int ParseVarLenInteger(const uint8_t *p_buff, uint64_t i_buff_len, uint64_t *pi_value_out, int32_t *i_len_out );
> +static inline double GetDBLBE(const uint8_t *p);
> +static inline bool NeedsPacketTable(demux_sys_t *p_sys);
> +static inline bool IsCompressed(demux_sys_t *p_sys);
> +static int64_t TotalNumFrames( demux_t *p_demux );
> +static int64_t TotalNumSamples( demux_t *p_demux );

I would prefer that you reorder the code so declarations are not needed,
especially for static inlines.

> +/*****************************************************************************
> + * Open
> + *****************************************************************************/
> +static int Open( vlc_object_t *p_this )
> +{
> +    demux_t     *p_demux = (demux_t*)p_this;
> +    demux_sys_t *p_sys;
> +
> +    const uint8_t *p_peek;
> +
> +    if( stream_Peek( p_demux->s, &p_peek, 8 ) < 8 )
> +        return VLC_EGENERIC;
> +
> +    /* Is it a caf file? */
> +    if( memcmp( p_peek, "caff", 4 ))
> +        return VLC_EGENERIC;
> +
> +    /* check file version (we only handle version 1) */
> +    if( GetWBE( p_peek + 4 ) != 1 )
> +    {
> +        msg_Dbg(p_demux, "Unknown caf file version.");

Worth pointing the version number in the message?

> +        return VLC_EGENERIC;
> +    }
> +
> +    /* check file flags (must be 0) */
> +    if( GetWBE( p_peek + 6 ) != 0 )
> +    {
> +        msg_Dbg( p_demux, "Unknown caf file flags." );

Idem for the flags.

> +        return VLC_EGENERIC;
> +    }
> +
> +    if( stream_Read( p_demux->s, NULL, 8) < 8 )
> +        return VLC_EGENERIC; /* This would be very strange since we justed peeked at these bytes. */
> +
> +    DEMUX_INIT_COMMON();
> +    p_sys = p_demux->p_sys;
> +    es_format_Init( &p_sys->fmt, UNKNOWN_ES, 0 );
> +
> +    vlc_fourcc_t i_fcc;
> +    int64_t i_size;
> +    int64_t i_idx = 0;
> +
> +    while( NextChunk( p_demux, &i_fcc, &i_size ) == VLC_SUCCESS )
> +    {
> +        if( i_fcc == 'desc' )

'desc' is not valid C, is it? You should use VLC_FOURCC('d','e','s','t')
I think, or VLC_FOURCC('t','s','e','d')

> +        {
> +
> +            msg_Dbg( p_demux, "Found 'desc' chunk." );

That message should be factored out of the if/else.

> +
> +            if( i_idx != 0 )
> +                msg_Warn( p_demux, "The audio description chunk must be the first chunk in a caf file. Continuing anyways." );

Hm if we're continuing anyway without trouble should that really be a
warning?

> +
> +            if ( stream_Peek( p_demux->s, &p_peek, 8 + 6 * 4 ) < ( 8 + 6 * 4) ) {
> +                goto error;
> +            }
> +
> +            vlc_fourcc_t i_fmt = GetDWBE( p_peek + 8 );
> +            uint32_t i_fmt_flags = GetDWBE( p_peek + 12 );
> +
> +            uint32_t i_bits_per_channel = GetDWBE( p_peek + 28 );
> +            uint32_t i_bytes_per_packet = GetDWBE( p_peek + 16 );
> +            uint32_t i_frames_per_packet = GetDWBE( p_peek + 20 );
> +            uint32_t i_channels_per_frame = GetDWBE( p_peek + 24 );
> +            uint32_t i_unpacked_bits_per_sample = ( i_bytes_per_packet / i_frames_per_packet / i_channels_per_frame ) * 8;

Division by zero here!

> +
> +            if( i_fmt == 'lpcm') {

Inconsistent brace placement, both on newline and on same line than if
are ok but please use the same everywhere :)

> +

Too much whitespace here.

> +                bool b_is_float = !!( i_fmt_flags & ( 1 << 0 ) );
> +                bool b_is_be = !( i_fmt_flags & ( 1 << 1 ) );
> +
> +                vlc_fourcc_t i_basic_codec = 0;
> +
> +                if( !b_is_float )
> +                {
> +
> +                    i_basic_codec = b_is_be ? VLC_FOURCC( 't', 'w', 'o', 's' ) : VLC_FOURCC( 's', 'o', 'w', 't' );
> +                    es_format_Init( &p_sys->fmt, AUDIO_ES, vlc_fourcc_GetCodecAudio( i_basic_codec, i_unpacked_bits_per_sample ));
> +                }
> +                else
> +                {
> +                    if( i_bits_per_channel == 32 )
> +                        i_basic_codec = b_is_be ? VLC_CODEC_F32B : VLC_CODEC_F32L;
> +                    else if( i_bits_per_channel == 64 )
> +                        i_basic_codec = b_is_be ? VLC_CODEC_F64B : VLC_CODEC_F64L;
> +
> +                    if( i_basic_codec )
> +                        es_format_Init( &p_sys->fmt, AUDIO_ES, vlc_fourcc_GetCodecAudio( i_basic_codec, i_bits_per_channel ));
> +                }
> +            }
> +            else if( i_fmt == 'aac ' )
> +            {
> +                int kMP4Audio_AAC_LC_ObjectType = 2;
> +
> +                if( i_fmt_flags != kMP4Audio_AAC_LC_ObjectType )
> +                {
> +                    msg_Warn( p_demux, "The only documented format flag for aac is 2 (kMP4Audio_AAC_LC_ObjectType), but is %i. Continuing anyways.", i_fmt_flags );
> +                }
> +
> +                es_format_Init( &p_sys->fmt, AUDIO_ES, vlc_fourcc_GetCodecAudio( VLC_CODEC_MP4A, 0 ));
> +
> +            }
> +            else
> +            {
> +                es_format_Init( &p_sys->fmt, AUDIO_ES, vlc_fourcc_GetCodecAudio( hton32( i_fmt ), 0 ));
> +            }
> +
> +            if( !p_sys->fmt.i_codec )
> +            {
> +                msg_Err( p_demux, "could not determine codec" );
> +                goto error;
> +            }
> +
> +            p_sys->fmt.audio.i_rate = (unsigned int)GetDBLBE( p_peek );
> +            p_sys->fmt.audio.i_channels = i_channels_per_frame;
> +            p_sys->fmt.audio.i_bytes_per_frame = i_bytes_per_packet; /* "mBytesPerPacket" in Apple parlance */
> +            p_sys->fmt.audio.i_frame_length = i_frames_per_packet; /* "mFramesPerPacket" in Apple parlance */
> +            p_sys->fmt.audio.i_bitspersample = i_bits_per_channel; /* mBitsPerChannel */
> +            p_sys->fmt.audio.i_blockalign = i_bytes_per_packet;
> +            p_sys->fmt.i_bitrate = i_bits_per_channel * p_sys->fmt.audio.i_rate * i_channels_per_frame;
> +            p_sys->i_unpacked_bits = i_unpacked_bits_per_sample;
> +
> +        }
> +        else if( i_fcc == 'data' )
> +        {
> +            msg_Dbg( p_demux, "Found 'data' chunk." );
> +
> +            p_sys->i_data_offset = stream_Tell( p_demux->s ) + 4; /* skip edit count */
> +            p_sys->i_data_size = i_size < 0 ? -1 : ( i_size - 4 );
> +        }
> +        else if( i_fcc == 'pakt' )
> +        {
> +            msg_Dbg( p_demux, "Found 'pakt' chunk." );
> +
> +            if ( stream_Peek( p_demux->s, &p_peek, 8 + 8 + 4 + 4 ) < ( 8 + 8 + 4 + 4 ))
> +            {
> +                msg_Err(p_demux, "Couldn't peek packet descriptions");
> +                goto error;
> +            }
> +
> +            p_sys->packet_table.i_num_packets = GetQWBE( p_peek );
> +            p_sys->packet_table.i_num_valid_frames = GetQWBE( p_peek + 8 );
> +            p_sys->packet_table.i_num_priming_frames = GetDWBE(p_peek + 16 );
> +            p_sys->packet_table.i_num_remainder_frames = GetDWBE(p_peek + 20 );
> +            p_sys->packet_table.i_descriptions_start = stream_Tell( p_demux->s ) + 24;
> +        }
> +        else if( i_fcc == 'kuki' )
> +        {
> +            msg_Dbg( p_demux, "Found 'kuki' chunk." );
> +
> +            if( stream_Peek( p_demux->s, &p_peek, (int)i_size ) < i_size )

The cast does not look good, what happens if it's negative when casted
to int?

> +            {
> +                msg_Err(p_demux, "Couldn't peek extra data");
> +                goto error;
> +            }
> +
> +            #define P_EXTRA ((char *)p_sys->fmt.p_extra) /* Avoid pointer arithmetic warnings. */

uint8_t *p = p_sys->fmt.p_extra;

Then you can use p.

> +
> +            if( p_sys->fmt.i_codec  == VLC_CODEC_ALAC )
> +            {
> +                /*
> +                 HACK! No idea what I'm really doing here, this is lifted from cafdec.c libavformat.
> +                 In an ideal world this would be part of the relevant codec module, because we don't
> +                 care about any of this at the demuxer level.
> +                 */

This looks right and totally relevant to demuxer.
Are there files in the wild with the 2 formats? (24 and 36).

> +                int kALAC_NEW_KUKI_SIZE = 24;
> +                int kALAC_LIB_REQ_KUKI_SIZE = 36;
> +                int i_extra;
> +
> +                if( i_size == kALAC_NEW_KUKI_SIZE || i_size == kALAC_LIB_REQ_KUKI_SIZE )
> +                {
> +                    i_extra = kALAC_LIB_REQ_KUKI_SIZE;
> +                }
> +                else
> +                {
> +                    msg_Err(p_demux, "Unknown alac magic cookie. Passing it on to the decoder as is and hoping for the best." );
> +                    i_extra = (int)i_size;
> +                }
> +
> +                p_sys->fmt.i_extra = i_extra;
> +                p_sys->fmt.p_extra = malloc( i_extra );
> +
> +                if( !p_sys->fmt.p_extra )
> +                {
> +                    msg_Err(p_demux, "Could not allocate extra information." );

Don't use msg_* if malloc() fails.

> +                    goto error;
> +                }
> +
> +                if( i_size == kALAC_NEW_KUKI_SIZE ) {
> +
> +                    SetDWBE( P_EXTRA, 36 );
> +                    memcpy( P_EXTRA + 4, "alac", 4 );
> +                    SetDWBE( P_EXTRA + 8, 0 );
> +                    memcpy( P_EXTRA + 12, p_peek, 24 );
> +                }
> +                else
> +                {
> +                    memcpy( p_sys->fmt.p_extra, p_peek, i_size );
> +                }
> +            }
> +            else if( p_sys->fmt.i_codec == VLC_CODEC_MP4A )
> +            {
> +
> +                /*
> +                 HACK! No idea what I'm really doing here, this is lifted from libmp4.c in the mp4 demuxer module.
> +                 In an ideal world this would be part of the relevant codec module, because we don't
> +                 care about any of this at the demuxer level.
> +                 */
> +
> +
> +                #define AAC_GET_TAG do {                    \
> +                    if( i_offset + 1 <= i_size )            \
> +                    {                                       \
> +                        i_tag = *( p_peek + i_offset++ );   \
> +                    }                                       \
> +                    else                                    \
> +                    {                                       \
> +                        goto aac_kuki_finish;                \
> +                    }                                       \
> +                } while(0)
> +
> +                #define AAC_CHECK_LEN(length) do {          \
> +                    if( i_offset + length > i_size )        \
> +                    {                                       \
> +                        goto aac_kuki_finish;                \
> +                    }                                       \
> +                } while(0)
> +
> +                #define AAC_GET_TAG_LENGTH do {             \
> +                    if( ParseVarLenInteger( p_peek + i_offset, i_size - i_offset, &i_tag_size, &i_int_size )) \
> +                    {                                       \
> +                        goto aac_kuki_finish;                \
> +                    }                                       \
> +                    i_offset += i_int_size;                 \
> +                } while(0)

Please no macros, perhaps this can be factorized and split in small
functions?

> +                const uint8_t kAAC_ES_DESCR_TAG = 3;
> +                const uint8_t kAAC_DEC_CONFIG_DESCR_TAG = 4;
> +                const uint8_t kAAC_DEC_SPEC_INFO_TAG = 5;
> +
> +                int64_t i_offset = 0;
> +                int64_t i_kuki_size = 0;
> +                uint64_t i_tag_size;
> +                int32_t i_int_size;
> +                uint8_t i_tag;
> +
> +                AAC_GET_TAG;
> +
> +                if( i_tag == kAAC_ES_DESCR_TAG )
> +                {
> +
> +                    AAC_GET_TAG_LENGTH;
> +
> +                    AAC_CHECK_LEN(3);
> +                    i_offset += 2; /* don't care (ES ID) */
> +                    uint8_t i_flags = *(p_peek + i_offset++ );
> +
> +                    if( i_flags&0x80 )
> +                    {
> +                        AAC_CHECK_LEN(2);
> +                        i_offset += 2; /* don't care (dependance) */
> +                    }
> +                    if( i_flags&0x40 )
> +                    {
> +                        AAC_CHECK_LEN(1);
> +                        uint8_t i_url_len = *( p_peek + i_offset++ );
> +                        i_offset += i_url_len; /* don't care (url) */
> +                    }
> +                    if( i_flags&0x20 )
> +                    {
> +                        AAC_CHECK_LEN(2);
> +                        i_offset += 2; /* don't care (OCR) */
> +                    }
> +
> +                    AAC_GET_TAG;
> +                }
> +                if( i_tag != kAAC_DEC_CONFIG_DESCR_TAG )
> +                    goto aac_kuki_finish;
> +
> +                AAC_GET_TAG_LENGTH;
> +
> +                AAC_CHECK_LEN( 1 + 1 + 3 + 4 + 4);
> +                i_offset += ( 1 + 1 + 3 + 4 + 4 ); /* don't care */
> +
> +                AAC_GET_TAG;
> +
> +                if( i_tag != kAAC_DEC_SPEC_INFO_TAG ) /* this is the one we need */
> +                    goto aac_kuki_finish;
> +
> +                AAC_GET_TAG_LENGTH;
> +
> +                if( i_offset + i_tag_size > i_size )
> +                    goto aac_kuki_finish;
> +
> +                i_kuki_size = i_tag_size;
> +
> +            aac_kuki_finish:
> +
> +                if( !i_kuki_size)
> +                {
> +                    msg_Warn(p_demux, "Error parsing aac cookie. Passing it on to the decoder as is and hoping for the best.");
> +                    i_kuki_size = i_size;
> +                    i_offset = 0;
> +                }
> +
> +                p_sys->fmt.i_extra = (int)i_kuki_size;
> +                p_sys->fmt.p_extra = malloc(i_kuki_size);
> +
> +                if( !p_sys->fmt.p_extra )
> +                {
> +                    msg_Err(p_demux, "Could not allocate extra information." );
> +                    goto error;
> +                }
> +
> +                memcpy( p_sys->fmt.p_extra, p_peek + i_offset, i_kuki_size );
> +            }
> +            else
> +            {
> +                p_sys->fmt.i_extra = (int)i_size;
> +                p_sys->fmt.p_extra = malloc(i_size);
> +
> +                if( !p_sys->fmt.p_extra )
> +                {
> +                    msg_Err(p_demux, "Could not allocate extra information." );
> +                    goto error;
> +                }
> +                memcpy(p_sys->fmt.p_extra, p_peek, p_sys->fmt.i_extra);
> +            }
> +        }
> +        else
> +        {
> +            vlc_fourcc_t i_fcc_be = hton32( i_fcc );

The fcc comes from the stream (GetDWBE) so hton32 usage looks wrong.

> +            msg_Dbg( p_demux, "Ignoring '%4.4s' chunk.", ( char * )&i_fcc_be );
> +        }
> +
> +        if( i_size == -1 )
> +            break;
> +
> +        if( stream_Seek( p_demux->s, stream_Tell( p_demux->s ) + i_size ) != VLC_SUCCESS )
> +            break;
> +
> +        i_idx++;
> +    }
> +
> +    if ( !p_sys->i_data_offset || p_sys->fmt.i_cat != AUDIO_ES ||
> +        ( NeedsPacketTable( p_sys ) && !p_sys->packet_table.i_descriptions_start ))
> +    {
> +        msg_Err( p_demux, "Did not find all necessary chunks." );
> +        goto error;
> +    }
> +
> +    p_sys->es = es_out_Add( p_demux->out, &p_sys->fmt );
> +
> +    return VLC_SUCCESS;
> +
> +error:
> +    free( p_sys  );
> +    return VLC_EGENERIC;
> +}

That function is terribly long and should definitely be splitted.

One function per codec at least?

> +/*****************************************************************************
> + * Demux:
> + *****************************************************************************/
> +static int Demux( demux_t *p_demux )
> +{
> +    demux_sys_t *p_sys = p_demux->p_sys;
> +    block_t     *p_block;
> +
> +    if( p_sys->i_data_size != -1 && p_sys->position.i_bytes >= p_sys->i_data_size )
> +    {
> +        /* EOF */
> +        return 0;
> +    }
> +
> +    frame_span_t advance = {0};
> +
> +    /* we will read 50ms at once */
> +    uint64_t i_req_samples = ( __MAX( p_sys->fmt.audio.i_rate / 20, 1 ));

Superfluous parens.

> +
> +    if( !NeedsPacketTable(p_sys) ) /* PCM/IMA4 */
> +    {
> +        uint64_t i_req_frames = ( i_req_samples + ( p_sys->fmt.audio.i_frame_length - 1 )) / p_sys->fmt.audio.i_frame_length;
> +
> +        if( p_sys->i_data_size != -1 && ( p_sys->position.i_bytes + i_req_frames * p_sys->fmt.audio.i_bytes_per_frame ) > p_sys->i_data_size )
> +        {
> +            i_req_frames = ( p_sys->i_data_size - p_sys->position.i_frames * p_sys->fmt.audio.i_bytes_per_frame) / p_sys->fmt.audio.i_bytes_per_frame;
> +        }
> +
> +        advance.i_frames = i_req_frames;
> +        advance.i_samples = i_req_frames * p_sys->fmt.audio.i_frame_length;
> +        advance.i_bytes = p_sys->fmt.audio.i_bytes_per_frame * advance.i_frames;
> +    }
> +    else /* use packet table */
> +    {
> +        do
> +        {
> +            if( FrameSpanAddDescription( p_demux, p_sys->position.i_desc_bytes + advance.i_desc_bytes, &advance ))
> +                break;
> +        }
> +        while (( i_req_samples > advance.i_samples) && ( p_sys->position.i_frames + advance.i_frames ) < p_sys->packet_table.i_num_packets );
> +    }
> +
> +    if( !advance.i_frames )
> +    {
> +        msg_Err(p_demux, "Unexpected end of file");
> +        return -1;
> +    }
> +
> +    if( stream_Seek( p_demux->s, p_sys->i_data_offset + p_sys->position.i_bytes ))
> +    {
> +        msg_Err( p_demux, "cannot seek data" );
> +        return -1;
> +    }
> +
> +    if( ( p_block = stream_Block(p_demux->s, ( int )advance.i_bytes )) == NULL )
> +    {
> +        msg_Err( p_demux, "cannot read data" );
> +        return -1;
> +    }
> +
> +    p_block->i_dts =
> +    p_block->i_pts = VLC_TS_0 + FrameSpanGetTime( &p_sys->position, p_sys->fmt.audio.i_rate );
> +
> +    FrameSpanAddSpan(&p_sys->position, &advance);
> +
> +    /* set PCR */
> +    es_out_Control( p_demux->out, ES_OUT_SET_PCR, p_block->i_pts );
> +
> +    es_out_Send( p_demux->out, p_sys->es, p_block );
> +
> +    return 1;
> +}
> +
> +/*****************************************************************************
> + * Control:
> + *****************************************************************************/
> +static int Control( demux_t *p_demux, int i_query, va_list args )
> +{
> +    int64_t i64, *pi64, i_sample;
> +    double f, *pf;
> +    frame_span_t position;
> +
> +    demux_sys_t *p_sys  = p_demux->p_sys;
> +    uint64_t i_num_samples = TotalNumSamples( p_demux );
> +
> +    switch( i_query ) {
> +
> +        case DEMUX_GET_LENGTH:
> +            pi64 = ( int64_t* )va_arg( args, int64_t * );
> +            *pi64 = INT64_C( 1000000 ) * ( i_num_samples / p_sys->fmt.audio.i_rate );

Please use CLOCK_FREQ instead of INT64_C(1000000).

> +            return VLC_SUCCESS;
> +
> +        case DEMUX_GET_TIME:
> +            pi64 = ( int64_t* )va_arg( args, int64_t * );
> +            *pi64 = INT64_C( 1000000 ) * ( p_sys->position.i_samples / p_sys->fmt.audio.i_rate );
> +            return VLC_SUCCESS;
> +
> +        case DEMUX_GET_POSITION:
> +            pf = (double*)va_arg( args, double * );
> +            *pf = i_num_samples ? (double)p_sys->position.i_samples / (double)i_num_samples : 0.0;
> +            return VLC_SUCCESS;
> +
> +        case DEMUX_SET_POSITION:
> +            f = (double)va_arg( args, double );
> +            i_sample = f * i_num_samples;
> +            if( SetSpanWithSample( p_demux, &position, i_sample ))
> +                return VLC_EGENERIC;
> +            p_sys->position = position;
> +            return VLC_SUCCESS;
> +
> +        case DEMUX_SET_TIME:
> +            i64 = (int64_t)va_arg( args, int64_t );
> +            i_sample = i64 * p_sys->fmt.audio.i_rate / INT64_C(1000000);
> +            if( SetSpanWithSample( p_demux, &position, i_sample ))
> +                return VLC_EGENERIC;
> +            p_sys->position = position;
> +            return VLC_SUCCESS;
> +
> +        case DEMUX_GET_META:
> +            return stream_Control( p_demux->s, STREAM_GET_META, args );
> +
> +        default:
> +            return VLC_EGENERIC;
> +    }
> +
> +    return VLC_EGENERIC;
> +}
> +
> +
> +/*****************************************************************************
> + * Close
> + *****************************************************************************/
> +static void Close( vlc_object_t *p_this )
> +{
> +    demux_t     *p_demux = (demux_t*)p_this;
> +    demux_sys_t *p_sys = p_demux->p_sys;
> +
> +    free(p_sys->fmt.p_extra);
> +    free( p_sys );
> +}
> +
> +/***************************** Local Functions *******************************/
> +
> +
> +/*****************************************************************************
> + * FrameSpan Functions
> + *****************************************************************************
> + *  A FrameSpan structure contains the relationship between a number of
> + frames, the number of samples they contain, the amount of data they
> + use, and the length of their packet descriptions (if any).
> + *****************************************************************************/
> +
> +/* FrameSpanAddSpan adds span2 to span1 */
> +
> +static inline void FrameSpanAddSpan( frame_span_t *span1, frame_span_t *span2 )
> +{
> +    span1->i_frames += span2->i_frames;
> +    span1->i_samples += span2->i_samples;
> +    span1->i_bytes += span2->i_bytes;

Since this is only used to know position in file, shouldn't we store
only one of them and calculate the others?

> +    span1->i_desc_bytes += span2->i_desc_bytes;
> +}
> +
> +/* Adds the frame that is described at i_desc_offset to span */
> +
> +static int FrameSpanAddDescription( demux_t *p_demux, uint64_t i_desc_offset, frame_span_t *span )
> +{
> +    demux_sys_t *p_sys  = p_demux->p_sys;
> +
> +    /* Avoid seeking + peeking for simple case (PCM) */
> +    if( p_sys->fmt.audio.i_bytes_per_frame && p_sys->fmt.audio.i_frame_length )
> +    {
> +        span->i_bytes += p_sys->fmt.audio.i_bytes_per_frame;
> +        span->i_samples += p_sys->fmt.audio.i_frame_length;
> +        span->i_frames++;
> +        return VLC_SUCCESS;
> +    }
> +
> +    uint32_t i_desc_size = 0;
> +
> +    if( stream_Seek( p_demux->s, p_sys->packet_table.i_descriptions_start + i_desc_offset ))
> +    {
> +        msg_Err(p_demux, "Couldn't seek packet description.");
> +        return VLC_EGENERIC;
> +    }
> +
> +    const uint8_t *p_peek;
> +    uint32_t i_peek_len = stream_Peek( p_demux->s, &p_peek, 2 * 10 ); /* Peeking the maximum number of bytes that two 64 bit numbers could use (( 64 + 6 ) / 7 = 10 ). */
> +
> +    if( p_sys->fmt.audio.i_bytes_per_frame )
> +    {
> +        span->i_bytes += p_sys->fmt.audio.i_bytes_per_frame;
> +    }
> +    else
> +    {
> +        uint64_t i_size;
> +        int32_t i_this_int;
> +        if( ParseVarLenInteger( p_peek, i_peek_len, &i_size, &i_this_int ))
> +        {
> +            return VLC_EGENERIC;
> +        }
> +
> +        i_desc_size += i_this_int;
> +        span->i_bytes += i_size;
> +    }
> +
> +    if( p_sys->fmt.audio.i_frame_length)
> +    {
> +        span->i_samples += p_sys->fmt.audio.i_frame_length;
> +    }
> +    else
> +    {
> +        if( i_desc_size >= i_peek_len )
> +        {
> +            return VLC_EGENERIC;
> +        }
> +
> +        uint64_t i_num_samples;
> +        int32_t i_this_int;
> +        if( ParseVarLenInteger( p_peek + i_desc_size, i_peek_len - i_desc_size, &i_num_samples, &i_this_int ))
> +        {
> +            return VLC_EGENERIC;
> +        }
> +
> +        i_desc_size += i_this_int;
> +        span->i_samples += i_num_samples;
> +    }
> +    span->i_desc_bytes += i_desc_size;
> +    span->i_frames++;
> +
> +    return VLC_SUCCESS;
> +}
> +
> +/* FrameSpanGetTime returns the time span represented by the frame span. */
> +
> +static inline mtime_t FrameSpanGetTime( frame_span_t *span, uint32_t i_sample_rate)
> +{
> +    date_t date;
> +
> +    date_Init( &date, ( uint32_t )i_sample_rate, 1 );
> +    date_Set( &date, 1 );
> +    date_Increment( &date, ( uint32_t )span->i_samples );
> +
> +    return date_Get( &date );

Initializing the date on every call looks wrong.

Usually the date is stored in sys_t structure and Incremented each time
we send a block.

> +}
> +
> +/* SetSpanWithSample returns the span from the beginning of the file up to and
> +   including the specified sample. This works at frame granularity, so the
> +   returned span may not represent the exact target sample.
> +   Since we might have to lineraly search the packet descriptions, this is a
> +   potentially slow operation! */
> +
> +static int SetSpanWithSample(demux_t *p_demux, frame_span_t *span, int64_t i_target_sample)
> +{
> +    demux_sys_t *p_sys = p_demux->p_sys;
> +
> +    int64_t i_num_frames = TotalNumFrames( p_demux );
> +
> +    if( !NeedsPacketTable( p_sys ))
> +    {
> +        int64_t i_frame = i_target_sample / p_sys->fmt.audio.i_frame_length;
> +        int64_t i_remaining = i_target_sample - i_frame * p_sys->fmt.audio.i_frame_length;
> +        if( i_remaining > ( p_sys->fmt.audio.i_frame_length / 2 ))
> +            i_frame++;
> +
> +        if( i_frame > i_num_frames)
> +            i_frame = i_num_frames;
> +
> +        span->i_frames = i_frame;
> +        span->i_samples = i_frame * p_sys->fmt.audio.i_frame_length;
> +        span->i_bytes = i_frame * p_sys->fmt.audio.i_bytes_per_frame;
> +        span->i_desc_bytes = 0;
> +    }
> +    else {
> +
> +        memset(span, 0, sizeof( frame_span_t ));
> +        frame_span_t prev_span;
> +
> +        while( span->i_samples < i_target_sample && span->i_frames < i_num_frames ) {
> +
> +            prev_span = *span;
> +
> +            if( FrameSpanAddDescription( p_demux, span->i_desc_bytes, span ))
> +                return VLC_EGENERIC;
> +
> +            if( span->i_samples >= i_target_sample )
> +            {
> +                int64_t i_this_samples = span->i_samples - prev_span.i_samples;
> +
> +                if( i_target_sample - prev_span.i_samples < i_this_samples / 2 )
> +                    *span = prev_span;
> +
> +                break;
> +            }
> +        }
> +    }
> +
> +    return VLC_SUCCESS;
> +}
> +
> +/*****************************************************************************
> + * Various Utility Functions
> + *****************************************************************************/
> +
> +/* The NextChunk function returns the four char code and the size at the current file position.
> +   Upon return the file position points to the start of the chunk payload. */
> +
> +static int NextChunk( demux_t *p_demux, vlc_fourcc_t *p_fcc, int64_t *pi_size )
> +{
> +    const uint8_t *p_peek;
> +
> +    if( stream_Peek( p_demux->s, &p_peek, 12 ) < 12 )
> +        return VLC_EGENERIC;
> +
> +    *p_fcc = GetDWBE(p_peek);
> +    *pi_size = GetQWBE(p_peek + 4);
> +
> +    if( stream_Read( p_demux->s, NULL, 12 ) < 12 )
> +        return VLC_EGENERIC;

Why not use stream_Read directly ?

> +    return VLC_SUCCESS;
> +
> +}
> +
> +/* ParseVarLenInteger parses a var length integer as found in the packet descriptions
> +   (and in the aac magic cookie). In theorie a var length integer could be bigger than
> +   an uint64_t, but I think it's unlikely to ever be a problem... */
> +
> +static int ParseVarLenInteger(const uint8_t *p_buff, uint64_t i_buff_len, uint64_t *pi_value_out, int32_t *i_len_out )
> +{
> +    *i_len_out = 0;
> +
> +    uint64_t i_value = 0;
> +    bool finished = false;
> +
> +    for( uint32_t i = 0; i < i_buff_len; i++)
> +    {
> +        uint8_t i_byte = p_buff[i];
> +        i_value = ( i_value << 7 ) | ( i_byte & 0x7f );
> +
> +        ( *i_len_out )++;
> +
> +        if( !( i_byte & 0x80 ))
> +        {

*pi_value_out = i_value;
return VLC_SUCCESS;

> +            finished = true;
> +            break;
> +        }
> +    }
> +
> +    if( !finished )
> +    {
> +        return VLC_EGENERIC;
> +    }
> +
> +    *pi_value_out = i_value;
> +
> +    return VLC_SUCCESS;
> +}
> +
> +/* Utility function that reads a big endian double from the input buffer. */
> +
> +static inline double GetDBLBE(const uint8_t *p)
> +{
> +    uint64_t i_mem = GetQWBE(p);
> +    return *(double *)(&i_mem);

This is wrong because breaks strict aliasing rules, you should use union
here.

Storing a double in the stream looks terrible though, especially since
it's the sample rate and we convert it to integer anyway.

> +}
> +
> +static inline bool NeedsPacketTable(demux_sys_t *p_sys)
> +{
> +    return ( !p_sys->fmt.audio.i_bytes_per_frame || !p_sys->fmt.audio.i_frame_length);
> +}
> +
> +static inline bool IsCompressed(demux_sys_t *p_sys)
> +{
> +    return !p_sys->fmt.audio.i_bitspersample;
> +}

That one is never used

> +static int64_t TotalNumFrames( demux_t *p_demux )
> +{
> +    demux_sys_t *p_sys = p_demux->p_sys;
> +
> +    int64_t i_num_frames;
> +
> +    if( !NeedsPacketTable( p_sys ))
> +    {
> +        int64_t i_data_size;
> +
> +        if( p_sys->i_data_size >= 0 )
> +        {
> +            i_data_size = p_sys->i_data_size;
> +        }
> +        else
> +        {
> +            i_data_size = stream_Size( p_demux->s ) - p_sys->i_data_offset;
> +        }
> +
> +        if( i_data_size <= 0 )
> +            return 0;
> +
> +        i_num_frames = i_data_size / p_sys->fmt.audio.i_bytes_per_frame;
> +    }
> +    else
> +    {
> +        i_num_frames = p_sys->packet_table.i_num_packets;
> +    }
> +
> +    return i_num_frames;
> +}

Could use return directly (see below)

> +static int64_t TotalNumSamples( demux_t *p_demux )
> +{
> +    demux_sys_t *p_sys = p_demux->p_sys;
> +
> +    int64_t i_num_samples;
> +
> +    if( !NeedsPacketTable( p_sys ))
> +    {
> +        i_num_samples = TotalNumFrames( p_demux ) * p_sys->fmt.audio.i_frame_length;
> +    }
> +    else
> +    {
> +        i_num_samples = p_sys->packet_table.i_num_valid_frames + p_sys->packet_table.i_num_priming_frames + p_sys->packet_table.i_num_remainder_frames;
> +    }
> +
> +    return i_num_samples;
> +}
> +

if( !NeedsPacketTable( p_sys ))
    return TotalNumFrames( p_demux ) * p_sys->fmt.audio.i_frame_length;
else
    return p_sys->packet_table.i_num_valid_frames +
p_sys->packet_table.i_num_priming_frames +
p_sys->packet_table.i_num_remainder_frames;



More information about the vlc-devel mailing list