On Wed, Nov 26, 2014 at 12:10 AM, Victor Toso <victortoso@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Fabiano Fidêncio
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel