[vlc-devel] [PATCH 1/1] codec/dirac: Rewrite libdirac(-research) encoding support

[David Flynn]

On 2008-11-10, Laurent Aimar <fenrir at via.ecp.fr> wrote:
> Hi,
>
>> +#define ENC_PREFILTER "prefilter"
>> +#define ENC_PREFILTER_TEXT N_("Prefilter")
>> +#define ENC_PREFILTER_LONGTEXT N_("Enable adaptive prefiltering")
>> +static const char *const enc_prefilter_list[] =
>> +  { "none", "cwm", "rectlp", "diaglp" };
>> +static const char *const enc_prefilter_list_text[] =
>> +  { N_("none"), N_("cwm"), N_("rectlp"), N_("diaglp") };
>  Could you change them to what they mean ?
> (not blocking).

FIXED

>> +    add_integer( ENC_CFG_PREFIX ENC_TARGETRATE, -1, NULL,
>> +                 ENC_TARGETRATE_TEXT, ENC_TARGETRATE_LONGTEXT, false );
>> +    change_integer_range(0, INT_MAX);
>
>  I am not sure but using a default value of -1 and then a range of [0, >0]
> is probably buggy (and it may depends weither your conf file has been saved or
> not).
> (There is a lot of options in this case).

This is to work around a deficiency in VLC's options/command line
handling.  I want to be able to express optionality, eg in Haskell:

Maybe Integer
can be one of: Nothing | Just Integer

Ie, i want the ability to have a "don't care" option.  For almost all
the cases where i've specified -1 or "", i use it to allow the encoder
to pick a default, rather than try and do that inside vlc.

Eg, for some of the values which were add_bool, i could set them to -1
and var_Get() would return them as -1.  Unfortunately var_GetBool casts
it to _Bool which looses that effect, so i've had to use an Integer with
-1 default and valid range 0,1.

If this can't be done and you still insist on it not being set to say
-1, then i'll just delete all the options that are impossible.

>> +/*****************************************************************************
>> + * picture_pts_t : store pts alongside picture number, not carried through
>> + * encoder
>> + *****************************************************************************/
>> +struct picture_pts_t
>> +{
>> +   bool i_empty;      /* entry is invalid */
>  b_ iof i_

Ooops,
FIXED

>> +        p_sys->pts_tlb[i].i_empty = 1;
>  true iof 1.
>> +            p_sys->pts_tlb[i].i_empty = 0;
>  false iof 0.
>> +             p_sys->pts_tlb[i].i_empty = 1;
>  Idem.
SIGH,
FIXED.

>> +    if( ( p_sys->p_dts_fifo = block_FifoNew() ) == NULL )
>> +    {
>> +        CloseEncoder( p_this );
>               ^
>> +        return VLC_ENOMEM;
>> +    }
>> +
>> +    p_enc->p_sys = p_sys;
>               ^
>  You cannot call CloseEncoder before that line.

FIXED

>  psz_tmp may be NULL in case of allocation failure.`
Pointless allocation failure checks be damned.
I've just gone and deleted all the validation of psz_tmp sice it was
pointed out it returned "", i'm a little peeved at having to put them
all back.

(FIXED).

>  Beside it can either be NULL or "" (empty) for empty/non set options.
> I think you may want to use var_GetNonEmptyString (ie it will returns NULL
> iof "") and fix the test.
>  (Idem for the following calls to var_GetString)

FIXED.


>> +        p_sys->ctx.enc_params.trate = i_tmp;
>  What will happens with 0 (may be ok)?

Thats fine (enables constant quality mode)

>
>> +    psz_tmp = var_GetString( p_enc, ENC_CFG_PREFIX ENC_ME_SIMPLESEARCH );
>> +    if( *psz_tmp != '\0' ) {
>> +        /* of the form [0-9]+:[0-9]+ */
>> +        char *psz_start = psz_tmp;
>> +        char *psz_end = psz_tmp;
>> +        p_sys->ctx.enc_params.x_range_me = strtol(psz_start, &psz_end, 10);
>> +        if( *psz_end != ':'  || psz_end == psz_tmp ) {
>                                                  ^
>  Using psz_start may be easier to understand (and more consistant with next
> test).
> (not blocking)

FIXED

>> +    p_block = block_New( p_enc, 1 );
>> +    p_block->i_dts = p_pic->date - p_sys->i_pts_offset;
>> +    block_FifoPut( p_sys->p_dts_fifo, p_block );
>  Missing test on p_block.

FIXED

>> +            /* extract data from encoder temporary buffer. */
>  There, if p_sys->p_dirac->enc_buf.size >= p_sys->p_dirac I think you can
> warn about (probable) output buffer overflow.

I dont think it works like that.

>> +    if( p_sys->p_buffer_in )
>> +        free( p_sys->p_buffer_in );
> (not blocking)

FIXED

missed a missing: free( p_sys->p_buffer_out );
FIXED

>> +    block_FifoRelease( p_sys->p_dts_fifo );
>  Test against NULL needed.

Nicely documented.
Nice that it isn't an issue for ChainRelease, but is for FifoRelease.
FIXED.