[Spice-devel] [spice-gtk] RFC: Allow to limit the latency introduced by lip sync

[Frediano Ziglio]

> 
> Hi,
> 
> 
> On 08/15/2017 05:56 PM, Frediano Ziglio wrote:
> > This patch allows to reduce the time the client wait to display frames.
> > It can make the lip sync not working but it allows to see the
> > video latency introduced by this code.
> > This patch is meant to be used mainly for debugging:
> > - recently we are seeing some artifacts due to this delay and
> >    overlaying, removing the delay make easier to understand if the
> >    problem is related to this or other issues;
> > - we are testing guest encoding and we are experiencing a big lag.
> >    This avoid this part of the lag helping understand where the other
> >    part of the delay happens.
> 
> Actually IMHO this could be also good enough solution for the building
> latency issue
> for now.

The problem is that this patch hide the problem "under the carpet".
Latency should not build up. If the current code is causing that and
not a problem in the protocol or other external reasons (like network
issues). If this patch helps with latency on the video this seems to
indicate that the problem is not network (this patch don't improve
network in any way) or slow client (the frames are still decoded).

> I mentioned it also in Victor's mmtime-adjustment patch thread (at least
> I thought I
> did- just noticed i accidentally replayed to off-list :/), if we'll let
> the user the option
> to enable\disable this frames-scheduling-limitation (as presented here)
> on-the-fly
> (e.g as menu checkbox -as in victor's suggestion) he (the user) would be
> able
> to enable it in case he's feeling latency was built, and latency will
> disappear
> immediately (and audio will become out of sync). Adjusting mm_time alone
> doesn't seem to fix already built latency...
> 

What does it actually means out of sync? Is audio or video or both affected?

> Snir.
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> > This patch is not intended to be merged as is but to be used to fix
> > above problems or as a start for some better settings.
> > ---
> >   src/channel-display-gst.c   | 10 ++++++----
> >   src/channel-display-mjpeg.c | 24 +++++++++++++++++++-----
> >   src/channel-display-priv.h  |  3 +++
> >   3 files changed, 28 insertions(+), 9 deletions(-)
> >
> > Changes since last version:
> > - rebased on master.
> >
> > It seems is still useful, still not clear all sync process.
> >
> > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > index f978602..9ee6229 100644
> > --- a/src/channel-display-gst.c
> > +++ b/src/channel-display-gst.c
> > @@ -173,8 +173,10 @@ static void schedule_frame(SpiceGstDecoder *decoder)
> >               break;
> >           }
> >   
> > -        if (spice_mmtime_diff(now, gstframe->frame->mm_time) < 0) {
> > -            decoder->timer_id = g_timeout_add(gstframe->frame->mm_time -
> > now,
> > +        gint32 time_diff = spice_mmtime_diff(gstframe->frame->mm_time,
> > now);
> > +        time_diff = MIN(time_diff, max_frame_delay_ms);
> > +        if (time_diff > 0) {
> > +            decoder->timer_id = g_timeout_add(time_diff,
> >                                                 display_frame, decoder);
> >           } else if (g_queue_get_length(decoder->display_queue) == 1) {
> >               /* Still attempt to display the least out of date frame so
> >               the
> > @@ -182,8 +184,8 @@ static void schedule_frame(SpiceGstDecoder *decoder)
> >                */
> >               decoder->timer_id = g_timeout_add(0, display_frame, decoder);
> >           } else {
> > -            SPICE_DEBUG("%s: rendering too late by %u ms (ts: %u, mmtime:
> > %u), dropping",
> > -                        __FUNCTION__, now - gstframe->frame->mm_time,
> > +            SPICE_DEBUG("%s: rendering too late by %d ms (ts: %u, mmtime:
> > %u), dropping",
> > +                        __FUNCTION__, -time_diff,
> >                           gstframe->frame->mm_time, now);
> >               stream_dropped_frame_on_playback(decoder->base.stream);
> >               g_queue_pop_head(decoder->display_queue);
> > diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
> > index f0d55f6..cc9ddf1 100644
> > --- a/src/channel-display-mjpeg.c
> > +++ b/src/channel-display-mjpeg.c
> > @@ -189,6 +189,19 @@ static gboolean mjpeg_decoder_decode_frame(gpointer
> > video_decoder)
> >   
> >   /* ---------- VideoDecoder's queue scheduling ---------- */
> >   
> > +enum { MAX_DELAY_MS = 2000 };
> > +gint32 max_frame_delay_ms = MAX_DELAY_MS;
> > +
> > +SPICE_CONSTRUCTOR_FUNC(max_delay_init)
> > +{
> > +    const char *str_delay = g_getenv("SPICE_MAX_FRAME_DELAY");
> > +    if (str_delay) {
> > +        int delay = atoi(str_delay);
> > +        if (delay >= 0 && delay <= MAX_DELAY_MS)
> > +            max_frame_delay_ms = delay;
> > +    }
> > +}
> > +
> >   static void mjpeg_decoder_schedule(MJpegDecoder *decoder)
> >   {
> >       SPICE_DEBUG("%s", __FUNCTION__);
> > @@ -201,15 +214,16 @@ static void mjpeg_decoder_schedule(MJpegDecoder
> > *decoder)
> >       decoder->cur_frame = NULL;
> >       do {
> >           if (frame) {
> > -            if (spice_mmtime_diff(time, frame->mm_time) <= 0) {
> > -                guint32 d = frame->mm_time - time;
> > +            gint32 time_diff = spice_mmtime_diff(frame->mm_time, time);
> > +            time_diff = MIN(time_diff, max_frame_delay_ms);
> > +            if (time_diff >= 0) {
> >                   decoder->cur_frame = frame;
> > -                decoder->timer_id = g_timeout_add(d,
> > mjpeg_decoder_decode_frame, decoder);
> > +                decoder->timer_id = g_timeout_add(time_diff,
> > mjpeg_decoder_decode_frame, decoder);
> >                   break;
> >               }
> >   
> > -            SPICE_DEBUG("%s: rendering too late by %u ms (ts: %u, mmtime:
> > %u), dropping ",
> > -                        __FUNCTION__, time - frame->mm_time,
> > +            SPICE_DEBUG("%s: rendering too late by %d ms (ts: %u, mmtime:
> > %u), dropping ",
> > +                        __FUNCTION__, -time_diff,
> >                           frame->mm_time, time);
> >               stream_dropped_frame_on_playback(decoder->base.stream);
> >               free_spice_frame(frame);
> > diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
> > index 1389868..92bd85a 100644
> > --- a/src/channel-display-priv.h
> > +++ b/src/channel-display-priv.h
> > @@ -197,6 +197,9 @@ void stream_display_frame(display_stream *st,
> > SpiceFrame *frame, uint32_t width,
> >   gint64 get_stream_id_by_stream(SpiceChannel *channel, display_stream
> >   *st);
> >   
> >   
> > +/* maximum delay for frames */
> > +extern gint32 max_frame_delay_ms;
> > +
> >   G_END_DECLS
> >   
> >   #endif // CHANNEL_DISPLAY_PRIV_H_