[vlc-devel] [PATCH 2/2] Add a simple raw hevc demux

[Denis Charmet]

Hi,

Le mardi 11 février 2014 à 06:22:30, Vittorio Giovara a écrit :
> > +    for( int32_t i = 0; i < max_sub_layer_minus1; i++ )
> 
> 
> Shouldn't this be <= ?

Not if I believe the spec.
> > +    {
> > +        sub_layer_profile_present_flag[i] = bs_read1( bs );
> > +        sub_layer_level_present_flag[i] = bs_read1( bs );
> > +    }
> > +
> > +    if(max_sub_layer_minus1 > 0)
> > +        bs_skip( bs, (8 - max_sub_layer_minus1) * 2 );
> > +
> > +    for( int32_t i = 0; i < max_sub_layer_minus1; i++ )
> 
> 
> Ditto
 
Same

> >[...]
> >+    int32_t max_sub_layer_minus1 = bs_read( &bs, 3 );
> 
> 
> IMHO I find it clearer when the _minus1 is dropped from the variable and +1
> is summed to it, especially if you have to loop on them (but that's just a
> personal opinion

I may have done it if the loops were actually on max_sub_layer but none
of them is. I won't add a one just to substract it the line after.

I'm keeping the norm variable naming just to make it more readable when
you have to double check the code with the norm.

> > +    bs_skip( &bs, 17 );
> > +
> > +    skipProfileTiersLevel( &bs, max_sub_layer_minus1 );
> > +
> > +    int32_t vps_sub_layer_ordering_info_present_flag = bs_read1( &bs );
> > +    int32_t i = vps_sub_layer_ordering_info_present_flag? 0 :
> > max_sub_layer_minus1;
> > +    for( ; i <= max_sub_layer_minus1; i++ )
> 
> 
> Slightly hard to read, wouldn't a plain
> if (vps_sub_layer_ordering_info_present_flag)
> be just as effective?
> 

As effective yes, more readable is a matter of personal tastes and I find
the ternary operator quite readable.

Regards,

-- 
Denis Charmet - TypX
Le mauvais esprit est un art de vivre