[Spice-devel,spice-gtk,v1,4/4] channel-display-gst: early return if gst_init failed

Submitted by Victor Toso on Oct. 18, 2016, 9:27 p.m.

Details

Message ID 20161018212707.13092-4-victortoso@redhat.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Victor Toso Oct. 18, 2016, 9:27 p.m.
From: Victor Toso <me@victortoso.com>

If gst_init() fails, gstvideo_init() will warn once about the issue
and never attempt to try again, which is probably fine.

All the other APIs that might be called such as
create_gstreamer_decoder() should not fail silently.

Signed-off-by: Victor Toso <victortoso@redhat.com>
---
 src/channel-display-gst.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index c0f0a1e..c102966 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -459,23 +459,23 @@  static gboolean gstvideo_init(void)
 G_GNUC_INTERNAL
 VideoDecoder* create_gstreamer_decoder(int codec_type, display_stream *stream)
 {
-    SpiceGstDecoder *decoder = NULL;
-
-    if (gstvideo_init()) {
-        decoder = spice_new0(SpiceGstDecoder, 1);
-        decoder->base.destroy = spice_gst_decoder_destroy;
-        decoder->base.reschedule = spice_gst_decoder_reschedule;
-        decoder->base.queue_frame = spice_gst_decoder_queue_frame;
-        decoder->base.codec_type = codec_type;
-        decoder->base.stream = stream;
-        g_mutex_init(&decoder->queues_mutex);
-        decoder->decoding_queue = g_queue_new();
-        decoder->display_queue = g_queue_new();
-
-        if (!create_pipeline(decoder)) {
-            decoder->base.destroy((VideoDecoder*)decoder);
-            decoder = NULL;
-        }
+    SpiceGstDecoder *decoder;
+
+    g_return_val_if_fail(gstvideo_init(), NULL);
+
+    decoder = spice_new0(SpiceGstDecoder, 1);
+    decoder->base.destroy = spice_gst_decoder_destroy;
+    decoder->base.reschedule = spice_gst_decoder_reschedule;
+    decoder->base.queue_frame = spice_gst_decoder_queue_frame;
+    decoder->base.codec_type = codec_type;
+    decoder->base.stream = stream;
+    g_mutex_init(&decoder->queues_mutex);
+    decoder->decoding_queue = g_queue_new();
+    decoder->display_queue = g_queue_new();
+
+    if (!create_pipeline(decoder)) {
+        decoder->base.destroy((VideoDecoder*)decoder);
+        return NULL;
     }
 
     return (VideoDecoder*)decoder;

Comments

Hey,

On Tue, 2016-10-18 at 23:27 +0200, Victor Toso wrote:
> From: Victor Toso <me@victortoso.com>
> 
> If gst_init() fails, gstvideo_init() will warn once about the issue
> and never attempt to try again, which is probably fine.
> 
> All the other APIs that might be called such as
> create_gstreamer_decoder() should not fail silently.

It is used by gstvideo_has_codec() for testing the channel caps. With
the change it will produce a critical (in fact programmer error
critical) for testing.

Pavel

> 
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>  src/channel-display-gst.c | 34 +++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index c0f0a1e..c102966 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -459,23 +459,23 @@ static gboolean gstvideo_init(void)
>  G_GNUC_INTERNAL
>  VideoDecoder* create_gstreamer_decoder(int codec_type,
> display_stream *stream)
>  {
> -    SpiceGstDecoder *decoder = NULL;
> -
> -    if (gstvideo_init()) {
> -        decoder = spice_new0(SpiceGstDecoder, 1);
> -        decoder->base.destroy = spice_gst_decoder_destroy;
> -        decoder->base.reschedule = spice_gst_decoder_reschedule;
> -        decoder->base.queue_frame = spice_gst_decoder_queue_frame;
> -        decoder->base.codec_type = codec_type;
> -        decoder->base.stream = stream;
> -        g_mutex_init(&decoder->queues_mutex);
> -        decoder->decoding_queue = g_queue_new();
> -        decoder->display_queue = g_queue_new();
> -
> -        if (!create_pipeline(decoder)) {
> -            decoder->base.destroy((VideoDecoder*)decoder);
> -            decoder = NULL;
> -        }
> +    SpiceGstDecoder *decoder;
> +
> +    g_return_val_if_fail(gstvideo_init(), NULL);
> +
> +    decoder = spice_new0(SpiceGstDecoder, 1);
> +    decoder->base.destroy = spice_gst_decoder_destroy;
> +    decoder->base.reschedule = spice_gst_decoder_reschedule;
> +    decoder->base.queue_frame = spice_gst_decoder_queue_frame;
> +    decoder->base.codec_type = codec_type;
> +    decoder->base.stream = stream;
> +    g_mutex_init(&decoder->queues_mutex);
> +    decoder->decoding_queue = g_queue_new();
> +    decoder->display_queue = g_queue_new();
> +
> +    if (!create_pipeline(decoder)) {
> +        decoder->base.destroy((VideoDecoder*)decoder);
> +        return NULL;
>      }
>  
>      return (VideoDecoder*)decoder;
On Wed, 2016-10-19 at 13:31 +0200, Pavel Grunt wrote:
> Hey,
> 
> On Tue, 2016-10-18 at 23:27 +0200, Victor Toso wrote:
> > From: Victor Toso <me@victortoso.com>
> > 
> > If gst_init() fails, gstvideo_init() will warn once about the
> > issue
> > and never attempt to try again, which is probably fine.
> > 
> > All the other APIs that might be called such as
> > create_gstreamer_decoder() should not fail silently.
> 
> It is used by gstvideo_has_codec() for testing the channel caps.
> With
> the change it will produce a critical (in fact programmer error
> critical) for testing.

hmm, need to think, i see a critical even without this patch,
sorry

> 
> Pavel
> 
> > 
> > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > ---
> >  src/channel-display-gst.c | 34 +++++++++++++++++-----------------
> >  1 file changed, 17 insertions(+), 17 deletions(-)
> > 
> > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > index c0f0a1e..c102966 100644
> > --- a/src/channel-display-gst.c
> > +++ b/src/channel-display-gst.c
> > @@ -459,23 +459,23 @@ static gboolean gstvideo_init(void)
> >  G_GNUC_INTERNAL
> >  VideoDecoder* create_gstreamer_decoder(int codec_type,
> > display_stream *stream)
> >  {
> > -    SpiceGstDecoder *decoder = NULL;
> > -
> > -    if (gstvideo_init()) {
> > -        decoder = spice_new0(SpiceGstDecoder, 1);
> > -        decoder->base.destroy = spice_gst_decoder_destroy;
> > -        decoder->base.reschedule = spice_gst_decoder_reschedule;
> > -        decoder->base.queue_frame =
> > spice_gst_decoder_queue_frame;
> > -        decoder->base.codec_type = codec_type;
> > -        decoder->base.stream = stream;
> > -        g_mutex_init(&decoder->queues_mutex);
> > -        decoder->decoding_queue = g_queue_new();
> > -        decoder->display_queue = g_queue_new();
> > -
> > -        if (!create_pipeline(decoder)) {
> > -            decoder->base.destroy((VideoDecoder*)decoder);
> > -            decoder = NULL;
> > -        }
> > +    SpiceGstDecoder *decoder;
> > +
> > +    g_return_val_if_fail(gstvideo_init(), NULL);
> > +
> > +    decoder = spice_new0(SpiceGstDecoder, 1);
> > +    decoder->base.destroy = spice_gst_decoder_destroy;
> > +    decoder->base.reschedule = spice_gst_decoder_reschedule;
> > +    decoder->base.queue_frame = spice_gst_decoder_queue_frame;
> > +    decoder->base.codec_type = codec_type;
> > +    decoder->base.stream = stream;
> > +    g_mutex_init(&decoder->queues_mutex);
> > +    decoder->decoding_queue = g_queue_new();
> > +    decoder->display_queue = g_queue_new();
> > +
> > +    if (!create_pipeline(decoder)) {
> > +        decoder->base.destroy((VideoDecoder*)decoder);
> > +        return NULL;
> >      }
> >  
> >      return (VideoDecoder*)decoder;