[Spice-devel] [PATCH spice-server 2/7] VideoStream: store channel in stream
Frediano Ziglio
fziglio at redhat.com
Fri Dec 1 17:48:32 UTC 2017
>
> 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 at 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
More information about the Spice-devel
mailing list