[vlc-devel] [PATCH 1/1] codec/dirac: Rewrite libdirac encoding support; Remove decoding support

[David Flynn]

On 2008-11-10, Laurent Aimar <fenrir at via.ecp.fr> wrote:
> On Mon, Nov 10, 2008, David Flynn wrote:
>> On 2008-11-10, Laurent Aimar <fenrir at via.ecp.fr> wrote:
>> > On Mon, Nov 10, 2008, davidf+nntp at woaf.net wrote:
>> >> +#define ENC_BUFSIZE 1024*1024*10
>  Well assuming the standard worst case is 1:1 you have width*height*3/2 for
> i420 which is more than 10Mbytes for 8K video.
>  Anyway, it is a regression from the previous code if I am reading it right.
>  I would like to avoid that. A __MAX with the a*w*h and a constant value (even
> 10Mbytes) is a possible solution.

Previous version used 1MB.  I'll use the __MAX() route.

>  An error log when it is not enough for the output would be usefull to detect
> errors.

I shall see if that is possible.

>> >> +#define PTS_TLB_SIZE 32
>  Could you then add a comment. Compared to the encodeur buffer size, increasing
> it to 256 will not cost much but will be safer (32 B is not that big if you use
> hierarchical encoding, at least it's a possible case in h264).

Status: FIXED

>
>> >  +    encoder_sys_t *p_sys = p_enc->p_sys;
>> >> +    for( int i=0; i<PTS_TLB_SIZE; i++) {
>> >> +        p_sys->pts_tlb[i].i_empty = 1;
>> >  Isn't i_empty used as a bool and its types changed to it ?
>> 
>> It is a bool; i don't see much point in the C99 boolean type given the
>> stupid assumptions of compilers.
>  If it is a bool, then please use a bool instead of an int.

I'll relent.

Status: FIXED

>> >> +    p_sys->ctx.src_params.frame_rate.numerator = p_enc->fmt_in.video.i_frame_rate;
>> >> +    p_sys->ctx.src_params.frame_rate.denominator = p_enc->fmt_in.video.i_frame_rate_base;
>> >> +    vlc_ureduce( (unsigned*) &p_sys->ctx.src_params.pix_asr.numerator,
>> >> +                 (unsigned*) &p_sys->ctx.src_params.pix_asr.denominator,
>> >  Those casts seem ugly, using temporary variable would be better.
>> 
>> It is only shutting up the compiler.  How about:
>> &(unsigned)p_sys->...
>> ?
>  Using two temporary variable is still cleaner, it will avoid a cast that may hide
> a type change in the future.

Status: FIXED

>> The encoder needs to know what coding_mode to use (progressive or
>> interlaced).  If using automatic guessing, the first point that vlc
>> knows that the sequence is interlaced is by looking in the picture_t.
>  Ok, I see. Becarefull that you may have mixed contents (for example with
> TV channels).

Yes -- there is a /* TODO: detect change of interlace */ or somesuch at
the start of the function.

The encoder won't blow up in your face -- the quality just won't be as
good as it should be.  The eventual plan is for interlaced detection to
be moved inside the encoder -- it then becomes a non issue.

> Also, could you check the succes of:
>> + p_sys->p_dts_fifo = block_FifoNew();
> and
>> + p_sys->p_buffer_in = malloc( p_sys->i_buffer_in );
> in OpenEncoder() ?

Status: FIXED

> As a general coding style remark, you are mixing
> if ()
>  and
> if( )
> (if or while or for...).  Could you choose one and make the code consistant ?

Ok, i've been trying to keep in style by using if( foo ), must've missed
some.


> Same thing with 
> if() {
> }
> and if()
> {
> }

My personal taste (using the above vlc parenthesis placement) is to use:

if( ... ) {
  /* something short */
}

if( ( ... .... ... ) ||
    (              ) )
{
  /* any size */
}

but it does vary according to (a) what was there before, (b) asthetics
of the block of code, (c) amount of real estate to loose

I'll try and make it a little more vlc style.

..david

--