Please get another mailer, this Yahoo shit is unbearable.
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?
> [...]
> >> + 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 '='.
> [...]
> >> +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.
> [...]
> >> + } 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.
Diego
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
