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