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 */
> > > > > >
> > > > >
> > >
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel