[vlc-devel] [PATCH 2/3] packetizer/dirac: New fully featured dirac packetizer

[David Flynn]

On 2009-03-27, davidf+nntp at woaf.net <davidf+nntp at woaf.net> wrote:
> From: David Flynn <davidf at rd.bbc.co.uk>

For some split personality fun:

> +static int Open( vlc_object_t *p_this )
> +{
...
> +    p_dec->p_sys = p_sys = calloc( 1, sizeof( decoder_sys_t ) );

p_sys not checked for calloc failure.

> +static block_t *dirac_BuildEncapsulationUnit( decoder_t *p_dec, block_t *p_block )
...
> +    else if( 0 == p_block->p_buffer[4] )
> +    {
> +        p_sys->b_seen_seq_hdr = true;
> +        dirac_UnpackSeqHdr( &p_sys->seq_hdr, p_block );
> +        /* XXX, if this doesn't return true */

Should give up with this seqhdr, and all packets should be
dropped until one is found (since the timestamp generation
doesn't work until then.

...

> +        /* stash a copy of the seqhdr
> +         *  - required for ogg muxing
> +         *  - useful for error checking
> +         *  - it isn't allowed to change until an eos */
> +        if( p_es->p_extra )
> +            free( p_es->p_extra );
> +        p_es->p_extra = malloc( p_block->i_buffer );

p_extra not checked for malloc failure

> +        memcpy( p_es->p_extra, p_block->p_buffer, p_block->i_buffer );

As per [my] recomendations in the Ogg mapping spec, this should have
an EOS appended to it.

More general comment,  p_extra *really* sucks -- what if it were
not constant for different [de]muxers?

> +static uint32_t dirac_uint( bs_t *p_bs )
...
> +static int dirac_bool( bs_t *p_bs )
...
> +static bool dirac_UnpackSeqHdr( struct seq_hdr_t *p_sh, block_t *p_block )

Does anyone have a good idea how to share these functions with the ogg
demuxer?  It isn't nice having two authoritative implementations of the
same code.

> +static block_t *dirac_EmitEOS( decoder_t *p_dec, uint32_t i_prev_parse_offset )
> +{
> +    const uint8_t eos[] = { 'B','B','C','D',0x10,0,0,0,13,0,0,0,0 };
> +    block_t *p_block = block_New( p_dec, 13 );

Unchecked alloc.

> +static block_t *dirac_DoSync( decoder_t *p_dec )
...
> +    /* setup the data unit buffer */
> +    block_t *p_block = block_New( p_dec, pu.u_next_offset );

Unchecked alloc.

..david