[Spice-devel,v1,2/2] display-channel: Make video-codecs readwrite

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

Details

Message ID 20161202160733.4970-2-victortoso@redhat.com
State New
Headers show
Series "Series without cover letter" ( rev: 2 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>

By making video-codecs readeable, we can avoid other objects to access
the internals of DisplayChannel.

The GArray video-codecs is copied as static, no need to unref it
afterwards.

Signed-off-by: Victor Toso <victortoso@redhat.com>
---
 server/display-channel.c | 5 ++++-
 server/stream.c          | 6 ++++--
 2 files changed, 8 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/server/display-channel.c b/server/display-channel.c
index 2d475d1..8bad693 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -43,6 +43,9 @@  display_channel_get_property(GObject *object,
         case PROP_N_SURFACES:
             g_value_set_uint(value, self->priv->n_surfaces);
             break;
+        case PROP_VIDEO_CODECS:
+            g_value_set_static_boxed(value, self->priv->video_codecs);
+            break;
         default:
             G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id, pspec);
     }
@@ -2246,7 +2249,7 @@  display_channel_class_init(DisplayChannelClass *klass)
                                                        "Video Codecs",
                                                        G_TYPE_ARRAY,
                                                        G_PARAM_CONSTRUCT_ONLY |
-                                                       G_PARAM_WRITABLE |
+                                                       G_PARAM_READWRITE |
                                                        G_PARAM_STATIC_STRINGS));
 }
 
diff --git a/server/stream.c b/server/stream.c
index 3f3c286..bb2ca99 100644
--- a/server/stream.c
+++ b/server/stream.c
@@ -691,9 +691,11 @@  static VideoEncoder* dcc_create_video_encoder(DisplayChannelClient *dcc,
     RedChannelClient *rcc = RED_CHANNEL_CLIENT(dcc);
     int client_has_multi_codec = red_channel_client_test_remote_cap(rcc, SPICE_DISPLAY_CAP_MULTI_CODEC);
     int i;
+    GArray *video_codecs;
 
-    for (i = 0; i < display->priv->video_codecs->len; i++) {
-        RedVideoCodec* video_codec = &g_array_index (display->priv->video_codecs, RedVideoCodec, i);
+    g_object_get(display, "video-codecs", &video_codecs, NULL);
+    for (i = 0; i < video_codecs->len; i++) {
+        RedVideoCodec* video_codec = &g_array_index (video_codecs, RedVideoCodec, i);
 
         if (!client_has_multi_codec &&
             video_codec->type != SPICE_VIDEO_CODEC_TYPE_MJPEG) {

Comments

> 
> From: Victor Toso <me@victortoso.com>
> 
> By making video-codecs readeable, we can avoid other objects to access
> the internals of DisplayChannel.
> 
> The GArray video-codecs is copied as static, no need to unref it
> afterwards.
> 
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>  server/display-channel.c | 5 ++++-
>  server/stream.c          | 6 ++++--
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 2d475d1..8bad693 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -43,6 +43,9 @@ display_channel_get_property(GObject *object,
>          case PROP_N_SURFACES:
>              g_value_set_uint(value, self->priv->n_surfaces);
>              break;
> +        case PROP_VIDEO_CODECS:
> +            g_value_set_static_boxed(value, self->priv->video_codecs);
> +            break;
>          default:
>              G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id, pspec);
>      }
> @@ -2246,7 +2249,7 @@ display_channel_class_init(DisplayChannelClass *klass)
>                                                         "Video Codecs",
>                                                         G_TYPE_ARRAY,
>                                                         G_PARAM_CONSTRUCT_ONLY
>                                                         |
> -                                                       G_PARAM_WRITABLE |
> +                                                       G_PARAM_READWRITE |
>                                                         G_PARAM_STATIC_STRINGS));
>  }
>  
> diff --git a/server/stream.c b/server/stream.c
> index 3f3c286..bb2ca99 100644
> --- a/server/stream.c
> +++ b/server/stream.c
> @@ -691,9 +691,11 @@ static VideoEncoder*
> dcc_create_video_encoder(DisplayChannelClient *dcc,
>      RedChannelClient *rcc = RED_CHANNEL_CLIENT(dcc);
>      int client_has_multi_codec = red_channel_client_test_remote_cap(rcc,
>      SPICE_DISPLAY_CAP_MULTI_CODEC);
>      int i;
> +    GArray *video_codecs;
>  
> -    for (i = 0; i < display->priv->video_codecs->len; i++) {
> -        RedVideoCodec* video_codec = &g_array_index
> (display->priv->video_codecs, RedVideoCodec, i);
> +    g_object_get(display, "video-codecs", &video_codecs, NULL);
> +    for (i = 0; i < video_codecs->len; i++) {
> +        RedVideoCodec* video_codec = &g_array_index (video_codecs,
> RedVideoCodec, i);
>  
>          if (!client_has_multi_codec &&
>              video_codec->type != SPICE_VIDEO_CODEC_TYPE_MJPEG) {

I prefer accessors.
Also streaming code should have access to this information, in all
other DisplayChannel code video_codecs are useless.
That is the problem is that stream data are in DisplayChannel,
I think the right direction is encapsulate correctly stream code,
data related to stream should be in a "stream" structure.

Frediano
Hi,

On Fri, Dec 02, 2016 at 12:17:00PM -0500, Frediano Ziglio wrote:
> > 
> > From: Victor Toso <me@victortoso.com>
> > 
> > By making video-codecs readeable, we can avoid other objects to access
> > the internals of DisplayChannel.
> > 
> > The GArray video-codecs is copied as static, no need to unref it
> > afterwards.
> > 
> > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > ---
> >  server/display-channel.c | 5 ++++-
> >  server/stream.c          | 6 ++++--
> >  2 files changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/server/display-channel.c b/server/display-channel.c
> > index 2d475d1..8bad693 100644
> > --- a/server/display-channel.c
> > +++ b/server/display-channel.c
> > @@ -43,6 +43,9 @@ display_channel_get_property(GObject *object,
> >          case PROP_N_SURFACES:
> >              g_value_set_uint(value, self->priv->n_surfaces);
> >              break;
> > +        case PROP_VIDEO_CODECS:
> > +            g_value_set_static_boxed(value, self->priv->video_codecs);
> > +            break;
> >          default:
> >              G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id, pspec);
> >      }
> > @@ -2246,7 +2249,7 @@ display_channel_class_init(DisplayChannelClass *klass)
> >                                                         "Video Codecs",
> >                                                         G_TYPE_ARRAY,
> >                                                         G_PARAM_CONSTRUCT_ONLY
> >                                                         |
> > -                                                       G_PARAM_WRITABLE |
> > +                                                       G_PARAM_READWRITE |
> >                                                         G_PARAM_STATIC_STRINGS));
> >  }
> >  
> > diff --git a/server/stream.c b/server/stream.c
> > index 3f3c286..bb2ca99 100644
> > --- a/server/stream.c
> > +++ b/server/stream.c
> > @@ -691,9 +691,11 @@ static VideoEncoder*
> > dcc_create_video_encoder(DisplayChannelClient *dcc,
> >      RedChannelClient *rcc = RED_CHANNEL_CLIENT(dcc);
> >      int client_has_multi_codec = red_channel_client_test_remote_cap(rcc,
> >      SPICE_DISPLAY_CAP_MULTI_CODEC);
> >      int i;
> > +    GArray *video_codecs;
> >  
> > -    for (i = 0; i < display->priv->video_codecs->len; i++) {
> > -        RedVideoCodec* video_codec = &g_array_index
> > (display->priv->video_codecs, RedVideoCodec, i);
> > +    g_object_get(display, "video-codecs", &video_codecs, NULL);
> > +    for (i = 0; i < video_codecs->len; i++) {
> > +        RedVideoCodec* video_codec = &g_array_index (video_codecs,
> > RedVideoCodec, i);
> >  
> >          if (!client_has_multi_codec &&
> >              video_codec->type != SPICE_VIDEO_CODEC_TYPE_MJPEG) {
> 
> I prefer accessors.

Should I drop also the change from G_PARAM_WRITABLE to
G_PARAM_READWRITE or should I drop the patch entirely?

> Also streaming code should have access to this information, in all
> other DisplayChannel code video_codecs are useless.
> That is the problem is that stream data are in DisplayChannel,
> I think the right direction is encapsulate correctly stream code,
> data related to stream should be in a "stream" structure.
>
> Frediano

Indeed, it seems that it can be improved but I'm not sure how it should
be designed?

Nowadays:

host calls reds:spice_server_set_video_codecs() that calls
reds_on_vc_change() which end up sending
RED_WORKER_MESSAGE_SET_VIDEO_CODECS, meaning
display_channel_set_video_codecs() (from the first patch!) so stream.c
can use it in its helper function to call the best video_codec->create()

Bugs me that reds keeps the original GArray and each display-channel
refs it (They should weak-reference it from somewhere else, no?)

Let me know if I can help with something here, I don't understand all
the design in spice-server.

 Cheers,
> 
> Hi,
> 
> On Fri, Dec 02, 2016 at 12:17:00PM -0500, Frediano Ziglio wrote:
> > > 
> > > From: Victor Toso <me@victortoso.com>
> > > 
> > > By making video-codecs readeable, we can avoid other objects to access
> > > the internals of DisplayChannel.
> > > 
> > > The GArray video-codecs is copied as static, no need to unref it
> > > afterwards.
> > > 
> > > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > > ---
> > >  server/display-channel.c | 5 ++++-
> > >  server/stream.c          | 6 ++++--
> > >  2 files changed, 8 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/server/display-channel.c b/server/display-channel.c
> > > index 2d475d1..8bad693 100644
> > > --- a/server/display-channel.c
> > > +++ b/server/display-channel.c
> > > @@ -43,6 +43,9 @@ display_channel_get_property(GObject *object,
> > >          case PROP_N_SURFACES:
> > >              g_value_set_uint(value, self->priv->n_surfaces);
> > >              break;
> > > +        case PROP_VIDEO_CODECS:
> > > +            g_value_set_static_boxed(value, self->priv->video_codecs);
> > > +            break;
> > >          default:
> > >              G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id,
> > >              pspec);
> > >      }
> > > @@ -2246,7 +2249,7 @@ display_channel_class_init(DisplayChannelClass
> > > *klass)
> > >                                                         "Video Codecs",
> > >                                                         G_TYPE_ARRAY,
> > >                                                         G_PARAM_CONSTRUCT_ONLY
> > >                                                         |
> > > -                                                       G_PARAM_WRITABLE
> > > |
> > > +                                                       G_PARAM_READWRITE
> > > |
> > >                                                         G_PARAM_STATIC_STRINGS));
> > >  }
> > >  
> > > diff --git a/server/stream.c b/server/stream.c
> > > index 3f3c286..bb2ca99 100644
> > > --- a/server/stream.c
> > > +++ b/server/stream.c
> > > @@ -691,9 +691,11 @@ static VideoEncoder*
> > > dcc_create_video_encoder(DisplayChannelClient *dcc,
> > >      RedChannelClient *rcc = RED_CHANNEL_CLIENT(dcc);
> > >      int client_has_multi_codec = red_channel_client_test_remote_cap(rcc,
> > >      SPICE_DISPLAY_CAP_MULTI_CODEC);
> > >      int i;
> > > +    GArray *video_codecs;
> > >  
> > > -    for (i = 0; i < display->priv->video_codecs->len; i++) {
> > > -        RedVideoCodec* video_codec = &g_array_index
> > > (display->priv->video_codecs, RedVideoCodec, i);
> > > +    g_object_get(display, "video-codecs", &video_codecs, NULL);
> > > +    for (i = 0; i < video_codecs->len; i++) {
> > > +        RedVideoCodec* video_codec = &g_array_index (video_codecs,
> > > RedVideoCodec, i);
> > >  
> > >          if (!client_has_multi_codec &&
> > >              video_codec->type != SPICE_VIDEO_CODEC_TYPE_MJPEG) {
> > 
> > I prefer accessors.
> 
> Should I drop also the change from G_PARAM_WRITABLE to
> G_PARAM_READWRITE or should I drop the patch entirely?
> 
> > Also streaming code should have access to this information, in all
> > other DisplayChannel code video_codecs are useless.
> > That is the problem is that stream data are in DisplayChannel,
> > I think the right direction is encapsulate correctly stream code,
> > data related to stream should be in a "stream" structure.
> >
> > Frediano
> 
> Indeed, it seems that it can be improved but I'm not sure how it should
> be designed?
> 
> Nowadays:
> 
> host calls reds:spice_server_set_video_codecs() that calls
> reds_on_vc_change() which end up sending
> RED_WORKER_MESSAGE_SET_VIDEO_CODECS, meaning
> display_channel_set_video_codecs() (from the first patch!) so stream.c
> can use it in its helper function to call the best video_codec->create()
> 
> Bugs me that reds keeps the original GArray and each display-channel
> refs it (They should weak-reference it from somewhere else, no?)
> 

reds (SpiceServer) has to keep for the possible future DisplayChannels.
The RED_WORKER_MESSAGE_SET_VIDEO_CODECS is required to avoid race conditions.
But as far SpiceServer and DisplayChannel should not care about it.

Why weak reference? They all hold the array, both SpiceServer and DisplayChannel.

> Let me know if I can help with something here, I don't understand all
> the design in spice-server.
> 

Nobody does, that's the funny bit!

DisplayChannel is a good entry point for SpiceServer but basically the
DisplayChannel structure contains every fields needed by DisplayChannel,
Stream, StreamAgent and DisplayChannelClient, if you looks at the includes
currently all these objects know the implementation (even private structure)
of each others. I manage do disentangle image encoding and I tried to start
with Stream but looks like more harder.

OT: I'm still wondering if it's a good decision to have *Channel and 
*ChannelClient separate. Basically Channel store a SC state while ChannelClient
store a Sc state and send a SC-Sc update to the client so surely ChannelClient
has to know some state of Channel while Channel has to know when specifically
inform ChannelClient of any change and manage the various updates. Also noting
that some of the callbacks for Channel are managing ChannelClient requests.

Frediano
On Fri, Dec 02, 2016 at 06:37:26PM +0100, Victor Toso wrote:
> > I prefer accessors.
> 
> Should I drop also the change from G_PARAM_WRITABLE to
> G_PARAM_READWRITE or should I drop the patch entirely?

You can go with both :)

> 
> > Also streaming code should have access to this information, in all
> > other DisplayChannel code video_codecs are useless.
> > That is the problem is that stream data are in DisplayChannel,
> > I think the right direction is encapsulate correctly stream code,
> > data related to stream should be in a "stream" structure.
> >
> > Frediano
> 
> Indeed, it seems that it can be improved but I'm not sure how it should
> be designed?
> 
> Nowadays:
> 
> host calls reds:spice_server_set_video_codecs() that calls
> reds_on_vc_change() which end up sending

Fwiw, method named reds_on_yyy() might be good candidate for using a
signal rather than a function call now that we have more GObjects.

Christophe
Hi,

On Fri, Dec 02, 2016 at 01:09:54PM -0500, Frediano Ziglio wrote:
> > Indeed, it seems that it can be improved but I'm not sure how it should
> > be designed?
> >
> > Nowadays:
> >
> > host calls reds:spice_server_set_video_codecs() that calls
> > reds_on_vc_change() which end up sending
> > RED_WORKER_MESSAGE_SET_VIDEO_CODECS, meaning
> > display_channel_set_video_codecs() (from the first patch!) so stream.c
> > can use it in its helper function to call the best video_codec->create()
> >
> > Bugs me that reds keeps the original GArray and each display-channel
> > refs it (They should weak-reference it from somewhere else, no?)
> >
>
> reds (SpiceServer) has to keep for the possible future DisplayChannels.

Ah, right.

> The RED_WORKER_MESSAGE_SET_VIDEO_CODECS is required to avoid race conditions.
> But as far SpiceServer and DisplayChannel should not care about it.
>
> Why weak reference? They all hold the array, both SpiceServer and
> DisplayChannel.

Ah, it should not be a big deal. I was thinking if host sets a new
video-codecs preference, each display would be using the wrong
video-codecs array till it gets updated.

As WeakRef I mean that at the time the new GArray is set, the old
GArray reference would be NULL and while there is no update, there would
be no stream.

>
> > Let me know if I can help with something here, I don't understand all
> > the design in spice-server.
> >
>
> Nobody does, that's the funny bit!
>
> DisplayChannel is a good entry point for SpiceServer but basically the
> DisplayChannel structure contains every fields needed by DisplayChannel,
> Stream, StreamAgent and DisplayChannelClient, if you looks at the includes
> currently all these objects know the implementation (even private structure)
> of each others. I manage do disentangle image encoding and I tried to start
> with Stream but looks like more harder.

Ah, I see what you mean now.

>
> OT: I'm still wondering if it's a good decision to have *Channel and
> *ChannelClient separate. Basically Channel store a SC state while ChannelClient
> store a Sc state and send a SC-Sc update to the client so surely ChannelClient
> has to know some state of Channel while Channel has to know when specifically
> inform ChannelClient of any change and manage the various updates. Also noting
> that some of the callbacks for Channel are managing ChannelClient requests.
>
> Frediano

Thanks for your feedback and comments :)

  toso