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