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? > > > > Col > > > > > 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? 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. -- Tanu
