On Wed, 2012-11-07 at 14:21 +0100, David Henningsson wrote:
> On 11/07/2012 09:36 AM, Tanu Kaskinen wrote:
> > Previously, if there was a hole in a recording stream,
> > pa_stream_peek() would crash. Holes could be handled silently inside
> > pa_stream_peek() by generating silence (wouldn't work for compressed
> > streams, though) or by skipping any holes. However, I think it's
> > better to let the caller decide how the holes should be handled, so
> > in case of holes, pa_stream_peek() will return NULL data pointer and
> > the length of the hole in the nbytes argument.
> >
> > This change is technically an interface break, because previously the
> > documentation didn't mention the possibility of holes that need
> > special handling. However, since holes caused crashing anyway in the
> > past, it's not a regression if applications keep misbehaving due to
> > not handing holes properly.
> >
> > When adapting pacat to this change, a dependency to sample-util was
> > added, and that required moving some code from libpulsecore to
> > libpulsecommon.
>
> It was a little confusing to see both things in one. Maybe split the
> patch in one to add what's necessary to improve pa_stream_peek, and one
> to deal with the utils?
Yes, that's definitely better.
> Btw, exactly what did you need from sample-util? Maybe it's smarter to
> move that particular thing to pulse/something.h rather than moving
> sample-util to libpulsecommon?
I need pa_silence_memory(). It might be a useful utility function for
clients also, so moving it to libpulse might make sense. On the other
hand, pa_silence_memblock() and pa_silence_memchunk() are sort of
related, so it makes sense to have them in the same place as
pa_silence_memory(), and they don't belong in the public API... I won't
do changes for now. If you have a good concrete plan about what to move
and where, I can definitely consider it, but I don't see moving stuff
from libpulsecore to libpulsecommon as a very significant issue, so I
don't want to spend too much time thinking about it.
> > @@ -257,24 +258,36 @@ static void stream_read_callback(pa_stream *s, size_t length, void *userdata) {
> > return;
> > }
> >
> > - pa_assert(data);
> > pa_assert(length > 0);
> >
> > - if (buffer) {
> > - buffer = pa_xrealloc(buffer, buffer_length + length);
> > - memcpy((uint8_t*) buffer + buffer_length, data, length);
> > - buffer_length += length;
> > - } else {
> > - buffer = pa_xmalloc(length);
> > - memcpy(buffer, data, length);
> > - buffer_length = length;
> > - buffer_index = 0;
> > + /* If there is a hole in the stream, we generate silence, except
> > + * if it's a passthrough stream in which case we skip the hole. */
> > + if (data || !(flags & PA_STREAM_PASSTHROUGH)) {
>
> It looks like this can be more elegantly rewritten as
>
> if (!buffer)
> buffer_length = 0;
> as reallocs can take null pointers.
>
> > + if (buffer) {
> > + buffer = pa_xrealloc(buffer, buffer_length + length);
> > + if (data)
> > + memcpy((uint8_t *) buffer + buffer_length, data, length);
> > + else
> > + pa_silence_memory((uint8_t *) buffer + buffer_length, length, &sample_spec);
> > + buffer_length += length;
> > + } else {
>
> ...and then the entire section handling this case can be removed.
Very good suggestion. I didn't know/remember the exact realloc behavior.
--
Tanu
