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

[Laurent Aimar]

On Mon, Nov 10, 2008, David Flynn wrote:
> 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.
 Yes, but it is not that simple and bool is not adequate for your need.
If we have a bool with 'unspecified state' it becomes a tri-states variable
and not a bool.
 There is no GUI and no command line support for it.
 Besides, whatever your configuration file is, it is better to be able to
(re)set from the command line, a variable in the state 'undefined/auto'. So
the special value need to be accessible.

 The workaround I see, is to use 
 - a string list with something like "auto", "on", "off"
 - an integer list with -1, 0, 1 and comments.
 - an integer as you do but with a range including -1
 - a new variable type (a lot of work)

> 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.
 As said, you should simply set the valid range to [-1, 1]. This way the
'auto' value can be set from gui/command line.

> >> +        p_sys->ctx.enc_params.trate = i_tmp;
> >  What will happens with 0 (may be ok)?
> 
> Thats fine (enables constant quality mode)
 ok.

> 
> >> +            /* 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.
 Well bad copy past+mix up, sorry. I meant :
 p_sys->p_dirac->enc_buf.size >= p_sys->i_buffer_out
(not sure, as the dirac library may stop writing before the last
byte).
 If not that easy, not blocking.

> >> +    block_FifoRelease( p_sys->p_dts_fifo );
> >  Test against NULL needed.
> 
> Nicely documented.
 Well, unless specifically stated, NULL is forbidden in all (release) functions.

> Nice that it isn't an issue for ChainRelease, but is for FifoRelease.
 As not stated in the short comment, you should not rely on that fact.
It's more by luck that ChainRelease does not segfault on NULL (while() vs do while()).

-- 
fenrir