[pulseaudio-discuss] [PATCH 04/11] srchannel: Add the shared ringbuffer object

[Tanu Kaskinen]

On Tue, 2014-05-06 at 00:20 +0200, David Henningsson wrote:
> >> +ssize_t pa_srchannel_write(pa_srchannel *sr, const void *data, size_t l) {
> >> +    ssize_t written = 0;
> >
> > Why ssize_t instead of size_t?
> 
> I wanted it to look like pa_iochannel_write.

I see, but the reason why pa_iochannel_write() returns a signed value is
that the function can fail. pa_srchannel_write() can not fail, but
ssize_t suggests that it can, so the choice to imitate
pa_iochannel_write() causes some minor confusion, so I'd prefer you to
use an unsigned return value.

> I prefer using int out of simplicity, as size_t always gives these 
> "compared signed and unsigned" warnings.

Do you then prefer to ignore the unsigned int type also, and always use
int for simplicity? IMO it's useful to make it obvious (and enforce)
that a variable can never be negative.

> Maybe we can just convert all size_t and ssize_t's to int instead?

I rather like size_t, because it makes it obvious that the variable
represents a byte count, so I wouldn't like such conversion.

If my arguments failed to change any of your preferences, let's agree to
disagree, and you can leave the code as it is.

> >> +    while (l > 0) {
> >> +        int towrite;
> >> +        void *ptr = pa_ringbuffer_begin_write(&sr->rb_write, &towrite);
> >> +        if ((size_t) towrite > l)
> >
> > Why is towrite an int and not size_t? Probably because
> > pa_ringbuffer_begin_write() takes an int pointer, but why doesn't it
> > take a size_t pointer?
> 
> Same answer as above.
> 
> >> +            towrite = l;
> >> +        if (towrite == 0) {
> >> +#ifdef DEBUG_SRCHANNEL
> >> +            pa_log("srchannel output buffer full");
> >> +#endif
> >> +            break;
> >> +        }
> >> +        memcpy(ptr, data, towrite);
> >> +        pa_ringbuffer_end_write(&sr->rb_write, towrite);
> >> +        written += towrite;
> >> +        data = (uint8_t*) data + towrite;
> >> +        l -= towrite;
> >> +    }
> >> +#ifdef DEBUG_SRCHANNEL
> >> +    pa_log("Wrote %d bytes to srchannel, signalling fdsem", (int) written);
> >> +#endif
> >> +    pa_fdsem_post(sr->sem_write);
> >
> > This should be sem_read?
> 
> Eh, no? Not sure why you think that?
> 
> There is a gotcha that I should probably comment though, but not sure if 
> that's what confuses you.
> There is only one semaphore that we listen on. The other one is the one 
> we always write to.
> That means that we use the same semaphore for two different meanings:
> "I've written something in my outgoing buffer to you" and
> "I've read something in my incoming buffer, and my incoming buffer was 
> full, but it is no longer full".
> 
> I e, when the signalled process is woken up it should check its incoming 
> buffer to see if there's anything to read, and if it's waiting to write 
> more, it should check if it's okay to write more as well.

Ok, using the same semaphore for both notifications was indeed what
confused me. I assumed that the intention was to use separate
semaphores. A comment would be good.

Is it so that the user callback should get called when space becomes
available in a previously full write buffer? Currently the callback is
only called when there's more data to read.

-- 
Tanu