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