>
> On Wed, 2017-08-30 at 03:31 -0400, Frediano Ziglio wrote:
> > > This is an alternate proposal to the patch Frediano sent recently
> > > which
> > > includes the data argument in the ClientCbs struct and removes the
> > > g_object_get|set_data() calls.
> > >
> > > This series also removes the GObject data stuff, but also does some
> > > deeper refactoring.
> > >
> >
> >
> > Honestly does not seem that great.
> >
> > It assumes CursorChannel and DisplayChannel will always have an
> > associated QXL device. There are different patches that remove this
> > needs for CursorChannel.
>
> Yes, it does assume that DisplayChannel and CursorChannel will have an
> associated QXL device, but that's not that surprising since these
> classes were essentially designed around QXL devices. I know your
> recent patch removed that requirement from CursorChannel, but I'm not
> sure that's the best design, to be honest. Although it works, it feels
> a bit like we're trying to squeeze this class into a shape that it
> wasn't designed to fill.
>
The fact that something was designed in a given way does not
mean that cannot be changed. Currently the only reason for
QXL in CursorChannel is to free resources, stuff that can be
done easily in red_put_cursor_cmd.
Your patches move part of the responsibility to handle the dispatching
from RedQxl to CursorChannel and DisplayChannel. It's true that there's
lot of information in red-qxl.h but 80% of these are stuff for RedQxl
and RedWorker. Maybe would be a good idea to separate these part
in a red-qxl-private.h header.
>
> >
> > The original design was that these callbacks could be overridden
> > to make any channel (type+id meaning) run in a separate thread.
> > Now we are forced to either use from the main thread or from a
> > separate one.
> >
> > For instance in my streaming patches I use the CursorChannel
> > from the main thread, now I would have to:
> > - write a fake QXL device to make CursorChannel happy;
> > - create a new Dispatcher to handle requests from CursorChannel;
> > - create another thread to handle synchronous calls (like
> > disconnect)
>
> Or we could modify this patch to work without QXL as well. Something
> like:
>
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index d3c1dcc71..363fb0d76 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -442,16 +442,20 @@ static void red_qxl_set_cursor_peer(RedChannel
> *channel, RedClient *client, Reds
> {
> RedWorkerMessageCursorConnect payload = {0,};
> QXLInstance *qxl =
> common_graphics_channel_get_qxl(COMMON_GRAPHICS_CHANNEL(channel));
> - Dispatcher *dispatcher = red_qxl_get_dispatcher(qxl);
> - spice_printerr("");
> - payload.client = client;
> - payload.stream = stream;
> - payload.migration = migration;
> - red_channel_capabilities_init(&payload.caps, caps);
> -
> - dispatcher_send_message(dispatcher,
> - RED_WORKER_MESSAGE_CURSOR_CONNECT,
> - &payload);
> + if (qxl) {
> + Dispatcher *dispatcher = red_qxl_get_dispatcher(qxl);
> + spice_printerr("");
> + payload.client = client;
> + payload.stream = stream;
> + payload.migration = migration;
> + red_channel_capabilities_init(&payload.caps, caps);
> +
> + dispatcher_send_message(dispatcher,
> + RED_WORKER_MESSAGE_CURSOR_CONNECT,
> + &payload);
> + } else {
> + cursor_channel_connect((CURSOR_CHANNEL(channel), client,
> stream, migration, caps);
> + }
> }
>
> static void red_qxl_disconnect_cursor_peer(RedChannel *channel,
> RedChannelClient *rcc)
>
>
> >
> > Also make harder to understand why these calls are separate
> > as they are just some different methods while before were
> > contained in a different structure.
>
> I don't really see this as a disadvantage. Why does it matter that they
> are visually separate?
>
Well, in a separate structure are a bit more separate.
I don't disagree to have these callback moved to virtual functions,
only that they should not deal with dispatching and threading
but just deal with channel stuff.
I agree that the current design require to tweak some internal
callback to make the behaviour change but spread responsibility
freely looks like a move back when there was a single file with
all RedWorker, CursorChannel and DisplayChannel together.
Looking at the GStreamer issue was thinking about the possibility
to move the entire RedWorker to a separate process. Not easy
to achieve but I think a design that would suite this would be
more sensible to have.
> >
> > I really Nack this approach.
> >
> > Frediano
> >
> > > Jonathon Jongsma (4):
> > > red-worker: don't pass 'dispatcher' as data for ClientCbs
> > > red-qxl: remove use of g_object_get_data()
> > > Move Cursor and Display client cbs to channel file
> > > Convert ClientCbs to channel virtual functions
> > >
> > > server/common-graphics-channel.c | 1 -
> > > server/cursor-channel.c | 63 +++++++++++++++++++
> > > server/display-channel.c | 69 +++++++++++++++++++++
> > > server/inputs-channel.c | 11 ++--
> > > server/main-channel-client.c | 8 ---
> > > server/main-channel-client.h | 1 -
> > > server/main-channel.c | 13 ++--
> > > server/red-channel.c | 54 +++++++---------
> > > server/red-channel.h | 25 ++++----
> > > server/red-qxl.c | 129
> > > +--------------------------------------
> > > server/red-worker.c | 11 +---
> > > server/red-worker.h | 4 +-
> > > server/smartcard.c | 6 +-
> > > server/sound.c | 21 +++----
> > > server/spicevmc.c | 7 +--
> > > 15 files changed, 196 insertions(+), 227 deletions(-)
> > >
>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel