[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, 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.

Status: Dont want to fix in this patch.


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

The TLB is used for producing the correct PTS timestamp at the encoder
output and the FIFO is used for producing the correct DTS.

The only fix is to malloc the table based upon l1sep and some fudge
factor.  something > 32 frames is very very very long gop.

Status: Unknown

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

Status: Unknown

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

Status: No change required

>> +    /* guess the video format based upon number of lines and picture height */
>> +    int i = 0;
>> +    VideoFormat guessed_video_fmt = VIDEO_FORMAT_CUSTOM;
>> +    do {
>> +        if( dirac_format_guess[i].i_height != p_enc->fmt_in.video.i_height )
>> +            continue;
>> +        int src_fps = p_enc->fmt_in.video.i_frame_rate / p_enc->fmt_in.video.i_frame_rate_base;
>> +        int delta_fps = abs( dirac_format_guess[i].i_approx_fps - src_fps );
>> +        if( delta_fps > 2 )
>  Wouldn't a search minimizing the error be better ?

I was intending to reorder the table so that it fell back to picking
the next smallest frame height.  It doesn't make a lot of difference;
it is mostly about trying to detect the correct 525 or 625 line
standard.

Since you've commented on it i'll make that adjustment.

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

>> +    if( val.psz_string )
>> +        free( val.psz_string );
>  Useless check (for all of them).

removed.

Status: FIXED

>> +    var_Get( p_enc, ENC_CFG_PREFIX ENC_TARGETRATE, &val );
>> +    if( val.i_int > -1 )
>> +        p_sys->ctx.enc_params.trate = val.i_int;
>  Could you use the var_GetFloat/Integer/... variants ?
> (for all of them)

ok.

Status: FIXED

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

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

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

So, this is the correct place to initialize the encoder.

(An alternative would be to do a dummy init/destroy in the Open function
to test if the options are good).

Status: Unknown

>> +        if( 1 == p_sys->ctx.enc_params.picture_coding_mode ) {
>  Isn't there a define instead of magic value ? (if it is a bool then do not
> check against a specific value).

There isn't.  It isn't a bool either.

Status: Comment added

>> +    /* store dts in a queue, so that they appear in order in
>> +     * coded order */
>> +    p_block = block_New( p_enc, 1 );
>  Using block here seems a bit overkill, but is acceptable.

Saves inventing another linked list -- i couldn't find a generic one in
vlc.

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

..david