[pulseaudio-discuss] [PATCH] pulse: Fix hole handling in pa_stream_peek().

[Tanu Kaskinen]

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