[Spice-devel] [PATCH spice-server 2/7] VideoStream: store channel in stream

[Frediano Ziglio]

> 
> 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 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
> 
> > +
> > +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