[FFmpeg-devel] [PATCH] ALS decoder

Hervé W. H.O.W.aka.V+ffmpeg
Sun Aug 23 00:47:49 CEST 2009


Comments are inline. (My use of "perhaps" is not sarcasm or something).

2009/8/22 Thilo Borgmann <thilo.borgmann at googlemail.com>:

[...]

>>> + ? ?int bgmc_mode; ? ? ? ? ? ?///< BGMC Mode: 1 = on, 0 = off (Rice coding only)
>>
>> bgmc?
> Done.

I'm not sure, but it could be he intended for you to explain what BGMC
is (or give the full name in the comment)

>>> + ? ?int rlslms; ? ? ? ? ? ? ? ///< use RLS-LMS predictor: 1 = on, 0 = off
>>
>> RLSMLS?
>> no i did not read the spec ...
> It is correct as it is.

ditto

>>> + ? ?int aux_data_enabled; ? ? ///< indicates that auxiliary data is present
>>
>>> + ? ?int chan_config_info; ? ? ///< mapping of channels to loudspeaker locations
>>
>> never read
> But a TODO is set to use it in a future release.

I think the guideline is: don't add it until you need it.
Unless you can guarantee that you'll be around to implement that TODO.

>>> + ? ?int *chan_pos; ? ? ? ? ? ?///< original channel positions
>>
>> same
> Same.
> Do these hurd so much it has to be reinserted later?

Does it hurt so much to reinsert them later?

[...]

>>> + ? ? ? ? ? ? ? ?// read coefficients 2 to opt_order
>>> + ? ? ? ? ? ? ? ?for (k = 2; k < opt_order; k++) {
>>> + ? ? ? ? ? ? ? ? ? ?quant_index = get_bits(gb, 7) - 64;
>>> + ? ? ? ? ? ? ? ? ? ?quant_cof[k] = (quant_index << 14) + (1 << 13);
>>
>> the +- can be combined
> Sorry I can't see that..

Perhaps
quant_index = get_bits(gb, 7);
quant_cof[k] = (quant_index << 14) + (7F << 13);

is intended. I haven't looked at the rest of the related code.

>>> + ? ? ? ? ? ? ? ?k_max = FFMIN(20, opt_order);
>>> + ? ? ? ? ? ? ? ?for (k = 2; k < k_max; k++) {
>>> ...
>>> + ? ? ? ? ? ? ? ?}
>>> ...
>>> + ? ? ? ? ? ? ? ?k_max = FFMIN(127, opt_order);
>>> + ? ? ? ? ? ? ? ?for (k = 20; k < k_max; k++) {
>>
>> k=20 is redundant
>>
>>
>>> + ? ? ? ? ? ? ? ?// read coefficients 127 to opt_order
>>> + ? ? ? ? ? ? ? ?for (k = 127; k < opt_order; k++) {
>>
>> so is k=127
> No, this is correct.
> ?for(; k < 20;)
> ?for(k = 20;;)

Correct, yes. Also redundant.
If opt_order < 20 (for example 16)
then at the end of the 1st loop k = 16
At the start of the 2nd loop 16 is still bigger than k_max (which is
equal to opt_order, right? ).
3rd loop 16 is still bigger than opt_order.

if opt_order > 127
then at the end of the 1st loop k = 20
(k=19 //at the start of last loop
do something;
k++ // end of loop
compare k to k_max --> end loop)

Therefore k is already 20 at the start of the 2nd loop.

>>> + ? ? ? ? ? ?memcpy(ctx->prev_raw_samples, raw_samples - sconf->max_order,
>>> + ? ? ? ? ? ? ? ? ? sizeof(int64_t) * sconf->max_order);
>>
>> instead of duplicating the type (int64_t) the type of the array should be used
> Sorry I don't understand this. Please elaborate.

Perhaps
sizeof(typeof(array)) * sconf->max_order);


-V



More information about the ffmpeg-devel mailing list