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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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?

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

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

Attachment: signature.asc
Description: PGP signature

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]