[Spice-devel] [PATCH 2/2] audio: Avoid bad pipelines from gst_parse_launch

[Victor Toso]

Hi,

----- Original Message -----
> On Tue, 2014-11-25 at 17:25 +0100, Victor Toso wrote:
> > gst_parse_launch may return not NULL even when error is set.
> > This can lead to data loss when playing or recording audio.
> > ---
> >  gtk/spice-gstaudio.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/gtk/spice-gstaudio.c b/gtk/spice-gstaudio.c
> > index 5f9abb2..d6cc774 100644
> > --- a/gtk/spice-gstaudio.c
> > +++ b/gtk/spice-gstaudio.c
> > @@ -245,7 +245,7 @@ static void record_start(SpiceRecordChannel *channel,
> > gint format, gint channels
> >                              "appsink caps=\"%s\" name=appsink",
> >                              audio_caps);
> >  
> >          p->record.pipe = gst_parse_launch(pipeline, &error);
> > -        if (p->record.pipe == NULL) {
> > +        if (error != NULL) {
> >              g_warning("Failed to create pipeline: %s", error->message);
> >              goto lerr;
> >          }
> > @@ -269,6 +269,10 @@ static void record_start(SpiceRecordChannel *channel,
> > gint format, gint channels
> >  #endif
> >  
> >  lerr:
> > +        if (error != NULL && p->record.pipe != NULL) {
> > +            gst_object_unref(p->record.pipe);
> > +            p->record.pipe = NULL;
> > +        }
> 
> It's kind-of related to your code here ...
> I don't see a reason to have the goto and this lerr label here.
> - audio_caps could only be set when the pipeline is NULL and freed right
> inside this context.
> - pipeline could be freed just after gst_parse_launch() and right before
> the error checking.
> - the error checking could cleanup the error and pipe inside the if and
> simply return here.

I agree that the code could be a bit simpler. I tried to keep the current design while we are dealing with two versions of gstreamer.
Maybe we could do the refactoring when we do drop the gst 0.10 dependency? (removing ifdefs, for instance..)

> 
> 
> >          g_clear_error(&error);
> >          g_free(audio_caps);
> >          g_free(pipeline);
> > @@ -345,7 +349,7 @@ static void playback_start(SpicePlaybackChannel
> > *channel, gint format, gint chan
> >                                         "audioconvert ! audioresample !
> >                                         autoaudiosink name=\"audiosink\"",
> >                                         audio_caps);
> >          SPICE_DEBUG("audio pipeline: %s", pipeline);
> >          p->playback.pipe = gst_parse_launch(pipeline, &error);
> > -        if (p->playback.pipe == NULL) {
> > +        if (error != NULL) {
> >              g_warning("Failed to create pipeline: %s", error->message);
> >              goto lerr;
> >          }
> > @@ -355,6 +359,10 @@ static void playback_start(SpicePlaybackChannel
> > *channel, gint format, gint chan
> >          p->playback.channels = channels;
> >  
> >  lerr:
> > +        if (error != NULL && p->playback.pipe != NULL) {
> > +            gst_object_unref(p->playback.pipe);
> > +            p->playback.pipe = NULL;
> > +        }
> >          g_clear_error(&error);
> >          g_free(audio_caps);
> >          g_free(pipeline);
> 
> The comments more or less apply to this hunk as well. But here I think a
> goto exit would make sense. The label exit, however, should be in the
> line 373 (just before the if (p->mmtime_id ...))
> 
> Best Regards,
> --
> Fabiano Fidêncio
> 
>