[pulseaudio-discuss] Rewind Handling in module-lfe-lp

[Tanu Kaskinen]

On Thursday, March 28, 2013 03:33:41 PM Justin Chudgar wrote:
> On Wednesday, March 27, 2013 10:35:18 AM you wrote:
> > On Tue, 2013-03-26 at 19:34 -0700, Justin Chudgar wrote:
> > > The more focused on my little bit of code, the better, and thanks
> > > again.
> > 
> > In sink_input_pop_cb(), I think you should push to the biquad history
> > buffer the amount that you return to the caller in the "chunk" function
> > argument, that is, the amount that you processed. If the history buffer,
> > as a result, contains more data than max_rewind, you should drop the
> > excessive history data.
> 
> My initial implementation pushed into the history buffer after each
> calculation -- which occurs in the sink_input_pop_cb processing loop.
> 
> > sink_input_process_rewind_cb() is a bit more tricky, I have some trouble
> > wrapping my head around what is happening and what should be done...
> > nbytes is the amount of our old output that has been discarded and which
> > we should therefore regenerate. It sounds logical that this would be the
> > amount that we should jump back in the biquad sample history. But the
> > comment about resetting the filter is in a branch that is only executed
> > when we seek in u->memblockq, which would suggest that we should care
> > about the amount variable instead of (or in addition to) the nbytes
> > variable.
> 
> What does nbytes refer to?

I kind of answered that already in the text you quoted: "nbytes is the amount 
of our old output that has been discarded and which we should therefore 
regenerate." It's the amount of bytes that the write index of the 
render_memblockq of the sink input was moved back.

> Do I divide like:
> 
> rewind_frames = nbytes / (sizeof(u->sample_spec.format) * u-
>sample_spec.channels);

I answered this already in IRC, did you not see that? I'll paste my full reply 
(including an answer to another question):

[08:31:46] <tanuk> justinzane: Re: "anyone know a good time to catch tanuk?"
[08:32:40] <tanuk> justinzane: Now :)
[08:32:51] <tanuk> (It's morning here.)
[08:36:10] <tanuk> justinzane: That rewind_frames calculation is wrong, 
because sizeof(u->sample_spec.format) doesn't return the sample size, it 
returns the size of the format variable.
[08:36:33] <tanuk> justinzane: But if you meant that more as pseudo-code, then 
it's right.
[08:38:37] <tanuk> justinzane: There's already a convenience function for 
calculating the frame size: pa_frame_size(&u->sample_spec)

> 
> <me>grumble, doc comments</me>
> 
> > If u->memblockq contains data, it is unprocessed input, so I don't think
> > we should care about the amount of seeking we do in that buffer.
> > Unprocessed input hasn't yet had effect on our filter state. It seems to
> > me that the right thing to do is to care only about nbytes and jump back
> > in the biquad history buffer by that amount (and restore the filter
> > state accordingly).
> 
> I think I've understood, with two and a half caveats.
> 
> 1 - When does u->sink->thread_info.max_rewind get populated with a useful
> value? pa__init? Should I just sprinkle fprintf's around to see?

That's one way, or you can do

        git grep -n "thread_info.max_rewind = "

and read the code (as I have to do when figuring out an answer to your 
question).

max_rewind is set in pa_sink_set_max_rewind_within_thread(), which you call 
from sink_input_update_max_rewind_cb() and sink_input_attach_cb(). 
sink_input_attach_cb() is called when your sink input becomes "live" (attaches 
to the master sink), and it's called in pa_sink_input_put() (and also when the 
sink input moves to another sink), so yes, u->sink->thread_info.max_rewind is 
initialized during pa__init().

> 1.5 - Is the proper way to handle changes in max_rewind size just to
> realloc the history buffer and adjust the index?

That's the simplest way, and not particularly bad. The only downside is that 
it would be nice to avoid memory allocation in the realtime context, but if 
you realloc only when max_rewind gets bigger, the reallocations should be 
rare.

> 2 - max_rewind and max_request are sizes by 'size_t nbytes'. How does that
> relate to the number of frames? In other words, should I store (num_frames
> = nbytes / (sizeof(sample_spec.format) * sample_spec.channels)?

Yes, you get the number of frames by dividing the number of bytes by 
pa_frame_size().

> PS: I spent a few hours just writing doc comments for all the functions.
> That really helped me get a clearer picture of what is going on.

Nice! Note that if you plan to submit those comments for inclusion in the 
master source tree, there may be complaints about violating this coding style 
rule, if you have formatted the comments with doxygen syntax:

        11. Comments are good, use them wherever it makes sense.
            Excessive comments are bad. i.e. comment complicated code,
            don't comment trivial code. For hackers it's usually easier
            to understand well written code than prose. Use doxygen only
            for exported APIs, not for internal code.

In case you haven't done so yet, check out the full list of coding style 
rules: 
http://www.freedesktop.org/wiki/Software/PulseAudio/Documentation/Developer/CodingStyle

-- 
Tanu