[spice-server,2/7] VideoStream: store channel in stream

Submitted by Jonathon Jongsma on Nov. 29, 2017, 8:16 p.m.

Details

Message ID 20171129201610.30738-3-jjongsma@redhat.com
State New
Headers show
Series "Stream cleanups" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Jonathon Jongsma Nov. 29, 2017, 8:16 p.m.
This allows us to unref the stream directly rather than needing to pass
the associated DisplayChannel to stream_unref(). The same is also true
for stream_agent_unref, since the only reason that stream_agent_unref()
required a DisplayChannel parameter was to pass it to stream_unref().
This also resulted in some additional minor redesign due to the fact
that streams are pre-allocated in a pool in the display channel, and
freeing a stream simply means moving it to a different ring to be
re-used.

Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
---
 server/display-channel.c | 27 ++++++++++++++++++++++++++-
 server/display-channel.h |  1 +
 server/video-stream.c    | 44 ++++++++++++--------------------------------
 server/video-stream.h    |  8 ++++----
 4 files changed, 43 insertions(+), 37 deletions(-)

Patch hide | download patch | download mbox

diff --git a/server/display-channel.c b/server/display-channel.c
index 2caaa6433..d3db1e0db 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -272,7 +272,7 @@  static void stop_streams(DisplayChannel *display)
         VideoStream *stream = SPICE_CONTAINEROF(item, VideoStream, link);
         item = ring_next(ring, item);
         if (!stream->current) {
-            video_stream_stop(display, stream);
+            video_stream_stop(stream);
         } else {
             spice_debug("attached stream");
         }
@@ -2279,6 +2279,31 @@  display_channel_init(DisplayChannel *self)
     self->priv->image_surfaces.ops = &image_surfaces_ops;
 }
 
+static void display_channel_mark_stream_unused(DisplayChannel *display, VideoStream *stream)
+{
+    stream->next = display->priv->free_streams;
+    display->priv->free_streams = stream;
+}
+
+static void display_channel_init_video_streams(DisplayChannel *display)
+{
+    int i;
+
+    ring_init(&display->priv->streams);
+    display->priv->free_streams = NULL;
+    for (i = 0; i < NUM_STREAMS; i++) {
+        VideoStream *stream = display_channel_get_nth_video_stream(display, i);
+        ring_item_init(&stream->link);
+        display_channel_mark_stream_unused(display, stream);
+    }
+}
+
+void display_channel_free_video_stream(DisplayChannel *display, VideoStream *stream)
+{
+    display_channel_mark_stream_unused(stream->display, stream);
+    display->priv->stream_count--;
+}
+
 static void
 display_channel_constructed(GObject *object)
 {
diff --git a/server/display-channel.h b/server/display-channel.h
index 4f7def216..a0470ec67 100644
--- a/server/display-channel.h
+++ b/server/display-channel.h
@@ -168,6 +168,7 @@  gboolean display_channel_surface_has_canvas(DisplayChannel *display, uint32_t su
 void display_channel_reset_image_cache(DisplayChannel *self);
 
 void display_channel_debug_oom(DisplayChannel *display, const char *msg);
+void display_channel_free_video_stream(DisplayChannel *display, VideoStream *stream);
 
 G_END_DECLS
 
diff --git a/server/video-stream.c b/server/video-stream.c
index 65da3f8a0..a24c787ef 100644
--- a/server/video-stream.c
+++ b/server/video-stream.c
@@ -66,8 +66,7 @@  static void video_stream_agent_stats_print(VideoStreamAgent *agent)
 static void video_stream_create_destroy_item_release(RedPipeItem *base)
 {
     StreamCreateDestroyItem *item = SPICE_UPCAST(StreamCreateDestroyItem, base);
-    DisplayChannel *display = DCC_TO_DC(item->agent->dcc);
-    video_stream_agent_unref(display, item->agent);
+    video_stream_agent_unref(item->agent);
     g_free(item);
 }
 
@@ -94,8 +93,9 @@  static RedPipeItem *video_stream_destroy_item_new(VideoStreamAgent *agent)
 }
 
 
-void video_stream_stop(DisplayChannel *display, VideoStream *stream)
+void video_stream_stop(VideoStream *stream)
 {
+    DisplayChannel *display = stream->display;
     DisplayChannelClient *dcc;
     int stream_id = display_channel_get_video_stream_id(display, stream);
 
@@ -125,53 +125,32 @@  void video_stream_stop(DisplayChannel *display, VideoStream *stream)
     }
     display->priv->streams_size_total -= stream->width * stream->height;
     ring_remove(&stream->link);
-    video_stream_unref(display, stream);
+    video_stream_unref(stream);
 }
 
-static void video_stream_free(DisplayChannel *display, VideoStream *stream)
-{
-    stream->next = display->priv->free_streams;
-    display->priv->free_streams = stream;
-}
-
-void display_channel_init_video_streams(DisplayChannel *display)
-{
-    int i;
-
-    ring_init(&display->priv->streams);
-    display->priv->free_streams = NULL;
-    for (i = 0; i < NUM_STREAMS; i++) {
-        VideoStream *stream = display_channel_get_nth_video_stream(display, i);
-        ring_item_init(&stream->link);
-        video_stream_free(display, stream);
-    }
-}
-
-void video_stream_unref(DisplayChannel *display, VideoStream *stream)
+void video_stream_unref(VideoStream *stream)
 {
     if (--stream->refs != 0)
         return;
 
     spice_warn_if_fail(!ring_item_is_linked(&stream->link));
 
-    video_stream_free(display, stream);
-    display->priv->stream_count--;
+    display_channel_free_video_stream(stream->display, stream);
 }
 
-void video_stream_agent_unref(DisplayChannel *display, VideoStreamAgent *agent)
+void video_stream_agent_unref(VideoStreamAgent *agent)
 {
-    video_stream_unref(display, agent->stream);
+    video_stream_unref(agent->stream);
 }
 
 static void video_stream_clip_item_free(RedPipeItem *base)
 {
     g_return_if_fail(base != NULL);
     VideoStreamClipItem *item = SPICE_UPCAST(VideoStreamClipItem, base);
-    DisplayChannel *display = DCC_TO_DC(item->stream_agent->dcc);
 
     g_return_if_fail(item->base.refcount == 0);
 
-    video_stream_agent_unref(display, item->stream_agent);
+    video_stream_agent_unref(item->stream_agent);
     g_free(item->rects);
     g_free(item);
 }
@@ -375,6 +354,7 @@  static VideoStream *display_channel_stream_try_new(DisplayChannel *display)
     }
     stream = display->priv->free_streams;
     display->priv->free_streams = display->priv->free_streams->next;
+    stream->display = display;
     return stream;
 }
 
@@ -923,7 +903,7 @@  void video_stream_detach_and_stop(DisplayChannel *display)
         VideoStream *stream = SPICE_CONTAINEROF(stream_item, VideoStream, link);
 
         detach_video_stream_gracefully(display, stream, NULL);
-        video_stream_stop(display, stream);
+        video_stream_stop(stream);
     }
 }
 
@@ -939,7 +919,7 @@  void video_stream_timeout(DisplayChannel *display)
         item = ring_next(ring, item);
         if (now >= (stream->last_time + RED_STREAM_TIMEOUT)) {
             detach_video_stream_gracefully(display, stream, NULL);
-            video_stream_stop(display, stream);
+            video_stream_stop(stream);
         }
     }
 }
diff --git a/server/video-stream.h b/server/video-stream.h
index a4d149816..88d59c848 100644
--- a/server/video-stream.h
+++ b/server/video-stream.h
@@ -125,11 +125,11 @@  struct VideoStream {
     uint32_t num_input_frames;
     uint64_t input_fps_start_time;
     uint32_t input_fps;
+    DisplayChannel *display;
 };
 
-void display_channel_init_video_streams(DisplayChannel *display);
-void video_stream_stop(DisplayChannel *display, VideoStream *stream);
-void video_stream_unref(DisplayChannel *display, VideoStream *stream);
+void video_stream_stop(VideoStream *stream);
+void video_stream_unref(VideoStream *stream);
 void video_stream_trace_update(DisplayChannel *display, Drawable *drawable);
 void video_stream_maintenance(DisplayChannel *display, Drawable *candidate,
                               Drawable *prev);
@@ -139,7 +139,7 @@  void video_stream_trace_add_drawable(DisplayChannel *display, Drawable *item);
 void video_stream_detach_behind(DisplayChannel *display, QRegion *region,
                                 Drawable *drawable);
 
-void video_stream_agent_unref(DisplayChannel *display, VideoStreamAgent *agent);
+void video_stream_agent_unref(VideoStreamAgent *agent);
 void video_stream_agent_stop(VideoStreamAgent *agent);
 
 void video_stream_detach_drawable(VideoStream *stream);

Comments

> 
> This allows us to unref the stream directly rather than needing to pass
> the associated DisplayChannel to stream_unref(). The same is also true
> for stream_agent_unref, since the only reason that stream_agent_unref()
> required a DisplayChannel parameter was to pass it to stream_unref().
> This also resulted in some additional minor redesign due to the fact
> that streams are pre-allocated in a pool in the display channel, and
> freeing a stream simply means moving it to a different ring to be
> re-used.
> 
> Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> ---
>  server/display-channel.c | 27 ++++++++++++++++++++++++++-
>  server/display-channel.h |  1 +
>  server/video-stream.c    | 44 ++++++++++++--------------------------------
>  server/video-stream.h    |  8 ++++----
>  4 files changed, 43 insertions(+), 37 deletions(-)
> 
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 2caaa6433..d3db1e0db 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -272,7 +272,7 @@ static void stop_streams(DisplayChannel *display)
>          VideoStream *stream = SPICE_CONTAINEROF(item, VideoStream, link);
>          item = ring_next(ring, item);
>          if (!stream->current) {
> -            video_stream_stop(display, stream);
> +            video_stream_stop(stream);
>          } else {
>              spice_debug("attached stream");
>          }
> @@ -2279,6 +2279,31 @@ display_channel_init(DisplayChannel *self)
>      self->priv->image_surfaces.ops = &image_surfaces_ops;
>  }
>  
> +static void display_channel_mark_stream_unused(DisplayChannel *display,
> VideoStream *stream)
> +{
> +    stream->next = display->priv->free_streams;
> +    display->priv->free_streams = stream;
> +}
> +
> +static void display_channel_init_video_streams(DisplayChannel *display)
> +{
> +    int i;
> +
> +    ring_init(&display->priv->streams);
> +    display->priv->free_streams = NULL;
> +    for (i = 0; i < NUM_STREAMS; i++) {
> +        VideoStream *stream = display_channel_get_nth_video_stream(display,
> i);
> +        ring_item_init(&stream->link);
> +        display_channel_mark_stream_unused(display, stream);
> +    }
> +}
> +
> +void display_channel_free_video_stream(DisplayChannel *display, VideoStream
> *stream)
> +{
> +    display_channel_mark_stream_unused(stream->display, stream);
> +    display->priv->stream_count--;
> +}
> +
>  static void
>  display_channel_constructed(GObject *object)
>  {
> diff --git a/server/display-channel.h b/server/display-channel.h
> index 4f7def216..a0470ec67 100644
> --- a/server/display-channel.h
> +++ b/server/display-channel.h
> @@ -168,6 +168,7 @@ gboolean
> display_channel_surface_has_canvas(DisplayChannel *display, uint32_t su
>  void display_channel_reset_image_cache(DisplayChannel *self);
>  
>  void display_channel_debug_oom(DisplayChannel *display, const char *msg);
> +void display_channel_free_video_stream(DisplayChannel *display, VideoStream
> *stream);
>  
>  G_END_DECLS
>  
> diff --git a/server/video-stream.c b/server/video-stream.c
> index 65da3f8a0..a24c787ef 100644
> --- a/server/video-stream.c
> +++ b/server/video-stream.c
> @@ -66,8 +66,7 @@ static void video_stream_agent_stats_print(VideoStreamAgent
> *agent)
>  static void video_stream_create_destroy_item_release(RedPipeItem *base)
>  {
>      StreamCreateDestroyItem *item = SPICE_UPCAST(StreamCreateDestroyItem,
>      base);
> -    DisplayChannel *display = DCC_TO_DC(item->agent->dcc);
> -    video_stream_agent_unref(display, item->agent);
> +    video_stream_agent_unref(item->agent);
>      g_free(item);
>  }
>  
> @@ -94,8 +93,9 @@ static RedPipeItem
> *video_stream_destroy_item_new(VideoStreamAgent *agent)
>  }
>  
>  
> -void video_stream_stop(DisplayChannel *display, VideoStream *stream)
> +void video_stream_stop(VideoStream *stream)
>  {
> +    DisplayChannel *display = stream->display;
>      DisplayChannelClient *dcc;
>      int stream_id = display_channel_get_video_stream_id(display, stream);
>  
> @@ -125,53 +125,32 @@ void video_stream_stop(DisplayChannel *display,
> VideoStream *stream)
>      }
>      display->priv->streams_size_total -= stream->width * stream->height;
>      ring_remove(&stream->link);
> -    video_stream_unref(display, stream);
> +    video_stream_unref(stream);
>  }
>  
> -static void video_stream_free(DisplayChannel *display, VideoStream *stream)
> -{
> -    stream->next = display->priv->free_streams;
> -    display->priv->free_streams = stream;
> -}
> -
> -void display_channel_init_video_streams(DisplayChannel *display)
> -{
> -    int i;
> -
> -    ring_init(&display->priv->streams);
> -    display->priv->free_streams = NULL;
> -    for (i = 0; i < NUM_STREAMS; i++) {
> -        VideoStream *stream = display_channel_get_nth_video_stream(display,
> i);
> -        ring_item_init(&stream->link);
> -        video_stream_free(display, stream);
> -    }
> -}
> -
> -void video_stream_unref(DisplayChannel *display, VideoStream *stream)
> +void video_stream_unref(VideoStream *stream)
>  {
>      if (--stream->refs != 0)
>          return;
>  
>      spice_warn_if_fail(!ring_item_is_linked(&stream->link));
>  
> -    video_stream_free(display, stream);
> -    display->priv->stream_count--;
> +    display_channel_free_video_stream(stream->display, stream);
>  }
>  
> -void video_stream_agent_unref(DisplayChannel *display, VideoStreamAgent
> *agent)
> +void video_stream_agent_unref(VideoStreamAgent *agent)
>  {
> -    video_stream_unref(display, agent->stream);
> +    video_stream_unref(agent->stream);
>  }
>  
>  static void video_stream_clip_item_free(RedPipeItem *base)
>  {
>      g_return_if_fail(base != NULL);
>      VideoStreamClipItem *item = SPICE_UPCAST(VideoStreamClipItem, base);
> -    DisplayChannel *display = DCC_TO_DC(item->stream_agent->dcc);
>  
>      g_return_if_fail(item->base.refcount == 0);
>  
> -    video_stream_agent_unref(display, item->stream_agent);
> +    video_stream_agent_unref(item->stream_agent);
>      g_free(item->rects);
>      g_free(item);
>  }
> @@ -375,6 +354,7 @@ static VideoStream
> *display_channel_stream_try_new(DisplayChannel *display)
>      }
>      stream = display->priv->free_streams;
>      display->priv->free_streams = display->priv->free_streams->next;
> +    stream->display = display;
>      return stream;
>  }
>  
> @@ -923,7 +903,7 @@ void video_stream_detach_and_stop(DisplayChannel
> *display)
>          VideoStream *stream = SPICE_CONTAINEROF(stream_item, VideoStream,
>          link);
>  
>          detach_video_stream_gracefully(display, stream, NULL);
> -        video_stream_stop(display, stream);
> +        video_stream_stop(stream);
>      }
>  }
>  
> @@ -939,7 +919,7 @@ void video_stream_timeout(DisplayChannel *display)
>          item = ring_next(ring, item);
>          if (now >= (stream->last_time + RED_STREAM_TIMEOUT)) {
>              detach_video_stream_gracefully(display, stream, NULL);
> -            video_stream_stop(display, stream);
> +            video_stream_stop(stream);
>          }
>      }
>  }
> diff --git a/server/video-stream.h b/server/video-stream.h
> index a4d149816..88d59c848 100644
> --- a/server/video-stream.h
> +++ b/server/video-stream.h
> @@ -125,11 +125,11 @@ struct VideoStream {
>      uint32_t num_input_frames;
>      uint64_t input_fps_start_time;
>      uint32_t input_fps;
> +    DisplayChannel *display;
>  };
>  
> -void display_channel_init_video_streams(DisplayChannel *display);
> -void video_stream_stop(DisplayChannel *display, VideoStream *stream);
> -void video_stream_unref(DisplayChannel *display, VideoStream *stream);
> +void video_stream_stop(VideoStream *stream);
> +void video_stream_unref(VideoStream *stream);
>  void video_stream_trace_update(DisplayChannel *display, Drawable *drawable);
>  void video_stream_maintenance(DisplayChannel *display, Drawable *candidate,
>                                Drawable *prev);
> @@ -139,7 +139,7 @@ void video_stream_trace_add_drawable(DisplayChannel
> *display, Drawable *item);
>  void video_stream_detach_behind(DisplayChannel *display, QRegion *region,
>                                  Drawable *drawable);
>  
> -void video_stream_agent_unref(DisplayChannel *display, VideoStreamAgent
> *agent);
> +void video_stream_agent_unref(VideoStreamAgent *agent);
>  void video_stream_agent_stop(VideoStreamAgent *agent);
>  
>  void video_stream_detach_drawable(VideoStream *stream);

Looks like this patch is trying to do multiple stuff.
Yes, having display directly in the VideoStream helps but for instance
there are different functions (like display_channel_get_stream_id) that
would benefit from this change.
On the other side there's no much rationale on some function move.
This for this patch but also 3/7.
What should the rule why a given function should be in video-stream.c
and not on display-channel.c/dcc.c/dcc-send.c? Not that I have a precise
idea myself but these functions had been moved around multiple time
and I think the main reason is that we don't have an agreed reasoning
of this split. Said that streaming handling is part of the display channel
where should be the code that detect new streams reside? For instance
looks like the code that initialize a VideoStream is in display-channel.c
but all the *_new functions are usually in the same file that should
handle the structure, in this case video-stream.c.
If the VideoStreamAgent is specifically related to the client (so
DisplayChannelClient) should the code reside in dcc.c or video-stream.c?
Why the definition is in video-stream.h and some code in dcc.c?
I tried to do some split like what I did for image encoding but
got not really far, the code is too much mingled.

Frediano
On Thu, 2017-11-30 at 13:40 -0500, Frediano Ziglio wrote:
> > 
> > This allows us to unref the stream directly rather than needing to
> > pass
> > the associated DisplayChannel to stream_unref(). The same is also
> > true
> > for stream_agent_unref, since the only reason that
> > stream_agent_unref()
> > required a DisplayChannel parameter was to pass it to
> > stream_unref().
> > This also resulted in some additional minor redesign due to the
> > fact
> > that streams are pre-allocated in a pool in the display channel,
> > and
> > freeing a stream simply means moving it to a different ring to be
> > re-used.
> > 
> > Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> > ---
> >  server/display-channel.c | 27 ++++++++++++++++++++++++++-
> >  server/display-channel.h |  1 +
> >  server/video-stream.c    | 44 ++++++++++++----------------------
> > ----------
> >  server/video-stream.h    |  8 ++++----
> >  4 files changed, 43 insertions(+), 37 deletions(-)
> > 
> > diff --git a/server/display-channel.c b/server/display-channel.c
> > index 2caaa6433..d3db1e0db 100644
> > --- a/server/display-channel.c
> > +++ b/server/display-channel.c
> > @@ -272,7 +272,7 @@ static void stop_streams(DisplayChannel
> > *display)
> >          VideoStream *stream = SPICE_CONTAINEROF(item, VideoStream,
> > link);
> >          item = ring_next(ring, item);
> >          if (!stream->current) {
> > -            video_stream_stop(display, stream);
> > +            video_stream_stop(stream);
> >          } else {
> >              spice_debug("attached stream");
> >          }
> > @@ -2279,6 +2279,31 @@ display_channel_init(DisplayChannel *self)
> >      self->priv->image_surfaces.ops = &image_surfaces_ops;
> >  }
> >  
> > +static void display_channel_mark_stream_unused(DisplayChannel
> > *display,
> > VideoStream *stream)
> > +{
> > +    stream->next = display->priv->free_streams;
> > +    display->priv->free_streams = stream;
> > +}
> > +
> > +static void display_channel_init_video_streams(DisplayChannel
> > *display)
> > +{
> > +    int i;
> > +
> > +    ring_init(&display->priv->streams);
> > +    display->priv->free_streams = NULL;
> > +    for (i = 0; i < NUM_STREAMS; i++) {
> > +        VideoStream *stream =
> > display_channel_get_nth_video_stream(display,
> > i);
> > +        ring_item_init(&stream->link);
> > +        display_channel_mark_stream_unused(display, stream);
> > +    }
> > +}
> > +
> > +void display_channel_free_video_stream(DisplayChannel *display,
> > VideoStream
> > *stream)
> > +{
> > +    display_channel_mark_stream_unused(stream->display, stream);
> > +    display->priv->stream_count--;
> > +}
> > +
> >  static void
> >  display_channel_constructed(GObject *object)
> >  {
> > diff --git a/server/display-channel.h b/server/display-channel.h
> > index 4f7def216..a0470ec67 100644
> > --- a/server/display-channel.h
> > +++ b/server/display-channel.h
> > @@ -168,6 +168,7 @@ gboolean
> > display_channel_surface_has_canvas(DisplayChannel *display,
> > uint32_t su
> >  void display_channel_reset_image_cache(DisplayChannel *self);
> >  
> >  void display_channel_debug_oom(DisplayChannel *display, const char
> > *msg);
> > +void display_channel_free_video_stream(DisplayChannel *display,
> > VideoStream
> > *stream);
> >  
> >  G_END_DECLS
> >  
> > diff --git a/server/video-stream.c b/server/video-stream.c
> > index 65da3f8a0..a24c787ef 100644
> > --- a/server/video-stream.c
> > +++ b/server/video-stream.c
> > @@ -66,8 +66,7 @@ static void
> > video_stream_agent_stats_print(VideoStreamAgent
> > *agent)
> >  static void video_stream_create_destroy_item_release(RedPipeItem
> > *base)
> >  {
> >      StreamCreateDestroyItem *item =
> > SPICE_UPCAST(StreamCreateDestroyItem,
> >      base);
> > -    DisplayChannel *display = DCC_TO_DC(item->agent->dcc);
> > -    video_stream_agent_unref(display, item->agent);
> > +    video_stream_agent_unref(item->agent);
> >      g_free(item);
> >  }
> >  
> > @@ -94,8 +93,9 @@ static RedPipeItem
> > *video_stream_destroy_item_new(VideoStreamAgent *agent)
> >  }
> >  
> >  
> > -void video_stream_stop(DisplayChannel *display, VideoStream
> > *stream)
> > +void video_stream_stop(VideoStream *stream)
> >  {
> > +    DisplayChannel *display = stream->display;
> >      DisplayChannelClient *dcc;
> >      int stream_id = display_channel_get_video_stream_id(display,
> > stream);
> >  
> > @@ -125,53 +125,32 @@ void video_stream_stop(DisplayChannel
> > *display,
> > VideoStream *stream)
> >      }
> >      display->priv->streams_size_total -= stream->width * stream-
> > >height;
> >      ring_remove(&stream->link);
> > -    video_stream_unref(display, stream);
> > +    video_stream_unref(stream);
> >  }
> >  
> > -static void video_stream_free(DisplayChannel *display, VideoStream
> > *stream)
> > -{
> > -    stream->next = display->priv->free_streams;
> > -    display->priv->free_streams = stream;
> > -}
> > -
> > -void display_channel_init_video_streams(DisplayChannel *display)
> > -{
> > -    int i;
> > -
> > -    ring_init(&display->priv->streams);
> > -    display->priv->free_streams = NULL;
> > -    for (i = 0; i < NUM_STREAMS; i++) {
> > -        VideoStream *stream =
> > display_channel_get_nth_video_stream(display,
> > i);
> > -        ring_item_init(&stream->link);
> > -        video_stream_free(display, stream);
> > -    }
> > -}
> > -
> > -void video_stream_unref(DisplayChannel *display, VideoStream
> > *stream)
> > +void video_stream_unref(VideoStream *stream)
> >  {
> >      if (--stream->refs != 0)
> >          return;
> >  
> >      spice_warn_if_fail(!ring_item_is_linked(&stream->link));
> >  
> > -    video_stream_free(display, stream);
> > -    display->priv->stream_count--;
> > +    display_channel_free_video_stream(stream->display, stream);
> >  }
> >  
> > -void video_stream_agent_unref(DisplayChannel *display,
> > VideoStreamAgent
> > *agent)
> > +void video_stream_agent_unref(VideoStreamAgent *agent)
> >  {
> > -    video_stream_unref(display, agent->stream);
> > +    video_stream_unref(agent->stream);
> >  }
> >  
> >  static void video_stream_clip_item_free(RedPipeItem *base)
> >  {
> >      g_return_if_fail(base != NULL);
> >      VideoStreamClipItem *item = SPICE_UPCAST(VideoStreamClipItem,
> > base);
> > -    DisplayChannel *display = DCC_TO_DC(item->stream_agent->dcc);
> >  
> >      g_return_if_fail(item->base.refcount == 0);
> >  
> > -    video_stream_agent_unref(display, item->stream_agent);
> > +    video_stream_agent_unref(item->stream_agent);
> >      g_free(item->rects);
> >      g_free(item);
> >  }
> > @@ -375,6 +354,7 @@ static VideoStream
> > *display_channel_stream_try_new(DisplayChannel *display)
> >      }
> >      stream = display->priv->free_streams;
> >      display->priv->free_streams = display->priv->free_streams-
> > >next;
> > +    stream->display = display;
> >      return stream;
> >  }
> >  
> > @@ -923,7 +903,7 @@ void
> > video_stream_detach_and_stop(DisplayChannel
> > *display)
> >          VideoStream *stream = SPICE_CONTAINEROF(stream_item,
> > VideoStream,
> >          link);
> >  
> >          detach_video_stream_gracefully(display, stream, NULL);
> > -        video_stream_stop(display, stream);
> > +        video_stream_stop(stream);
> >      }
> >  }
> >  
> > @@ -939,7 +919,7 @@ void video_stream_timeout(DisplayChannel
> > *display)
> >          item = ring_next(ring, item);
> >          if (now >= (stream->last_time + RED_STREAM_TIMEOUT)) {
> >              detach_video_stream_gracefully(display, stream, NULL);
> > -            video_stream_stop(display, stream);
> > +            video_stream_stop(stream);
> >          }
> >      }
> >  }
> > diff --git a/server/video-stream.h b/server/video-stream.h
> > index a4d149816..88d59c848 100644
> > --- a/server/video-stream.h
> > +++ b/server/video-stream.h
> > @@ -125,11 +125,11 @@ struct VideoStream {
> >      uint32_t num_input_frames;
> >      uint64_t input_fps_start_time;
> >      uint32_t input_fps;
> > +    DisplayChannel *display;
> >  };
> >  
> > -void display_channel_init_video_streams(DisplayChannel *display);
> > -void video_stream_stop(DisplayChannel *display, VideoStream
> > *stream);
> > -void video_stream_unref(DisplayChannel *display, VideoStream
> > *stream);
> > +void video_stream_stop(VideoStream *stream);
> > +void video_stream_unref(VideoStream *stream);
> >  void video_stream_trace_update(DisplayChannel *display, Drawable
> > *drawable);
> >  void video_stream_maintenance(DisplayChannel *display, Drawable
> > *candidate,
> >                                Drawable *prev);
> > @@ -139,7 +139,7 @@ void
> > video_stream_trace_add_drawable(DisplayChannel
> > *display, Drawable *item);
> >  void video_stream_detach_behind(DisplayChannel *display, QRegion
> > *region,
> >                                  Drawable *drawable);
> >  
> > -void video_stream_agent_unref(DisplayChannel *display,
> > VideoStreamAgent
> > *agent);
> > +void video_stream_agent_unref(VideoStreamAgent *agent);
> >  void video_stream_agent_stop(VideoStreamAgent *agent);
> >  
> >  void video_stream_detach_drawable(VideoStream *stream);
> 
> Looks like this patch is trying to do multiple stuff.
> Yes, having display directly in the VideoStream helps but for
> instance
> there are different functions (like display_channel_get_stream_id)
> that
> would benefit from this change.
> On the other side there's no much rationale on some function move.
> This for this patch but also 3/7.

Yes, it does look a bit like it's doing more than it should. But this
is a bit of a complicated situation. These VideoStream/VideoStreamAgent
items are all preallocated in an array within the
DisplayChannel/DisplayChannelClient objects. So "freeing" a VideoStream
object is not as simple as de-allocating the memory. Instead we need to
tell the DisplayChannel (who owns the memory) that this VideoStream is
no longer used. This is done by video_stream_free(display, stream). But
this function really acts like a DisplayChannel member function  (for
example: display->free_video_stream(stream)) and requires access to the
internals of DisplayChannel. So I moved this function to display-
channel.[ch] and renamed it to display_channel_free_video_stream().

(I just noticed that video_stream_stop() also modifies the internals of
DisplayChannel, so 

> What should the rule why a given function should be in video-stream.c
> and not on display-channel.c/dcc.c/dcc-send.c? 

Well, my long-term goal was to have video-stream.[ch] only contain the
types and functions directly related to VideoStream (and potentially
VideoStreamAgent). In other words: member functions. It should not need
to access the internals of any other types (for example: DisplayChannel
or DisplayChannelClient). I agree that after this patch series, it's
nowhere near that goal, but I think it's a step in the right direction.
You may disagree though.

> Not that I have a precise
> idea myself but these functions had been moved around multiple time
> and I think the main reason is that we don't have an agreed reasoning
> of this split. Said that streaming handling is part of the display
> channel
> where should be the code that detect new streams reside? For instance
> looks like the code that initialize a VideoStream is in display-
> channel.c
> but all the *_new functions are usually in the same file that should
> handle the structure, in this case video-stream.c.

Well, since DisplayChannel maintains a pool of pre-allocated
VideoStream objects, this object is different than most in that we
don't have a *_new() function that allocates and initializes the
object. Nevertheless, I agree that the initialization of VideoStream
does not really belong in display-channel.c. I could have perhaps added
a video_stream_init(...) function that is called from
display_channel_create_stream().

> If the VideoStreamAgent is specifically related to the client (so
> DisplayChannelClient) should the code reside in dcc.c or video-
> stream.c?
> Why the definition is in video-stream.h and some code in dcc.c?
> I tried to do some split like what I did for image encoding but
> got not really far, the code is too much mingled.

Yes, I didn't try to untangle too much of the streamagent/dcc stuff yet
since it is much more entangled. As I said, this wasn't meant to be a
full solution, just a partial move in the right direction, I hope.

> 
> Frediano
> 
> On Thu, 2017-11-30 at 13:40 -0500, Frediano Ziglio wrote:
> > > 
> > > This allows us to unref the stream directly rather than needing to
> > > pass
> > > the associated DisplayChannel to stream_unref(). The same is also
> > > true
> > > for stream_agent_unref, since the only reason that
> > > stream_agent_unref()
> > > required a DisplayChannel parameter was to pass it to
> > > stream_unref().
> > > This also resulted in some additional minor redesign due to the
> > > fact
> > > that streams are pre-allocated in a pool in the display channel,
> > > and
> > > freeing a stream simply means moving it to a different ring to be
> > > re-used.
> > > 
> > > Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> > > ---
> > >  server/display-channel.c | 27 ++++++++++++++++++++++++++-
> > >  server/display-channel.h |  1 +
> > >  server/video-stream.c    | 44 ++++++++++++----------------------
> > > ----------
> > >  server/video-stream.h    |  8 ++++----
> > >  4 files changed, 43 insertions(+), 37 deletions(-)
> > > 
> > > diff --git a/server/display-channel.c b/server/display-channel.c
> > > index 2caaa6433..d3db1e0db 100644
> > > --- a/server/display-channel.c
> > > +++ b/server/display-channel.c
> > > @@ -272,7 +272,7 @@ static void stop_streams(DisplayChannel
> > > *display)
> > >          VideoStream *stream = SPICE_CONTAINEROF(item, VideoStream,
> > > link);
> > >          item = ring_next(ring, item);
> > >          if (!stream->current) {
> > > -            video_stream_stop(display, stream);
> > > +            video_stream_stop(stream);
> > >          } else {
> > >              spice_debug("attached stream");
> > >          }
> > > @@ -2279,6 +2279,31 @@ display_channel_init(DisplayChannel *self)
> > >      self->priv->image_surfaces.ops = &image_surfaces_ops;
> > >  }
> > >  
> > > +static void display_channel_mark_stream_unused(DisplayChannel
> > > *display,
> > > VideoStream *stream)
> > > +{
> > > +    stream->next = display->priv->free_streams;
> > > +    display->priv->free_streams = stream;
> > > +}
> > > +
> > > +static void display_channel_init_video_streams(DisplayChannel
> > > *display)
> > > +{
> > > +    int i;
> > > +
> > > +    ring_init(&display->priv->streams);
> > > +    display->priv->free_streams = NULL;
> > > +    for (i = 0; i < NUM_STREAMS; i++) {
> > > +        VideoStream *stream =
> > > display_channel_get_nth_video_stream(display,
> > > i);
> > > +        ring_item_init(&stream->link);
> > > +        display_channel_mark_stream_unused(display, stream);
> > > +    }
> > > +}
> > > +
> > > +void display_channel_free_video_stream(DisplayChannel *display,
> > > VideoStream
> > > *stream)
> > > +{
> > > +    display_channel_mark_stream_unused(stream->display, stream);
> > > +    display->priv->stream_count--;
> > > +}
> > > +
> > >  static void
> > >  display_channel_constructed(GObject *object)
> > >  {
> > > diff --git a/server/display-channel.h b/server/display-channel.h
> > > index 4f7def216..a0470ec67 100644
> > > --- a/server/display-channel.h
> > > +++ b/server/display-channel.h
> > > @@ -168,6 +168,7 @@ gboolean
> > > display_channel_surface_has_canvas(DisplayChannel *display,
> > > uint32_t su
> > >  void display_channel_reset_image_cache(DisplayChannel *self);
> > >  
> > >  void display_channel_debug_oom(DisplayChannel *display, const char
> > > *msg);
> > > +void display_channel_free_video_stream(DisplayChannel *display,
> > > VideoStream
> > > *stream);
> > >  
> > >  G_END_DECLS
> > >  
> > > diff --git a/server/video-stream.c b/server/video-stream.c
> > > index 65da3f8a0..a24c787ef 100644
> > > --- a/server/video-stream.c
> > > +++ b/server/video-stream.c
> > > @@ -66,8 +66,7 @@ static void
> > > video_stream_agent_stats_print(VideoStreamAgent
> > > *agent)
> > >  static void video_stream_create_destroy_item_release(RedPipeItem
> > > *base)
> > >  {
> > >      StreamCreateDestroyItem *item =
> > > SPICE_UPCAST(StreamCreateDestroyItem,
> > >      base);
> > > -    DisplayChannel *display = DCC_TO_DC(item->agent->dcc);
> > > -    video_stream_agent_unref(display, item->agent);
> > > +    video_stream_agent_unref(item->agent);
> > >      g_free(item);
> > >  }
> > >  
> > > @@ -94,8 +93,9 @@ static RedPipeItem
> > > *video_stream_destroy_item_new(VideoStreamAgent *agent)
> > >  }
> > >  
> > >  
> > > -void video_stream_stop(DisplayChannel *display, VideoStream
> > > *stream)
> > > +void video_stream_stop(VideoStream *stream)
> > >  {
> > > +    DisplayChannel *display = stream->display;
> > >      DisplayChannelClient *dcc;
> > >      int stream_id = display_channel_get_video_stream_id(display,
> > > stream);
> > >  
> > > @@ -125,53 +125,32 @@ void video_stream_stop(DisplayChannel
> > > *display,
> > > VideoStream *stream)
> > >      }
> > >      display->priv->streams_size_total -= stream->width * stream-
> > > >height;
> > >      ring_remove(&stream->link);
> > > -    video_stream_unref(display, stream);
> > > +    video_stream_unref(stream);
> > >  }
> > >  
> > > -static void video_stream_free(DisplayChannel *display, VideoStream
> > > *stream)
> > > -{
> > > -    stream->next = display->priv->free_streams;
> > > -    display->priv->free_streams = stream;
> > > -}
> > > -
> > > -void display_channel_init_video_streams(DisplayChannel *display)
> > > -{
> > > -    int i;
> > > -
> > > -    ring_init(&display->priv->streams);
> > > -    display->priv->free_streams = NULL;
> > > -    for (i = 0; i < NUM_STREAMS; i++) {
> > > -        VideoStream *stream =
> > > display_channel_get_nth_video_stream(display,
> > > i);
> > > -        ring_item_init(&stream->link);
> > > -        video_stream_free(display, stream);
> > > -    }
> > > -}
> > > -
> > > -void video_stream_unref(DisplayChannel *display, VideoStream
> > > *stream)
> > > +void video_stream_unref(VideoStream *stream)
> > >  {
> > >      if (--stream->refs != 0)
> > >          return;
> > >  
> > >      spice_warn_if_fail(!ring_item_is_linked(&stream->link));
> > >  
> > > -    video_stream_free(display, stream);
> > > -    display->priv->stream_count--;
> > > +    display_channel_free_video_stream(stream->display, stream);
> > >  }
> > >  
> > > -void video_stream_agent_unref(DisplayChannel *display,
> > > VideoStreamAgent
> > > *agent)
> > > +void video_stream_agent_unref(VideoStreamAgent *agent)
> > >  {
> > > -    video_stream_unref(display, agent->stream);
> > > +    video_stream_unref(agent->stream);
> > >  }
> > >  
> > >  static void video_stream_clip_item_free(RedPipeItem *base)
> > >  {
> > >      g_return_if_fail(base != NULL);
> > >      VideoStreamClipItem *item = SPICE_UPCAST(VideoStreamClipItem,
> > > base);
> > > -    DisplayChannel *display = DCC_TO_DC(item->stream_agent->dcc);
> > >  
> > >      g_return_if_fail(item->base.refcount == 0);
> > >  
> > > -    video_stream_agent_unref(display, item->stream_agent);
> > > +    video_stream_agent_unref(item->stream_agent);
> > >      g_free(item->rects);
> > >      g_free(item);
> > >  }
> > > @@ -375,6 +354,7 @@ static VideoStream
> > > *display_channel_stream_try_new(DisplayChannel *display)
> > >      }
> > >      stream = display->priv->free_streams;
> > >      display->priv->free_streams = display->priv->free_streams-
> > > >next;
> > > +    stream->display = display;
> > >      return stream;
> > >  }
> > >  
> > > @@ -923,7 +903,7 @@ void
> > > video_stream_detach_and_stop(DisplayChannel
> > > *display)
> > >          VideoStream *stream = SPICE_CONTAINEROF(stream_item,
> > > VideoStream,
> > >          link);
> > >  
> > >          detach_video_stream_gracefully(display, stream, NULL);
> > > -        video_stream_stop(display, stream);
> > > +        video_stream_stop(stream);
> > >      }
> > >  }
> > >  
> > > @@ -939,7 +919,7 @@ void video_stream_timeout(DisplayChannel
> > > *display)
> > >          item = ring_next(ring, item);
> > >          if (now >= (stream->last_time + RED_STREAM_TIMEOUT)) {
> > >              detach_video_stream_gracefully(display, stream, NULL);
> > > -            video_stream_stop(display, stream);
> > > +            video_stream_stop(stream);
> > >          }
> > >      }
> > >  }
> > > diff --git a/server/video-stream.h b/server/video-stream.h
> > > index a4d149816..88d59c848 100644
> > > --- a/server/video-stream.h
> > > +++ b/server/video-stream.h
> > > @@ -125,11 +125,11 @@ struct VideoStream {
> > >      uint32_t num_input_frames;
> > >      uint64_t input_fps_start_time;
> > >      uint32_t input_fps;
> > > +    DisplayChannel *display;
> > >  };
> > >  
> > > -void display_channel_init_video_streams(DisplayChannel *display);
> > > -void video_stream_stop(DisplayChannel *display, VideoStream
> > > *stream);
> > > -void video_stream_unref(DisplayChannel *display, VideoStream
> > > *stream);
> > > +void video_stream_stop(VideoStream *stream);
> > > +void video_stream_unref(VideoStream *stream);
> > >  void video_stream_trace_update(DisplayChannel *display, Drawable
> > > *drawable);
> > >  void video_stream_maintenance(DisplayChannel *display, Drawable
> > > *candidate,
> > >                                Drawable *prev);
> > > @@ -139,7 +139,7 @@ void
> > > video_stream_trace_add_drawable(DisplayChannel
> > > *display, Drawable *item);
> > >  void video_stream_detach_behind(DisplayChannel *display, QRegion
> > > *region,
> > >                                  Drawable *drawable);
> > >  
> > > -void video_stream_agent_unref(DisplayChannel *display,
> > > VideoStreamAgent
> > > *agent);
> > > +void video_stream_agent_unref(VideoStreamAgent *agent);
> > >  void video_stream_agent_stop(VideoStreamAgent *agent);
> > >  
> > >  void video_stream_detach_drawable(VideoStream *stream);
> > 
> > Looks like this patch is trying to do multiple stuff.
> > Yes, having display directly in the VideoStream helps but for
> > instance
> > there are different functions (like display_channel_get_stream_id)
> > that
> > would benefit from this change.
> > On the other side there's no much rationale on some function move.
> > This for this patch but also 3/7.
> 
> Yes, it does look a bit like it's doing more than it should. But this
> is a bit of a complicated situation. These VideoStream/VideoStreamAgent
> items are all preallocated in an array within the
> DisplayChannel/DisplayChannelClient objects. So "freeing" a VideoStream
> object is not as simple as de-allocating the memory. Instead we need to
> tell the DisplayChannel (who owns the memory) that this VideoStream is
> no longer used. This is done by video_stream_free(display, stream). But
> this function really acts like a DisplayChannel member function  (for
> example: display->free_video_stream(stream)) and requires access to the
> internals of DisplayChannel. So I moved this function to display-
> channel.[ch] and renamed it to display_channel_free_video_stream().
> 

In case of pool allocation usually you simply provide a init/deinit or
constructor/destructor functions and you do the allocation externally
so you have

   memory = allocate_memory(...);
   object_init(memory, ...);

and

   object_deinit(memory);
   free_memory(...);

In C++:
  obj = new Object(...);
  delete obj;

and Rust:

  let obj = Box::new(Object::new(...));

these lines use the normal allocator which you could change using
different way (in C++ you can define special class operators in Rust
you define a new Box type).

The question about the pool is however is a VideoStream context or really
a specific DisplayChannel property? Currently you can say that the
static/class members of VideoStream are in DisplayChannel.
In C there's no member or object, just structures and arrays to pack
data. Currently DisplayChannel access freely VideoStream and quite
freely vice versa. Adding a DisplayChannel pointer to VideoStream
is helpful and there's already a strict 1..N relation between
DisplayChannel and VideoStream but you have to consider that this pointer
make more stronger the coupling of the 2 structures. And as structures
I won't speak about members, members requires objects, objects require
more encapsulation and separation than structures and currently I
personally can't see enough decoupling to speak about objects.
The DisplayChannel just stores some video stream information in
an array of VideoStream.

I think that currently video-stream.c try to separate the code/functions
more related to streams, that deals with VideoStream (streams information
per stream on the channel), VideoStreamAgent (streams information client
specific) and ItemTrace (potentially streams).

