[pulseaudio-discuss] [PATCH 08/11] client: Allow client to receive the srchannel memblock

[David Henningsson]

On 2014-05-06 13:59, Tanu Kaskinen wrote:
> On Tue, 2014-04-29 at 17:29 +0200, David Henningsson wrote:
>>
>> On 2014-04-29 15:22, David Henningsson wrote:
>>> We assume it's an srchannel memblock if it is writable and does not come
>>> from our own mempool.
>>>
>>> In a future implementation, maybe we can have more than one
>>> srchannel open at the same time, but at this point we only
>>> support one.
>>>
>>> Signed-off-by: David Henningsson <david.henningsson at canonical.com>
>>> ---
>>>    src/pulse/context.c  | 26 ++++++++++++++++++++++++++
>>>    src/pulse/internal.h |  1 +
>>>    2 files changed, 27 insertions(+)
>>>
>>> diff --git a/src/pulse/context.c b/src/pulse/context.c
>>> index f3adf4c..7456ee5 100644
>>> --- a/src/pulse/context.c
>>> +++ b/src/pulse/context.c
>>> @@ -213,6 +213,11 @@ static void context_unlink(pa_context *c) {
>>>            c->pstream = NULL;
>>>        }
>>>
>>> +    if (c->srmemblock) {
>>> +        pa_memblock_unref(c->srmemblock);
>>> +        c->srmemblock = NULL;
>>> +    }
>>> +
>>>        if (c->client) {
>>>            pa_socket_client_unref(c->client);
>>>            c->client = NULL;
>>> @@ -338,6 +343,17 @@ static void pstream_packet_callback(pa_pstream *p, pa_packet *packet, const pa_a
>>>        pa_context_unref(c);
>>>    }
>>>
>>
>> Btw, I wasn't sure if the below is a good heuristic for figuring out
>> whether this is an srchannel memblock or not. I was considering
>> (ab)using pa_seek_mode_t instead, i e, add a new seek mode indicating
>> that this is an srmemblock. What do you think?
>
> It would nice to have explicit information available. What would you
> think about adding a new memblock type, or adding function
> pa_memblock_is_srchannel_buffer()?

Hmm, looking at the code now, it feels somewhat wrong to have that 
information on memblock level. On memblock level we can deal with 
whether the block is writable from our side, the other side, or both,
but not what the memblock is being used for.

Right now the only writable-from-both-sides type is the ringbuffer 
memblock but there can be potential future, other type of uses for that 
new functionality too.

And I'm not really sure how to best transfer that type information on 
memblock level.

>
>>> +static bool is_srmemblock(pa_context *c, const pa_memchunk *chunk)
>>> +{
>>> +    if (!chunk || !chunk->memblock)
>>> +        return false;
>>> +    if (pa_memblock_is_read_only(chunk->memblock))
>>> +        return false;
>>> +    if (pa_memblock_is_ours(chunk->memblock))
>>> +        return false;
>>> +    return true;
>>> +}
>>> +
>>>    static void pstream_memblock_callback(pa_pstream *p, uint32_t channel, int64_t offset, pa_seek_mode_t seek, const pa_memchunk *chunk, void *userdata) {
>>>        pa_context *c = userdata;
>>>        pa_stream *s;
>>> @@ -350,6 +366,16 @@ static void pstream_memblock_callback(pa_pstream *p, uint32_t channel, int64_t o
>>>
>>>        pa_context_ref(c);
>>>
>>> +    if (is_srmemblock(c, chunk)) {
>>> +        if (c->srmemblock) {
>>> +            pa_log_warn("There is already an srmemblock. Ignoring the new one.");
>>> +        }
>>> +        else {
>>> +            c->srmemblock = chunk->memblock;
>>> +            pa_memblock_ref(c->srmemblock);
>>> +        }
>
> I think it would be good to return here, or somehow skip the code that
> tries to treat the memblock as audio stream data.
>
>>> +    }
>>> +
>>>        if ((s = pa_hashmap_get(c->record_streams, PA_UINT32_TO_PTR(channel)))) {
>>>
>>>            if (chunk->memblock) {
>

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