Hi,
Thanks for taking a look!
On Mon, Mar 12, 2018 at 04:13:30PM +0100, Lukáš Hrázký wrote:
> On Wed, 2018-03-07 at 09:17 +0100, 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.
> >
> > Signed-off-by: Victor Toso <victortoso@xxxxxxxxxx>
> > ---
> > Changes from v2:
> > * using g_direct_hash/equal instead of the integer
> > conterpart.
> > * changed the signature from int to uint32_t for id in destroy_stream()
> >
> > 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);
> > 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;
> > }
>
> I haven't studied the code a whole lot, but this looks to be on
> the hot path of receiving every frame. The complexity is
> constant in both old and new version, but the hash function has
> a higher constant. I don't expect the constant to be
> significant in comparison to the context, but do you have any
> guesstimations and/or numbers? And do we want to rely on
> guesstimations or want numbers here?
I don't have numbers as I didn't consider it to be significant at
the time. My main concern was that it relies on something not
documented in the protocol itself (stream's ID being a small
number).
> Might be a non-issue, just pointing it out as probably the only
> real consideration with this patch?
Evaluating the difference between accessing an array an hash
table seems an overkill to me but, yeah, can be done...
I'm actually touching this part of the code thinking on
measurements but from the decoder's perspective (e.g from
SpiceFrame creation, decoding/rendering and deallocation).
Cheers,
toso
> > /* 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