On Wed, Feb 29, 2012 at 06:38:41AM -0800, Mashiat Sarker Shakkhar wrote: > ----- 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.
lol, that code won't do anything useful except for eating some CPU cycles and printing an arbitrary number of "skip1\n" messages in debug mode _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
