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

[Frediano Ziglio]

> 
> Hi,
> 
> On Fri, Dec 02, 2016 at 12:17:00PM -0500, Frediano Ziglio wrote:
> > > 
> > > From: Victor Toso <me at 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 at 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