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

[Fabiano Fidêncio]

On Wed, Nov 26, 2014 at 12:10 AM, Victor Toso <victortoso at redhat.com> wrote:
> 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..)

As you prefer, but it's 5 lines patch to be done before yours.

>
>>
>>
>> >          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
>>
>>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel



-- 
Fabiano Fidêncio