<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body text="#000000" bgcolor="#FFFFFF"> <div class="moz-cite-prefix">On 08/17/2017 03:35 PM, Frediano Ziglio wrote: </div> <blockquote type="cite" cite="mid:1830653756.1616197.1502973320742.JavaMail.zimbra@redhat.com"> <blockquote type="cite"> <pre wrap="">Hi, On 08/15/2017 05:56 PM, Frediano Ziglio wrote: </pre> <blockquote type="cite"> <pre wrap="">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. </pre> </blockquote> <pre wrap="">Actually IMHO this could be also good enough solution for the building latency issue for now. </pre> </blockquote> <pre wrap="">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). </pre> </blockquote> Hi, As I see it is that it does not really matter what is the reason for the latency (currently seems it could be load on client side, suspending and i' also not sure about the network) and i agree we have to avoid this latency build. My point is that in case the rectangle that is being streamed is not a pure video (i.e. the user need to interact with elements which are placed inside this rectangle (games, youtube videos, streaming the whole desktop...)) we could not allow to display frames in delay (maybe we could allow a minor delay) because it may cause actions that are made inside the streamed area to be displayed in delay. <blockquote type="cite" cite="mid:1830653756.1616197.1502973320742.JavaMail.zimbra@redhat.com"> <blockquote type="cite"> <pre wrap="">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... </pre> </blockquote> <pre wrap="">What does it actually means out of sync? Is audio or video or both affected?</pre> </blockquote> I meant audio will be out of sync with video.. As far i understand it currently audio frames are never dropped and displaying of video frames is synchronized with audio (using mm_time), when audio is delayed (which as you mentioned-it should not) video frames will try to catch up, means this timeout will be set and will cause video frames to also be delayed. If audio is delayed but we'll not let video frames to wait for it (as in rhis patch), audio that suits to current frame will be played after delay of "latency" milliseconds ("out of sync"). This was just a temporary simple suggestion, more comprehensive ideas mentioned before :P Snir <blockquote type="cite" cite="mid:1830653756.1616197.1502973320742.JavaMail.zimbra@redhat.com"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">Snir. </pre> <blockquote type="cite"> <pre wrap="">Signed-off-by: Frediano Ziglio <a class="moz-txt-link-rfc2396E" href="mailto:fziglio@redhat.com"><fziglio@redhat.com></a> --- 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_ </pre> </blockquote> </blockquote> </blockquote> </body> </html>