----- Original Message -----
> From: Diego Biurrun <[email protected]>
> To: libav development <[email protected]>
> Cc:
> Sent: Wednesday, February 29, 2012 6:44 PM
> Subject: Re: [libav-devel] [PATCH] Windows Media Audio Lossless decoder
>
> Please get another mailer, this Yahoo shit is unbearable.
I can get another mailer, but you will have to explain what exactly is
wrong with Yahoo. "this Yahoo shit is unbearable" doesn't communicate
anything to me. For the time being, I'll stick to yahoo.
>
> On Tue, Feb 28, 2012 at 06:16:59PM -0800, Mashiat Sarker Shakkhar wrote:
>> >> +static void decode_cdlms(WmallDecodeCtx *s)
>> >
>> > And what is cdlms anyway? You have so many Doxygen comments, but
>> > this is not explained anywhere.
>>
>> CDLMS is like a regular LMS filter, but only operates on a single channel
>> to reduce intra-channel redundancy. The exact acronym seems to be M$-
>> specific; and it expands to "Cascaded Least Mean Squared" (found
> it in an
>> M$ patent).
>>
>> No need to explain that in the code, anyone who knows audio codecs knows
>> what an LMS filter is. (I don't know audio codecs, and even I know what
>> an LMS filter is.) The multi-channel variant of the above is called MCLMS
>> in this code and in the spec.
>
> Let me get this straight - you had to look this up in a patent yourself
> but you feel there is no need to explain it to others?
I had to look up the exact expansion to satisfy your curiosity, the expansion
in itself is not very useful anyway. It's just another LMS filter, a class of
filter that is extensively used in audio coding. (Another thing is that, I am
not experienced enough to explain audio stuffs, I might end up writing something
misleading.)
>
>> [...]
>> >> + s->ave_sum[ch] = residue + s->ave_sum[ch] -
>> >> + (s->ave_sum[ch] >> s->movave_scaling);
>> >
>> > Indentation is off.
>> [...]
>> >> + for (icoef = 0; icoef < s->cdlms[ich][ilms].order;
> icoef++)
>> >> + pred += s->cdlms[ich][ilms].coefs[icoef] *
>> >> + s->cdlms[ich][ilms].lms_prevvalues[icoef
> + recent];
>> >
>> > Indentation is off.
>>
>> For a moment, I thought that I understood what you mean, but the two seems
>> to disagree. Indent it with reference to what? The '+=' or
> 's->' or 'pred'?
>
> Align after the '='.
Will be included in the next update of the patch. I'll also include a minor
version bump, which I keep forgetting to do.
>
>> [...]
>> >> +static void lms_update(WmallDecodeCtx *s, int ich, int ilms, int
> input,
>> > int residue)
>> >
>> > nit: long line
>>
>> I am not sure how to break it,
>
> After a comma of course.
>
>> and it's not that long a line anyway.
>
> It is > 80 characters.
>
>> [...]
>>
>> >> + av_dlog(s->avctx, AV_LOG_DEBUG, "RAWPCM %d bits
> per
>> > sample. "
>> >> + "total %d bits, remain=%d\n", bits,
>> >> + bits * s->num_channels * subframe_len,
>> > get_bits_count(&s->gb));
>> >
>> > Indentation is off.
>>
>> Again, I don't understand what's wrong with indentation here.
>
> We don't mechanically indent function arguments with four spaces;
> and you don't either in the rest of the file.
OK. So I take that you mean align the second line of arguments with the
first argument (after the opening bracket). Will be included in the next
updated patch.
>
>> [...]
>> >> + } else {
>> >> +/*
>> >> + while (get_bits_count(gb) < s->num_saved_bits
> &&
>> > get_bits1(gb) == 0) {
>> >> + av_dlog(s->avctx, AV_LOG_DEBUG,
> "skip1\n");
>> >> + }
>> >> +*/
>> >> + }
>> >
>> > ?
>> >
>> > Also, indentation is off.
>>
>> That's something the spec says we should do, but Andreas decided
>> to disable it for some reason that I don't know / understand. I'd
>> like to keep that code around in some form, may be someday we will
>> come across a sample which uses that feature.
>
> Please no commented-out cruft, much less badly formatted and completely
> without explanation.
OK. Let me check if I can find any sample the above code breaks. Then
I'd feel comfortable about removing it. Otherwise, I'm going to enable
it in the next update.
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
