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

[Laurent Aimar]

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
> > If it is the size of the output buffer, and the librairie cannot to the right
> > allocation by itself, I think it would be better to compute it base on raw
> > video size (assuming that default case will not expand).
> 
> Again, i had thought of that, but i as of yet don't know what ratio to
> pick.  It is certainally more than sufficient for the moment.
 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.

> Status: Dont want to fix in this patch.
 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.

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

> >> +#define PTS_TLB_SIZE 32
> >  Is it a encoding picture reordering (for timestamp), if so doesn't the librairie
> > outputs/defines the max value ?
> 
> No, they don't.  They do define the pts_offset (units of pictures) (or at
> least will in 1.0.2).  However this isn't quite the same.  It is
> essentially the distance between traditional style 'P' frames.
 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).

> >  +    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.

> >> -        case STATE_INVALID:
> >> -            msg_Dbg( p_dec, "STATE_INVALID" );
> >> -            break;
> >> +            p_sys->pts_tlb[i].u_pnum = u_pnum;
> >> +            p_sys->pts_tlb[i].i_pts = i_pts;
> >> +            p_sys->pts_tlb[i].i_empty = 0;
> >  Could you add the return statement ? It prevents future errors.
> 
> Err, i don't follow you -- i'm guessing that the patch looks a bit
> munged there, because there is a return statement.
Oops my bad.

> >> +    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.

> >> +/* Attempt to find dirac picture number in an encapsulation unit */
> >> +static int ReadDiracPictureNumber( uint32_t *p_picnum, block_t *p_block )
> >> +{
> >> +    uint32_t u_pos = 4;
> >> +    /* protect against falling off the edge */
> >> +    while ( u_pos + 13 < p_block->i_buffer ) {
> >> +        /* find the picture startcode */
> >> +        if ( p_block->p_buffer[u_pos] & 0x08 ) {
> >> +            *p_picnum = GetDWBE( p_block->p_buffer + u_pos + 9 );
> >> +            return 1;
> >> +        }
> >> +        /* skip to the next dirac data unit */
> >> +        uint32_t u_npo = GetDWBE( p_block->p_buffer + u_pos + 1 );
> >> +        if (u_npo == 0)
> >> +            u_npo = 13;
> >> +        u_pos += u_npo;
> >  As it can overflow, I think you may have an infinite loop with corrupted
> > stream.
> 
> I don't think that is an issue here; this data is being extracted from
> the encoder which should be a valid stream by definition.
 ok.
> 
> I can add an assert(u_npo <= UINT32_MAX - u_pos) if that would make you
> feel happier. (NB, this does need to be added to a similar function
> in other places).
> 
> Status: FIXED
 Thanks.

> >> +        /* Initialise the encoder with the encoder context */
> >  Why not doing it inside the encoder Open function.
> 
> 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).

> (An alternative would be to do a dummy init/destroy in the Open function
> to test if the options are good).
 That would be better but I don't require it for the inclusion of this patch.

> >> +             /* store all extracted blocks in a chain and gather up when an
> >> +              * entire encapsulation unit is avaliable (ends with a picture) */
> >> +             block_ChainAppend( &p_sys->p_chain, p_block );
> >  It is way faster to use block_ChainAppendLast.
> 
> For upto typically one or two pointer indirections -- seems kind of
> pointless given the encoder overhead.  (for the current libdirac
> encoder, the block will be appended to the empty list head and
> immediately removed); previous versions don't do this.
 Ok.

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() ?

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 ?

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

-- 
fenrir