[Spice-devel,v1,1/2] display-channel: reuse function to set video codecs

Submitted by Victor Toso on Dec. 2, 2016, 4:07 p.m.

Details

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

Not browsing as part of any series.

Commit Message

Victor Toso Dec. 2, 2016, 4:07 p.m.
From: Victor Toso <me@victortoso.com>

display_channel_set_video_codecs() already exists.

Signed-off-by: Victor Toso <victortoso@redhat.com>
---
 server/display-channel.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

Patch hide | download patch | download mbox

diff --git a/server/display-channel.c b/server/display-channel.c
index 52f0a3d..2d475d1 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -62,10 +62,7 @@  display_channel_set_property(GObject *object,
             self->priv->n_surfaces = g_value_get_uint(value);
             break;
         case PROP_VIDEO_CODECS:
-            if (self->priv->video_codecs) {
-                g_array_unref(self->priv->video_codecs);
-            }
-            self->priv->video_codecs = g_array_ref(g_value_get_boxed(value));
+            display_channel_set_video_codecs(self, g_value_get_boxed(value));
             break;
         default:
             G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id, pspec);

Comments

> From: Victor Toso <me@victortoso.com>
> 
> display_channel_set_video_codecs() already exists.
> 
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>  server/display-channel.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 52f0a3d..2d475d1 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -62,10 +62,7 @@ display_channel_set_property(GObject *object,
>              self->priv->n_surfaces = g_value_get_uint(value);
>              break;
>          case PROP_VIDEO_CODECS:
> -            if (self->priv->video_codecs) {
> -                g_array_unref(self->priv->video_codecs);
> -            }
> -            self->priv->video_codecs =
> g_array_ref(g_value_get_boxed(value));
> +            display_channel_set_video_codecs(self,
> g_value_get_boxed(value));
>              break;
>          default:
>              G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id, pspec);

Not exactly the same. display_channel_set_video_codecs has not the check
for video_codes NULL. This should generate a warning that currently you
don't have.

Frediano
Hi,

On Fri, Dec 02, 2016 at 12:12:02PM -0500, Frediano Ziglio wrote:
> > From: Victor Toso <me@victortoso.com>
> > 
> > display_channel_set_video_codecs() already exists.
> > 
> > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > ---
> >  server/display-channel.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> > 
> > diff --git a/server/display-channel.c b/server/display-channel.c
> > index 52f0a3d..2d475d1 100644
> > --- a/server/display-channel.c
> > +++ b/server/display-channel.c
> > @@ -62,10 +62,7 @@ display_channel_set_property(GObject *object,
> >              self->priv->n_surfaces = g_value_get_uint(value);
> >              break;
> >          case PROP_VIDEO_CODECS:
> > -            if (self->priv->video_codecs) {
> > -                g_array_unref(self->priv->video_codecs);
> > -            }
> > -            self->priv->video_codecs =
> > g_array_ref(g_value_get_boxed(value));
> > +            display_channel_set_video_codecs(self,
> > g_value_get_boxed(value));
> >              break;
> >          default:
> >              G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id, pspec);
>
> Not exactly the same. display_channel_set_video_codecs has not the check
> for video_codes NULL. This should generate a warning that currently you
> don't have.
>
> Frediano

Yay, my bad. I'll change that to g_clear_pointer() and send a v2 :)

  toso
> 
> Hi,
> 
> On Fri, Dec 02, 2016 at 12:12:02PM -0500, Frediano Ziglio wrote:
> > > From: Victor Toso <me@victortoso.com>
> > > 
> > > display_channel_set_video_codecs() already exists.
> > > 
> > > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > > ---
> > >  server/display-channel.c | 5 +----
> > >  1 file changed, 1 insertion(+), 4 deletions(-)
> > > 
> > > diff --git a/server/display-channel.c b/server/display-channel.c
> > > index 52f0a3d..2d475d1 100644
> > > --- a/server/display-channel.c
> > > +++ b/server/display-channel.c
> > > @@ -62,10 +62,7 @@ display_channel_set_property(GObject *object,
> > >              self->priv->n_surfaces = g_value_get_uint(value);
> > >              break;
> > >          case PROP_VIDEO_CODECS:
> > > -            if (self->priv->video_codecs) {
> > > -                g_array_unref(self->priv->video_codecs);
> > > -            }
> > > -            self->priv->video_codecs =
> > > g_array_ref(g_value_get_boxed(value));
> > > +            display_channel_set_video_codecs(self,
> > > g_value_get_boxed(value));
> > >              break;
> > >          default:
> > >              G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id,
> > >              pspec);
> >
> > Not exactly the same. display_channel_set_video_codecs has not the check
> > for video_codes NULL. This should generate a warning that currently you
> > don't have.
> >
> > Frediano
> 
> Yay, my bad. I'll change that to g_clear_pointer() and send a v2 :)
> 
>   toso
> 

Why not adding the if (ptr) to display_channel_set_video_codecs ?
Oh... you mean g_clear_pointer in display_channel_set_video_codecs!
Mumble... g_clear_pointer is available from 2.34. You should either
do the normal if or define a replacements in glib-compat.h.

Frediano
Hi,

On Fri, Dec 02, 2016 at 12:27:45PM -0500, Frediano Ziglio wrote:
> > 
> > Hi,
> > 
> > On Fri, Dec 02, 2016 at 12:12:02PM -0500, Frediano Ziglio wrote:
> > > > From: Victor Toso <me@victortoso.com>
> > > > 
> > > > display_channel_set_video_codecs() already exists.
> > > > 
> > > > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > > > ---
> > > >  server/display-channel.c | 5 +----
> > > >  1 file changed, 1 insertion(+), 4 deletions(-)
> > > > 
> > > > diff --git a/server/display-channel.c b/server/display-channel.c
> > > > index 52f0a3d..2d475d1 100644
> > > > --- a/server/display-channel.c
> > > > +++ b/server/display-channel.c
> > > > @@ -62,10 +62,7 @@ display_channel_set_property(GObject *object,
> > > >              self->priv->n_surfaces = g_value_get_uint(value);
> > > >              break;
> > > >          case PROP_VIDEO_CODECS:
> > > > -            if (self->priv->video_codecs) {
> > > > -                g_array_unref(self->priv->video_codecs);
> > > > -            }
> > > > -            self->priv->video_codecs =
> > > > g_array_ref(g_value_get_boxed(value));
> > > > +            display_channel_set_video_codecs(self,
> > > > g_value_get_boxed(value));
> > > >              break;
> > > >          default:
> > > >              G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id,
> > > >              pspec);
> > >
> > > Not exactly the same. display_channel_set_video_codecs has not the check
> > > for video_codes NULL. This should generate a warning that currently you
> > > don't have.
> > >
> > > Frediano
> > 
> > Yay, my bad. I'll change that to g_clear_pointer() and send a v2 :)
> > 
> >   toso
> > 
> 
> Why not adding the if (ptr) to display_channel_set_video_codecs ?
> Oh... you mean g_clear_pointer in display_channel_set_video_codecs!
> Mumble... g_clear_pointer is available from 2.34. You should either
> do the normal if or define a replacements in glib-compat.h.

I'll do that. I use g_clear_pointer a lot :)

>
> Frediano