On Wed, 2013-04-03 at 16:29 -0700, Justin Chudgar wrote: > Gentlemen: > > I believe that I have gotten rewind working, and I have moved the biquad > filtering specific code to its own location in order to be able to share it with > the forthcoming "module-xover-sink" that is intended to correctly implement > remixing with a Linkwitz-Riley 4th order crossover. I have started to cleanup > the code to meet the style guide, as well. > > Would you kindly take a look at the code in: > > https://github.com/justinzane/pulseaudio/tree/master/src/modules > > - module-lfe-lp.c > - biquad/* > - Makefile.am > > and let me know what more I need to do to get finished with module-lfe-lp and > move onto module-xover-sink (oh, and heftig, feel free to name it whatever you > think is proper, you idea = your naming rights :) ). (Continuing review. I noticed that I complained yesterday about several things that Alexander had already mentioned. Sorry, Alexander, for ignoring your message.) In sink_input_process_rewind_cb(): if (u->s1histbuf->idx > rewind_samples) { //we're cool, no wrap required u->s1histbuf->idx -= rewind_samples; } The comparison operator should be ">=" instead of ">". Only the stage 1 history buffer idx is moved backwards, the stage 2 history buffer idx is kept unchanged. // add the 2 frame backlog for the biquad rewind_frames += 2; The increment should be 3 instead of 2. The last frame in the history contains the most recent (x0, y0) pairs, and the history idx points to the place where the next frame will be start. Let's imagine that we want to rewind 0 frames and restore the filter state from the history buffer (a silly thing to do, but this is just an illustrative example). We need to restore the last three frames from the history buffer, i.e. read buffer[idx - 3 * channels + i], buffer[idx - 2 * channels + i] and buffer[idx - 1 * channels + i] for each channel i. Buffer[idx] is invalid data, we don't want to access that. So, as you can see, with rewind size 0 we need to go back 3 frames. With rewind size 1 we would go back 4 frames etc. The history buffer indexing in sink_input_process_rewind_cb() looks wrong: frame_size = u->sample_spec.channels * sizeof(biquad_data_element); for (i = 0; i < u->sample_spec.channels; i++) { switch (u->filter_map[i]) { case 'l': { // stage 1 u->s1lpdt[i].w2 = u->s1histbuf->buffer[i].w0; u->s1lpdt[i].w1 = u->s1histbuf->buffer[i + frame_size].w0; u->s1lpdt[i].w0 = u->s1histbuf->buffer[i + 2 * frame_size].w0; u->s1histbuf->buffer is an array of biquad_data_elements, not a byte array, so you don't need to care about the frame size in bytes. Instead of frame_size, you should use the number of channels as the multiplier. Also, s1histbuf->idx is not taken into account, so the data is always read from the beginning of the history buffer. So, the last line, for example, should look like this: u->s1lpdt[i].w0 = u->s1histbuf->buffer[history_idx + 2 * channels + i].w0; ...and that's still broken, because it doesn't take into account that history_idx may point to the last or second last frame of the buffer, causing invalid memory to be accessed due to the addition of "2 * channels". // move the buffer forward 2 frames so we can write right u->s1histbuf->idx += frame_size; u->s2histbuf->idx += frame_size; The increment amount should be "3 * channels" instead of "frame_size". I said this already in an earlier mail: the "(5) PUT YOUR CODE HERE TO REWIND YOUR FILTER" comment seems to be in the wrong place, because the "amount" variable in process_rewind_cb() is the amount that u->memblockq's write index is moved backwards, but the filter actually operates where u->memblockq's read index points to. process_rewind_cb() may apparently in some situations move the read index more than the write index, so you should not use "amount" as the amount to rewind the biquad history buffer, because that may be too little. Instead, use "rewind_bytes", because that's the amount that u->memblockq's read index is moved, and move the biquad rewinding code outside the "if (u->sink->thread_info.rewind_nbytes > 0)" block. The references to bug 53709 aren't correct, because your module doesn't contain any buffering that would increase latency. The whole bug might be invalid, because I created it based on incorrect assumptions, but it might apply to some modules. The "if (max_rewind == u->sink->thread_info.max_rewind)" check in update_max_rewind_cb() is not necessary. It's not an unexpected situation if the function is called even if there ends up being no difference to the old value. It can happen when creating or moving the sink input. The "if ( (max_rewind / u->sz_frm) < MIN_MAX_REWIND_FRAMES)" check is unnecessary too. There's a comment saying that the check exists to avoid excessive reallocing, but that can be avoided simply by reallocing only when the history buffer is too small. There's no good reason to ever shrink the history buffer. Sure, it can save some memory in some situations, but I don't think it's worth the effort. The field names in biquad_data_element could be just x and y instead of x0 and y0, since there's only only one of each, and they don't have any particular connection to biquad_data's x0 and y0 (the history data applies equally much to x1, x2, y1 and x2 too). growth = (max_rewind / u->sz_smp) - u->s1histbuf->length; should be growth = (max_rewind / u->sz_smp) + 3 * channels - u->s1histbuf->length; because max_rewind doesn't contain the 3 extra frames that you need to keep in the history. The history buffers are completely wiped in update_max_rewind_cb(). The history data must not be lost when resizing the buffer. Only the new empty space in the buffer should be zeroed. Note that before zeroing anything, you need to move the data after idx to the end of the new buffer, and then zero the area between idx and the beginning of the moved part. You could add a comment above each function in module-lfe-lp.c telling from which thread they are called. Hmm, actually, contrary to what I have written in an earlier mail, I think you don't need to (and shouldn't, at least using the current code, because it has some bugs) allocate the history buffers in sink_input_attach_cb(). It seems that sink_input_update_max_rewind_cb() will always be called after the attach() callback (see PA_SINK_MESSAGE_ADD_INPUT and PA_SINK_MESSAGE_FINISH_MOVE handling in pa_sink_process_msg() in sink.c), so it's sufficient to do the buffer allocation in sink_input_update_max_rewind_cb(). The long comment in pa__init() about the history buffer initialization should be cleaned up, even though it's nice to be quoted :) It contains some incorrect information (see the previous paragraph). So update_max_rewind_cb() will always be called after attach_cb(). The same is true for update_max_request_cb(). This means that the pa_sink_set_max_request_within_thread() and pa_sink_set_max_rewind_within_thread() calls in attach_cb() are redundant, because they will be called anyway again from the corresponding update callbacks. Those redundant calls could be removed from module-virtual-sink.c too (added to my todo list). sink_input_may_move_to_cb() is entirely redundant, because the loop check that it does is nowadays implemented in the core code. (I have a todo item for removing similarly redundant may_move_to() implementations from other modules too.) You use pa_proplist_gets(u->sink->proplist, "device.vsink.name") in sink_input_moving_cb(), but you don't set that property anywhere. module-virtual-sink seems to set "device.vsink.name" to the name of the virtual sink, so you could do the same, but I don't really see the point. You can just use u->sink->name directly. In pa__init(): u->lpfreq = atof(pa_modargs_get_value(ma, "lpfreq", "100.0")); atof() doesn't provide proper error handling. Use pa_modargs_get_value_double() instead. The pa_modargs_get_sample_spec_and_channel_map() call is redundant, because your module doesn't accept any arguments that could modify the sample spec or channel map. Perhaps it should accept some, though? The pa_memblockq_new() call uses tabs to align the comments, but at least in my browser code the comments aren't result is not a neatly aligned in the github code view. Please never use use tabs for anything (except makefiles) when working on PulseAudio. Use pa_xnew0() instead of malloc() and calloc() and pa_xrealloc() instead of realloc(). And pa_xfree() instead of free(). The "fail" label in pa__init() should be on its own line and should have no indentation. MIN_CUTOFF_FREQ and MAX_CUTOFF_FREQ seem to be used only for sanity checking the module arguments, and the biquad filters could also work with values outside that range, so I think it's better to move those constants inside module-lfe-lp.c. If the constants were kept in biquad-filter.h, they should be prefixed with "PA_BIQUAD_". MEMBLOCKQ_MAXLENGTH doesn't belong in biquad-filter.h either, nor the PI constant. I don't think the PI constant is needed at all, just use M_PI everywhere. you have #ifndef M_PI, is there some platform that doesn't have M_PI defined in math.h? More about the copyright notice: if you are going to choose GPLv3 as the license, the copyright notice at the top of the files shouldn't make statements about PulseAudio, because PulseAudio is covered by a different license in general. You should limit the notice so that it only talks about your own code. Also, the doxygen tags at the top of the files are redundant and IMHO ugly. I said yesterday that the "extern" specifiers are useless in biquad-filter.h. For completeness, I'll point out here that they are equally useless in biquad-filter.c. filter_biquad() should drop the "struct" specifier from the bqdt and bqfs arguments. In my opinion it would be more logical to do the x1, x2, y1 and y2 assignments before the x0 and y0 assignments in filter_biquad(). That way x2, x1 and x0 would always mean the last three input samples. With the current code, x0 and x1 both refer to the last input sample and x2 refers to the second last sample (as pointed out by Alexander too). This breaks loading state from the history, at least with the changes that I suggested (going back 2 instead of 3 extra frames when rewinding might actually successfully compensate for this "oddity" in the current filter_biquad() implementation). Q = sqrt(1.0) / 2.0; This formula is not from the EQ cookbook, so please add a comment where it comes from. Also, without any comments, this looks like an overly complicated way to say "Q = 0.5" :) ...And that's all. This became quite a long mail... the good news is, if you fix everything that has been pointed out (also by Alexander), there shouldn't be much left to do before the code can be accepted upstream. -- Tanu
