[vlc-devel] [PATCH] Add Dirac encoding support to VLC using the Schroedinger library.

[Jean-Baptiste Kempf]

Well, first, I would like to thank you a lot about this patch.
I would love to remove libdirac_plugin.dll on Windows, which is 1.1MB.

Here is a quick review:


On Fri, Dec 03, 2010 at 11:41:35PM +1100, Anuradha Suraparaju wrote :
> +#define ENC_MCBLK_SIZE "motion_block_size"
> +#define ENC_MCBLK_SIZE_TEXT N_("Size of motion compensation blocks")
> +#define ENC_MCBLK_SIZE_LONGTEXT N_("")
Do not set empty LONGTEXT, just don't define them and use twice _TEXT in
variable defition.


> +    add_submodule()
> +    set_description( N_("Schroedinger video encoder") )
Dirac video encoder using libschrodinger, maybe?

> +    set_capability( "encoder", 200 )
Why 200 ?

> +    /* NOTE: do we need to check for Schro version here */
> +    /* advanced option only */
Check it in the configure.ac

> @@ -287,7 +741,7 @@ static picture_t *DecodeBlock( decoder_t *p_dec, block_t **pp_block )
>  
>      if( !pp_block ) return NULL;
>  
> -    if ( *pp_block ) {
> +    if( *pp_block ) {
>          block_t *p_block = *pp_block;
>  
>          /* reset the decoder when seeking as the decode in progress is invalid */

Cosmetic, ok, but should be separated, IMVHO.

> +#define SCHRO_SET_ENUM(list,psz_name, psz_name_text, psz_value) \

maybe some static inline would be better, if it was possible.

> +    if( list && psz_name_text && psz_name && psz_value ) { \
> +        int i; \
> +        int i_index = -1; \
> +        int i_list_size = ARRAY_SIZE(list); \
> +        for( i = 0; i < i_list_size; ++i ) { \
declare int in the for.


> +static int OpenEncoder( vlc_object_t *p_this )


> +    p_sys->p_schro = schro_encoder_new();
Can't schro_encoder_new return NULL?
Then, it should be checked before next instruction:

> +    p_sys->p_format = schro_encoder_get_video_format( p_sys->p_schro );
Can schro_encoder_get_video_format return NULL? (I guess not)


> +    /* constants set from the input video format */
> +    p_sys->p_format->width = p_enc->fmt_in.video.i_width;
> +    p_sys->p_format->height = p_enc->fmt_in.video.i_height;
> +    p_sys->p_format->frame_rate_numerator = p_enc->fmt_in.video.i_frame_rate;
> +    p_sys->p_format->frame_rate_denominator = p_enc->fmt_in.video.i_frame_rate_base;
cosmetic alignment around the "=" would be cool, I guess.

> +    psz_tmp = var_GetString( p_enc, ENC_CFG_PREFIX ENC_RATE_CONTROL );
> +    if( !psz_tmp )
> +        goto error;
> +    else if( *psz_tmp != '\0') {
> +        SCHRO_SET_ENUM(enc_rate_control_list, ENC_RATE_CONTROL, ENC_RATE_CONTROL_TEXT, psz_tmp);
> +    }
> +    free( psz_tmp );

You are using this pseudo-code a lot, can you do some macros for that?

Probably same remark for Float and Integer.


> +    /* NOTE: do we need to check to Schro version here */
Same remark as above.

> +    /* Allocate the buffer for inputing frames into the encoder */
> +    if( ( p_sys->p_buffer_in = malloc( p_sys->i_buffer_in ) ) == NULL )
Use unlikely( ) here

> +    p_free = malloc( sizeof( *p_free ) );
Check for NULL


It seems that most of the code is options handling. Would it be possible
to automatize this part and reduce the code size?
I am not sure we need proper internalization of all the options, but
maybe I am wrong.

Best Regards,

-- 
Jean-Baptiste Kempf
http://www.jbkempf.com/
+33 672 704 734