[vlc-devel] [PATCH 1/1] codec/dirac: Rewrite libdirac encoding support; Remove decoding support
David Flynn
davidf+nntp at woaf.net
Mon Nov 10 19:08:08 CET 2008
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
--
More information about the vlc-devel
mailing list