[spice-gtk,v1,04/11] channel-display: use GHashTable to keep stream's structure

Submitted by Victor Toso on March 13, 2018, 11:25 a.m.

Details

Message ID 20180313112542.32238-5-victortoso@redhat.com
State New
Headers show
Series "misc improvements in channel-display" ( rev: 2 1 ) in Spice

Not browsing as part of any series.

Commit Message

Victor Toso March 13, 2018, 11:25 a.m.
From: Victor Toso <me@victortoso.com>

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.

Signed-off-by: Victor Toso <victortoso@redhat.com>
---
 src/channel-display.c | 73 ++++++++++++++++++++++-----------------------------
 1 file changed, 32 insertions(+), 41 deletions(-)

Patch hide | download patch | download mbox

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);
     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);
     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 */

Comments

On 03/13/2018 01:25 PM, Victor Toso wrote:
> From: Victor Toso <me@victortoso.com>
> 
> 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.
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)
3. O(1) is not exactly 1 and also hash consumes (a little bit)
    more memory.

Other than those and some comments below, it looks good.

> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>   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.


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

- Why make it different than the line above (using NULL instead of
   specifying the default functions)

Uri.

>       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 */
>
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@victortoso.com>
> > 
> > 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?

> 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@redhat.com>
> > ---
> >   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.

> >       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);

> 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 */
> > 
>
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@victortoso.com>
>>>
>>> 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@redhat.com>
>>> ---
>>>    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 */
>>>
>>
Hi,

On Tue, Apr 10, 2018 at 07:46:46PM +0300, Uri Lublin wrote:
> On 04/10/2018 02:58 PM, Victor Toso wrote:
> > On Mon, Apr 09, 2018 at 02:14:15PM +0300, Uri Lublin wrote:
> > > 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.

Indeed, I don't think we use that much.

If we would limit that in the protocol and state that ids are
always between 0-63 for instance (so we can drop ids < 0 and >
63), I'd be happy to stay with an array
 
> > > 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.

Ok

> 
> > 
> > > >        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).

Now that I re-read what I said, yeah, not sure what I meant
either. Sorry :)

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

NULL doesn't say much. With NULL one needs to check the
documentation and see what's going on (this is the first time I
saw using NULL for instance).

I'd change the surface's hash table new to use g_direct_* instead
but I don't mind to change my patch to NULL instead if are common
practice.

Best,
        toso
> 
> 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 */
> > > > 
> > > 
>
> 
> Hi,
> 
> On Tue, Apr 10, 2018 at 07:46:46PM +0300, Uri Lublin wrote:
> > On 04/10/2018 02:58 PM, Victor Toso wrote:
> > > On Mon, Apr 09, 2018 at 02:14:15PM +0300, Uri Lublin wrote:
> > > > 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.
> 
> Indeed, I don't think we use that much.
> 
> If we would limit that in the protocol and state that ids are
> always between 0-63 for instance (so we can drop ids < 0 and >
> 63), I'd be happy to stay with an array
> 

I would rather goes on this patch, maybe a 256 limit to start, just
to be sure.
Allowing 32 bit for ID potentially requires more than 1TB of RAM,
looks like not a good idea to allow all the range, there's no point,
the current server limit is 50 and I never manage to get further than
3/4 using the "all" option.

OT: Maybe would be great to add a @max specification in the protocol
file to allow to limit some constants (very OT).
 
> > > > 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.
> 
> Ok
> 
> > 
> > > 
> > > > >        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).
> 
> Now that I re-read what I said, yeah, not sure what I meant
> either. Sorry :)
> 
> > 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.
> 
> NULL doesn't say much. With NULL one needs to check the
> documentation and see what's going on (this is the first time I
> saw using NULL for instance).
> 
> I'd change the surface's hash table new to use g_direct_* instead
> but I don't mind to change my patch to NULL instead if are common
> practice.
> 
> Best,
>         toso
> > 
> > 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 */
> > > > > 
> > > > 
> >
Hi,

On Tue, Apr 17, 2018 at 06:54:27AM -0400, Frediano Ziglio wrote:
> > 
> > Hi,
> > 
> > On Tue, Apr 10, 2018 at 07:46:46PM +0300, Uri Lublin wrote:
> > > On 04/10/2018 02:58 PM, Victor Toso wrote:
> > > > On Mon, Apr 09, 2018 at 02:14:15PM +0300, Uri Lublin wrote:
> > > > > 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.
> > 
> > Indeed, I don't think we use that much.
> > 
> > If we would limit that in the protocol and state that ids are
> > always between 0-63 for instance (so we can drop ids < 0 and >
> > 63), I'd be happy to stay with an array
> > 
> 
> I would rather goes on this patch, maybe a 256 limit to start, just
> to be sure.
> Allowing 32 bit for ID potentially requires more than 1TB of RAM,
> looks like not a good idea to allow all the range, there's no point,
> the current server limit is 50 and I never manage to get further than
> 3/4 using the "all" option.
> 
> OT: Maybe would be great to add a @max specification in the protocol
> file to allow to limit some constants (very OT).

My point with hash table is basically due the fact that we don't
have that in the protocol...

I'll address some non-existing-in-the-protocol limitation while
keeping the array (instead of the hashtable), I hope to send that
later this week...

Cheers,

>  
> > > > > 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.
> > 
> > Ok
> > 
> > > 
> > > > 
> > > > > >        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).
> > 
> > Now that I re-read what I said, yeah, not sure what I meant
> > either. Sorry :)
> > 
> > > 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.
> > 
> > NULL doesn't say much. With NULL one needs to check the
> > documentation and see what's going on (this is the first time I
> > saw using NULL for instance).
> > 
> > I'd change the surface's hash table new to use g_direct_* instead
> > but I don't mind to change my patch to NULL instead if are common
> > practice.
> > 
> > Best,
> >         toso
> > > 
> > > 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 */
> > > > > > 
> > > > > 
> > >