[Spice-devel] [PATCH v4 07/17] sound: Convert SndChannelClient to GObject

[Jonathon Jongsma]

On Thu, 2016-12-01 at 17:21 +0100, Christophe Fergeau wrote:
> On Thu, Dec 01, 2016 at 11:24:29AM +0000, Frediano Ziglio wrote:
> > 
> > This patch is quite huge but have some benefits:
> > - reduce dependency (DummyChannel* and some RedChannelClient
> > internals);
> > - reuse RedChannelClient instead of reading from the RedsStream
> >   directly.
> > 
> > You can see that SndChannelClient has much less field
> > as the code to read/write from/to client is reused from
> > RedChannelClient instead of creating a fake RedChannelClient
> > just to make the system happy.
> > 
> > One of the different between the old sound code and all other
> > RedChannelClient objects was that the sound channel don't use
> > a queue while RedChannelClient use RedPipeItem object. This was
> > the main reason why RedChannelClient was not used. To implement
> > the old behaviour a "persistent_pipe_item" is used. This
> > RedPipeItem
> > will be queued to RedChannelClient (only one!) so signal code we
> > have data to send. The {playback,record}_channel_send_item will
> > then send the messages to the client using RedChannelClient
> > functions.
> > For this reason snd_reset_send_data is replaced by a call to
> > red_channel_client_init_send_data and snd_begin_send_message is
> > replaced by red_channel_client_begin_send_message.
> > 
> > Another difference is the AudioFrame allocation. Previously these
> > frames were allocated inside PlaybackChannelClient while now they
> > are allocated in a different structure. This allows the samples
> > to outlive the PlaybackChannelClient. This was possible even before
> > and the client was kept alive. However having RedChannelClient as
> > as base class can be an issue as this will cause the entire
> > RedChannel and RedChannelClient to stay alive. To avoid this
> > potential problem allocate the frames in a different structure
> > keeping a reference from PlaybackChannelClient. When the client
> > is freed the AudioFrameContainer is just unreferences.
> 
> No luck in adding a few more introductory patches to split this one
> more?
> 
> Christophe


I agree with Christophe. This is quite difficult to review properly.

Jonathon