[Spice-devel] [spice-gtk v1 06/11] channel-display: add spice_frame_new() helper

[Victor Toso]

On Tue, Apr 17, 2018 at 07:10:43AM -0400, Frediano Ziglio wrote:
> 
> > From: Victor Toso <me at victortoso.com>
> > 
> > As it makes easier to track what is allocated in its own function and
> > ultimately what needs to be freed later on.
> > 
> > Signed-off-by: Victor Toso <victortoso at redhat.com>
> > ---
> >  src/channel-display.c | 30 ++++++++++++++++++++++--------
> >  1 file changed, 22 insertions(+), 8 deletions(-)
> > 
> > diff --git a/src/channel-display.c b/src/channel-display.c
> > index a134516..2ee761e 100644
> > --- a/src/channel-display.c
> > +++ b/src/channel-display.c
> > @@ -1485,6 +1485,27 @@ static void
> > display_session_mm_time_reset_cb(SpiceSession *session, gpointer dat
> >  
> >  #define STREAM_PLAYBACK_SYNC_DROP_SEQ_LEN_LIMIT 5
> >  
> > +static SpiceFrame *spice_frame_new(display_stream *st,
> > +                                   SpiceMsgIn *in,
> > +                                   guint32 server_mmtime)
> > +{
> > +    SpiceFrame *frame;
> > +    guint8 *data_ptr;
> > +    const SpiceRect *dest_rect = stream_get_dest(st, in);
> > +    guint32 data_size = spice_msg_in_frame_data(in, &data_ptr);
> > +
> 
> why all these variables? I prefer the old syntax.

1) I was actually thinking in changing the helper functions but
   not super important and didn't do it.

2) Because this makes it easier to identify where data is coming
   from? (a _new() function which needs to call 2 helpers to get
   the right parameters for SpiceFrame initialization)

This is really person preference, not important. I'll change it.

> Also would be more a clean factorization.

Indeed

> 
> > +    frame = g_new(SpiceFrame, 1);
> > +    frame->mm_time = server_mmtime;
> > +    frame->dest = *dest_rect;
> > +    frame->data = data_ptr;
> > +    frame->size = data_size;
> > +    frame->data_opaque = in;
> > +    frame->ref_data = (void*)spice_msg_in_ref;
> > +    frame->unref_data = (void*)spice_msg_in_unref;
> > +    frame->free = (void*)g_free;
> > +    return frame;
> > +}
> > +
> >  G_GNUC_INTERNAL
> >  void spice_frame_free(SpiceFrame *frame)
> >  {
> > @@ -1551,14 +1572,7 @@ static void display_handle_stream_data(SpiceChannel
> > *channel, SpiceMsgIn *in)
> >       * decoding and best decide if/when to drop them when they are late,
> >       * taking into account the impact on later frames.
> >       */
> > -    frame = g_new(SpiceFrame, 1);
> > -    frame->mm_time = op->multi_media_time;
> > -    frame->dest = *stream_get_dest(st, in);
> > -    frame->size = spice_msg_in_frame_data(in, &frame->data);
> > -    frame->data_opaque = in;
> > -    frame->ref_data = (void*)spice_msg_in_ref;
> > -    frame->unref_data = (void*)spice_msg_in_unref;
> > -    frame->free = (void*)g_free;
> > +    frame = spice_frame_new(st, in, op->multi_media_time);
> >      if (!st->video_decoder->queue_frame(st->video_decoder, frame, latency))
> >      {
> >          destroy_stream(channel, op->id);
> >          report_invalid_stream(channel, op->id);
> 
> Frediano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180418/e5887f5f/attachment-0001.sig>