[pulseaudio-discuss] [PATCH] stream: Return error in case a client peeks to early

[David Henningsson]

On 11/04/2012 02:22 PM, Tanu Kaskinen wrote:
> On Sat, 2012-11-03 at 19:51 +0100, David Henningsson wrote:
>> 2012-11-03 17:19, Colin Guthrie skrev:
>>> 'Twas brillig, and Tanu Kaskinen at 05/10/12 13:58 did gyre and gimble:
>>>> On Wed, 2012-10-03 at 08:50 +0200, David Henningsson wrote:
>>>>> On 10/02/2012 10:38 PM, Tanu Kaskinen wrote:
>>>>>> On Mon, 2012-10-01 at 17:06 +0200, David Henningsson wrote:
>>>>>>> If there is no silence memblock and no data, pa_memblockq_peek can
>>>>>>> return NULL. In this case, do not crash on an assertion in
>>>>>>> pa_memblock_acquire, but instead return a proper error to the client.
>>>>>> If there is no data in the buffer, pa_stream_peek() is supposed to
>>>>>> return NULL according to the documentation. And it does that: if there's
>>>>>> no data, pa_memblock_peek() will return a negative value, causing
>>>>>> pa_stream_peek() to return NULL.
>>>>>>
>>>>>> The problem is the case where the buffer does contain data, but not at
>>>>>> the read index. That is, there is a hole in the buffer. The client
>>>>>> documentation doesn't have any warnings about holes, so the only safe
>>>>>> way to handle holes is to return silence. Fixing this should be a simple
>>>>>> matter of giving a silence memchunk when creating record_memblockq.
>>>>> I'm not so sure. Silence, as in all zeroes, might work for S16 audio
>>>>> data, but what about other formats? Compressed audio? Peak audio (which
>>>>> I think is the case here)? Etc.
>>>> Good point. Regarding PCM, if pa_memchunk_silence() is used, the
>>>> function will take care of filling the memory with appropriate content.
>>>> But that doesn't work with compressed audio.
>>>>
>>>>> Also maybe it could also be valuable for the client to distinguish
>>>>> between no data available, and valid zero data.
>>>>>
>>>>> How about returning NULL and adding to the documentation something like:
>>>>>
>>>>> -If no data is available this will return a NULL pointer.
>>>>> +If no data is available (at the current read position), this will
>>>>> return a NULL pointer.
>>>> An addition: the client probably wants to know how large the hole is. It
>>>> might be possible to figure that out somehow from the read index, but I
>>>> think it would make sense to return the hole size in the length
>>>> parameter.
>>> This discussion seemed to stagnate. Is this worth fixing/documenting for
>>> the 3.0 release?
>>>
>> Returning NULL seems to be the right thing to do here, even if
>> gnome-control-center does not handle that very well IIRC. So we might
>> need an additional patch in g-c-c.
>> So assuming I commit a patch doing that. If somebody else wants to add
>> logic to figure out how large the hole is, that could be discussed
>> separately.
>> Any objections?
>
> It's not clear what you meant by "add logic to figure out how large the
> hole is". Add to where? pa_stream_peek() or gnome-control-center?

I was referring to your earlier comment "An addition: the client 
probably wants to know how large the hole is.", i e pa_stream_peek.

> To me, reporting the hole length in the "nbytes" parameter of
> pa_stream_peek() seems like the right thing to do, so I hope your patch
> will do this.

It does not: I was just suggesting to discuss that separately.


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