On 04/10/2018 02:58 PM, Victor Toso wrote:
Hi,
On Mon, Apr 09, 2018 at 02:14:15PM +0300, Uri Lublin wrote:
On 03/13/2018 01:25 PM, Victor Toso wrote:
From: Victor Toso <me@xxxxxxxxxxxxxx>
The major issue with the current approach is that it relies on
the ID from SpiceMsgDisplayStreamCreate to create the smallest 2^n
sized array that could fit the stream's id as index.
This is potentially dangerous as the ID value is not documented and
could have any uint32_t value. We depend on Spice server's
implementation which currently defines max of 50 streams. >
/** Maximum number of streams created by spice-server */
#define NUM_STREAMS 50
The first ID has the value of 49* while the c->streams array would be
allocated to 64. We could only benefit from allocating on high
creating/removal of streams, which is not the case.
We can improve the above situations by using a hash table.
Some thoughts:
1. It make sense to make it 64 on the server side.
Why? Just so we can alloc a power of two?
Yes, there are 14 entries that are allocated but not used.
It probably does not matter much, as mostly 50 is not reached.
2. Perhaps we should limit the total number of stream allowed
(and/or add a message telling the client how many streams
the server allocates)
I don't know why we should limit it at this poing.
3. O(1) is not exactly 1 and also hash consumes (a little bit)
more memory.
Indeed. Hash table takes more time then a direct access in an
array and uses more memory too but I'm thinking that we are not
dealing with enough data to consider hash collisions to be a
problem.
Other than those and some comments below, it looks good.
Thanks,
Signed-off-by: Victor Toso <victortoso@xxxxxxxxxx>
---
src/channel-display.c | 73 ++++++++++++++++++++++-----------------------------
1 file changed, 32 insertions(+), 41 deletions(-)
diff --git a/src/channel-display.c b/src/channel-display.c
index 2ea0922..ec94cf8 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -63,8 +63,7 @@ struct _SpiceDisplayChannelPrivate {
SpicePaletteCache palette_cache;
SpiceImageSurfaces image_surfaces;
SpiceGlzDecoderWindow *glz_window;
- display_stream **streams;
- int nstreams;
+ GHashTable *streams;
gboolean mark;
guint mark_false_event_id;
GArray *monitors;
@@ -168,7 +167,7 @@ static void spice_display_channel_finalize(GObject *object)
g_clear_pointer(&c->monitors, g_array_unref);
clear_surfaces(SPICE_CHANNEL(object), FALSE);
g_hash_table_unref(c->surfaces);
- clear_streams(SPICE_CHANNEL(object));
+ g_clear_pointer(&c->streams, g_hash_table_unref);
Is it safer to keep clear_streams similar to clear_surfaces ?
There is a difference, clear_surfaces also emits a signal.
The main use for clear_streams() is to clear the streams, which
does that by calling g_hash_table_remove_all(). In the finalize
method we are destroying the object so I think g_clear_pointer()
with unref for the GHashTable works fine.
Maybe I did not understand what you mean with clear_surfaces. I
did not touch that part but as you said clear_surfaces() is more
elaborated then clear_streams(). Still, I'd mind to change the
g_hash_table_unref(c->surfaces) to be a g_clear_pointer(..) too.
What I meant is that the hash table entries are being released only
once anyway, so the overhead is not that big.
I read in g_hash_table_new_full() documentation that sometimes
it's better to call g_hash_table_remove_all before _unref.
However, I think in this case it's not required.
Also I saw that for surfaces too, the entries are freed
before the g_hash_table_unref.
You can leave it as is.
g_clear_pointer(&c->palettes, cache_free);
if (G_OBJECT_CLASS(spice_display_channel_parent_class)->finalize)
@@ -863,6 +862,7 @@ static void spice_display_channel_init(SpiceDisplayChannel *channel)
c = channel->priv = SPICE_DISPLAY_CHANNEL_GET_PRIVATE(channel);
c->surfaces = g_hash_table_new_full(NULL, NULL, NULL, destroy_surface);
+ c->streams = g_hash_table_new_full(g_direct_hash, g_direct_equal, NULL, display_stream_destroy);
- Stream id is an integer, why not keep integer keys
(g_int_hash, g_int_equal)?
Probably it does not matter much.
It is a unsigned integer. Yes, I was using it in v1 but it wasn't
working properly while with g_direct_hash() I never had a
problem. Probably my fault.
- Why make it different than the line above (using NULL instead of
specifying the default functions)
Because the there is not allocation to the hash table's key
(pointer to uint);
Either I do not understand you or vice versa (or both).
I meant, why not
c->surfaces = g_hash_table_new_full(NULL, NULL, NULL, destroy_surface);
+c->streams = g_hash_table_new_full(NULL, NULL, NULL,
display_stream_destroy);
It does not really matter, just more consistent with current code.
Uri.
Thanks for the review, let me know if there anything you want me
to change ;) >
Cheers,
toso
c->image_cache.ops = &image_cache_ops;
c->palette_cache.ops = &palette_cache_ops;
c->image_surfaces.ops = &image_surfaces_ops;
@@ -1207,15 +1207,18 @@ static void report_invalid_stream(SpiceChannel *channel, uint32_t id)
static display_stream *get_stream_by_id(SpiceChannel *channel, uint32_t id)
{
+ display_stream *st;
SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
- if (c != NULL && c->streams != NULL && id < c->nstreams &&
- c->streams[id] != NULL) {
- return c->streams[id];
+ g_return_val_if_fail(c != NULL && c->streams != NULL, NULL);
+
+ st = g_hash_table_lookup(c->streams, GUINT_TO_POINTER(id));
+ if (st == NULL) {
+ spice_printerr("couldn't find stream %u", id);
+ report_invalid_stream(channel, id);
}
- report_invalid_stream(channel, id);
- return NULL;
+ return st;
}
/* coroutine context */
@@ -1257,45 +1260,36 @@ static display_stream *display_stream_create(SpiceChannel *channel,
return st;
}
-static void destroy_stream(SpiceChannel *channel, int id)
+static void destroy_stream(SpiceChannel *channel, uint32_t id)
{
SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
g_return_if_fail(c != NULL);
g_return_if_fail(c->streams != NULL);
- g_return_if_fail(c->nstreams > id);
- g_clear_pointer(&c->streams[id], display_stream_destroy);
+ g_hash_table_remove(c->streams, GUINT_TO_POINTER(id));
}
static void display_handle_stream_create(SpiceChannel *channel, SpiceMsgIn *in)
{
SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
SpiceMsgDisplayStreamCreate *op = spice_msg_in_parsed(in);
+ display_stream *st;
CHANNEL_DEBUG(channel, "%s: id %u", __FUNCTION__, op->id);
+ g_return_if_fail(c->streams != NULL);
- if (op->id >= c->nstreams) {
- int n = c->nstreams;
- if (!c->nstreams) {
- c->nstreams = 1;
- }
- while (op->id >= c->nstreams) {
- c->nstreams *= 2;
- }
- c->streams = realloc(c->streams, c->nstreams * sizeof(c->streams[0]));
- memset(c->streams + n, 0, (c->nstreams - n) * sizeof(c->streams[0]));
- }
- g_return_if_fail(c->streams[op->id] == NULL);
-
- c->streams[op->id] = display_stream_create(channel, op->id, op->surface_id,
- op->flags, op->codec_type,
- &op->dest, &op->clip);
- if (c->streams[op->id] == NULL) {
+ st = display_stream_create(channel, op->id, op->surface_id,
+ op->flags, op->codec_type,
+ &op->dest, &op->clip);
+ if (st == NULL) {
spice_printerr("could not create the %u video stream", op->id);
destroy_stream(channel, op->id);
report_invalid_stream(channel, op->id);
+ return;
}
+
+ g_hash_table_insert(c->streams, GUINT_TO_POINTER(op->id), st);
}
static const SpiceRect *stream_get_dest(display_stream *st, SpiceMsgIn *frame_msg)
@@ -1470,18 +1464,21 @@ static void display_session_mm_time_reset_cb(SpiceSession *session, gpointer dat
{
SpiceChannel *channel = data;
SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
- guint i;
+ GHashTableIter iter;
+ gpointer key, value;
CHANNEL_DEBUG(channel, "%s", __FUNCTION__);
- for (i = 0; i < c->nstreams; i++) {
- display_stream *st;
+ g_hash_table_iter_init (&iter, c->streams);
+ while (g_hash_table_iter_next (&iter, &key, &value)) {
+ display_stream *st = value;
- if (c->streams[i] == NULL) {
+ if (st == NULL) {
+ g_warn_if_reached();
continue;
}
- SPICE_DEBUG("%s: stream-id %u", __FUNCTION__, i);
- st = c->streams[i];
+
+ SPICE_DEBUG("%s: stream-id %u", __FUNCTION__, GPOINTER_TO_UINT(key));
st->video_decoder->reschedule(st->video_decoder);
}
}
@@ -1626,13 +1623,7 @@ static void display_stream_destroy(gpointer st_pointer)
static void clear_streams(SpiceChannel *channel)
{
SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
- int i;
-
- for (i = 0; i < c->nstreams; i++) {
- destroy_stream(channel, i);
- }
- g_clear_pointer(&c->streams, g_free);
- c->nstreams = 0;
+ g_hash_table_remove_all(c->streams);
}
/* coroutine context */
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel