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

[David Henningsson]

On 2014-05-06 12:14, Tanu Kaskinen wrote:
> 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.

Changed to size_t. It did not seem to cause many warnings.

>
>>>> +    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.

Comment added in next revision.

> 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.

Ah, good catch, fixed.

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic