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@xxxxxxxxxx>
> > ---
> > 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel