[vlc-devel] [PATCH 2/3] Introduce new codec module to decode dirac video via libschroedinger

[Rémi Denis-Courmont]

	Hello,

Looks nice overall, but a few comments inline,

Le jeudi 26 juin 2008 20:03:35 jrosser, vous avez écrit :
> +AC_ARG_ENABLE(schroedinger,
> +[  --enable-schroedinger   high performance dirac codec (default
> disabled)]) 
> +if test "${enable_schroedinger}" = "yes"; then 
> +  PKG_CHECK_MODULES(SCHROEDINGER,[schroedinger-1.0 >= 1.0], [
> +      VLC_ADD_PLUGIN([schroedinger])
> +      VLC_ADD_CFLAGS([schroedinger],[$SCHROEDINGER_CFLAGS])
> +      VLC_ADD_LIBS([schroedinger],[$SCHROEDINGER_LIBS -lstdc++]) ],[
> +      AC_MSG_ERROR([libschroedinger doesn't appear to be installed on you
> system.]) +  ])
> +fi

You should probably default to autodetection rather than disabled.

> + * Copyright (C) 1999-2001 the VideoLAN team

Seems wrong (or you have been keeping that patch for a very very long time).

>+struct picture_pts_t
> +{
> +   uint32_t u_pnum;  //picture number from dirac header
> +   mtime_t i_pts;    //pts for this picture
> +   int i_empty;      //not in use
> +};

On 64-bits platform, you could save 8 bytes by changing the order. Unless you 
have a specific reason not to do so.

> +    /* Initialise the schroedinger decoder */
> +    schro_init();
> +    if( !(p_schro = schro_decoder_new()) ) return VLC_EGENERIC;

Is schro_init() re-entrant and thread-safe? Is there a schro_deinit() or 
equivalent?

> +    u_pnum = p_block->p_buffer[13] << 24;
> +    u_pnum += p_block->p_buffer[14] << 16;
> +    u_pnum += p_block->p_buffer[15] << 8;
> +    u_pnum += p_block->p_buffer[16];

We have GetDBWE() for this.

> +/*************************************************************************
>**** + * CreateSchroFrameFromPic: wrap a picture_t in a SchroFrame
> +
> ***************************************************************************
>**/ +static SchroFrame *CreateSchroFrameFromPic( decoder_t *p_dec )
> +{
> +    decoder_sys_t *p_sys = p_dec->p_sys;
> +    SchroFrame *p_schroframe = schro_frame_new();
> +    picture_t *p_pic = p_dec->pf_vout_buffer_new( p_dec );
> +
> +    if( !p_schroframe )
> +        return NULL;
> +
> +    if( !p_pic )
> +        return NULL;


pf_vout_buffer_new should probably be switched with the !p_schroframe test, to 
avoid a leak.

>+ * SchroBufferFree: schro_buffer callback to release the associated 
> block_t
> + * FIXME - this does not work yet 
...
> +static void SchroBufferFree( SchroBuffer *buf, void *priv)
> +{
> +    block_t *p_block = priv;
> +
> +    if( !p_block )
> +        return;
> +
> +    p_block->i_buffer = 0;

Not needed before block_Release(), but I guess this function needs to be 
revisited anyway.

> +    block_Release( priv );
> +}

(...)
> +            msg_Dbg( p_dec, "SCHRO_DECODER_NEED_BITS len=%d pts=%lld",
> +                (uint32_t)p_block->i_buffer,
> +                p_block->i_pts);

uint32_t != int. This will fail on 64-bits platform, you should probably cast 
to unsigned instead.
Also the pedantically correct format for mtime_t = int64_t is "%"PRId64, 
rather than "%lld" as long long could be different from in64_t.

> +  /* attempt to wrap the block_t in a schrobuffer */
> +  /* i don't know how to properly take ownership of the block_t */
> +  /* and release it when done, so this does not yet work */

You have ownership of the block already. That's why you have to call 
block_Release(), or give it to someone else. Note that VLC has no such thing 
as "cloned" blocks, so you are free to modify the content.

> +            return p_pic;
> +            break;

> +        case SCHRO_DECODER_ERROR:
> +            msg_Dbg( p_dec, "SCHRO_DECODER_ERROR");
> +            return NULL;
> +            break;

No need for break after return.

Thanks, regards,

-- 
Rémi Denis-Courmont
http://www.remlab.net/