> (I just noticed that video_stream_stop() also modifies the internals of
> DisplayChannel, so
> 
> > What should the rule why a given function should be in video-stream.c
> > and not on display-channel.c/dcc.c/dcc-send.c?
> 
> Well, my long-term goal was to have video-stream.[ch] only contain the
> types and functions directly related to VideoStream (and potentially
> VideoStreamAgent). In other words: member functions. It should not need
> to access the internals of any other types (for example: DisplayChannel
> or DisplayChannelClient). I agree that after this patch series, it's
> nowhere near that goal, but I think it's a step in the right direction.
> You may disagree though.
> 

As said without much decoupling I won't even talk about members.
Not saying they should not be but moving functions at the current
state does not help looking tomorrow at the history and as you said
this split is a long-term goal.
I would limit the moves and instead add some comments.
If you looks at the history this code has been moved here and there
too many times.

> > Not that I have a precise
> > idea myself but these functions had been moved around multiple time
> > and I think the main reason is that we don't have an agreed reasoning
> > of this split. Said that streaming handling is part of the display
> > channel
> > where should be the code that detect new streams reside? For instance
> > looks like the code that initialize a VideoStream is in display-
> > channel.c
> > but all the *_new functions are usually in the same file that should
> > handle the structure, in this case video-stream.c.
> 
> Well, since DisplayChannel maintains a pool of pre-allocated
> VideoStream objects, this object is different than most in that we
> don't have a *_new() function that allocates and initializes the
> object. Nevertheless, I agree that the initialization of VideoStream
> does not really belong in display-channel.c. I could have perhaps added
> a video_stream_init(...) function that is called from
> display_channel_create_stream().
> 

Yes, I think this goes in the right direction. But currently is more
an attempt to the right direction, that's why I would avoid moving
functions so easily.

> > If the VideoStreamAgent is specifically related to the client (so
> > DisplayChannelClient) should the code reside in dcc.c or video-
> > stream.c?
> > Why the definition is in video-stream.h and some code in dcc.c?
> > I tried to do some split like what I did for image encoding but
> > got not really far, the code is too much mingled.
> 
> Yes, I didn't try to untangle too much of the streamagent/dcc stuff yet
> since it is much more entangled. As I said, this wasn't meant to be a
> full solution, just a partial move in the right direction, I hope.
> 

Looking at how are split these stuff before video-stream.h also contains
RedPipeItem definition that is is supposed to also deal with client/pipe
stuff.

I tried as an experiment to remove/rename fields in VideoStream and see
which module didn't compile and mostly of them are accessed in different
files.

Talking about "right direction" is usually easy to define once you have a
start point (A) and a destination point (B). The start is easy, the current
code, the destination unfortunately is not. Maybe a conditional code with
not too much #if I_WANT_STREAM_SUPPORT would be an idea.

I tried to separate stream stuff and create this:
https://cgit.freedesktop.org/~fziglio/spice-server/log/?h=stream_separation
but is really experimental.

I rebased your work, there's a conflicting patch, if you need is at
https://cgit.freedesktop.org/~fziglio/spice-server/log/?h=jj_video_streams

Frediano
On 11/29/2017 10:16 PM, Jonathon Jongsma wrote:
> This allows us to unref the stream directly rather than needing to pass
> the associated DisplayChannel to stream_unref(). The same is also true
> for stream_agent_unref, since the only reason that stream_agent_unref()
> required a DisplayChannel parameter was to pass it to stream_unref().
> This also resulted in some additional minor redesign due to the fact
> that streams are pre-allocated in a pool in the display channel, and
> freeing a stream simply means moving it to a different ring to be
> re-used.
> 
> Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> ---
>   server/display-channel.c | 27 ++++++++++++++++++++++++++-
>   server/display-channel.h |  1 +
>   server/video-stream.c    | 44 ++++++++++++--------------------------------
>   server/video-stream.h    |  8 ++++----
>   4 files changed, 43 insertions(+), 37 deletions(-)
> 
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 2caaa6433..d3db1e0db 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c

> +
> +void display_channel_free_video_stream(DisplayChannel *display, VideoStream *stream)

Hi Jonathon,

You do not need to pass "display" here. Just use stream->display.

> +{
> +    display_channel_mark_stream_unused(stream->display, stream);
> +    display->priv->stream_count--;
> +}
> +


> diff --git a/server/display-channel.h b/server/display-channel.h
> index 4f7def216..a0470ec67 100644
> --- a/server/display-channel.h
> +++ b/server/display-channel.h
> @@ -168,6 +168,7 @@ gboolean display_channel_surface_has_canvas(DisplayChannel *display, uint32_t su
>   void display_channel_reset_image_cache(DisplayChannel *self);
>   
>   void display_channel_debug_oom(DisplayChannel *display, const char *msg);
> +void display_channel_free_video_stream(DisplayChannel *display, VideoStream *stream);

And here.

>   
>   G_END_DECLS
>   
> diff --git a/server/video-stream.c b/server/video-stream.c
> index 65da3f8a0..a24c787ef 100644
> --- a/server/video-stream.c
> +++ b/server/video-stream.c

<snip>

> -void video_stream_unref(DisplayChannel *display, VideoStream *stream)
> +void video_stream_unref(VideoStream *stream)
>   {
>       if (--stream->refs != 0)
>           return;
>   
>       spice_warn_if_fail(!ring_item_is_linked(&stream->link));
>   
> -    video_stream_free(display, stream);
> -    display->priv->stream_count--;
> +    display_channel_free_video_stream(stream->display, stream);

This is the only caller and it uses stream->display.

Thanks,
     Uri.
> 
> On 11/29/2017 10:16 PM, Jonathon Jongsma wrote:
> > This allows us to unref the stream directly rather than needing to pass
> > the associated DisplayChannel to stream_unref(). The same is also true
> > for stream_agent_unref, since the only reason that stream_agent_unref()
> > required a DisplayChannel parameter was to pass it to stream_unref().
> > This also resulted in some additional minor redesign due to the fact
> > that streams are pre-allocated in a pool in the display channel, and
> > freeing a stream simply means moving it to a different ring to be
> > re-used.
> > 
> > Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> > ---
> >   server/display-channel.c | 27 ++++++++++++++++++++++++++-
> >   server/display-channel.h |  1 +
> >   server/video-stream.c    | 44
> >   ++++++++++++--------------------------------
> >   server/video-stream.h    |  8 ++++----
> >   4 files changed, 43 insertions(+), 37 deletions(-)
> > 
> > diff --git a/server/display-channel.c b/server/display-channel.c
> > index 2caaa6433..d3db1e0db 100644
> > --- a/server/display-channel.c
> > +++ b/server/display-channel.c
> 
> > +
> > +void display_channel_free_video_stream(DisplayChannel *display,
> > VideoStream *stream)
> 
> Hi Jonathon,
> 
> You do not need to pass "display" here. Just use stream->display.
> 
> > +{
> > +    display_channel_mark_stream_unused(stream->display, stream);
> > +    display->priv->stream_count--;
> > +}
> > +
> 

Jonathon is also trying to separate streaming in a "right" way so
if display-channel.c needs to access a new VideoStream field is not
really ideal.

> 
> > diff --git a/server/display-channel.h b/server/display-channel.h
> > index 4f7def216..a0470ec67 100644
> > --- a/server/display-channel.h
> > +++ b/server/display-channel.h
> > @@ -168,6 +168,7 @@ gboolean
> > display_channel_surface_has_canvas(DisplayChannel *display, uint32_t su
> >   void display_channel_reset_image_cache(DisplayChannel *self);
> >   
> >   void display_channel_debug_oom(DisplayChannel *display, const char *msg);
> > +void display_channel_free_video_stream(DisplayChannel *display,
> > VideoStream *stream);
> 
> And here.
> 
> >   
> >   G_END_DECLS
> >   
> > diff --git a/server/video-stream.c b/server/video-stream.c
> > index 65da3f8a0..a24c787ef 100644
> > --- a/server/video-stream.c
> > +++ b/server/video-stream.c
> 
> <snip>
> 
> > -void video_stream_unref(DisplayChannel *display, VideoStream *stream)
> > +void video_stream_unref(VideoStream *stream)
> >   {
> >       if (--stream->refs != 0)
> >           return;
> >   
> >       spice_warn_if_fail(!ring_item_is_linked(&stream->link));
> >   
> > -    video_stream_free(display, stream);
> > -    display->priv->stream_count--;
> > +    display_channel_free_video_stream(stream->display, stream);
> 
> This is the only caller and it uses stream->display.
> 
> Thanks,
>      Uri.

Uri,
   why is not clear to you the design point? Is the comment not clear
about? I personally think that "This also resulted in some additional
minor redesign"... sentence does not explain why this resulted in
redesign and which is the redesign.

Frediano
On 12/04/2017 06:07 PM, Frediano Ziglio wrote:
>>
>> On 11/29/2017 10:16 PM, Jonathon Jongsma wrote:
>>> This allows us to unref the stream directly rather than needing to pass
>>> the associated DisplayChannel to stream_unref(). The same is also true
>>> for stream_agent_unref, since the only reason that stream_agent_unref()
>>> required a DisplayChannel parameter was to pass it to stream_unref().
>>> This also resulted in some additional minor redesign due to the fact
>>> that streams are pre-allocated in a pool in the display channel, and
>>> freeing a stream simply means moving it to a different ring to be
>>> re-used.
>>>
>>> Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
>>> ---
>>>    server/display-channel.c | 27 ++++++++++++++++++++++++++-
>>>    server/display-channel.h |  1 +
>>>    server/video-stream.c    | 44
>>>    ++++++++++++--------------------------------
>>>    server/video-stream.h    |  8 ++++----
>>>    4 files changed, 43 insertions(+), 37 deletions(-)
>>>
>>> diff --git a/server/display-channel.c b/server/display-channel.c
>>> index 2caaa6433..d3db1e0db 100644
>>> --- a/server/display-channel.c
>>> +++ b/server/display-channel.c
>>
>>> +
>>> +void display_channel_free_video_stream(DisplayChannel *display,
>>> VideoStream *stream)
>>
>> Hi Jonathon,
>>
>> You do not need to pass "display" here. Just use stream->display.
>>
>>> +{
>>> +    display_channel_mark_stream_unused(stream->display, stream);
>>> +    display->priv->stream_count--;
>>> +}
>>> +
>>
> 
> Jonathon is also trying to separate streaming in a "right" way so
> if display-channel.c needs to access a new VideoStream field is not
> really ideal.
> 
>>
>>> diff --git a/server/display-channel.h b/server/display-channel.h
>>> index 4f7def216..a0470ec67 100644
>>> --- a/server/display-channel.h
>>> +++ b/server/display-channel.h
>>> @@ -168,6 +168,7 @@ gboolean
>>> display_channel_surface_has_canvas(DisplayChannel *display, uint32_t su
>>>    void display_channel_reset_image_cache(DisplayChannel *self);
>>>    
>>>    void display_channel_debug_oom(DisplayChannel *display, const char *msg);
>>> +void display_channel_free_video_stream(DisplayChannel *display,
>>> VideoStream *stream);
>>
>> And here.
>>
>>>    
>>>    G_END_DECLS
>>>    
>>> diff --git a/server/video-stream.c b/server/video-stream.c
>>> index 65da3f8a0..a24c787ef 100644
>>> --- a/server/video-stream.c
>>> +++ b/server/video-stream.c
>>
>> <snip>
>>
>>> -void video_stream_unref(DisplayChannel *display, VideoStream *stream)
>>> +void video_stream_unref(VideoStream *stream)
>>>    {
>>>        if (--stream->refs != 0)
>>>            return;
>>>    
>>>        spice_warn_if_fail(!ring_item_is_linked(&stream->link));
>>>    
>>> -    video_stream_free(display, stream);
>>> -    display->priv->stream_count--;
>>> +    display_channel_free_video_stream(stream->display, stream);
>>
>> This is the only caller and it uses stream->display.
>>
>> Thanks,
>>       Uri.
> 
> Uri,
>     why is not clear to you the design point? Is the comment not clear
> about? I personally think that "This also resulted in some additional
> minor redesign"... sentence does not explain why this resulted in
> redesign and which is the redesign.
> 
> Frediano
> 

My comment is not about the design, but about the specific patch.
The patch itself does not seems to achieve the design goal, but as
Jonathon mentioned (in one of his replies) he feels it's a move
in the right direction.

Now for the specific patch.
Since struct VideoStream  now contains DisplayChannel *display
1. You may as well use it in when calling 
display_channel_free_video_stream()
2. In display_channel_free_video_stream itself:
+    display_channel_mark_stream_unused(stream->display, stream);
+    display->priv->stream_count--;

    The first line uses stream->display
    The second line uses display directly.


If you want to pass "display" and not use stream->display
the first line should be changed accordingly.

Uri.
> 
> On 12/04/2017 06:07 PM, Frediano Ziglio wrote:
> >>
> >> On 11/29/2017 10:16 PM, Jonathon Jongsma wrote:
> >>> This allows us to unref the stream directly rather than needing to pass
> >>> the associated DisplayChannel to stream_unref(). The same is also true
> >>> for stream_agent_unref, since the only reason that stream_agent_unref()
> >>> required a DisplayChannel parameter was to pass it to stream_unref().
> >>> This also resulted in some additional minor redesign due to the fact
> >>> that streams are pre-allocated in a pool in the display channel, and
> >>> freeing a stream simply means moving it to a different ring to be
> >>> re-used.
> >>>
> >>> Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> >>> ---
> >>>    server/display-channel.c | 27 ++++++++++++++++++++++++++-
> >>>    server/display-channel.h |  1 +
> >>>    server/video-stream.c    | 44
> >>>    ++++++++++++--------------------------------
> >>>    server/video-stream.h    |  8 ++++----
> >>>    4 files changed, 43 insertions(+), 37 deletions(-)
> >>>
> >>> diff --git a/server/display-channel.c b/server/display-channel.c
> >>> index 2caaa6433..d3db1e0db 100644
> >>> --- a/server/display-channel.c
> >>> +++ b/server/display-channel.c
> >>
> >>> +
> >>> +void display_channel_free_video_stream(DisplayChannel *display,
> >>> VideoStream *stream)
> >>
> >> Hi Jonathon,
> >>
> >> You do not need to pass "display" here. Just use stream->display.
> >>
> >>> +{
> >>> +    display_channel_mark_stream_unused(stream->display, stream);
> >>> +    display->priv->stream_count--;
> >>> +}
> >>> +
> >>
> > 
> > Jonathon is also trying to separate streaming in a "right" way so
> > if display-channel.c needs to access a new VideoStream field is not
> > really ideal.
> > 
> >>
> >>> diff --git a/server/display-channel.h b/server/display-channel.h
> >>> index 4f7def216..a0470ec67 100644
> >>> --- a/server/display-channel.h
> >>> +++ b/server/display-channel.h
> >>> @@ -168,6 +168,7 @@ gboolean
> >>> display_channel_surface_has_canvas(DisplayChannel *display, uint32_t su
> >>>    void display_channel_reset_image_cache(DisplayChannel *self);
> >>>    
> >>>    void display_channel_debug_oom(DisplayChannel *display, const char
> >>>    *msg);
> >>> +void display_channel_free_video_stream(DisplayChannel *display,
> >>> VideoStream *stream);
> >>
> >> And here.
> >>
> >>>    
> >>>    G_END_DECLS
> >>>    
> >>> diff --git a/server/video-stream.c b/server/video-stream.c
> >>> index 65da3f8a0..a24c787ef 100644
> >>> --- a/server/video-stream.c
> >>> +++ b/server/video-stream.c
> >>
> >> <snip>
> >>
> >>> -void video_stream_unref(DisplayChannel *display, VideoStream *stream)
> >>> +void video_stream_unref(VideoStream *stream)
> >>>    {
> >>>        if (--stream->refs != 0)
> >>>            return;
> >>>    
> >>>        spice_warn_if_fail(!ring_item_is_linked(&stream->link));
> >>>    
> >>> -    video_stream_free(display, stream);
> >>> -    display->priv->stream_count--;
> >>> +    display_channel_free_video_stream(stream->display, stream);
> >>
> >> This is the only caller and it uses stream->display.
> >>
> >> Thanks,
> >>       Uri.
> > 
> > Uri,
> >     why is not clear to you the design point? Is the comment not clear
> > about? I personally think that "This also resulted in some additional
> > minor redesign"... sentence does not explain why this resulted in
> > redesign and which is the redesign.
> > 
> > Frediano
> > 
> 
> My comment is not about the design, but about the specific patch.
> The patch itself does not seems to achieve the design goal, but as
> Jonathon mentioned (in one of his replies) he feels it's a move
> in the right direction.
> 

The patch specifically says "This also resulted in some additional
minor redesign", your comment refers to part of the patch.
I think we all agree the patch does not achieve a goal but from
your "he feels it's a move in the right direction" I can understand
you also are not sure about that direction.

From Jonathon comments:
- "my long-term goal was to have video-stream.[ch] only contain the
  types and functions directly related to VideoStream (and potentially
  VideoStreamAgent). In other words: member functions"
  So member functions and VideoStream means to me VideoStream encapsulation
  that is all or most of the fields in this structure should be accesses
  from video-stream.c. Accessing an additional "display" field from
  display-channel.c goes in the exact opposite direction of this.
  Also the "potentially VideoStreamAgent" clearly indicate that there
  are some uncertainty on the design. Not a bit issue but we should not
  follow the uncertainty.
- "It should not need to access the internals of any other types (for
  example: DisplayChannel or DisplayChannelClient)". This talks about
  responsibility and dependency. Adding a display field to VideoStream
  goes in the exact opposite direction of the dependency (if the previous
  sentence was supposing dependency).
As I mentioned above it seems to me that there's are no much certainty
on the "design" and that is hard to say if this patch goes into the
right direction if you don't know what do you want. And as I say above I
also see that some changes are going in the exact opposite direction of
the partial design idea.

Looking at the partial design mentioned above and at the actual code
I would really try to add a VideoStreams objects. It seems to me that
there is a strong indication of a global VideoStream state that is
partially in DisplayChannel. For instance:
- VideoStream has a next field. So.. what's this next field? To me
  it seems to indicate that VideoStream is by definition contained
  in a container; Somebody could argue that this field is only used
  by the pooling so could be moved to DisplayChannel somehow;
- VideoStream has a link field. This looks similar to the above case
  but this field is used a lot in video-stream.c which more strongly
  indicate that being in a container is a property of this object
  (if we want to have an object);
- VideoStream has an id. Yes, currently this id is retrieved with
  display_channel_get_video_stream_id but this still have clear
  VideoStream -> DisplayChannel dependency (which seems to go again
  the above design") and is still suggesting to have a container;
  If we agree on the VideoStream inside a container property would
  be sensible to have this field inside VideoStream.

As said I don't have exactly a 100% design in mind and are not ack/nack
about this series but I want to have some more clear understand of
the design we want to be able to say "yes, this is going in the
direction we want" instead of a "ok, YOU feel that way I trust you, ack"
which is IMHO a wrong way to review patches (should be "ok, I feel that
way too, ack").

My initial series (never posted) for this "separation" was dragged
by the idea that "some fields in DisplayChannel are dealing with streaming
and they should be in stream.c". I think the design from Jonathon
are more on the "I feel VideoStream should be an object".
Both are partial design/solution I think and as such they both partially
goes in some not really specified direction. I think would be great to
have a well defined target.

> Now for the specific patch.
> Since struct VideoStream  now contains DisplayChannel *display
> 1. You may as well use it in when calling
> display_channel_free_video_stream()
> 2. In display_channel_free_video_stream itself:
> +    display_channel_mark_stream_unused(stream->display, stream);
> +    display->priv->stream_count--;
> 
>     The first line uses stream->display
>     The second line uses display directly.
> 
> 
> If you want to pass "display" and not use stream->display
> the first line should be changed accordingly.
> 
> Uri.
> 

Frediano