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