[Spice-devel] [PATCH client] channel-display: Fix wording of the deep_element_added_cb() documentation
Signed-off-by: Francois Gouget --- src/channel-display-gst.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c index 6fccf621..bd72625d 100644 --- a/src/channel-display-gst.c +++ b/src/channel-display-gst.c @@ -469,7 +469,7 @@ sink_event_probe(GstPad *pad, GstPadProbeInfo *info, gpointer data) return GST_PAD_PROBE_OK; } -/* This function is called to used to set a probe on the sink */ +/* This function is used to set a probe on the sink */ static void deep_element_added_cb(GstBin *pipeline, GstBin *bin, GstElement *element, SpiceGstDecoder *decoder) -- 2.20.1 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH client] build: Avoid line continuation for compatibility with older Meson
This fixes building spice-gtk on Debian 10.
Signed-off-by: Francois Gouget
---
See https://github.com/mesonbuild/meson/issues/4720
tests/meson.build | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/tests/meson.build b/tests/meson.build
index 57bd2cc5..bc5be5fd 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -23,9 +23,8 @@ endif
# create a static library from a shared one extracting all objects
# this allows to rewrite part of it if necessary for mocking
-test_lib = \
- static_library('test-lib',
- objects : spice_client_glib_lib.extract_all_objects())
+test_lib = static_library('test-lib',
+ objects :
spice_client_glib_lib.extract_all_objects())
foreach src : tests_sources
name = 'test-@0@'.format(src).split('.')[0]
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [protocol] protocol: Add some comments to vd_agentd.h
On Thu, 1 Aug 2019, Frediano Ziglio wrote:
[...]
> > @@ -254,7 +261,7 @@ typedef struct SPICE_ATTR_PACKED
> > VDAgentDeviceDisplayInfo
> > {
> > uint32_t monitor_id;
> > uint32_t device_display_id;
> > uint32_t device_address_len;
> > -uint8_t device_address[0]; // a zero-terminated string
> > +uint8_t device_address[0]; /* a zero-terminated string */
>
> Not really strong about it.
It's the only place where a C++-style comment is used which makes it
inconsistent with the style of the rest of the code.
> Can I send an update to this patch ?
Sure.
I'm fine with the proposed changes.
--
Francois Gouget
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk v3 1/6] channel-display: Minimize the stream lag by ignoring the server one
Actually I found something I'm a bit unhappy with in this patch. So one goal is to make sure all streams are in sync. This is why the mmtime conversion is performed globally: if two frames from different streams have the same mmtime, the resulting client-side timestamps should be identical too. That's only possible by applying the same offset to all streams, which is what spice_session_mmtime2client_time() does. But then each stream applies its own delay to make sure it has enough margin to ensure a smooth playback. Thus, since the delays are different the frame playback may not be in sync across streams. So I think the delay management should be handled globally too which I think should be quite doable. But I have been busy with other stuff so I did not get time to investigate and resubmit. -- Francois Gouget ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [protocol] protocol: Add some comments to vd_agentd.h
Signed-off-by: Francois Gouget
---
Just a minor patch partly inspired by a patch from Frediano Ziglio.
5975a98a94e0 at git://people.freedesktop.org/~fziglio/spice-protocol
The "client|server" comments bear verification: they're based on a
comment in do_client_monitors() in vdagentd.c that implies
VD_AGENT_MONITORS_CONFIG can be sent by either the client or server
which I'm not sure is true.
spice/vd_agent.h | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/spice/vd_agent.h b/spice/vd_agent.h
index 42ec77a..0662e44 100644
--- a/spice/vd_agent.h
+++ b/spice/vd_agent.h
@@ -62,15 +62,22 @@ typedef struct SPICE_ATTR_PACKED VDAgentMessage {
#define VD_AGENT_CLIPBOARD_MAX_SIZE_ENV "SPICE_CLIPBOARD_MAX_SIZE"
#endif
+
+/* vdagentd socket messages and types */
+
enum {
+/* server -> agent */
VD_AGENT_MOUSE_STATE = 1,
+/* client|server -> agent (acknowledged using VD_AGENT_REPLY) */
VD_AGENT_MONITORS_CONFIG,
+/* agent -> client|server */
VD_AGENT_REPLY,
/* Set clipboard data (both directions).
* Message comes with type and data.
* See VDAgentClipboard structure.
*/
VD_AGENT_CLIPBOARD,
+/* client -> agent */
VD_AGENT_DISPLAY_CONFIG,
VD_AGENT_ANNOUNCE_CAPABILITIES,
/* Asks to listen for clipboard changes (both directions).
@@ -254,7 +261,7 @@ typedef struct SPICE_ATTR_PACKED VDAgentDeviceDisplayInfo {
uint32_t monitor_id;
uint32_t device_display_id;
uint32_t device_address_len;
-uint8_t device_address[0]; // a zero-terminated string
+uint8_t device_address[0]; /* a zero-terminated string */
} VDAgentDeviceDisplayInfo;
@@ -270,6 +277,9 @@ typedef struct SPICE_ATTR_PACKED VDAgentGraphicsDeviceInfo {
VDAgentDeviceDisplayInfo display_info[0];
} VDAgentGraphicsDeviceInfo;
+
+/* Capabilities definitions */
+
enum {
VD_AGENT_CAP_MOUSE_STATE = 0,
VD_AGENT_CAP_MONITORS_CONFIG,
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [spice v4] utils: Make all the NSEC_PER_XXX constants 64 bit
Code dealing with nanosecond timestamps normally uses 64 bit integers and not long long ints. So this makes it easier to print the value of expressions using these constants. Signed-off-by: Francois Gouget --- v3: https://lists.freedesktop.org/archives/spice-devel/2019-June/049361.html v4: Following the discussion above, switch to using INT64_C(). This does not modify MSEC_PER_SEC. server/utils.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/server/utils.h b/server/utils.h index 54bc9d490..2cbd779c3 100644 --- a/server/utils.h +++ b/server/utils.h @@ -52,9 +52,9 @@ static inline int test_bit(int index, uint32_t val) typedef int64_t red_time_t; -#define NSEC_PER_SEC 10LL -#define NSEC_PER_MILLISEC 100 -#define NSEC_PER_MICROSEC 1000 +#define NSEC_PER_SEC INT64_C(10) +#define NSEC_PER_MILLISEC INT64_C(100) +#define NSEC_PER_MICROSEC INT64_C(1000) /* g_get_monotonic_time() does not have enough precision */ static inline red_time_t spice_get_monotonic_time_ns(void) -- 2.20.1 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice v3] utils: Remove the LL suffix from NSEC_PER_SEC
On Wed, 26 Jun 2019, Uri Lublin wrote: [...] > > I'd very much prefer the cast to be in the expression rather than hidden > > in some far away macro. > > Is that true even if the cast is needed in all the expressions > that use this constant ? Yes. That's far from being the case though. I count 8 casts out of 26 instances of NSEC_PER_SEC and its derivatives. Anyway, since there's opposition to the 'no cast' approach I'll send a patch based on INT64_C() instead. -- Francois Gouget ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [client v3 1/6] channel-display: Minimize the stream lag by ignoring the server one
The client is in a better position than the server to pick the minimum
lag needed to compensate for frame arrival time jitter and ensure
smooth video playback.
To do so:
- It ignores the lag specified by the server through the mmtime clock
adjustments (but this lag is still tracked for the stream reports).
- It performs its own frame mmtime conversion to the local monotonic
clock spice_session_mmtime2client_time() since the server-controlled
mmtime clock cannot be relied on. This conversion uses data from all
streams but is global so all streams have a common time reference.
- spice_session_mmtime2client_time() also handles the mmtime clock
changes caused by server migration.
- It tracks the margin between the converted time-to-display frame
timestamp and the current time and adds a delay to ensure this margin
remains positive.
- This delay introduces the video stream lag. It is continuously
adjusted to either be as low as possible, or match the audio
playback delay for proper lip sync.
- Delay adjustments are gradual, speeding up or slowing down video
playback until the average margin matches the target lag.
- Changes in the average margin are tracked (see margin_spread) to
avoid nudging the delay in response to minor random variations.
Signed-off-by: Francois Gouget
---
v3: Many changes based on previous feedback.
src/channel-display-gst.c | 20 ++--
src/channel-display-mjpeg.c | 14 +--
src/channel-display-priv.h | 24 -
src/channel-display.c | 180
src/spice-session-priv.h| 1 +
src/spice-session.c | 46 +
6 files changed, 247 insertions(+), 38 deletions(-)
diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 6fccf621..7ad6009b 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -50,7 +50,7 @@ typedef struct SpiceGstDecoder {
/* -- Decoding and display queues -- */
-uint32_t last_mm_time;
+uint32_t last_frame_time;
GMutex queues_mutex;
GQueue *decoding_queue;
@@ -297,8 +297,8 @@ static void schedule_frame(SpiceGstDecoder *decoder)
break;
}
-if (spice_mmtime_diff(gstframe->encoded_frame->mm_time, now) >= 0) {
-decoder->timer_id = g_timeout_add(gstframe->encoded_frame->mm_time
- now,
+if (spice_mmtime_diff(gstframe->encoded_frame->time, now) >= 0) {
+decoder->timer_id = g_timeout_add(gstframe->encoded_frame->time -
now,
display_frame, decoder);
} else if (decoder->display_frame && !decoder->pending_samples) {
/* Still attempt to display the least out of date frame so the
@@ -307,8 +307,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->encoded_frame->mm_time,
-gstframe->encoded_frame->mm_time, now);
+__FUNCTION__, now - gstframe->encoded_frame->time,
+gstframe->encoded_frame->time, now);
stream_dropped_frame_on_playback(decoder->base.stream);
decoder->display_frame = NULL;
free_gst_frame(gstframe);
@@ -449,7 +449,7 @@ sink_event_probe(GstPad *pad, GstPadProbeInfo *info,
gpointer data)
* or more frame intervals.
*/
record(frames_stats,
- "frame mm_time %u size %u creation time %" PRId64
+ "frame time %u size %u creation time %" PRId64
" decoded time %" PRId64 " queue %u before %u",
frame->mm_time, frame->size, frame->creation_time, duration,
decoder->decoding_queue->length, gstframe->queue_len);
@@ -689,13 +689,13 @@ static gboolean
spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
return TRUE;
}
-if (spice_mmtime_diff(frame->mm_time, decoder->last_mm_time) < 0) {
+if (spice_mmtime_diff(frame->time, decoder->last_frame_time) < 0) {
SPICE_DEBUG("new-frame-time < last-frame-time (%u < %u):"
" resetting stream",
-frame->mm_time, decoder->last_mm_time);
+frame->time, decoder->last_frame_time);
/* Let GStreamer deal with the frame anyway */
}
-decoder->last_mm_time = frame->mm_time;
+decoder->last_frame_time = frame->time;
if (margin < 0 &&
decoder->base.codec_type == SPICE_VIDEO_CODEC_TYPE_MJPEG) {
@@ -778,7 +778,7 @@ V
[Spice-devel] [client v3 4/6] channel-display: Remove playback_sync_drops_seq_len
display_handle_stream_data() now has its own mechanism to avoid
dropping frames which does not depend on the playback latency.
Signed-off-by: Francois Gouget
---
src/channel-display-priv.h | 2 --
src/channel-display.c | 8
src/channel-playback-priv.h | 1 -
src/channel-playback.c | 9 -
src/spice-session-priv.h| 1 -
src/spice-session.c | 15 ---
6 files changed, 36 deletions(-)
diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
index 6628951d..a055b203 100644
--- a/src/channel-display-priv.h
+++ b/src/channel-display-priv.h
@@ -151,8 +151,6 @@ struct display_stream {
GArray *drops_seqs_stats_arr;
uint32_t num_drops_seqs;
-uint32_t playback_sync_drops_seq_len;
-
/* playback quality report to server */
gboolean report_is_active;
uint32_t report_id;
diff --git a/src/channel-display.c b/src/channel-display.c
index 9ef2054e..001fbc5c 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -1484,8 +1484,6 @@ static void
display_update_stream_report(SpiceDisplayChannel *channel, uint32_t
}
}
-#define STREAM_PLAYBACK_SYNC_DROP_SEQ_LEN_LIMIT 5
-
static void display_stream_stats_debug(display_stream *st)
{
guint64 drops_duration_total = 0;
@@ -1567,7 +1565,6 @@ static void display_stream_stats_save(display_stream *st,
st->cur_drops_seq_stats.start_mm_time = frame_mmtime;
}
st->cur_drops_seq_stats.len++;
-st->playback_sync_drops_seq_len++;
return;
}
@@ -1579,7 +1576,6 @@ static void display_stream_stats_save(display_stream *st,
memset(>cur_drops_seq_stats, 0, sizeof(st->cur_drops_seq_stats));
st->num_drops_seqs++;
}
-st->playback_sync_drops_seq_len = 0;
}
static SpiceFrame *spice_frame_new(display_stream *st,
@@ -1802,10 +1798,6 @@ static void display_handle_stream_data(SpiceChannel
*channel, SpiceMsgIn *in)
if (c->enable_adaptive_streaming) {
display_update_stream_report(SPICE_DISPLAY_CHANNEL(channel), op->id,
op->multi_media_time, mmtime_margin);
-if (st->playback_sync_drops_seq_len >=
STREAM_PLAYBACK_SYNC_DROP_SEQ_LEN_LIMIT) {
-
spice_session_sync_playback_latency(spice_channel_get_session(channel));
-st->playback_sync_drops_seq_len = 0;
-}
}
}
diff --git a/src/channel-playback-priv.h b/src/channel-playback-priv.h
index aa33d2c4..dc89e2d8 100644
--- a/src/channel-playback-priv.h
+++ b/src/channel-playback-priv.h
@@ -20,5 +20,4 @@
gboolean spice_playback_channel_is_active(SpicePlaybackChannel *channel);
guint32 spice_playback_channel_get_latency(SpicePlaybackChannel *channel);
-void spice_playback_channel_sync_latency(SpicePlaybackChannel *channel);
#endif
diff --git a/src/channel-playback.c b/src/channel-playback.c
index 656a4037..0e439eff 100644
--- a/src/channel-playback.c
+++ b/src/channel-playback.c
@@ -495,12 +495,3 @@ guint32
spice_playback_channel_get_latency(SpicePlaybackChannel *channel)
}
return channel->priv->latency;
}
-
-G_GNUC_INTERNAL
-void spice_playback_channel_sync_latency(SpicePlaybackChannel *channel)
-{
-g_return_if_fail(SPICE_IS_PLAYBACK_CHANNEL(channel));
-g_return_if_fail(channel->priv->is_active);
-SPICE_DEBUG("%s: notify latency update %u", __FUNCTION__,
channel->priv->min_latency);
-g_coroutine_object_notify(G_OBJECT(SPICE_CHANNEL(channel)), "min-latency");
-}
diff --git a/src/spice-session-priv.h b/src/spice-session-priv.h
index d88276f1..d0d7be8e 100644
--- a/src/spice-session-priv.h
+++ b/src/spice-session-priv.h
@@ -86,7 +86,6 @@ void spice_session_set_uuid(SpiceSession *session, guint8
uuid[16]);
void spice_session_set_name(SpiceSession *session, const gchar *name);
gboolean spice_session_is_playback_active(SpiceSession *session);
guint32 spice_session_get_playback_latency(SpiceSession *session);
-void spice_session_sync_playback_latency(SpiceSession *session);
gboolean spice_session_get_audio_enabled(SpiceSession *session);
gboolean spice_session_get_smartcard_enabled(SpiceSession *session);
gboolean spice_session_get_usbredir_enabled(SpiceSession *session);
diff --git a/src/spice-session.c b/src/spice-session.c
index e6a96133..04a2da96 100644
--- a/src/spice-session.c
+++ b/src/spice-session.c
@@ -2660,21 +2660,6 @@ void spice_session_set_name(SpiceSession *session, const
gchar *name)
g_coroutine_object_notify(G_OBJECT(session), "name");
}
-G_GNUC_INTERNAL
-void spice_session_sync_playback_latency(SpiceSession *session)
-{
-g_return_if_fail(SPICE_IS_SESSION(session));
-
-SpiceSessionPrivate *s = session->priv;
-
-if (s->playback_channel &&
-spice_playback_channel_is_active(s->playback_channel)) {
-spice_playback_channel_sync_latency(s->
[Spice-devel] [client v3 3/6] channel-display: No need to rechedule on mmtime offset changes
The frame display time is no longer based on the mmtime clock and thus
is not impacted by mmtime offset changes.
Signed-off-by: Francois Gouget
---
src/channel-display-gst.c | 21 -
src/channel-display-mjpeg.c | 13
src/channel-display-priv.h | 3 --
src/channel-display.c | 59 -
4 files changed, 96 deletions(-)
diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 7ad6009b..11d78afc 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -583,26 +583,6 @@ static gboolean create_pipeline(SpiceGstDecoder *decoder)
/* -- VideoDecoder's public API -- */
-static void spice_gst_decoder_reschedule(VideoDecoder *video_decoder)
-{
-SpiceGstDecoder *decoder = (SpiceGstDecoder*)video_decoder;
-
-if (!decoder->appsink) {
-return;
-}
-guint timer_id;
-
-g_mutex_lock(>queues_mutex);
-timer_id = decoder->timer_id;
-decoder->timer_id = 0;
-g_mutex_unlock(>queues_mutex);
-
-if (timer_id != 0) {
-g_source_remove(timer_id);
-}
-schedule_frame(decoder);
-}
-
/* main context */
static void spice_gst_decoder_destroy(VideoDecoder *video_decoder)
{
@@ -774,7 +754,6 @@ VideoDecoder* create_gstreamer_decoder(int codec_type,
display_stream *stream)
if (gstvideo_init()) {
decoder = g_new0(SpiceGstDecoder, 1);
decoder->base.destroy = spice_gst_decoder_destroy;
-decoder->base.reschedule = spice_gst_decoder_reschedule;
decoder->base.queue_frame = spice_gst_decoder_queue_frame;
decoder->base.codec_type = codec_type;
decoder->base.stream = stream;
diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
index d3ab77c8..b058424e 100644
--- a/src/channel-display-mjpeg.c
+++ b/src/channel-display-mjpeg.c
@@ -250,18 +250,6 @@ static gboolean mjpeg_decoder_queue_frame(VideoDecoder
*video_decoder,
return TRUE;
}
-static void mjpeg_decoder_reschedule(VideoDecoder *video_decoder)
-{
-MJpegDecoder *decoder = (MJpegDecoder*)video_decoder;
-
-SPICE_DEBUG("%s", __FUNCTION__);
-if (decoder->timer_id != 0) {
-g_source_remove(decoder->timer_id);
-decoder->timer_id = 0;
-}
-mjpeg_decoder_schedule(decoder);
-}
-
static void mjpeg_decoder_destroy(VideoDecoder* video_decoder)
{
MJpegDecoder *decoder = (MJpegDecoder*)video_decoder;
@@ -281,7 +269,6 @@ VideoDecoder* create_mjpeg_decoder(int codec_type,
display_stream *stream)
MJpegDecoder *decoder = g_new0(MJpegDecoder, 1);
decoder->base.destroy = mjpeg_decoder_destroy;
-decoder->base.reschedule = mjpeg_decoder_reschedule;
decoder->base.queue_frame = mjpeg_decoder_queue_frame;
decoder->base.codec_type = codec_type;
decoder->base.stream = stream;
diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
index 2ca43e25..6628951d 100644
--- a/src/channel-display-priv.h
+++ b/src/channel-display-priv.h
@@ -59,9 +59,6 @@ struct VideoDecoder {
/* Releases the video decoder's resources */
void (*destroy)(VideoDecoder *decoder);
-/* Notifies the decoder that the mm-time clock changed. */
-void (*reschedule)(VideoDecoder *decoder);
-
/* Takes ownership of the specified frame, decompresses it,
* and displays it at the right time.
*
diff --git a/src/channel-display.c b/src/channel-display.c
index 21d5e7c7..9ef2054e 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -107,7 +107,6 @@ static void spice_display_channel_reset(SpiceChannel
*channel, gboolean migratin
static void spice_display_channel_set_capabilities(SpiceChannel *channel);
static void destroy_canvas(display_surface *surface);
static void display_stream_destroy(gpointer st);
-static void display_session_mm_time_reset_cb(SpiceSession *session, gpointer
data);
static SpiceGlScanout* spice_gl_scanout_copy(const SpiceGlScanout *scanout);
G_DEFINE_BOXED_TYPE(SpiceGlScanout, spice_gl_scanout,
@@ -189,9 +188,6 @@ static void spice_display_channel_constructed(GObject
*object)
g_return_if_fail(c->palettes != NULL);
c->monitors = g_array_new(FALSE, TRUE, sizeof(SpiceDisplayMonitorConfig));
-spice_g_signal_connect_object(s, "mm-time-reset",
- G_CALLBACK(display_session_mm_time_reset_cb),
- SPICE_CHANNEL(object), 0);
spice_display_channel_set_capabilities(SPICE_CHANNEL(object));
@@ -1488,61 +1484,6 @@ static void
display_update_stream_report(SpiceDisplayChannel *channel, uint32_t
}
}
-/*
- * Migration can occur between 2 spice-servers with different mm-times.
- * Then, the following cases can happen after migration completes:
- * (We refer to src/dst-time as the mm-times on the src/dst servers):
- *
- * (case 1) Frames with time ~= dst-time arrive to the client before th
[Spice-devel] [client v3 2/6] playback: Use the audio timestamps for the global mmtime conversion
More data helps improve the accuracy of the estimation of the true clock
offset and minimum network latency.
Signed-off-by: Francois Gouget
---
src/channel-playback.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/src/channel-playback.c b/src/channel-playback.c
index a00706fe..656a4037 100644
--- a/src/channel-playback.c
+++ b/src/channel-playback.c
@@ -302,14 +302,16 @@ static void playback_handle_data(SpiceChannel *channel,
SpiceMsgIn *in)
{
SpicePlaybackChannelPrivate *c = SPICE_PLAYBACK_CHANNEL(channel)->priv;
SpiceMsgPlaybackPacket *packet = spice_msg_in_parsed(in);
+SpiceSession *session;
#ifdef DEBUG
CHANNEL_DEBUG(channel, "%s: time %u data %p size %d", __FUNCTION__,
packet->time, packet->data, packet->data_size);
#endif
-if (spice_mmtime_diff(c->last_time, packet->time) > 0)
-g_warn_if_reached();
+/* This also updates the time offset */
+session = spice_channel_get_session(channel);
+spice_session_mmtime2client_time(session, packet->time);
c->last_time = packet->time;
@@ -361,11 +363,16 @@ static void playback_handle_start(SpiceChannel *channel,
SpiceMsgIn *in)
{
SpicePlaybackChannelPrivate *c = SPICE_PLAYBACK_CHANNEL(channel)->priv;
SpiceMsgPlaybackStart *start = spice_msg_in_parsed(in);
+SpiceSession *session;
CHANNEL_DEBUG(channel, "%s: fmt %u channels %u freq %u time %u mode %s",
__FUNCTION__,
start->format, start->channels, start->frequency,
start->time,
spice_audio_data_mode_to_string(c->mode));
+/* This also updates the time offset */
+session = spice_channel_get_session(channel);
+spice_session_mmtime2client_time(session, start->time);
+
c->frame_count = 0;
c->last_time = start->time;
c->is_active = TRUE;
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [client v3 6/6] mjpeg: Take the decoding time into account to display frames
Signed-off-by: Francois Gouget
---
src/channel-display-mjpeg.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
index 20e10d9b..764f0611 100644
--- a/src/channel-display-mjpeg.c
+++ b/src/channel-display-mjpeg.c
@@ -189,6 +189,9 @@ static void mjpeg_decoder_schedule(MJpegDecoder *decoder)
if (frame) {
if (spice_mmtime_diff(time, frame->time) <= 0) {
guint32 d = frame->time - time;
+if (d > decoder->base.decoding_time) {
+d -= decoder->base.decoding_time;
+}
decoder->cur_frame = frame;
decoder->timer_id = g_timeout_add(d,
mjpeg_decoder_decode_frame, decoder);
break;
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [client v3 5/6] spice-session: Keep track of the global streams lag
Each video and audio stream has its own lag: for video streams it is
the decoding time and for audio ones buffering by the audio subsystem.
The only way to keep them all in sync is to synchronize to the most
laggy stream.
Signed-off-by: Francois Gouget
---
src/channel-display-gst.c | 9
src/channel-display-mjpeg.c | 6 ++
src/channel-display-priv.h | 4
src/channel-display.c | 33 +++--
src/channel-display.h | 2 ++
src/channel-playback-priv.h | 3 ++-
src/channel-playback.c | 22 ---
src/spice-session-priv.h| 6 +-
src/spice-session.c | 42 +++--
9 files changed, 118 insertions(+), 9 deletions(-)
diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 11d78afc..e19cfa73 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -454,6 +454,15 @@ sink_event_probe(GstPad *pad, GstPadProbeInfo *info,
gpointer data)
frame->mm_time, frame->size, frame->creation_time, duration,
decoder->decoding_queue->length, gstframe->queue_len);
+/* And the average decoding time. Only take into account frames
+ * that were not delayed (see above), or that prove the current
+ * average is clearly overestimated.
+ */
+if (gstframe->queue_len < MAX_DECODED_FRAMES ||
+duration / 1000 < decoder->base.decoding_time) {
+decoder->base.decoding_time = (decoder->base.decoding_time *
15 + duration / 1000) / 16;
+}
+
if (!decoder->appsink) {
/* The sink will display the frame directly so this
* SpiceGstFrame and those of any dropped frame are no longer
diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
index b058424e..20e10d9b 100644
--- a/src/channel-display-mjpeg.c
+++ b/src/channel-display-mjpeg.c
@@ -86,6 +86,7 @@ static gboolean mjpeg_decoder_decode_frame(gpointer
video_decoder)
JDIMENSION width, height;
uint8_t *dest;
uint8_t *lines[4];
+int64_t start = g_get_monotonic_time();
jpeg_read_header(>mjpeg_cinfo, 1);
width = decoder->mjpeg_cinfo.image_width;
@@ -156,6 +157,11 @@ static gboolean mjpeg_decoder_decode_frame(gpointer
video_decoder)
}
jpeg_finish_decompress(>mjpeg_cinfo);
+uint32_t duration = (g_get_monotonic_time() - start) / 1000;
+decoder->base.decoding_time = decoder->base.decoding_time ?
+(decoder->base.decoding_time * 15 + duration) / 16 :
+duration;
+
/* Display the frame and dispose of it */
stream_display_frame(decoder->base.stream, decoder->cur_frame,
width, height, SPICE_UNKNOWN_STRIDE,
decoder->out_frame);
diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
index a055b203..addaf69e 100644
--- a/src/channel-display-priv.h
+++ b/src/channel-display-priv.h
@@ -76,6 +76,9 @@ struct VideoDecoder {
/* The associated display stream. */
display_stream *stream;
+
+/* The average time it takes to decode a frame. */
+float decoding_time;
};
@@ -122,6 +125,7 @@ struct display_stream {
int have_region;
VideoDecoder*video_decoder;
+guint decoder_lag;
SpiceChannel*channel;
diff --git a/src/channel-display.c b/src/channel-display.c
index 001fbc5c..c4ea6312 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -1411,6 +1411,27 @@ void stream_display_frame(display_stream *st, SpiceFrame
*frame,
}
}
+G_GNUC_INTERNAL
+guint32 spice_display_channel_get_lag(SpiceDisplayChannel *channel)
+{
+SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
+int i;
+guint lag;
+
+lag = 0;
+if (c->streams) {
+for (i = 0; i < c->nstreams; i++) {
+display_stream *st = c->streams[i];
+if (st != NULL && st->video_decoder) {
+st->decoder_lag = st->video_decoder->decoding_time;
+lag = MAX(lag, st->decoder_lag);
+}
+}
+}
+
+return lag;
+}
+
G_GNUC_INTERNAL
gboolean hand_pipeline_to_widget(display_stream *st, GstPipeline *pipeline)
{
@@ -1467,7 +1488,7 @@ static void
display_update_stream_report(SpiceDisplayChannel *channel, uint32_t
report.num_drops = st-> report_num_drops;
report.last_frame_delay = margin;
if (spice_session_is_playback_active(session)) {
-report.audio_delay = spice_session_get_playback_latency(session);
+report.audio_delay = spice_session_get_playback_lag(session);
} else {
report.audio_delay = UINT_MAX;
}
@@ -1686,7 +1707,7 @@ static void display_handle_stream_data(SpiceChannel
Re: [Spice-devel] [client v2 07/12] channel-display: Minimize the stream lag by ignoring the server one
et_lag,
> > + st->min_margin));
> > +}
> > +if (nudge) {
> > +st->delay += nudge;
> > +frame_time += nudge;
> > +
> > +/* The delay nudge also impacts the margin history */
> > +st->min_margin_next += nudge;
> > +st->min_margin += nudge;
> > +st->avg_margin += nudge;
> > +st->min_avg_margin += nudge;
> > +st->max_avg_margin += nudge;
> > +}
> > }
> > }
>
> I will have to read all these formulaes again but looks like you have
> part of the code which is doing some smooth changes but at some intervals
> you do some bigger adjustments.
> It looks quite complicated and it seems hard to see if it's good or not.
> Not sure if it would be possible to encapsulate all that complexity
> and have some tests.
> What the cases you have in mind to resolv? Just ignoring peaks?
The goal is to have a solid minimum margin value, not one that's based
on the past couple of milliseconds because we just reset to a new
period.
So min_margin is the value we got from the last period. And
min_margin_next is the one we compute for use next. But when we adjust
the delay this impacts all margin values, current and next, hence the
adjustments above.
The same goes for min_avg_margin, etc.
> > @@ -2385,6 +2391,46 @@ int spice_session_get_connection_id(SpiceSession
> > *session)
> > return s->connection_id;
> > }
> >
> > +G_GNUC_INTERNAL
> > +guint32 spice_session_get_client_time(SpiceSession *session, guint32
> > mmtime)
> > +{
>
> I agree with Snir that a "get" function doing so much stuff is confusing,
> maybe a "spice_session_adjust_to_client_time" or a
> "spice_session_mmtime2client_time".
I can go with spice_session_mmtime2client_time().
> > +g_return_val_if_fail(SPICE_IS_SESSION(session),
> > g_get_monotonic_time());
> > +
>
> I suppose here should be "g_get_monotonic_time() / 1000", that is next "now"
> value.
Right.
>
> > +SpiceSessionPrivate *s = session->priv;
> > +
> > +guint32 now = g_get_monotonic_time() / 1000;
> > +gint64 new_offset = now - mmtime;
>
> This should be an unsigned 32 bit, all arithmetic are modulo 2**32.
Right, that should work. Done.
> > +if (new_offset < s->client_time_offset - 1000 ||
> > +new_offset > s->client_time_offset + 1000) {
> > +/* The change in offset was likely caused by a server migration.
> > + * Reset the time offset.
> > + */
>
> Was this tested? Is there no other event to specifically detect migration?
I added code to simulate a migration based on the
display_session_mm_time_reset_cb() comment in channel-display.c. But I
have no way of actually testing it.
> We (the client) needs to reconnect during migration so surely there's
> another event during this change.
The problem as I understand it is that the playback channel may be
migrated and start receiving new mmtime timestamps before the old video
stream has been shut down. This can result in a discrepency in the
time base and if there is a clean way to detect and deal with it that
comment does not say how.
> > +s->client_time_offset = new_offset;
> > +return now;
> > +}
> > +if (new_offset < s->client_time_offset) {
> > +/* The previous message we used to compute the offset was probably
> > + * delayed resulting in a too large offset. Eventually the offset
> > + * should settle to the true clock offset plus the network latency,
> > + * excluding the network jitter.
> > + */
>
> A bit OT: maybe would be great to have the "client time" when we start
> receiving
> the message from the server instead of taking the time after the full message
> arrives. Should be more consistent with server mmtime changes (server
> timestamp
> messages before sending).
I don't think that's possible without having lower layers know way too
much for their own good.
--
Francois Gouget
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice v3] utils: Remove the LL suffix from NSEC_PER_SEC
On Tue, 25 Jun 2019, Frediano Ziglio wrote:
[...]
> > uint64_t foo = 1234;
> > spice_debug("foo=%" PRId64, foo / NSEC_PER_SEC);
[...]
> If you assume long long == 64 bit should not be a big problem
> although you can still have the warning.
Not a warning. A compilation error:
CC gstreamer-encoder.lo
In file included from red-common.h:28,
from gstreamer-encoder.c:30:
gstreamer-encoder.c: In function 'get_min_playback_delay':
../subprojects/spice-common/common/log.h:67:62: error: format '%ld'
expects argument of type 'long int', but argument 5 has type 'long long
unsigned int' [-Werror=format=]
spice_log(G_LOG_LEVEL_DEBUG, SPICE_STRLOC, __FUNCTION__, "" format, ##
__VA_ARGS__); \
^~
gstreamer-encoder.c:606:5: note: in expansion of macro 'spice_debug'
spice_debug("foo=%" PRId64, foo / NSEC_PER_SEC);
^~~
In file included from gstreamer-encoder.c:21:
/usr/include/inttypes.h:57:34: note: format string is defined here
# define PRId64 __PRI64_PREFIX "d"
cc1: all warnings being treated as errors
> > The principle of least surprise would dictate that for maintainable
> > code:
>
> Surprise/astonishment are really subject to human opinion. They are based
> on habits and expectations. Saying that "principle of least surprise"
> dictates something is pretending all people have the same habits/expectations.
And saying that means you consider the principle of least surprise to be
completely useless.
> > 1. A set of related constants should all have the same type.
> >
>
> It sounds reasonable, all these constants are defined really closed in
> the source and express the same precision (nanoseconds). I don't expect
> MSEC_PER_SEC to have the same precision/type.
I can make an exception for MSEC_PER_SEC.
> > 2. If at all possible constants should not force their type on that
> > of expressions.
> >
>
> C++ added an extension to make enumeration typed, C has some extension
> to achieve that and compilers have options for this so it seems in
> different cases it's wanted.
I'd very much prefer the cast to be in the expression rather than hidden
in some far away macro.
For instance:
int frames_count;
...
if (duration > NSEC_PER_SEC * drawable->frames_count / MAX_FPS) {
Anyone checking this calculation would think that there is a risk of
overflow. And it's only when tracing NSEC_PER_SEC to utils.h that they
would discover that NSEC_PER_SEC forces 64 bit calculation.
At least this form is clear right away:
if (duration > ((uint64_t)NSEC_PER_SEC) * drawable->frames_count / MAX_FPS)
{
> > As the code currently stands that's only true for expressions that use
> > NSEC_PER_SEC. All expressions that use the other time constants have the
> > type of the variables used in the expression which means one should use
> > either %d/%u or %ld/%lu.
> >
>
> Most are 64 bit so all %d, %u, %ld and %lu are not good.
You mean in case one compiles to 32 bit?
Using PRId64 and PRIu64 would still not work with NSEC_PER_SEC which is
the issue here.
> > > > traces require lots of guessing and recompilations.
> > > >
> > >
> > > That's why we use -Wformat so compiler will tell you which ones are wrong.
> > > I don't see why you need to guess and recompile,
> > > beside I suppose the first time you are writing the code.
> >
> >
> > Precisely. And since it's used in one-off debugging expressions it keeps
> > coming up. Plus half the spice_debug() arguments are hidden so that when
> > the compiler says there's a problem with argument 5 it's anybody's guess
> > as to where the problem actually lies.
>
> But most of types are 64 bit so you have the same issue too, isn't it?
No. If I want to trace a 64 bit variable I just use %ld/%lu and that's
it. No need to look at utils.h to refresh my memory on whether a
specific macro is a long long unlike all the other similarly named ones.
> And this patch is not fixing these macros.
It removes the LL suffix from NSEC_PER_SEC, thus making it consistent
with all other NSEC_PER_XXX macros and even MSEC_PER_SEC. So yes it
does. Or where you speaking of some other macros?
After this patch the only constants that keep a 64 bit type are those
that don't fit in 32 bit, and at least those are longs, not long longs.
--
Francois Gouget
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice v3] utils: Remove the LL suffix from NSEC_PER_SEC
Uri Lublin wrote:
> When the variable is 64 bit, you can use a 64bit macro for printing,
> like PRId64.
Wrong. Spice will fail to produce a 64 bit library if you add this
anywhere:
uint64_t foo = 1234;
spice_debug("foo=%" PRId64, foo / NSEC_PER_SEC);
Knowing that the variable is 64 bit is not sufficient to know what
format to use. And once a developer has figured out that the time
constants are long longs he'll get an error with this:
uint64_t bar = 1234;
spice_debug("bar=%lld", bar / NSEC_PER_MICROSEC);
These are traps. It's on the same level as:
#define ONE 2
Useless inconsistencies and misdirections.
The principle of least surprise would dictate that for maintainable
code:
1. A set of related constants should all have the same type.
2. If at all possible constants should not force their type on that
of expressions.
So the best solution is to not have any suffix on any of the time
constants since none of them needs it. That's what I have proposed.
The next best solution is to have all of them be int64_t since that's
the type of the variables they are usually (but not necessarily) used
with. That includes NSEC_PER_MICROSEC and MSEC_PER_SEC. This violates
the second point though.
In a distant third place is adding the LL suffix to all constants or
casting them all to some long long type. It's not as good as the
solution above since we essentially don't use long long variables
anywhere (I count a total of three long long variables in some dark
corner of spice-common) and thus will catch developers off-guard when
they try to trace their values.
And the worst option is the current situation where half the constants
force an unused type on all expressions where they are used in and half
don't.
On Tue, 25 Jun 2019, Frediano Ziglio wrote:
[...]
> > Whenever NSEC_PER_MILLISEC or NSEC_PER_SEC are used in a spice_debug()
> > parameter one cannot use %u or %lu as the format. But not so for
>
> Being signed you would use %lld or similars.
As the code currently stands that's only true for expressions that use
NSEC_PER_SEC. All expressions that use the other time constants have the
type of the variables used in the expression which means one should use
either %d/%u or %ld/%lu.
> > NSEC_PER_MICROSEC or MSEC_PER_SEC. This is inconsistent so that timing
>
> No, you cannot use long or not long for other constants too, they are
> different for 32-bit so with both you cannot mix.
???
> > traces require lots of guessing and recompilations.
> >
>
> That's why we use -Wformat so compiler will tell you which ones are wrong.
> I don't see why you need to guess and recompile,
> beside I suppose the first time you are writing the code.
Precisely. And since it's used in one-off debugging expressions it keeps
coming up. Plus half the spice_debug() arguments are hidden so that when
the compiler says there's a problem with argument 5 it's anybody's guess
as to where the problem actually lies.
Sure all is well if you memorize that NSEC_PER_SEC and NSEC_PER_MILLISEC
are long longs, but NSEC_PER_MILLISEC is not anymore, and neither are
any of the other time constants and that spice_debug() has precisely x
hidden arguments which you must substract from compiler error messages.
But that's just setting up traps for developers who I argue have much
better things to memorize and think about.
> And if you need to guess it means you don't know the types you are
> using and so you are not sure about overflows so you are not paying
> much attention to the code you are writing.
I know what the types of the variables I'm using are, thank you. It's
the constants that have inconsistent and confusing types and change the
type of the expression I'm using them with.
Also, with C's implicit casts the LL suffix is not any better at
avoiding overflows than having an int64_t cast backed into the
constants.
It's only when using the constants are used as arguments to a
printf()-equivalent that the LL trap is sprung. And I'd argue this is
not a case where there's much of an overflow risk to start with, and not
one where it would even matter much since most of those are temporary
one-off debugging traces.
--
Francois Gouget
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice v3] utils: Remove the LL suffix from NSEC_PER_SEC
On Mon, 17 Jun 2019, Uri Lublin wrote: > On 6/15/19 2:59 PM, Frediano Ziglio wrote: > >> > >> This constant fits in a 32 bit signed integer so it does not need the > >> suffix. However some of the derived constants don't so use an uint64_t > >> cast to avoid the long vs long long confusion (such as in print > >> statements). > >> Also some of the expressions these constants are used in may overflow so > >> perform the appropriate casts in those locations. This makes it clearer > >> that these operations are 64 bit. > >> > >> Signed-off-by: Francois Gouget > > > > ack for me, waiting Uri to confirm > > Honestly, I do not see the value of making NSEC_PER_SEC a 32bit integer. > Most of its usage is in 64bit expressions. It's not a 32 bit vs. 64 bit issue. It's a long vs. long long one. Whenever NSEC_PER_MILLISEC or NSEC_PER_SEC are used in a spice_debug() parameter one cannot use %u or %lu as the format. But not so for NSEC_PER_MICROSEC or MSEC_PER_SEC. This is inconsistent so that timing traces require lots of guessing and recompilations. -- Francois Gouget ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [protocol v2] macros: Remove unused SPICE_GNUC_XXX macros
We don't want to maintain more macros than necessary and these have been unused for over two years. Signed-off-by: Francois Gouget --- v2: Remove unused macros altogether rather than marking them as deprecated (no use of them has been found in spice, spice-gtk, spice-common, spice-protocol, vd_agent and xf86-video-qxl in the past two years). spice/macros.h | 16 1 file changed, 16 deletions(-) diff --git a/spice/macros.h b/spice/macros.h index ab1d056..a23e866 100644 --- a/spice/macros.h +++ b/spice/macros.h @@ -34,19 +34,11 @@ #include #if__GNUC__ > 2 || (__GNUC__ == 2 && __GNUC_MINOR__ >= 96) -#define SPICE_GNUC_PURE __attribute__((__pure__)) #define SPICE_GNUC_MALLOC __attribute__((__malloc__)) #else -#define SPICE_GNUC_PURE #define SPICE_GNUC_MALLOC #endif -#if __GNUC__ >= 4 -#define SPICE_GNUC_NULL_TERMINATED __attribute__((__sentinel__)) -#else -#define SPICE_GNUC_NULL_TERMINATED -#endif - #ifndef __has_feature #define __has_feature(x) 0 /* Compatibility with non-clang compilers. */ #endif @@ -62,20 +54,12 @@ #if __GNUC__ > 2 || (__GNUC__ == 2 && __GNUC_MINOR__ > 4) #define SPICE_GNUC_PRINTF( format_idx, arg_idx ) __attribute__((__format__ (__printf__, format_idx, arg_idx))) -#define SPICE_GNUC_SCANF( format_idx, arg_idx ) __attribute__((__format__ (__scanf__, format_idx, arg_idx))) -#define SPICE_GNUC_FORMAT( arg_idx ) __attribute__((__format_arg__ (arg_idx))) #define SPICE_GNUC_NORETURN __attribute__((__noreturn__)) -#define SPICE_GNUC_CONST __attribute__((__const__)) #define SPICE_GNUC_UNUSED __attribute__((__unused__)) -#define SPICE_GNUC_NO_INSTRUMENT __attribute__((__no_instrument_function__)) #else /* !__GNUC__ */ #define SPICE_GNUC_PRINTF( format_idx, arg_idx ) -#define SPICE_GNUC_SCANF( format_idx, arg_idx ) -#define SPICE_GNUC_FORMAT( arg_idx ) #define SPICE_GNUC_NORETURN -#define SPICE_GNUC_CONST #define SPICE_GNUC_UNUSED -#define SPICE_GNUC_NO_INSTRUMENT #endif /* !__GNUC__ */ #ifdef G_DEPRECATED -- 2.20.1 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [protocol] macros: Mark unused SPICE_GNUC_XXX macros as deprecated
We don't want to maintain more macros than necessary and in the end
the equivalent G_GNUC_XXX macros should be preferred.
Should any project actually depend on these macros they can keep using
them by defining the usual SPICE_DEPRECATED macro until they migrate
away from them or the macros are reinstated.
Signed-off-by: Francois Gouget
---
I noticed that this patch fell off despite what looked like general
agreement. I re-grepped for these macros and did not find them used in
the current Spice codebase which means they have not been used for at
least two years.
https://lists.freedesktop.org/archives/spice-devel/2016-December/034578.html
spice/macros.h | 18 --
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/spice/macros.h b/spice/macros.h
index ab1d056..95c12d4 100644
--- a/spice/macros.h
+++ b/spice/macros.h
@@ -33,8 +33,14 @@
#include
+#ifndef SPICE_DEPRECATED
+#define SPICE_ATTRIBUTE_DEPRECATED(attribute, name) __attribute__((attribute,
warning("deprecated, use -DSPICE_DEPRECATED or " name " instead")))
+#else
+#define SPICE_ATTRIBUTE_DEPRECATED(attribute, name) __attribute__((attribute))
+#endif
+
#if__GNUC__ > 2 || (__GNUC__ == 2 && __GNUC_MINOR__ >= 96)
-#define SPICE_GNUC_PURE __attribute__((__pure__))
+#define SPICE_GNUC_PURE SPICE_ATTRIBUTE_DEPRECATED(__pure__, "G_GNUC_PURE")
#define SPICE_GNUC_MALLOC __attribute__((__malloc__))
#else
#define SPICE_GNUC_PURE
@@ -42,7 +48,7 @@
#endif
#if __GNUC__ >= 4
-#define SPICE_GNUC_NULL_TERMINATED __attribute__((__sentinel__))
+#define SPICE_GNUC_NULL_TERMINATED SPICE_ATTRIBUTE_DEPRECATED(__sentinel__,
"G_GNUC_NULL_TERMINATED")
#else
#define SPICE_GNUC_NULL_TERMINATED
#endif
@@ -62,12 +68,12 @@
#if __GNUC__ > 2 || (__GNUC__ == 2 && __GNUC_MINOR__ > 4)
#define SPICE_GNUC_PRINTF( format_idx, arg_idx ) __attribute__((__format__
(__printf__, format_idx, arg_idx)))
-#define SPICE_GNUC_SCANF( format_idx, arg_idx ) __attribute__((__format__
(__scanf__, format_idx, arg_idx)))
-#define SPICE_GNUC_FORMAT( arg_idx ) __attribute__((__format_arg__ (arg_idx)))
+#define SPICE_GNUC_SCANF( format_idx, arg_idx )
SPICE_ATTRIBUTE_DEPRECATED(__format__ (__scanf__, format_idx, arg_idx),
"G_GNUC_SCANF")
+#define SPICE_GNUC_FORMAT( arg_idx ) SPICE_ATTRIBUTE_DEPRECATED(__format_arg__
(arg_idx), "G_GNUC_FORMAT")
#define SPICE_GNUC_NORETURN __attribute__((__noreturn__))
-#define SPICE_GNUC_CONST __attribute__((__const__))
+#define SPICE_GNUC_CONST SPICE_ATTRIBUTE_DEPRECATED(__const__, "G_GNUC_CONST")
#define SPICE_GNUC_UNUSED __attribute__((__unused__))
-#define SPICE_GNUC_NO_INSTRUMENT __attribute__((__no_instrument_function__))
+#define SPICE_GNUC_NO_INSTRUMENT
SPICE_ATTRIBUTE_DEPRECATED(__no_instrument_function__, "G_GNUC_NO_INSTRUMENT")
#else /* !__GNUC__ */
#define SPICE_GNUC_PRINTF( format_idx, arg_idx )
#define SPICE_GNUC_SCANF( format_idx, arg_idx )
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [client v2 06/12] channel-display: Rename the frame mmtime variables
On Tue, 18 Jun 2019, Frediano Ziglio wrote: > > > > > > The point of the mmtime timestamps is that they are the same on the > > > server and client thanks to the client running its own mmtime clock > > > synchronized, modulo a server-controlled offset, to the server's > > Not sure about the "modulo a server-controlled offset", I would > say "biased by a server-controlled offset". I'm ok with the wording change. > > > mmtime clock. > > > So the frame mmtime timestamps are neither tied to the server nor the > > > client. They are however tied to the frame. > > > > I got to read this last paragraph couple of times to make it sounds right, > > but I don't have suggestions. Maybe: Thus prefixing mmtime variable names with client or server does not really make sense so it is better to use the prefix to indicate what they are related to such as the frame or the current time. [...] > > - guint32 server_mmtime, > > - guint32 client_mmtime) > > + guint32 frame_mmtime, > > + guint32 current_mmtime) [...] -- Francois Gouget ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [client v2 12/12] mjpeg: Take the decoding time into account to display frames
Signed-off-by: Francois Gouget
---
src/channel-display-mjpeg.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
index 20e10d9b..764f0611 100644
--- a/src/channel-display-mjpeg.c
+++ b/src/channel-display-mjpeg.c
@@ -189,6 +189,9 @@ static void mjpeg_decoder_schedule(MJpegDecoder *decoder)
if (frame) {
if (spice_mmtime_diff(time, frame->time) <= 0) {
guint32 d = frame->time - time;
+if (d > decoder->base.decoding_time) {
+d -= decoder->base.decoding_time;
+}
decoder->cur_frame = frame;
decoder->timer_id = g_timeout_add(d,
mjpeg_decoder_decode_frame, decoder);
break;
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [client v2 11/12] spice-session: Keep track of the global streams lag
Each video and audio stream has its own lag: for video streams it is
the decoding time and for audio ones buffering by the audio subsystem.
The only way to keep them all in sync is to synchronize to the most
laggy stream.
Signed-off-by: Francois Gouget
---
There may be a thread conetxt issue with the spice_session_update_lag()
call in display_handle_stream_data(). I have not noticed this causing
any problem though.
src/channel-display-gst.c | 9
src/channel-display-mjpeg.c | 6 ++
src/channel-display-priv.h | 4
src/channel-display.c | 33 +++--
src/channel-display.h | 2 ++
src/channel-playback-priv.h | 3 ++-
src/channel-playback.c | 22 ---
src/spice-session-priv.h| 6 +-
src/spice-session.c | 42 +++--
9 files changed, 118 insertions(+), 9 deletions(-)
diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 460d0ab9..d5a5d431 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -454,6 +454,15 @@ sink_event_probe(GstPad *pad, GstPadProbeInfo *info,
gpointer data)
frame->time, frame->size, frame->creation_time, duration,
decoder->decoding_queue->length, gstframe->queue_len);
+/* And the average decoding time. Only take into account frames
+ * that were not delayed (see above), or that prove the current
+ * average is clearly overestimated.
+ */
+if (gstframe->queue_len < MAX_DECODED_FRAMES ||
+duration / 1000 < decoder->base.decoding_time) {
+decoder->base.decoding_time = (decoder->base.decoding_time *
15 + duration / 1000) / 16;
+}
+
if (!decoder->appsink) {
/* The sink will display the frame directly so this
* SpiceGstFrame and those of any dropped frame are no longer
diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
index b058424e..20e10d9b 100644
--- a/src/channel-display-mjpeg.c
+++ b/src/channel-display-mjpeg.c
@@ -86,6 +86,7 @@ static gboolean mjpeg_decoder_decode_frame(gpointer
video_decoder)
JDIMENSION width, height;
uint8_t *dest;
uint8_t *lines[4];
+int64_t start = g_get_monotonic_time();
jpeg_read_header(>mjpeg_cinfo, 1);
width = decoder->mjpeg_cinfo.image_width;
@@ -156,6 +157,11 @@ static gboolean mjpeg_decoder_decode_frame(gpointer
video_decoder)
}
jpeg_finish_decompress(>mjpeg_cinfo);
+uint32_t duration = (g_get_monotonic_time() - start) / 1000;
+decoder->base.decoding_time = decoder->base.decoding_time ?
+(decoder->base.decoding_time * 15 + duration) / 16 :
+duration;
+
/* Display the frame and dispose of it */
stream_display_frame(decoder->base.stream, decoder->cur_frame,
width, height, SPICE_UNKNOWN_STRIDE,
decoder->out_frame);
diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
index 7c978780..03f0c992 100644
--- a/src/channel-display-priv.h
+++ b/src/channel-display-priv.h
@@ -72,6 +72,9 @@ struct VideoDecoder {
/* The associated display stream. */
display_stream *stream;
+
+/* The average time it takes to decode a frame. */
+float decoding_time;
};
@@ -118,6 +121,7 @@ struct display_stream {
int have_region;
VideoDecoder*video_decoder;
+guint decoder_lag;
SpiceChannel*channel;
diff --git a/src/channel-display.c b/src/channel-display.c
index 03110df6..b6569d04 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -1411,6 +1411,27 @@ void stream_display_frame(display_stream *st, SpiceFrame
*frame,
}
}
+G_GNUC_INTERNAL
+guint32 spice_display_channel_get_lag(SpiceDisplayChannel *channel)
+{
+SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
+int i;
+guint lag;
+
+lag = 0;
+if (c->streams) {
+for (i = 0; i < c->nstreams; i++) {
+display_stream *st = c->streams[i];
+if (st != NULL && st->video_decoder) {
+st->decoder_lag = st->video_decoder->decoding_time;
+lag = MAX(lag, st->decoder_lag);
+}
+}
+}
+
+return lag;
+}
+
G_GNUC_INTERNAL
gboolean hand_pipeline_to_widget(display_stream *st, GstPipeline *pipeline)
{
@@ -1467,7 +1488,7 @@ static void
display_update_stream_report(SpiceDisplayChannel *channel, uint32_t
report.num_drops = st-> report_num_drops;
report.last_frame_delay = margin;
if (spice_session_is_playback_active(session)) {
-report.audio_delay = spice_session_get_playback_latency(session);
+report.audio_delay = spice_session_get_playbac
[Spice-devel] [client v2 10/12] channel-display: Remove playback_sync_drops_seq_len
display_handle_stream_data() now has its own mechanism to avoid
dropping frames which does not depend on the playback latency.
Signed-off-by: Francois Gouget
---
src/channel-display-priv.h | 2 --
src/channel-display.c | 8
src/channel-playback-priv.h | 1 -
src/channel-playback.c | 9 -
src/spice-session-priv.h| 1 -
src/spice-session.c | 15 ---
6 files changed, 36 deletions(-)
diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
index a2634ee5..7c978780 100644
--- a/src/channel-display-priv.h
+++ b/src/channel-display-priv.h
@@ -145,8 +145,6 @@ struct display_stream {
GArray *drops_seqs_stats_arr;
uint32_t num_drops_seqs;
-uint32_t playback_sync_drops_seq_len;
-
/* playback quality report to server */
gboolean report_is_active;
uint32_t report_id;
diff --git a/src/channel-display.c b/src/channel-display.c
index aee1e844..03110df6 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -1484,8 +1484,6 @@ static void
display_update_stream_report(SpiceDisplayChannel *channel, uint32_t
}
}
-#define STREAM_PLAYBACK_SYNC_DROP_SEQ_LEN_LIMIT 5
-
static void display_stream_stats_debug(display_stream *st)
{
guint64 drops_duration_total = 0;
@@ -1567,7 +1565,6 @@ static void display_stream_stats_save(display_stream *st,
st->cur_drops_seq_stats.start_mm_time = frame_mmtime;
}
st->cur_drops_seq_stats.len++;
-st->playback_sync_drops_seq_len++;
return;
}
@@ -1579,7 +1576,6 @@ static void display_stream_stats_save(display_stream *st,
memset(>cur_drops_seq_stats, 0, sizeof(st->cur_drops_seq_stats));
st->num_drops_seqs++;
}
-st->playback_sync_drops_seq_len = 0;
}
static SpiceFrame *spice_frame_new(display_stream *st,
@@ -1800,10 +1796,6 @@ static void display_handle_stream_data(SpiceChannel
*channel, SpiceMsgIn *in)
if (c->enable_adaptive_streaming) {
display_update_stream_report(SPICE_DISPLAY_CHANNEL(channel), op->id,
op->multi_media_time, mmtime_margin);
-if (st->playback_sync_drops_seq_len >=
STREAM_PLAYBACK_SYNC_DROP_SEQ_LEN_LIMIT) {
-
spice_session_sync_playback_latency(spice_channel_get_session(channel));
-st->playback_sync_drops_seq_len = 0;
-}
}
}
diff --git a/src/channel-playback-priv.h b/src/channel-playback-priv.h
index aa33d2c4..dc89e2d8 100644
--- a/src/channel-playback-priv.h
+++ b/src/channel-playback-priv.h
@@ -20,5 +20,4 @@
gboolean spice_playback_channel_is_active(SpicePlaybackChannel *channel);
guint32 spice_playback_channel_get_latency(SpicePlaybackChannel *channel);
-void spice_playback_channel_sync_latency(SpicePlaybackChannel *channel);
#endif
diff --git a/src/channel-playback.c b/src/channel-playback.c
index bd551b37..052e3036 100644
--- a/src/channel-playback.c
+++ b/src/channel-playback.c
@@ -495,12 +495,3 @@ guint32
spice_playback_channel_get_latency(SpicePlaybackChannel *channel)
}
return channel->priv->latency;
}
-
-G_GNUC_INTERNAL
-void spice_playback_channel_sync_latency(SpicePlaybackChannel *channel)
-{
-g_return_if_fail(SPICE_IS_PLAYBACK_CHANNEL(channel));
-g_return_if_fail(channel->priv->is_active);
-SPICE_DEBUG("%s: notify latency update %u", __FUNCTION__,
channel->priv->min_latency);
-g_coroutine_object_notify(G_OBJECT(SPICE_CHANNEL(channel)), "min-latency");
-}
diff --git a/src/spice-session-priv.h b/src/spice-session-priv.h
index 794d86a7..1e91f627 100644
--- a/src/spice-session-priv.h
+++ b/src/spice-session-priv.h
@@ -86,7 +86,6 @@ void spice_session_set_uuid(SpiceSession *session, guint8
uuid[16]);
void spice_session_set_name(SpiceSession *session, const gchar *name);
gboolean spice_session_is_playback_active(SpiceSession *session);
guint32 spice_session_get_playback_latency(SpiceSession *session);
-void spice_session_sync_playback_latency(SpiceSession *session);
gboolean spice_session_get_audio_enabled(SpiceSession *session);
gboolean spice_session_get_smartcard_enabled(SpiceSession *session);
gboolean spice_session_get_usbredir_enabled(SpiceSession *session);
diff --git a/src/spice-session.c b/src/spice-session.c
index 4e5a7823..a1a96829 100644
--- a/src/spice-session.c
+++ b/src/spice-session.c
@@ -2660,21 +2660,6 @@ void spice_session_set_name(SpiceSession *session, const
gchar *name)
g_coroutine_object_notify(G_OBJECT(session), "name");
}
-G_GNUC_INTERNAL
-void spice_session_sync_playback_latency(SpiceSession *session)
-{
-g_return_if_fail(SPICE_IS_SESSION(session));
-
-SpiceSessionPrivate *s = session->priv;
-
-if (s->playback_channel &&
-spice_playback_channel_is_active(s->playback_channel)) {
-spice_playback_channel_sync_latency(s->
[Spice-devel] [client v2 06/12] channel-display: Rename the frame mmtime variables
The point of the mmtime timestamps is that they are the same on the
server and client thanks to the client running its own mmtime clock
synchronized, modulo a server-controlled offset, to the server's
mmtime clock.
So the frame mmtime timestamps are neither tied to the server nor the
client. They are however tied to the frame.
Signed-off-by: Francois Gouget
---
src/channel-display.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/src/channel-display.c b/src/channel-display.c
index cda0fcdd..b26326d6 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -1603,27 +1603,27 @@ static void display_stream_stats_debug(display_stream
*st)
static void display_stream_stats_save(display_stream *st,
- guint32 server_mmtime,
- guint32 client_mmtime)
+ guint32 frame_mmtime,
+ guint32 current_mmtime)
{
-gint32 margin = server_mmtime - client_mmtime;
+gint32 margin = frame_mmtime - current_mmtime;
if (!st->num_input_frames) {
-st->first_frame_mm_time = server_mmtime;
+st->first_frame_mm_time = frame_mmtime;
}
st->num_input_frames++;
if (margin < 0) {
CHANNEL_DEBUG(st->channel, "stream data too late by %u ms (ts: %u,
mmtime: %u)",
- client_mmtime - server_mmtime, server_mmtime,
client_mmtime);
-st->arrive_late_time += client_mmtime - server_mmtime;
+ current_mmtime - frame_mmtime, frame_mmtime,
current_mmtime);
+st->arrive_late_time += current_mmtime - frame_mmtime;
st->arrive_late_count++;
/* Late frames are counted as drops in the stats but aren't
necessarily dropped - depends
* on codec and decoder
*/
if (!st->cur_drops_seq_stats.len) {
-st->cur_drops_seq_stats.start_mm_time = server_mmtime;
+st->cur_drops_seq_stats.start_mm_time = frame_mmtime;
}
st->cur_drops_seq_stats.len++;
st->playback_sync_drops_seq_len++;
@@ -1632,7 +1632,7 @@ static void display_stream_stats_save(display_stream *st,
CHANNEL_DEBUG(st->channel, "video margin: %d", margin);
if (st->cur_drops_seq_stats.len) {
-st->cur_drops_seq_stats.duration = server_mmtime -
+st->cur_drops_seq_stats.duration = frame_mmtime -
st->cur_drops_seq_stats.start_mm_time;
g_array_append_val(st->drops_seqs_stats_arr, st->cur_drops_seq_stats);
memset(>cur_drops_seq_stats, 0, sizeof(st->cur_drops_seq_stats));
@@ -1643,7 +1643,7 @@ static void display_stream_stats_save(display_stream *st,
static SpiceFrame *spice_frame_new(display_stream *st,
SpiceMsgIn *in,
- guint32 server_mmtime)
+ guint32 frame_mmtime)
{
SpiceFrame *frame;
guint8 *data_ptr;
@@ -1651,7 +1651,7 @@ static SpiceFrame *spice_frame_new(display_stream *st,
guint32 data_size = spice_msg_in_frame_data(in, _ptr);
frame = g_new(SpiceFrame, 1);
-frame->mm_time = server_mmtime;
+frame->mm_time = frame_mmtime;
frame->dest = *dest_rect;
frame->data = data_ptr;
frame->size = data_size;
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [client v2 09/12] channel-display: No need to rechedule on mmtime offset changes
The frame display time is no longer based on the mmtime clock and thus
is not impacted by mmtime offset changes.
Signed-off-by: Francois Gouget
---
src/channel-display-gst.c | 21 -
src/channel-display-mjpeg.c | 13
src/channel-display-priv.h | 3 --
src/channel-display.c | 59 -
4 files changed, 96 deletions(-)
diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 673f4177..460d0ab9 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -583,26 +583,6 @@ static gboolean create_pipeline(SpiceGstDecoder *decoder)
/* -- VideoDecoder's public API -- */
-static void spice_gst_decoder_reschedule(VideoDecoder *video_decoder)
-{
-SpiceGstDecoder *decoder = (SpiceGstDecoder*)video_decoder;
-
-if (!decoder->appsink) {
-return;
-}
-guint timer_id;
-
-g_mutex_lock(>queues_mutex);
-timer_id = decoder->timer_id;
-decoder->timer_id = 0;
-g_mutex_unlock(>queues_mutex);
-
-if (timer_id != 0) {
-g_source_remove(timer_id);
-}
-schedule_frame(decoder);
-}
-
/* main context */
static void spice_gst_decoder_destroy(VideoDecoder *video_decoder)
{
@@ -774,7 +754,6 @@ VideoDecoder* create_gstreamer_decoder(int codec_type,
display_stream *stream)
if (gstvideo_init()) {
decoder = g_new0(SpiceGstDecoder, 1);
decoder->base.destroy = spice_gst_decoder_destroy;
-decoder->base.reschedule = spice_gst_decoder_reschedule;
decoder->base.queue_frame = spice_gst_decoder_queue_frame;
decoder->base.codec_type = codec_type;
decoder->base.stream = stream;
diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
index d3ab77c8..b058424e 100644
--- a/src/channel-display-mjpeg.c
+++ b/src/channel-display-mjpeg.c
@@ -250,18 +250,6 @@ static gboolean mjpeg_decoder_queue_frame(VideoDecoder
*video_decoder,
return TRUE;
}
-static void mjpeg_decoder_reschedule(VideoDecoder *video_decoder)
-{
-MJpegDecoder *decoder = (MJpegDecoder*)video_decoder;
-
-SPICE_DEBUG("%s", __FUNCTION__);
-if (decoder->timer_id != 0) {
-g_source_remove(decoder->timer_id);
-decoder->timer_id = 0;
-}
-mjpeg_decoder_schedule(decoder);
-}
-
static void mjpeg_decoder_destroy(VideoDecoder* video_decoder)
{
MJpegDecoder *decoder = (MJpegDecoder*)video_decoder;
@@ -281,7 +269,6 @@ VideoDecoder* create_mjpeg_decoder(int codec_type,
display_stream *stream)
MJpegDecoder *decoder = g_new0(MJpegDecoder, 1);
decoder->base.destroy = mjpeg_decoder_destroy;
-decoder->base.reschedule = mjpeg_decoder_reschedule;
decoder->base.queue_frame = mjpeg_decoder_queue_frame;
decoder->base.codec_type = codec_type;
decoder->base.stream = stream;
diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
index badd9d42..a2634ee5 100644
--- a/src/channel-display-priv.h
+++ b/src/channel-display-priv.h
@@ -55,9 +55,6 @@ struct VideoDecoder {
/* Releases the video decoder's resources */
void (*destroy)(VideoDecoder *decoder);
-/* Notifies the decoder that the mm-time clock changed. */
-void (*reschedule)(VideoDecoder *decoder);
-
/* Takes ownership of the specified frame, decompresses it,
* and displays it at the right time.
*
diff --git a/src/channel-display.c b/src/channel-display.c
index 815242de..aee1e844 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -107,7 +107,6 @@ static void spice_display_channel_reset(SpiceChannel
*channel, gboolean migratin
static void spice_display_channel_set_capabilities(SpiceChannel *channel);
static void destroy_canvas(display_surface *surface);
static void display_stream_destroy(gpointer st);
-static void display_session_mm_time_reset_cb(SpiceSession *session, gpointer
data);
static SpiceGlScanout* spice_gl_scanout_copy(const SpiceGlScanout *scanout);
G_DEFINE_BOXED_TYPE(SpiceGlScanout, spice_gl_scanout,
@@ -189,9 +188,6 @@ static void spice_display_channel_constructed(GObject
*object)
g_return_if_fail(c->palettes != NULL);
c->monitors = g_array_new(FALSE, TRUE, sizeof(SpiceDisplayMonitorConfig));
-spice_g_signal_connect_object(s, "mm-time-reset",
- G_CALLBACK(display_session_mm_time_reset_cb),
- SPICE_CHANNEL(object), 0);
spice_display_channel_set_capabilities(SPICE_CHANNEL(object));
@@ -1488,61 +1484,6 @@ static void
display_update_stream_report(SpiceDisplayChannel *channel, uint32_t
}
}
-/*
- * Migration can occur between 2 spice-servers with different mm-times.
- * Then, the following cases can happen after migration completes:
- * (We refer to src/dst-time as the mm-times on the src/dst servers):
- *
- * (case 1) Frames with time ~= dst-time arrive to the client before th
[Spice-devel] [client v2 08/12] playback: Use the audio timestamps for the global mmtime conversion
More data helps improve the accuracy of the estimation of the true clock
offset and minimum network latency.
Signed-off-by: Francois Gouget
---
src/channel-playback.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/src/channel-playback.c b/src/channel-playback.c
index a00706fe..bd551b37 100644
--- a/src/channel-playback.c
+++ b/src/channel-playback.c
@@ -302,14 +302,16 @@ static void playback_handle_data(SpiceChannel *channel,
SpiceMsgIn *in)
{
SpicePlaybackChannelPrivate *c = SPICE_PLAYBACK_CHANNEL(channel)->priv;
SpiceMsgPlaybackPacket *packet = spice_msg_in_parsed(in);
+SpiceSession *session;
#ifdef DEBUG
CHANNEL_DEBUG(channel, "%s: time %u data %p size %d", __FUNCTION__,
packet->time, packet->data, packet->data_size);
#endif
-if (spice_mmtime_diff(c->last_time, packet->time) > 0)
-g_warn_if_reached();
+/* This also updates the time offset */
+session = spice_channel_get_session(channel);
+spice_session_get_client_time(session, packet->time);
c->last_time = packet->time;
@@ -361,11 +363,16 @@ static void playback_handle_start(SpiceChannel *channel,
SpiceMsgIn *in)
{
SpicePlaybackChannelPrivate *c = SPICE_PLAYBACK_CHANNEL(channel)->priv;
SpiceMsgPlaybackStart *start = spice_msg_in_parsed(in);
+SpiceSession *session;
CHANNEL_DEBUG(channel, "%s: fmt %u channels %u freq %u time %u mode %s",
__FUNCTION__,
start->format, start->channels, start->frequency,
start->time,
spice_audio_data_mode_to_string(c->mode));
+/* This also updates the time offset */
+session = spice_channel_get_session(channel);
+spice_session_get_client_time(session, start->time);
+
c->frame_count = 0;
c->last_time = start->time;
c->is_active = TRUE;
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [client v2 05/12] channel-display: The video latency is in fact a margin
'Latency' is confusing because it evokes either the network latency
or the audio latency, neither of which match what the latency
variables contain. Instead they record how much margin there was
between when the frame was received, and when it is supposed to be
displayed. Hence the new name.
Signed-off-by: Francois Gouget
---
It's less confusing when using the right terminology so I'm starting
with this before the more involved patches.
src/channel-display-gst.c | 6 +++---
src/channel-display-mjpeg.c | 4 ++--
src/channel-display.c | 26 +-
3 files changed, 18 insertions(+), 18 deletions(-)
diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 449b9cb8..6fccf621 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -679,7 +679,7 @@ static void spice_gst_decoder_destroy(VideoDecoder
*video_decoder)
* spice_frame_free() and free its encoded_frame.
*/
static gboolean spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
- SpiceFrame *frame, int latency)
+ SpiceFrame *frame, int margin)
{
SpiceGstDecoder *decoder = (SpiceGstDecoder*)video_decoder;
@@ -697,7 +697,7 @@ static gboolean spice_gst_decoder_queue_frame(VideoDecoder
*video_decoder,
}
decoder->last_mm_time = frame->mm_time;
-if (latency < 0 &&
+if (margin < 0 &&
decoder->base.codec_type == SPICE_VIDEO_CODEC_TYPE_MJPEG) {
/* Dropping MJPEG frames has no impact on those that follow and
* saves CPU so do it.
@@ -726,7 +726,7 @@ static gboolean spice_gst_decoder_queue_frame(VideoDecoder
*video_decoder,
frame->data, frame->size,
0, frame->size,
frame, (GDestroyNotify)
spice_frame_free);
-GstClockTime pts = gst_clock_get_time(decoder->clock) -
gst_element_get_base_time(decoder->pipeline) + ((uint64_t)MAX(0, latency)) *
1000 * 1000;
+GstClockTime pts = gst_clock_get_time(decoder->clock) -
gst_element_get_base_time(decoder->pipeline) + ((uint64_t)MAX(0, margin)) *
1000 * 1000;
GST_BUFFER_DURATION(buffer) = GST_CLOCK_TIME_NONE;
GST_BUFFER_DTS(buffer) = GST_CLOCK_TIME_NONE;
GST_BUFFER_PTS(buffer) = pts;
diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
index ba7f266e..647d31b0 100644
--- a/src/channel-display-mjpeg.c
+++ b/src/channel-display-mjpeg.c
@@ -219,7 +219,7 @@ static void mjpeg_decoder_drop_queue(MJpegDecoder *decoder)
/* -- VideoDecoder's public API -- */
static gboolean mjpeg_decoder_queue_frame(VideoDecoder *video_decoder,
- SpiceFrame *frame, int32_t latency)
+ SpiceFrame *frame, int32_t margin)
{
MJpegDecoder *decoder = (MJpegDecoder*)video_decoder;
SpiceFrame *last_frame;
@@ -239,7 +239,7 @@ static gboolean mjpeg_decoder_queue_frame(VideoDecoder
*video_decoder,
/* Dropped MJPEG frames don't impact the ones that come after.
* So drop late frames as early as possible to save on processing time.
*/
-if (latency < 0) {
+if (margin < 0) {
SPICE_DEBUG("dropping a late MJPEG frame");
spice_frame_free(frame);
return TRUE;
diff --git a/src/channel-display.c b/src/channel-display.c
index 6c2a99c7..cda0fcdd 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -1432,7 +1432,7 @@ gboolean hand_pipeline_to_widget(display_stream *st,
GstPipeline *pipeline)
#define STREAM_REPORT_DROP_SEQ_LEN_LIMIT 3
static void display_update_stream_report(SpiceDisplayChannel *channel,
uint32_t stream_id,
- uint32_t frame_time, int32_t latency)
+ uint32_t frame_time, int32_t margin)
{
display_stream *st = get_stream_by_id(SPICE_CHANNEL(channel), stream_id);
guint64 now;
@@ -1449,7 +1449,7 @@ static void
display_update_stream_report(SpiceDisplayChannel *channel, uint32_t
}
st->report_num_frames++;
-if (latency < 0) { // drop
+if (margin < 0) { // drop
st->report_num_drops++;
st->report_drops_seq_len++;
} else {
@@ -1469,7 +1469,7 @@ static void
display_update_stream_report(SpiceDisplayChannel *channel, uint32_t
report.end_frame_mm_time = frame_time;
report.num_frames = st->report_num_frames;
report.num_drops = st-> report_num_drops;
-report.last_frame_delay = latency;
+report.last_frame_delay = margin;
if (spice_session_is_playback_active(session)) {
report.audio_delay = spice_session_get_playback_latency(session);
} else {
@@ -1606,14 +1606,14 @@ static void display_stream_sta
[Spice-devel] [client v2 07/12] channel-display: Minimize the stream lag by ignoring the server one
The client is in a better position than the server to pick the minimum
lag needed to compensate for frame arrival time jitter and ensure
smooth video playback.
To do so:
- It ignores the lag specified by the server through the mmtime clock
adjustments (but this lag is still tracked for the stream reports).
- It performs its own frame mmtime conversion to the local monotonic
clock spice_session_get_client_time() since the server-controlled
mmtime clock cannot be relied on. This conversion uses data from all
streams but is global so all streams ihave a common time reference.
- This is also where mmtime clock changes caused by server migration
are handled.
- It tracks the margin between the converted time-to-display frame
timestamp and the curren time and adds a delay to ensure this margin
remains positive.
- This delay introduces the video stream lag. It is continuously
adjusted to either be as low as possible, or match the audio
playback delay for proper lip sync.
- Delay adjustments are gradual, speeding up or slowing down video
playback until the average margin matches the target lag.
- Changes in the average margin are tracked (see margin_spread) to
avoid nudging the delay in response to minor random variations.
Signed-off-by: Francois Gouget
---
src/channel-display-gst.c | 22 ++---
src/channel-display-mjpeg.c | 14 +--
src/channel-display-priv.h | 18 +++-
src/channel-display.c | 178
src/spice-session-priv.h| 1 +
src/spice-session.c | 46 ++
6 files changed, 240 insertions(+), 39 deletions(-)
diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 6fccf621..673f4177 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -50,7 +50,7 @@ typedef struct SpiceGstDecoder {
/* -- Decoding and display queues -- */
-uint32_t last_mm_time;
+uint32_t last_frame_time;
GMutex queues_mutex;
GQueue *decoding_queue;
@@ -297,8 +297,8 @@ static void schedule_frame(SpiceGstDecoder *decoder)
break;
}
-if (spice_mmtime_diff(gstframe->encoded_frame->mm_time, now) >= 0) {
-decoder->timer_id = g_timeout_add(gstframe->encoded_frame->mm_time
- now,
+if (spice_mmtime_diff(gstframe->encoded_frame->time, now) >= 0) {
+decoder->timer_id = g_timeout_add(gstframe->encoded_frame->time -
now,
display_frame, decoder);
} else if (decoder->display_frame && !decoder->pending_samples) {
/* Still attempt to display the least out of date frame so the
@@ -307,8 +307,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->encoded_frame->mm_time,
-gstframe->encoded_frame->mm_time, now);
+__FUNCTION__, now - gstframe->encoded_frame->time,
+gstframe->encoded_frame->time, now);
stream_dropped_frame_on_playback(decoder->base.stream);
decoder->display_frame = NULL;
free_gst_frame(gstframe);
@@ -449,9 +449,9 @@ sink_event_probe(GstPad *pad, GstPadProbeInfo *info,
gpointer data)
* or more frame intervals.
*/
record(frames_stats,
- "frame mm_time %u size %u creation time %" PRId64
+ "frame time %u size %u creation time %" PRId64
" decoded time %" PRId64 " queue %u before %u",
- frame->mm_time, frame->size, frame->creation_time, duration,
+ frame->time, frame->size, frame->creation_time, duration,
decoder->decoding_queue->length, gstframe->queue_len);
if (!decoder->appsink) {
@@ -689,13 +689,13 @@ static gboolean
spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
return TRUE;
}
-if (spice_mmtime_diff(frame->mm_time, decoder->last_mm_time) < 0) {
+if (spice_mmtime_diff(frame->time, decoder->last_frame_time) < 0) {
SPICE_DEBUG("new-frame-time < last-frame-time (%u < %u):"
" resetting stream",
-frame->mm_time, decoder->last_mm_time);
+frame->time, decoder->last_frame_time);
/* Let GStreamer deal with the frame anyway */
}
-decoder->last_mm_time = frame->mm_time;
+decoder->last_frame_time = frame->time;
if (margin < 0 &&
decoder->base.codec_type == S
[Spice-devel] [client v2 04/12] gstreamer: Add the encoded frame's order the statistics
The number of frames that were sitting in the decoding_queue before the
current frame was added is crucial to correctly interpret the decoding
time:
* Less than MAX_DECODED_FRAMES means nothing blocked the decoding of
that frame.
* More than MAX_DECODED_FRAMES means decoding was delayed by one or more
frame intervals.
Signed-off-by: Francois Gouget
---
v2: Renamed rank to queue_len.
Moved it to the last position in the statistics string.
src/channel-display-gst.c | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index c2b24ea7..449b9cb8 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -90,6 +90,7 @@ struct SpiceGstFrame {
GstBuffer *encoded_buffer;
SpiceFrame *encoded_frame;
GstSample *decoded_sample;
+guint queue_len;
};
static SpiceGstFrame *create_gst_frame(GstBuffer *buffer, SpiceFrame *frame)
@@ -440,11 +441,18 @@ sink_event_probe(GstPad *pad, GstPadProbeInfo *info,
gpointer data)
SpiceGstFrame *gstframe = l->data;
const SpiceFrame *frame = gstframe->encoded_frame;
int64_t duration = g_get_monotonic_time() - frame->creation_time;
+/* Note that queue_len (the length of the queue prior to adding
+ * this frame) is crucial to correctly interpret the decoding time:
+ * - Less than MAX_DECODED_FRAMES means nothing blocked the
+ * decoding of that frame.
+ * - More than MAX_DECODED_FRAMES means decoding was delayed by one
+ * or more frame intervals.
+ */
record(frames_stats,
"frame mm_time %u size %u creation time %" PRId64
- " decoded time %" PRId64 " queue %u",
+ " decoded time %" PRId64 " queue %u before %u",
frame->mm_time, frame->size, frame->creation_time, duration,
- decoder->decoding_queue->length);
+ decoder->decoding_queue->length, gstframe->queue_len);
if (!decoder->appsink) {
/* The sink will display the frame directly so this
@@ -729,6 +737,7 @@ static gboolean spice_gst_decoder_queue_frame(VideoDecoder
*video_decoder,
SpiceGstFrame *gst_frame = create_gst_frame(buffer, frame);
g_mutex_lock(>queues_mutex);
+gst_frame->queue_len = decoder->decoding_queue->length;
g_queue_push_tail(decoder->decoding_queue, gst_frame);
g_mutex_unlock(>queues_mutex);
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [client v2 03/12] gstreamer: Improve the statistics collection
By the time schedule_frame() pulls a sample off GStreamer's pipeline the
frame may have been decoded for hundreds of milliseconds, making the
decoding time all but meaningless.
This patch ensures the statistics are always collected by
sink_event_probe() which is called as soon as the decoded frame is
available.
Note that even so the decoding time may be overestimated as the frame
may have been waiting for a while in encoded form for a spot to be freed
in the GStreamer pipeline's sink queue.
Signed-off-by: Francois Gouget
---
v2: No change.
src/channel-display-gst.c | 105 +-
1 file changed, 59 insertions(+), 46 deletions(-)
diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 93064625..c2b24ea7 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -1,6 +1,6 @@
/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
/*
- Copyright (C) 2015-2016 CodeWeavers, Inc
+ Copyright (C) 2015-2016, 2019 CodeWeavers, Inc
This library is free software; you can redistribute it and/or
modify it under the terms of the GNU Lesser General Public
@@ -185,13 +185,16 @@ static gboolean display_frame(gpointer video_decoder)
return G_SOURCE_REMOVE;
}
-/* Get the decoded frame relative to buffer or NULL if not found.
- * Dequeue the frame from decoding_queue and return it, caller
- * is responsible to free the pointer.
+/* Returns the decoding queue entry that matches the specified GStreamer
buffer.
+ *
+ * The entry is identified based on the buffer timestamp. However sometimes
+ * GStreamer returns the same buffer twice (and the second time the entry may
+ * have been removed already) or buffers that have a modified, and thus
+ * unrecognizable, timestamp. In such cases NULL is returned.
*
* queues_mutex must be held.
*/
-static SpiceGstFrame *get_decoded_frame(SpiceGstDecoder *decoder, GstBuffer
*buffer)
+static GList *find_frame_entry(SpiceGstDecoder *decoder, GstBuffer *buffer)
{
GstClockTime buffer_ts = GST_BUFFER_PTS(buffer);
#if GST_CHECK_VERSION(1,14,0)
@@ -203,49 +206,34 @@ static SpiceGstFrame *get_decoded_frame(SpiceGstDecoder
*decoder, GstBuffer *buf
}
#endif
-/* Gstreamer sometimes returns the same buffer twice
- * or buffers that have a modified, and thus unrecognizable, PTS.
- * Blindly removing frames from the decoding_queue until we find a
- * match would only empty the queue, resulting in later buffers not
- * finding a match either, etc. So check the buffer has a matching
- * frame first.
- */
-SpiceGstFrame *gstframe = NULL;
GList *l = g_queue_peek_head_link(decoder->decoding_queue);
while (l) {
-gstframe = l->data;
+const SpiceGstFrame *gstframe = l->data;
if (gstframe->timestamp == buffer_ts) {
-break;
+return l;
}
-gstframe = NULL;
l = l->next;
}
-if (gstframe != NULL) {
-/* Now that we know there is a match, remove it and the older
- * frames from the decoding queue */
-SpiceGstFrame *late_frame;
-guint num_frames_dropped = 0;
-
-/* The GStreamer pipeline dropped the corresponding buffer. */
-while ((late_frame = g_queue_pop_head(decoder->decoding_queue)) !=
gstframe) {
-num_frames_dropped++;
-free_gst_frame(late_frame);
-}
+return NULL;
+}
-if (num_frames_dropped != 0) {
-SPICE_DEBUG("the GStreamer pipeline dropped %u frames",
num_frames_dropped);
-}
+/* Pops the queued frames up to and including the specified frame.
+ * All frames are freed except that last frame which belongs to the caller.
+ * Returns the number of freed frames.
+ *
+ * queues_mutex must be held.
+ */
+static guint32 pop_up_to_frame(SpiceGstDecoder *decoder, const SpiceGstFrame
*popframe)
+{
+SpiceGstFrame *gstframe;
+guint32 freed = 0;
-const SpiceFrame *frame = gstframe->encoded_frame;
-int64_t duration = g_get_monotonic_time() - frame->creation_time;
-record(frames_stats,
- "frame mm_time %u size %u creation time %" PRId64
- " decoded time %" PRId64 " queue %u",
- frame->mm_time, frame->size, frame->creation_time,
- duration, decoder->decoding_queue->length);
+while ((gstframe = g_queue_pop_head(decoder->decoding_queue)) != popframe)
{
+free_gst_frame(gstframe);
+freed++;
}
-return gstframe;
+return freed;
}
/* Helper for schedule_frame().
@@ -268,8 +256,16 @@ static void fetch_pending_sample(SpiceGstDecoder *decoder)
* finding a match either, etc. So check the buffer has a matching
* frame first.
*/
-SpiceGstFrame *gstframe = get_decoded_frame(decoder, buffer);
-if (gstframe) {
+GList *l
[Spice-devel] [client v2 01/12] gstreamer: Avoid a couple of forward function declarations
Signed-off-by: Francois Gouget
---
v2: No change.
src/channel-display-gst.c | 81 ---
1 file changed, 42 insertions(+), 39 deletions(-)
diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 91ece0fa..ec8a7d39 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -120,8 +120,6 @@ static void free_gst_frame(SpiceGstFrame *gstframe)
/* -- GStreamer pipeline -- */
static void schedule_frame(SpiceGstDecoder *decoder);
-static void fetch_pending_sample(SpiceGstDecoder *decoder);
-static SpiceGstFrame *get_decoded_frame(SpiceGstDecoder *decoder, GstBuffer
*buffer);
RECORDER(frames_stats, 64, "Frames statistics");
@@ -184,46 +182,10 @@ static gboolean display_frame(gpointer video_decoder)
return G_SOURCE_REMOVE;
}
-/* main loop or GStreamer streaming thread */
-static void schedule_frame(SpiceGstDecoder *decoder)
-{
-guint32 now = stream_get_time(decoder->base.stream);
-g_mutex_lock(>queues_mutex);
-
-while (!decoder->timer_id) {
-while (decoder->display_frame == NULL && decoder->pending_samples) {
-fetch_pending_sample(decoder);
-}
-
-SpiceGstFrame *gstframe = decoder->display_frame;
-if (!gstframe) {
-break;
-}
-
-if (spice_mmtime_diff(gstframe->encoded_frame->mm_time, now) >= 0) {
-decoder->timer_id = g_timeout_add(gstframe->encoded_frame->mm_time
- now,
- display_frame, decoder);
-} else if (decoder->display_frame && !decoder->pending_samples) {
-/* Still attempt to display the least out of date frame so the
- * video is not completely frozen for an extended period of time.
- */
-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->encoded_frame->mm_time,
-gstframe->encoded_frame->mm_time, now);
-stream_dropped_frame_on_playback(decoder->base.stream);
-decoder->display_frame = NULL;
-free_gst_frame(gstframe);
-}
-}
-
-g_mutex_unlock(>queues_mutex);
-}
-
/* Get the decoded frame relative to buffer or NULL if not found.
* Dequeue the frame from decoding_queue and return it, caller
* is responsible to free the pointer.
+ *
* queues_mutex must be held.
*/
static SpiceGstFrame *get_decoded_frame(SpiceGstDecoder *decoder, GstBuffer
*buffer)
@@ -283,6 +245,10 @@ static SpiceGstFrame *get_decoded_frame(SpiceGstDecoder
*decoder, GstBuffer *buf
return gstframe;
}
+/* Helper for schedule_frame().
+ *
+ * queues_mutex must be held.
+ */
static void fetch_pending_sample(SpiceGstDecoder *decoder)
{
GstSample *sample = gst_app_sink_pull_sample(decoder->appsink);
@@ -315,6 +281,43 @@ static void fetch_pending_sample(SpiceGstDecoder *decoder)
}
}
+/* main loop or GStreamer streaming thread */
+static void schedule_frame(SpiceGstDecoder *decoder)
+{
+guint32 now = stream_get_time(decoder->base.stream);
+g_mutex_lock(>queues_mutex);
+
+while (!decoder->timer_id) {
+while (decoder->display_frame == NULL && decoder->pending_samples) {
+fetch_pending_sample(decoder);
+}
+
+SpiceGstFrame *gstframe = decoder->display_frame;
+if (!gstframe) {
+break;
+}
+
+if (spice_mmtime_diff(gstframe->encoded_frame->mm_time, now) >= 0) {
+decoder->timer_id = g_timeout_add(gstframe->encoded_frame->mm_time
- now,
+ display_frame, decoder);
+} else if (decoder->display_frame && !decoder->pending_samples) {
+/* Still attempt to display the least out of date frame so the
+ * video is not completely frozen for an extended period of time.
+ */
+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->encoded_frame->mm_time,
+gstframe->encoded_frame->mm_time, now);
+stream_dropped_frame_on_playback(decoder->base.stream);
+decoder->display_frame = NULL;
+free_gst_frame(gstframe);
+}
+}
+
+g_mutex_unlock(>queues_mutex);
+}
+
/* GStreamer thread
*
* We cannot use GStreamer's signals because they are not always run in
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [client v2 00/12] Client-side video stream lag reduction
The first few patches in this series are the same as those I sent previously. But the series main goal is to reduce the video stream lag. Currently the server sets the client's mmtime offset which is the source of the video stream lag. But as was discussed before this offset is entangled with the available network bandwidth estimations as well as with the actual video bitrate which makes it tricky (although mostly doable) to reduce it. Furthermore the server is 5 frame intervals and a network roundtrip away which delays any action it can take. In contrast the client knows immediately when a frame is late and has all the information it needs to figure out how much buffering is needed. So this is the approach taken in this patchset. See patch 7 and up for more details. Francois Gouget (12): gstreamer: Avoid a couple of forward function declarations gstreamer: Fix the spice_gst_decoder_queue_frame() documentation gstreamer: Improve the statistics collection gstreamer: Add the encoded frame's rank to the statistics channel-display: The video latency is in fact a margin channel-display: Rename the frame mmtime variables channel-display: Minimize the stream lag by ignoring the server one playback: Use the audio timestamps for the global mmtime conversion channel-display: No need to rechedule on mmtime offset changes channel-display: Remove playback_sync_drops_seq_len spice-session: Keep track of the global streams lag mjpeg: Take the decoding time into account to display frames src/channel-display-gst.c | 304 +++- src/channel-display-mjpeg.c | 40 +++-- src/channel-display-priv.h | 27 +++- src/channel-display.c | 304 src/channel-display.h | 2 + src/channel-playback-priv.h | 4 +- src/channel-playback.c | 32 ++-- src/spice-session-priv.h| 8 +- src/spice-session.c | 103 ++-- 9 files changed, 525 insertions(+), 299 deletions(-) -- 2.20.1 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [spice v3] utils: Remove the LL suffix from NSEC_PER_SEC
This constant fits in a 32 bit signed integer so it does not need the
suffix. However some of the derived constants don't so use an uint64_t
cast to avoid the long vs long long confusion (such as in print
statements).
Also some of the expressions these constants are used in may overflow so
perform the appropriate casts in those locations. This makes it clearer
that these operations are 64 bit.
Signed-off-by: Francois Gouget
---
v3: Turned all the timeout constants with casts to int64_t.
server/common-graphics-channel.h | 2 +-
server/dcc.h | 2 +-
server/gstreamer-encoder.c | 2 +-
server/mjpeg-encoder.c | 2 +-
server/utils.h | 4 ++--
server/video-stream.c| 4 ++--
server/video-stream.h| 2 +-
7 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/server/common-graphics-channel.h b/server/common-graphics-channel.h
index d23f0c695..cccd24d44 100644
--- a/server/common-graphics-channel.h
+++ b/server/common-graphics-channel.h
@@ -27,7 +27,7 @@ G_BEGIN_DECLS
bool common_channel_client_config_socket(RedChannelClient *rcc);
-#define COMMON_CLIENT_TIMEOUT (NSEC_PER_SEC * 30)
+#define COMMON_CLIENT_TIMEOUT (((int64_t)NSEC_PER_SEC) * 30)
#define TYPE_COMMON_GRAPHICS_CHANNEL common_graphics_channel_get_type()
diff --git a/server/dcc.h b/server/dcc.h
index 76c078bf5..c35824d54 100644
--- a/server/dcc.h
+++ b/server/dcc.h
@@ -66,7 +66,7 @@ GType display_channel_client_get_type(void) G_GNUC_CONST;
#define PALETTE_CACHE_HASH_KEY(id) ((id) & PALETTE_CACHE_HASH_MASK)
#define CLIENT_PALETTE_CACHE_SIZE 128
-#define DISPLAY_CLIENT_MIGRATE_DATA_TIMEOUT (NSEC_PER_SEC * 10)
+#define DISPLAY_CLIENT_MIGRATE_DATA_TIMEOUT (((int64_t)NSEC_PER_SEC) * 10)
#define DISPLAY_CLIENT_RETRY_INTERVAL 1 //micro
/* Each drawable can refer to at most 3 images: src, brush and mask */
diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
index 6416b688d..f2f1cf61e 100644
--- a/server/gstreamer-encoder.c
+++ b/server/gstreamer-encoder.c
@@ -563,7 +563,7 @@ static void update_next_frame_mm_time(SpiceGstEncoder
*encoder)
/* Figure out how many frames to drop to not exceed the current bit rate.
* Use nanoseconds to avoid precision loss.
*/
-uint64_t delay_ns = -encoder->vbuffer_free * 8 * NSEC_PER_SEC /
encoder->bit_rate;
+uint64_t delay_ns = ((uint64_t)-encoder->vbuffer_free) * 8 * NSEC_PER_SEC
/ encoder->bit_rate;
uint32_t drops = (delay_ns + period_ns - 1) / period_ns; /* round up */
spice_debug("drops=%u vbuffer %d/%d", drops, encoder->vbuffer_free,
encoder->vbuffer_size);
diff --git a/server/mjpeg-encoder.c b/server/mjpeg-encoder.c
index 4a02e7c8b..56381b1c7 100644
--- a/server/mjpeg-encoder.c
+++ b/server/mjpeg-encoder.c
@@ -65,7 +65,7 @@ static const int
mjpeg_quality_samples[MJPEG_QUALITY_SAMPLE_NUM] = {20, 30, 40,
* are not necessarily related to mis-estimation of the bit rate, and we would
* like to wait till the stream stabilizes.
*/
-#define MJPEG_WARMUP_TIME (NSEC_PER_SEC * 3)
+#define MJPEG_WARMUP_TIME (((int64_t)NSEC_PER_SEC) * 3)
/* The compressed buffer initial size. */
#define MJPEG_INITIAL_BUFFER_SIZE (32 * 1024)
diff --git a/server/utils.h b/server/utils.h
index 54bc9d490..4bbd45dff 100644
--- a/server/utils.h
+++ b/server/utils.h
@@ -52,7 +52,7 @@ static inline int test_bit(int index, uint32_t val)
typedef int64_t red_time_t;
-#define NSEC_PER_SEC 10LL
+#define NSEC_PER_SEC 10
#define NSEC_PER_MILLISEC 100
#define NSEC_PER_MICROSEC 1000
@@ -62,7 +62,7 @@ static inline red_time_t spice_get_monotonic_time_ns(void)
struct timespec time;
clock_gettime(CLOCK_MONOTONIC, );
-return NSEC_PER_SEC * time.tv_sec + time.tv_nsec;
+return ((uint64_t)NSEC_PER_SEC) * time.tv_sec + time.tv_nsec;
}
#define MSEC_PER_SEC 1000
diff --git a/server/video-stream.c b/server/video-stream.c
index 4ac25af8b..7a2dca7dd 100644
--- a/server/video-stream.c
+++ b/server/video-stream.c
@@ -415,8 +415,8 @@ static void display_channel_create_stream(DisplayChannel
*display, Drawable *dra
* the nearest integer, for instance 24 for 23.976.
*/
uint64_t duration = drawable->creation_time - drawable->first_frame_time;
-if (duration > NSEC_PER_SEC * drawable->frames_count / MAX_FPS) {
-stream->input_fps = (NSEC_PER_SEC * drawable->frames_count + duration
/ 2) / duration;
+if (duration > ((uint64_t)NSEC_PER_SEC) * drawable->frames_count /
MAX_FPS) {
+stream->input_fps = (((uint64_t)NSEC_PER_SEC) * drawable->frames_count
+ duration / 2) / duration;
} else {
stream->input_fps = MAX_FPS;
}
diff --git a/server/video-stream.h b/server/video-stream.h
index 46b076fd7..cbd347b8a 100644
--- a/server/video-stream.h
+++ b/server/video-stream.h
@@ -34,7 +34,7 @@
#define RED_STREAM_GRADUAL_FRAMES_S
Re: [Spice-devel] [client 1/5] gstreamer: Avoid direct access to GQueue fields
On Fri, 14 Jun 2019, Frediano Ziglio wrote: > > > > Signed-off-by: Francois Gouget > > Considering that the field is public and that code will get > slower and bigger at least would be good to describe the reason > why you consider it better. Consistency mostly. This is the only place where we access the GQueue fields. In other places we use g_queue_peek_head_link() instead of decoding_queue->head for instance. But I'm fine with keeping decoding_queue->length if that's preferred. -- Francois Gouget ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [client 4/5] gstreamer: Improve the statistics collection
By the time schedule_frame() pulls a sample off GStreamer's pipeline the
frame may have been sitting in the sink's queue for hundreds of
milliseconds, making the decoding time meaningless.
This patch ensures the statistics are always collected by
sink_event_probe() which is called as soon as the decoded frame is
available.
Note that even so the decoding time may be overestimated as the frame
may have been waiting for a while in encoded form for a spot to be freed
in the GStreamer pipeline's sink queue.
Signed-off-by: Francois Gouget
---
src/channel-display-gst.c | 105 +-
1 file changed, 59 insertions(+), 46 deletions(-)
diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index b5b8d51a..fc338dff 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -1,6 +1,6 @@
/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
/*
- Copyright (C) 2015-2016 CodeWeavers, Inc
+ Copyright (C) 2015-2016, 2019 CodeWeavers, Inc
This library is free software; you can redistribute it and/or
modify it under the terms of the GNU Lesser General Public
@@ -185,13 +185,16 @@ static gboolean display_frame(gpointer video_decoder)
return G_SOURCE_REMOVE;
}
-/* Get the decoded frame relative to buffer or NULL if not found.
- * Dequeue the frame from decoding_queue and return it, caller
- * is responsible to free the pointer.
+/* Returns the decoding queue entry that matches the specified GStreamer
buffer.
+ *
+ * The entry is identified based on the buffer timestamp. However sometimes
+ * GStreamer returns the same buffer twice (and the second time the entry may
+ * have been removed already) or buffers that have a modified, and thus
+ * unrecognizable, timestamp. In such cases NULL is returned.
*
* queues_mutex must be held.
*/
-static SpiceGstFrame *get_decoded_frame(SpiceGstDecoder *decoder, GstBuffer
*buffer)
+static GList *find_frame_entry(SpiceGstDecoder *decoder, GstBuffer *buffer)
{
GstClockTime buffer_ts = GST_BUFFER_PTS(buffer);
#if GST_CHECK_VERSION(1,14,0)
@@ -203,49 +206,34 @@ static SpiceGstFrame *get_decoded_frame(SpiceGstDecoder
*decoder, GstBuffer *buf
}
#endif
-/* Gstreamer sometimes returns the same buffer twice
- * or buffers that have a modified, and thus unrecognizable, PTS.
- * Blindly removing frames from the decoding_queue until we find a
- * match would only empty the queue, resulting in later buffers not
- * finding a match either, etc. So check the buffer has a matching
- * frame first.
- */
-SpiceGstFrame *gstframe = NULL;
GList *l = g_queue_peek_head_link(decoder->decoding_queue);
while (l) {
-gstframe = l->data;
+const SpiceGstFrame *gstframe = l->data;
if (gstframe->timestamp == buffer_ts) {
-break;
+return l;
}
-gstframe = NULL;
l = l->next;
}
-if (gstframe != NULL) {
-/* Now that we know there is a match, remove it and the older
- * frames from the decoding queue */
-SpiceGstFrame *late_frame;
-guint num_frames_dropped = 0;
-
-/* The GStreamer pipeline dropped the corresponding buffer. */
-while ((late_frame = g_queue_pop_head(decoder->decoding_queue)) !=
gstframe) {
-num_frames_dropped++;
-free_gst_frame(late_frame);
-}
+return NULL;
+}
-if (num_frames_dropped != 0) {
-SPICE_DEBUG("the GStreamer pipeline dropped %u frames",
num_frames_dropped);
-}
+/* Pops the queued frames up to and including the specified frame.
+ * All frames are freed except that last frame which belongs to the caller.
+ * Returns the number of freed frames.
+ *
+ * queues_mutex must be held.
+ */
+static guint32 pop_up_to_frame(SpiceGstDecoder *decoder, const SpiceGstFrame
*popframe)
+{
+SpiceGstFrame *gstframe;
+guint32 freed = 0;
-const SpiceFrame *frame = gstframe->encoded_frame;
-int64_t duration = g_get_monotonic_time() - frame->creation_time;
-record(frames_stats,
- "frame mm_time %u size %u creation time %" PRId64
- " decoded time %" PRId64 " queue %u",
- frame->mm_time, frame->size, frame->creation_time,
- duration, g_queue_get_length(decoder->decoding_queue));
+while ((gstframe = g_queue_pop_head(decoder->decoding_queue)) != popframe)
{
+free_gst_frame(gstframe);
+freed++;
}
-return gstframe;
+return freed;
}
/* Helper for schedule_frame().
@@ -268,8 +256,16 @@ static void fetch_pending_sample(SpiceGstDecoder *decoder)
* finding a match either, etc. So check the buffer has a matching
* frame first.
*/
-SpiceGstFrame *gstframe = get_decoded_frame(decoder, buffer);
-if (gstframe) {
+GList *l =
[Spice-devel] [client 5/5] gstreamer: Add the encoded frame's rank to the statistics
The number of frames that were sitting in the decoding_queue before the
current frame was added is crucial to correctly interpret the decoding
time:
* Less than MAX_DECODED_FRAMES means nothing blocked the decoding of
that frame.
* More than MAX_DECODED_FRAMES means decoding was delayed by one or more
frame intervals.
Signed-off-by: Francois Gouget
---
src/channel-display-gst.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index fc338dff..b8f0c2ee 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -90,6 +90,7 @@ struct SpiceGstFrame {
GstBuffer *encoded_buffer;
SpiceFrame *encoded_frame;
GstSample *decoded_sample;
+guint rank;
};
static SpiceGstFrame *create_gst_frame(GstBuffer *buffer, SpiceFrame *frame)
@@ -442,9 +443,9 @@ sink_event_probe(GstPad *pad, GstPadProbeInfo *info,
gpointer data)
int64_t duration = g_get_monotonic_time() - frame->creation_time;
record(frames_stats,
"frame mm_time %u size %u creation time %" PRId64
- " decoded time %" PRId64 " queue %u",
+ " decoded time %" PRId64 " rank %u queue %u",
frame->mm_time, frame->size, frame->creation_time, duration,
- g_queue_get_length(decoder->decoding_queue));
+ gstframe->rank,
g_queue_get_length(decoder->decoding_queue));
if (!decoder->appsink) {
/* The sink will display the frame directly so this
@@ -729,6 +730,7 @@ static gboolean spice_gst_decoder_queue_frame(VideoDecoder
*video_decoder,
SpiceGstFrame *gst_frame = create_gst_frame(buffer, frame);
g_mutex_lock(>queues_mutex);
+gst_frame->rank = g_queue_get_length(decoder->decoding_queue);
g_queue_push_tail(decoder->decoding_queue, gst_frame);
g_mutex_unlock(>queues_mutex);
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [client 2/5] gstreamer: Avoid a couple of forward function declarations
Signed-off-by: Francois Gouget
---
src/channel-display-gst.c | 81 ---
1 file changed, 42 insertions(+), 39 deletions(-)
diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index c756f916..50f29060 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -120,8 +120,6 @@ static void free_gst_frame(SpiceGstFrame *gstframe)
/* -- GStreamer pipeline -- */
static void schedule_frame(SpiceGstDecoder *decoder);
-static void fetch_pending_sample(SpiceGstDecoder *decoder);
-static SpiceGstFrame *get_decoded_frame(SpiceGstDecoder *decoder, GstBuffer
*buffer);
RECORDER(frames_stats, 64, "Frames statistics");
@@ -184,46 +182,10 @@ static gboolean display_frame(gpointer video_decoder)
return G_SOURCE_REMOVE;
}
-/* main loop or GStreamer streaming thread */
-static void schedule_frame(SpiceGstDecoder *decoder)
-{
-guint32 now = stream_get_time(decoder->base.stream);
-g_mutex_lock(>queues_mutex);
-
-while (!decoder->timer_id) {
-while (decoder->display_frame == NULL && decoder->pending_samples) {
-fetch_pending_sample(decoder);
-}
-
-SpiceGstFrame *gstframe = decoder->display_frame;
-if (!gstframe) {
-break;
-}
-
-if (spice_mmtime_diff(gstframe->encoded_frame->mm_time, now) >= 0) {
-decoder->timer_id = g_timeout_add(gstframe->encoded_frame->mm_time
- now,
- display_frame, decoder);
-} else if (decoder->display_frame && !decoder->pending_samples) {
-/* Still attempt to display the least out of date frame so the
- * video is not completely frozen for an extended period of time.
- */
-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->encoded_frame->mm_time,
-gstframe->encoded_frame->mm_time, now);
-stream_dropped_frame_on_playback(decoder->base.stream);
-decoder->display_frame = NULL;
-free_gst_frame(gstframe);
-}
-}
-
-g_mutex_unlock(>queues_mutex);
-}
-
/* Get the decoded frame relative to buffer or NULL if not found.
* Dequeue the frame from decoding_queue and return it, caller
* is responsible to free the pointer.
+ *
* queues_mutex must be held.
*/
static SpiceGstFrame *get_decoded_frame(SpiceGstDecoder *decoder, GstBuffer
*buffer)
@@ -283,6 +245,10 @@ static SpiceGstFrame *get_decoded_frame(SpiceGstDecoder
*decoder, GstBuffer *buf
return gstframe;
}
+/* Helper for schedule_frame().
+ *
+ * queues_mutex must be held.
+ */
static void fetch_pending_sample(SpiceGstDecoder *decoder)
{
GstSample *sample = gst_app_sink_pull_sample(decoder->appsink);
@@ -315,6 +281,43 @@ static void fetch_pending_sample(SpiceGstDecoder *decoder)
}
}
+/* main loop or GStreamer streaming thread */
+static void schedule_frame(SpiceGstDecoder *decoder)
+{
+guint32 now = stream_get_time(decoder->base.stream);
+g_mutex_lock(>queues_mutex);
+
+while (!decoder->timer_id) {
+while (decoder->display_frame == NULL && decoder->pending_samples) {
+fetch_pending_sample(decoder);
+}
+
+SpiceGstFrame *gstframe = decoder->display_frame;
+if (!gstframe) {
+break;
+}
+
+if (spice_mmtime_diff(gstframe->encoded_frame->mm_time, now) >= 0) {
+decoder->timer_id = g_timeout_add(gstframe->encoded_frame->mm_time
- now,
+ display_frame, decoder);
+} else if (decoder->display_frame && !decoder->pending_samples) {
+/* Still attempt to display the least out of date frame so the
+ * video is not completely frozen for an extended period of time.
+ */
+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->encoded_frame->mm_time,
+gstframe->encoded_frame->mm_time, now);
+stream_dropped_frame_on_playback(decoder->base.stream);
+decoder->display_frame = NULL;
+free_gst_frame(gstframe);
+}
+}
+
+g_mutex_unlock(>queues_mutex);
+}
+
/* GStreamer thread
*
* We cannot use GStreamer's signals because they are not always run in
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [client 3/5] gstreamer: Fix the spice_gst_decoder_queue_frame() documentation
Take into account changes made in 8835e757922c, 8c2101254051 and
possibly other other commits.
Add MAX_DECODED_FRAMES to define how many decoded frames GStreamer
should queue.
Signed-off-by: Francois Gouget
---
src/channel-display-gst.c | 63 +++
1 file changed, 38 insertions(+), 25 deletions(-)
diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 50f29060..b5b8d51a 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -62,6 +62,9 @@ typedef struct SpiceGstDecoder {
#define VALID_VIDEO_CODEC_TYPE(codec) \
(codec > 0 && codec < G_N_ELEMENTS(gst_opts))
+/* Decoded frames are big so limit how many are queued by GStreamer */
+#define MAX_DECODED_FRAMES 2
+
/* GstPlayFlags enum is in plugin's header which should not be exported.
* https://bugzilla.gnome.org/show_bug.cgi?id=784279
*/
@@ -320,11 +323,14 @@ static void schedule_frame(SpiceGstDecoder *decoder)
/* GStreamer thread
*
- * We cannot use GStreamer's signals because they are not always run in
- * the main context. So use a callback (lower overhead) and have it pull
- * the sample to avoid a race with free_pipeline(). This means queuing the
- * decoded frames outside GStreamer. So while we're at it, also schedule
- * the frame display ourselves in schedule_frame().
+ * Decoded frames are big so we rely on GStreamer to limit how many are
+ * buffered (see MAX_DECODED_FRAMES). This means we must not pull the samples
+ * as soon as they become available. Instead just increment pending_samples so
+ * schedule_frame() knows whether it can pull a new sample when it needs one.
+ *
+ * Note that GStreamer's signals are not always run in the main context, hence
+ * the schedule_frame() + display_frame() mechanism. So we might as well use
+ * a callback here (lower overhead).
*/
static GstFlowReturn new_sample(GstAppSink *gstappsink, gpointer video_decoder)
{
@@ -535,7 +541,7 @@ static gboolean create_pipeline(SpiceGstDecoder *decoder)
GstAppSinkCallbacks appsink_cbs = { NULL };
appsink_cbs.new_sample = new_sample;
gst_app_sink_set_callbacks(decoder->appsink, _cbs, decoder,
NULL);
-gst_app_sink_set_max_buffers(decoder->appsink, 2);
+gst_app_sink_set_max_buffers(decoder->appsink, MAX_DECODED_FRAMES);
gst_app_sink_set_drop(decoder->appsink, FALSE);
}
bus = gst_pipeline_get_bus(GST_PIPELINE(decoder->pipeline));
@@ -612,37 +618,44 @@ static void spice_gst_decoder_destroy(VideoDecoder
*video_decoder)
*
* 1) frame->data, which contains the compressed frame data, is wrapped in a
GstBuffer
*(encoded_buffer) which owns the SpiceFrame.
- * 2) A SpiceGstFrame is created to keep track of SpiceFrame (encoded_frame),
and some
- *additional metadata. The encoded_buffer is referenced and the
SpiceGstFrame is then
- *pushed into the decoding_queue.
+ * 2) A SpiceGstFrame is created to keep track of SpiceFrame (encoded_frame),
+ *and additional metadata among which GStreamer's encoded_buffer the
+ *refcount of which is incremented. The SpiceGstFrame is then pushed into
+ *the decoding_queue.
*
* If GstVideoOverlay is used (window handle was obtained successfully at the
widget):
* 3) Decompressed frames will be rendered to widget directly from
GStreamer's pipeline
* using some GStreamer sink plugin which implements the GstVideoOverlay
interface
* (last step).
* 4) As soon as GStreamer's pipeline no longer needs the compressed frame
it will
- * unref the encoded_buffer
- * 5) Once a decoded buffer arrives to the sink (notified by probe event) we
will pop
+ * unref the encoded_buffer.
+ * 5) Once a decoded buffer arrives to the sink sink_event_probe() will pop
* its matching SpiceGstFrame from the decoding_queue and free it using
- * free_gst_frame, this will also unref the encoded_buffer which will
allow
- * GStreamer to call spice_frame_free and free its encoded_frame.
+ * free_gst_frame(). This will also unref the encoded_buffer which will
+ * allow GStreamer to call spice_frame_free() and free its encoded_frame.
*
* Otherwise appsink is used:
* 3) Once the decompressed frame is available the GStreamer pipeline calls
* new_sample() in the GStreamer thread.
- * 4) new_sample() then matches the decompressed frame to a SpiceGstFrame
from
- * the decoding queue using the GStreamer timestamp information to deal
with
- * dropped frames. The SpiceGstFrame is popped from the decoding_queue.
- * 5) new_sample() then attaches the decompressed frame to the SpiceGstFrame,
- * set into display_frame and calls schedule_frame().
- * 6) schedule_frame() then uses gstframe->encoded_frame->mm_time to arrange
for
- * display_frame() to be called, in the main thread, at the right time for
- * the next frame.
- * 7) display_frame() uses Spic
[Spice-devel] [client 1/5] gstreamer: Avoid direct access to GQueue fields
Signed-off-by: Francois Gouget --- src/channel-display-gst.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c index 91ece0fa..c756f916 100644 --- a/src/channel-display-gst.c +++ b/src/channel-display-gst.c @@ -278,7 +278,7 @@ static SpiceGstFrame *get_decoded_frame(SpiceGstDecoder *decoder, GstBuffer *buf "frame mm_time %u size %u creation time %" PRId64 " decoded time %" PRId64 " queue %u", frame->mm_time, frame->size, frame->creation_time, - duration, decoder->decoding_queue->length); + duration, g_queue_get_length(decoder->decoding_queue)); } return gstframe; } -- 2.20.1 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [client] gstreamer: Fix the decoding time when not using GstVideoOverlay
On Wed, 12 Jun 2019, Snir Sheriber wrote:
> Hi,
>
> On 6/11/19 9:42 PM, Francois Gouget wrote:
> > schedule_frame() only pulls frames out of GStreamer's pipeline once all
> > previous decoded frames have been displayed. Thus when the video delay
>
>
> IIRC we used to pull when samples arrived but it was changed to this so
> pending frames will be queued
> inside gstreamer and let it do throttling (or something similar)
I see. For reference that was commit 8835e757922c.
That's going to make it harder to get the correct decoding time :-(
> > +static gboolean attach_decoded_sample(SpiceGstDecoder *decoder, GstSample
> *sample)
> > {
> > -GstSample *sample = gst_app_sink_pull_sample(decoder->appsink);
> > -if (sample) {
> > -// account for the fetched sample
> > -decoder->pending_samples--;
> > +GstBuffer *buffer = gst_sample_get_buffer(sample);
> > +GList *l = find_decoded_entry(decoder, buffer);
> > +if (l == NULL) {
> > +return FALSE;
>
>
> Is it possible to have a sample with no matching entry? how this sample
> is unrefed in that case?
There's a gst_sample_unref(sample) missing somewhere.
[...]
> > @@ -429,10 +429,19 @@ sink_event_probe(GstPad *pad, GstPadProbeInfo *info,
> > gpointer data)
[...]
> > +/* As a side-effect this updates the decoder statistics */
> > +GList *l = find_decoded_entry(decoder, buffer);
> > +
> > +/* Drop all entries up to this one */
> > +while (l) {
> > +free_gst_frame((SpiceGstFrame*)l->data);
> > +
> > +GList *p = l->prev;
> > +g_queue_delete_link(decoder->decoding_queue, l);
> > +l = p;
> >
>
>
> Isn't it done on attach_decoded_sample?
sink_event_probe() cannot call attach_decoded_sample() because all it
has is a buffer. This is why it calls find_decoded_entry() instead which
does not remove older entries.
> Also would be nice to update the comment above spice_gst_decoder_queue_frame
> with the current flow, will make it easier to follow.
Ok.
--
Francois Gouget
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [spice v2 2/2] utils: Remove the LL suffix from NSEC_PER_SEC
This constant fits in a 32 bit signed integer so it does not need the
suffix. However some of the derived constants don't so use an uint64_t
cast to avoid the long vs long long confusion (such as in print
statements).
Also some of the expressions these constants are used in may overflow so
perform the appropriate casts in those locations. This makes it clearer
that these operations are 64 bit.
Signed-off-by: Francois Gouget
---
v2: Cast COMMON_CLIENT_TIMEOUT to an int64_t instead of an uint64_t.
v1: https://lists.freedesktop.org/archives/spice-devel/2019-May/049193.html
server/common-graphics-channel.h | 2 +-
server/dcc.h | 2 +-
server/gstreamer-encoder.c | 2 +-
server/mjpeg-encoder.c | 2 +-
server/utils.h | 4 ++--
server/video-stream.c| 4 ++--
server/video-stream.h| 2 +-
7 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/server/common-graphics-channel.h b/server/common-graphics-channel.h
index d23f0c695..cccd24d44 100644
--- a/server/common-graphics-channel.h
+++ b/server/common-graphics-channel.h
@@ -27,7 +27,7 @@ G_BEGIN_DECLS
bool common_channel_client_config_socket(RedChannelClient *rcc);
-#define COMMON_CLIENT_TIMEOUT (NSEC_PER_SEC * 30)
+#define COMMON_CLIENT_TIMEOUT (((int64_t)NSEC_PER_SEC) * 30)
#define TYPE_COMMON_GRAPHICS_CHANNEL common_graphics_channel_get_type()
diff --git a/server/dcc.h b/server/dcc.h
index 76c078bf5..da8697762 100644
--- a/server/dcc.h
+++ b/server/dcc.h
@@ -66,7 +66,7 @@ GType display_channel_client_get_type(void) G_GNUC_CONST;
#define PALETTE_CACHE_HASH_KEY(id) ((id) & PALETTE_CACHE_HASH_MASK)
#define CLIENT_PALETTE_CACHE_SIZE 128
-#define DISPLAY_CLIENT_MIGRATE_DATA_TIMEOUT (NSEC_PER_SEC * 10)
+#define DISPLAY_CLIENT_MIGRATE_DATA_TIMEOUT (((uint64_t)NSEC_PER_SEC) * 10)
#define DISPLAY_CLIENT_RETRY_INTERVAL 1 //micro
/* Each drawable can refer to at most 3 images: src, brush and mask */
diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
index 5f39ed877..91dd475ac 100644
--- a/server/gstreamer-encoder.c
+++ b/server/gstreamer-encoder.c
@@ -556,7 +556,7 @@ static void update_next_frame_mm_time(SpiceGstEncoder
*encoder)
/* Figure out how many frames to drop to not exceed the current bit rate.
* Use nanoseconds to avoid precision loss.
*/
-uint64_t delay_ns = -encoder->vbuffer_free * 8 * NSEC_PER_SEC /
encoder->bit_rate;
+uint64_t delay_ns = ((uint64_t)-encoder->vbuffer_free) * 8 * NSEC_PER_SEC
/ encoder->bit_rate;
uint32_t drops = (delay_ns + period_ns - 1) / period_ns; /* round up */
spice_debug("drops=%u vbuffer %d/%d", drops, encoder->vbuffer_free,
encoder->vbuffer_size);
diff --git a/server/mjpeg-encoder.c b/server/mjpeg-encoder.c
index 4a02e7c8b..d22e7d18b 100644
--- a/server/mjpeg-encoder.c
+++ b/server/mjpeg-encoder.c
@@ -65,7 +65,7 @@ static const int
mjpeg_quality_samples[MJPEG_QUALITY_SAMPLE_NUM] = {20, 30, 40,
* are not necessarily related to mis-estimation of the bit rate, and we would
* like to wait till the stream stabilizes.
*/
-#define MJPEG_WARMUP_TIME (NSEC_PER_SEC * 3)
+#define MJPEG_WARMUP_TIME (((uint64_t)NSEC_PER_SEC) * 3)
/* The compressed buffer initial size. */
#define MJPEG_INITIAL_BUFFER_SIZE (32 * 1024)
diff --git a/server/utils.h b/server/utils.h
index 54bc9d490..4bbd45dff 100644
--- a/server/utils.h
+++ b/server/utils.h
@@ -52,7 +52,7 @@ static inline int test_bit(int index, uint32_t val)
typedef int64_t red_time_t;
-#define NSEC_PER_SEC 10LL
+#define NSEC_PER_SEC 10
#define NSEC_PER_MILLISEC 100
#define NSEC_PER_MICROSEC 1000
@@ -62,7 +62,7 @@ static inline red_time_t spice_get_monotonic_time_ns(void)
struct timespec time;
clock_gettime(CLOCK_MONOTONIC, );
-return NSEC_PER_SEC * time.tv_sec + time.tv_nsec;
+return ((uint64_t)NSEC_PER_SEC) * time.tv_sec + time.tv_nsec;
}
#define MSEC_PER_SEC 1000
diff --git a/server/video-stream.c b/server/video-stream.c
index 4ac25af8b..7a2dca7dd 100644
--- a/server/video-stream.c
+++ b/server/video-stream.c
@@ -415,8 +415,8 @@ static void display_channel_create_stream(DisplayChannel
*display, Drawable *dra
* the nearest integer, for instance 24 for 23.976.
*/
uint64_t duration = drawable->creation_time - drawable->first_frame_time;
-if (duration > NSEC_PER_SEC * drawable->frames_count / MAX_FPS) {
-stream->input_fps = (NSEC_PER_SEC * drawable->frames_count + duration
/ 2) / duration;
+if (duration > ((uint64_t)NSEC_PER_SEC) * drawable->frames_count /
MAX_FPS) {
+stream->input_fps = (((uint64_t)NSEC_PER_SEC) * drawable->frames_count
+ duration / 2) / duration;
} else {
stream->input_fps = MAX_FPS;
}
diff --git a/server/video-stream.h b/server/video-stream.h
index 46b076fd7..6c18d00f7 100644
--- a/server/vide
[Spice-devel] [spice v2 1/2] utils: Remove the LL suffix from NSEC_PER_MILLISEC
This constant fits in a regular 32 bit signed integer so it does not need the suffix. It is also not used in expressions that may overflow. Signed-off-by: Francois Gouget --- v2: No changes. v1: https://lists.freedesktop.org/archives/spice-devel/2019-May/049193.html server/utils.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/utils.h b/server/utils.h index ea0de1529..54bc9d490 100644 --- a/server/utils.h +++ b/server/utils.h @@ -53,7 +53,7 @@ static inline int test_bit(int index, uint32_t val) typedef int64_t red_time_t; #define NSEC_PER_SEC 10LL -#define NSEC_PER_MILLISEC 100LL +#define NSEC_PER_MILLISEC 100 #define NSEC_PER_MICROSEC 1000 /* g_get_monotonic_time() does not have enough precision */ -- 2.20.1 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [client] gstreamer: Fix the decoding time when not using GstVideoOverlay
schedule_frame() only pulls frames out of GStreamer's pipeline once all
previous decoded frames have been displayed. Thus when the video delay
is high a decoded frame may have to wait for several frame intervals
before get_decoded_frame() looks at it and computes its decoding time.
So attach decoded frame samples to the corresponding decoding_queue
entry as soon as new_sample() is called, and compute the decoding time
and associated statistics then. Similarly, match sink_event_probe()'s
new buffer to a decoding_queue entry and update the statistics in the
process. This ensures that there is no extra delay in either case.
Signed-off-by: Francois Gouget
---
src/channel-display-gst.c | 183 --
1 file changed, 96 insertions(+), 87 deletions(-)
diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 91ece0fa..fed73592 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -1,6 +1,6 @@
/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
/*
- Copyright (C) 2015-2016 CodeWeavers, Inc
+ Copyright (C) 2015-2016, 2019 CodeWeavers, Inc
This library is free software; you can redistribute it and/or
modify it under the terms of the GNU Lesser General Public
@@ -54,9 +54,9 @@ typedef struct SpiceGstDecoder {
GMutex queues_mutex;
GQueue *decoding_queue;
+guint decoded_frames;
SpiceGstFrame *display_frame;
guint timer_id;
-guint pending_samples;
} SpiceGstDecoder;
#define VALID_VIDEO_CODEC_TYPE(codec) \
@@ -120,8 +120,6 @@ static void free_gst_frame(SpiceGstFrame *gstframe)
/* -- GStreamer pipeline -- */
static void schedule_frame(SpiceGstDecoder *decoder);
-static void fetch_pending_sample(SpiceGstDecoder *decoder);
-static SpiceGstFrame *get_decoded_frame(SpiceGstDecoder *decoder, GstBuffer
*buffer);
RECORDER(frames_stats, 64, "Frames statistics");
@@ -191,21 +189,26 @@ static void schedule_frame(SpiceGstDecoder *decoder)
g_mutex_lock(>queues_mutex);
while (!decoder->timer_id) {
-while (decoder->display_frame == NULL && decoder->pending_samples) {
-fetch_pending_sample(decoder);
+GList *head = g_queue_peek_head_link(decoder->decoding_queue);
+if (!head) {
+/* No frame in the queue */
+break;
}
-
-SpiceGstFrame *gstframe = decoder->display_frame;
-if (!gstframe) {
+SpiceGstFrame *gstframe = head->data;
+if (!gstframe->decoded_sample) {
+/* This frame has not been decoded yet */
break;
}
+g_queue_pop_head(decoder->decoding_queue);
+decoder->display_frame = gstframe;
+decoder->decoded_frames--;
if (spice_mmtime_diff(gstframe->encoded_frame->mm_time, now) >= 0) {
decoder->timer_id = g_timeout_add(gstframe->encoded_frame->mm_time
- now,
display_frame, decoder);
-} else if (decoder->display_frame && !decoder->pending_samples) {
-/* Still attempt to display the least out of date frame so the
- * video is not completely frozen for an extended period of time.
+} else if (decoder->display_frame && decoder->decoded_frames == 0) {
+/* This is the least out of date frame. Display it since that's
+ * better than having frozen video for an extended period of time.
*/
decoder->timer_id = g_timeout_add(0, display_frame, decoder);
} else {
@@ -221,12 +224,16 @@ static void schedule_frame(SpiceGstDecoder *decoder)
g_mutex_unlock(>queues_mutex);
}
-/* Get the decoded frame relative to buffer or NULL if not found.
- * Dequeue the frame from decoding_queue and return it, caller
- * is responsible to free the pointer.
+/* Returns the decoding queue entry that matches the specified GStreamer
buffer.
+ *
+ * The entry is identified based on the buffer timestamp. However sometimes
+ * GStreamer returns the same buffer twice (and the second time the entry may
+ * have been removed already) or buffers that have a modified, and thus
+ * unrecognizable, timestamp. In such cases NULL is returned.
+ *
* queues_mutex must be held.
*/
-static SpiceGstFrame *get_decoded_frame(SpiceGstDecoder *decoder, GstBuffer
*buffer)
+static GList *find_decoded_entry(SpiceGstDecoder *decoder, GstBuffer *buffer)
{
GstClockTime buffer_ts = GST_BUFFER_PTS(buffer);
#if GST_CHECK_VERSION(1,14,0)
@@ -238,81 +245,71 @@ static SpiceGstFrame *get_decoded_frame(SpiceGstDecoder
*decoder, GstBuffer *buf
}
#endif
-/* Gstreamer sometimes returns the same buffer twice
- * or buffers that have a modified, and thus unrecognizable, PTS.
- * Blindly removing frames from the decoding_queue until we find a
- * match would only empty the queue,
Re: [Spice-devel] [client] gstreamer: A frame is not late if it should be displayed immediately
On Tue, 11 Jun 2019, Victor Toso wrote: [...] > > Thankfully the frame would usually be the last decoded frame > > and would thus be displayed anyway. > > Have you found while reading the code or some other way? Just > curious. I'm working on the client-side stream lag and I occasionally had the "rendering too late" trace which did not seem right. > > Also reverse the inequality to make it easier to understand. > > That's relative but I don't mind ;) Sure but it looks more like "frame->time >= now" so I like it better. I'm not dead set on one for or another though. -- Francois Gouget ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [client] gstreamer: A frame is not late if it should be displayed immediately
schedule_frame() was expecting to be called at least 1 ms before having
to display the frame which is wrong: being called 0 ms before is
acceptable too. Thankfully the frame would usually be the last decoded
frame and would thus be displayed anyway.
Also reverse the inequality to make it easier to understand.
Signed-off-by: Francois Gouget
---
src/channel-display-gst.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 858e8aeb..91ece0fa 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -200,7 +200,7 @@ static void schedule_frame(SpiceGstDecoder *decoder)
break;
}
-if (spice_mmtime_diff(now, gstframe->encoded_frame->mm_time) < 0) {
+if (spice_mmtime_diff(gstframe->encoded_frame->mm_time, now) >= 0) {
decoder->timer_id = g_timeout_add(gstframe->encoded_frame->mm_time
- now,
display_frame, decoder);
} else if (decoder->display_frame && !decoder->pending_samples) {
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [client] gstreamer: Initialize last_mm_time to avoid an overflow
The mm_time is an unsigned 32 bit int but spice_mmtime_diff() returns a signed 32 bit int. That's reasonable because we normally substract frame times which should be at most seconds apart. But last_mm_time was zero on stream startup, resulting in an overflow and an uncalled for warning for the first frame in spice_gst_decoder_queue_frame() if the uptime was greater than about 25 days. Signed-off-by: Francois Gouget --- src/channel-display-gst.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c index 88918f10..858e8aeb 100644 --- a/src/channel-display-gst.c +++ b/src/channel-display-gst.c @@ -740,6 +740,7 @@ VideoDecoder* create_gstreamer_decoder(int codec_type, display_stream *stream) decoder->base.queue_frame = spice_gst_decoder_queue_frame; decoder->base.codec_type = codec_type; decoder->base.stream = stream; +decoder->last_mm_time = stream_get_time(stream); g_mutex_init(>queues_mutex); decoder->decoding_queue = g_queue_new(); -- 2.20.1 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [spice 1/2] utils: Remove the LL suffix from NSEC_PER_MILLISEC
This constant fits in a regular 32 bit signed integer so it does not need the suffix. It is also not used in expressions that may overflow. Signed-off-by: Francois Gouget --- server/utils.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/utils.h b/server/utils.h index ea0de1529..54bc9d490 100644 --- a/server/utils.h +++ b/server/utils.h @@ -53,7 +53,7 @@ static inline int test_bit(int index, uint32_t val) typedef int64_t red_time_t; #define NSEC_PER_SEC 10LL -#define NSEC_PER_MILLISEC 100LL +#define NSEC_PER_MILLISEC 100 #define NSEC_PER_MICROSEC 1000 /* g_get_monotonic_time() does not have enough precision */ -- 2.20.1 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [spice 2/2] utils: Remove the LL suffix from NSEC_PER_SEC
This constant fits in a 32 bit signed integer so it does not need the
suffix. However some of the derived constants don't so use an uint64_t
cast to avoid the long vs long long confusion (such as in print
statements).
Also some of the expressions these constants are used in may overflow so
perform the appropriate casts in those locations. This makes it clearer
that these operations are 64 bit.
Signed-off-by: Francois Gouget
---
server/common-graphics-channel.h | 2 +-
server/dcc.h | 2 +-
server/gstreamer-encoder.c | 2 +-
server/mjpeg-encoder.c | 2 +-
server/utils.h | 4 ++--
server/video-stream.c| 4 ++--
server/video-stream.h| 2 +-
7 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/server/common-graphics-channel.h b/server/common-graphics-channel.h
index d23f0c695..dcd527718 100644
--- a/server/common-graphics-channel.h
+++ b/server/common-graphics-channel.h
@@ -27,7 +27,7 @@ G_BEGIN_DECLS
bool common_channel_client_config_socket(RedChannelClient *rcc);
-#define COMMON_CLIENT_TIMEOUT (NSEC_PER_SEC * 30)
+#define COMMON_CLIENT_TIMEOUT (((uint64_t)NSEC_PER_SEC) * 30)
#define TYPE_COMMON_GRAPHICS_CHANNEL common_graphics_channel_get_type()
diff --git a/server/dcc.h b/server/dcc.h
index 76c078bf5..da8697762 100644
--- a/server/dcc.h
+++ b/server/dcc.h
@@ -66,7 +66,7 @@ GType display_channel_client_get_type(void) G_GNUC_CONST;
#define PALETTE_CACHE_HASH_KEY(id) ((id) & PALETTE_CACHE_HASH_MASK)
#define CLIENT_PALETTE_CACHE_SIZE 128
-#define DISPLAY_CLIENT_MIGRATE_DATA_TIMEOUT (NSEC_PER_SEC * 10)
+#define DISPLAY_CLIENT_MIGRATE_DATA_TIMEOUT (((uint64_t)NSEC_PER_SEC) * 10)
#define DISPLAY_CLIENT_RETRY_INTERVAL 1 //micro
/* Each drawable can refer to at most 3 images: src, brush and mask */
diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
index 5f39ed877..91dd475ac 100644
--- a/server/gstreamer-encoder.c
+++ b/server/gstreamer-encoder.c
@@ -556,7 +556,7 @@ static void update_next_frame_mm_time(SpiceGstEncoder
*encoder)
/* Figure out how many frames to drop to not exceed the current bit rate.
* Use nanoseconds to avoid precision loss.
*/
-uint64_t delay_ns = -encoder->vbuffer_free * 8 * NSEC_PER_SEC /
encoder->bit_rate;
+uint64_t delay_ns = ((uint64_t)-encoder->vbuffer_free) * 8 * NSEC_PER_SEC
/ encoder->bit_rate;
uint32_t drops = (delay_ns + period_ns - 1) / period_ns; /* round up */
spice_debug("drops=%u vbuffer %d/%d", drops, encoder->vbuffer_free,
encoder->vbuffer_size);
diff --git a/server/mjpeg-encoder.c b/server/mjpeg-encoder.c
index 4a02e7c8b..d22e7d18b 100644
--- a/server/mjpeg-encoder.c
+++ b/server/mjpeg-encoder.c
@@ -65,7 +65,7 @@ static const int
mjpeg_quality_samples[MJPEG_QUALITY_SAMPLE_NUM] = {20, 30, 40,
* are not necessarily related to mis-estimation of the bit rate, and we would
* like to wait till the stream stabilizes.
*/
-#define MJPEG_WARMUP_TIME (NSEC_PER_SEC * 3)
+#define MJPEG_WARMUP_TIME (((uint64_t)NSEC_PER_SEC) * 3)
/* The compressed buffer initial size. */
#define MJPEG_INITIAL_BUFFER_SIZE (32 * 1024)
diff --git a/server/utils.h b/server/utils.h
index 54bc9d490..4bbd45dff 100644
--- a/server/utils.h
+++ b/server/utils.h
@@ -52,7 +52,7 @@ static inline int test_bit(int index, uint32_t val)
typedef int64_t red_time_t;
-#define NSEC_PER_SEC 10LL
+#define NSEC_PER_SEC 10
#define NSEC_PER_MILLISEC 100
#define NSEC_PER_MICROSEC 1000
@@ -62,7 +62,7 @@ static inline red_time_t spice_get_monotonic_time_ns(void)
struct timespec time;
clock_gettime(CLOCK_MONOTONIC, );
-return NSEC_PER_SEC * time.tv_sec + time.tv_nsec;
+return ((uint64_t)NSEC_PER_SEC) * time.tv_sec + time.tv_nsec;
}
#define MSEC_PER_SEC 1000
diff --git a/server/video-stream.c b/server/video-stream.c
index 4ac25af8b..7a2dca7dd 100644
--- a/server/video-stream.c
+++ b/server/video-stream.c
@@ -415,8 +415,8 @@ static void display_channel_create_stream(DisplayChannel
*display, Drawable *dra
* the nearest integer, for instance 24 for 23.976.
*/
uint64_t duration = drawable->creation_time - drawable->first_frame_time;
-if (duration > NSEC_PER_SEC * drawable->frames_count / MAX_FPS) {
-stream->input_fps = (NSEC_PER_SEC * drawable->frames_count + duration
/ 2) / duration;
+if (duration > ((uint64_t)NSEC_PER_SEC) * drawable->frames_count /
MAX_FPS) {
+stream->input_fps = (((uint64_t)NSEC_PER_SEC) * drawable->frames_count
+ duration / 2) / duration;
} else {
stream->input_fps = MAX_FPS;
}
diff --git a/server/video-stream.h b/server/video-stream.h
index 46b076fd7..6c18d00f7 100644
--- a/server/video-stream.h
+++ b/server/video-stream.h
@@ -34,7 +34,7 @@
#define RED_STREAM_GRADUAL_FRAMES_START_CONDITION 0.2
#define RED_STREAM_FRAMES_RE
[Spice-devel] [spice 0/2] utils: Remove the LL suffix from the NSEC_PER_* constants
The LL suffix forces the expressions these constants are used in to be computed in 64 bit. The good thing is that this avoids overflows. The downside is that, because the LL suffix is hidden in a far away macro, looking at the expression will not reveal whether it operates on 32 or 64 bit integers. This is made even more confusing by the fact that not all the NSEC_PER_* constants have the LL suffix. Then when a developper adds a spice_debug() trace using one of these constants it also makes it hard to know in advance whether %ld or %lld should be used, causing unnecessary compilation errors. Finally, while the main NSEC_PER_* constants fit in 32 bit some of the derived ones such as COMMON_CLIENT_TIMEOUT don't. For those use an uint64_t cast rather than an LL suffix to avoid the %ld vs. %lld confusion (again for debugging). Francois Gouget (2): utils: Remove the LL suffix from NSEC_PER_MILLISEC utils: Remove the LL suffix from NSEC_PER_SEC server/common-graphics-channel.h | 2 +- server/dcc.h | 2 +- server/gstreamer-encoder.c | 2 +- server/mjpeg-encoder.c | 2 +- server/utils.h | 6 +++--- server/video-stream.c| 4 ++-- server/video-stream.h| 2 +- 7 files changed, 10 insertions(+), 10 deletions(-) -- 2.20.1 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [spice v2 1/2] mjpeg: Constify some MJpegEncoder* parameters
Signed-off-by: Francois Gouget
---
server/mjpeg-encoder.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/server/mjpeg-encoder.c b/server/mjpeg-encoder.c
index 1400519bb..b03fffe14 100644
--- a/server/mjpeg-encoder.c
+++ b/server/mjpeg-encoder.c
@@ -335,13 +335,13 @@ spice_jpeg_mem_dest(j_compress_ptr cinfo,
}
/* end of code from libjpeg */
-static inline uint32_t mjpeg_encoder_get_source_fps(MJpegEncoder *encoder)
+static inline uint32_t mjpeg_encoder_get_source_fps(const MJpegEncoder
*encoder)
{
return encoder->cbs.get_source_fps ?
encoder->cbs.get_source_fps(encoder->cbs.opaque) : MJPEG_MAX_FPS;
}
-static inline uint32_t mjpeg_encoder_get_latency(MJpegEncoder *encoder)
+static inline uint32_t mjpeg_encoder_get_latency(const MJpegEncoder *encoder)
{
return encoder->cbs.get_roundtrip_ms ?
encoder->cbs.get_roundtrip_ms(encoder->cbs.opaque) / 2 : 0;
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [spice v2 2/2] mjpeg: Pull more code in get_min_required_playback_delay()
This reduces code duplication and passing the MJpegEncoder object
makes it possible to modify the playback calculation without adding
more arguments.
Signed-off-by: Francois Gouget
---
v2: Reduced the diff and constified the MJpegEncoder* parameter.
server/mjpeg-encoder.c | 20
1 file changed, 8 insertions(+), 12 deletions(-)
diff --git a/server/mjpeg-encoder.c b/server/mjpeg-encoder.c
index b03fffe14..4a02e7c8b 100644
--- a/server/mjpeg-encoder.c
+++ b/server/mjpeg-encoder.c
@@ -181,9 +181,8 @@ typedef struct MJpegEncoder {
} MJpegEncoder;
static void mjpeg_encoder_process_server_drops(MJpegEncoder *encoder);
-static uint32_t get_min_required_playback_delay(uint64_t frame_enc_size,
-uint64_t byte_rate,
-uint32_t latency);
+static uint32_t get_min_required_playback_delay(const MJpegEncoder *encoder,
+uint64_t frame_enc_size);
static void mjpeg_video_buffer_free(VideoBuffer *video_buffer)
{
@@ -534,10 +533,7 @@ complete_sample:
spice_debug("MJpeg quality sample end %p: quality %d fps %d",
encoder, mjpeg_quality_samples[rate_control->quality_id],
rate_control->fps);
if (encoder->cbs.update_client_playback_delay) {
-uint32_t latency = mjpeg_encoder_get_latency(encoder);
-uint32_t min_delay =
get_min_required_playback_delay(final_quality_enc_size,
-
rate_control->byte_rate,
- latency);
+uint32_t min_delay = get_min_required_playback_delay(encoder,
final_quality_enc_size);
encoder->cbs.update_client_playback_delay(encoder->cbs.opaque,
min_delay);
}
@@ -1166,10 +1162,11 @@ static void
mjpeg_encoder_handle_positive_client_stream_report(MJpegEncoder *enc
* the video playback jitter buffer should be at least (send_time*2 +
net_latency) for
* preventing underflow
*/
-static uint32_t get_min_required_playback_delay(uint64_t frame_enc_size,
-uint64_t byte_rate,
-uint32_t latency)
+static uint32_t get_min_required_playback_delay(const MJpegEncoder *encoder,
+uint64_t frame_enc_size)
{
+uint64_t byte_rate = encoder->rate_control.byte_rate;
+uint32_t latency = mjpeg_encoder_get_latency(encoder);
uint32_t one_frame_time;
uint32_t min_delay;
@@ -1219,8 +1216,7 @@ static void
mjpeg_encoder_client_stream_report(VideoEncoder *video_encoder,
rate_control->num_recent_enc_frames;
}
spice_debug("recent size avg %.2f (KB)", avg_enc_size / 1024.0);
-min_playback_delay = get_min_required_playback_delay(avg_enc_size,
rate_control->byte_rate,
-
mjpeg_encoder_get_latency(encoder));
+min_playback_delay = get_min_required_playback_delay(encoder,
avg_enc_size);
spice_debug("min-delay %u client-delay %d", min_playback_delay,
end_frame_delay);
if (min_playback_delay > end_frame_delay) {
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [spice] gstreamer-encoder: Constify some SpiceGstEncoder* parameters
Signed-off-by: Francois Gouget
---
server/gstreamer-encoder.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
index 110d12981..cae22c100 100644
--- a/server/gstreamer-encoder.c
+++ b/server/gstreamer-encoder.c
@@ -333,13 +333,13 @@ static inline double get_mbps(uint64_t bit_rate)
/* Returns the source frame rate which may change at any time so don't store
* the result.
*/
-static uint32_t get_source_fps(SpiceGstEncoder *encoder)
+static uint32_t get_source_fps(const SpiceGstEncoder *encoder)
{
return encoder->cbs.get_source_fps ?
encoder->cbs.get_source_fps(encoder->cbs.opaque) :
SPICE_GST_DEFAULT_FPS;
}
-static uint32_t get_network_latency(SpiceGstEncoder *encoder)
+static uint32_t get_network_latency(const SpiceGstEncoder *encoder)
{
/* Assume that the network latency is symmetric */
return encoder->cbs.get_roundtrip_ms ?
@@ -370,7 +370,7 @@ static void free_pipeline(SpiceGstEncoder *encoder)
/* -- Encoded frame statistics -- */
-static inline uint32_t get_last_frame_mm_time(SpiceGstEncoder *encoder)
+static inline uint32_t get_last_frame_mm_time(const SpiceGstEncoder *encoder)
{
return encoder->history[encoder->history_last].mm_time;
}
@@ -378,7 +378,7 @@ static inline uint32_t
get_last_frame_mm_time(SpiceGstEncoder *encoder)
/* Returns the current bit rate based on the last
* SPICE_GST_FRAME_STATISTICS_COUNT frames.
*/
-static uint64_t get_effective_bit_rate(SpiceGstEncoder *encoder)
+static uint64_t get_effective_bit_rate(const SpiceGstEncoder *encoder)
{
uint32_t next_mm_time = encoder->next_frame_mm_time ?
encoder->next_frame_mm_time :
@@ -388,7 +388,7 @@ static uint64_t get_effective_bit_rate(SpiceGstEncoder
*encoder)
return elapsed ? encoder->stat_size_sum * 8 * MSEC_PER_SEC / elapsed : 0;
}
-static uint64_t get_average_encoding_time(SpiceGstEncoder *encoder)
+static uint64_t get_average_encoding_time(const SpiceGstEncoder *encoder)
{
uint32_t count = encoder->history_last +
(encoder->history_last < encoder->stat_first ? SPICE_GST_HISTORY_SIZE
: 0) -
@@ -396,7 +396,7 @@ static uint64_t get_average_encoding_time(SpiceGstEncoder
*encoder)
return encoder->stat_duration_sum / count;
}
-static uint32_t get_average_frame_size(SpiceGstEncoder *encoder)
+static uint32_t get_average_frame_size(const SpiceGstEncoder *encoder)
{
uint32_t count = encoder->history_last +
(encoder->history_last < encoder->stat_first ? SPICE_GST_HISTORY_SIZE
: 0) -
@@ -430,8 +430,8 @@ static uint32_t get_maximum_frame_size(SpiceGstEncoder
*encoder)
/* Returns the bit rate of the specified period. from and to must be the
* mm time of the first and last frame to consider.
*/
-static uint64_t get_period_bit_rate(SpiceGstEncoder *encoder, uint32_t from,
-uint32_t to)
+static uint64_t get_period_bit_rate(const SpiceGstEncoder *encoder,
+uint32_t from, uint32_t to)
{
uint32_t sum = 0;
uint32_t last_mm_time = 0;
@@ -594,7 +594,7 @@ static void update_next_frame_mm_time(SpiceGstEncoder
*encoder)
* This is based on a 10x compression ratio which should be more than enough
* for even MJPEG to provide good quality.
*/
-static uint64_t get_bit_rate_cap(SpiceGstEncoder *encoder)
+static uint64_t get_bit_rate_cap(const SpiceGstEncoder *encoder)
{
uint32_t raw_frame_bits = encoder->width * encoder->height *
encoder->format->bpp;
return raw_frame_bits * get_source_fps(encoder) / 10;
@@ -882,7 +882,7 @@ static GstFlowReturn new_sample(GstAppSink *gstappsink,
gpointer video_encoder)
return GST_FLOW_OK;
}
-static const gchar* get_gst_codec_name(SpiceGstEncoder *encoder)
+static const gchar* get_gst_codec_name(const SpiceGstEncoder *encoder)
{
switch (encoder->base.codec_type)
{
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [spice] gstreamer-encoder: Document get_maximum_frame_size()
Signed-off-by: Francois Gouget
---
server/gstreamer-encoder.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
index bccbe0660..110d12981 100644
--- a/server/gstreamer-encoder.c
+++ b/server/gstreamer-encoder.c
@@ -404,6 +404,13 @@ static uint32_t get_average_frame_size(SpiceGstEncoder
*encoder)
return encoder->stat_size_sum / count;
}
+/* Look for the largest frame and store it in stat_size_max to reduce how
+ * often we have to scan the history for the largest frame.
+ * Then all we need to keep things consistent is to:
+ * - Update stat_size_max when adding a larger frame to the history.
+ * - Reset stat_size_max to zero when the largest frame falls out of
+ * the history.
+ */
static uint32_t get_maximum_frame_size(SpiceGstEncoder *encoder)
{
if (encoder->stat_size_max == 0) {
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [spice v2] gstreamer-encoder: Return the average frame size as a 32 bit int
It makes no sense to expect average frame sizes anywhere close to 2GB.
But then make sure to avoid arithmetic overflows.
Signed-off-by: Francois Gouget
---
In get_min_playback_delay() I opted for the cast approach as this makes
what happens clearer. I deemed the assignment (uint32_t) unnecessary
though.
server/gstreamer-encoder.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
index 0ff1201a9..0101b36fa 100644
--- a/server/gstreamer-encoder.c
+++ b/server/gstreamer-encoder.c
@@ -396,7 +396,7 @@ static uint64_t get_average_encoding_time(SpiceGstEncoder
*encoder)
return encoder->stat_duration_sum / count;
}
-static uint64_t get_average_frame_size(SpiceGstEncoder *encoder)
+static uint32_t get_average_frame_size(SpiceGstEncoder *encoder)
{
uint32_t count = encoder->history_last +
(encoder->history_last < encoder->stat_first ? SPICE_GST_HISTORY_SIZE
: 0) -
@@ -520,8 +520,8 @@ static uint32_t get_min_playback_delay(SpiceGstEncoder
*encoder)
* an I frame) and an average frame. This also takes into account the
* frames dropped by the encoder bit rate control.
*/
-uint64_t size = get_maximum_frame_size(encoder) +
get_average_frame_size(encoder);
-uint32_t send_time = MSEC_PER_SEC * size * 8 / encoder->bit_rate;
+uint32_t size = get_maximum_frame_size(encoder) +
get_average_frame_size(encoder);
+uint32_t send_time = ((uint64_t)MSEC_PER_SEC * 8) * size /
encoder->bit_rate;
/* Also factor in the network latency with a margin for jitter. */
uint32_t net_latency = get_network_latency(encoder) * (1.0 +
SPICE_GST_LATENCY_MARGIN);
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [spice v2] gstreamer-encoder: Show the source fps when the system is too slow
The source framerate is as important as the resolution when trying to
understand if the system should be fast enough to encode the video
stream in real time.
Signed-off-by: Francois Gouget
---
v2: Tweaked the message.
server/gstreamer-encoder.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
index 0101b36fa..bccbe0660 100644
--- a/server/gstreamer-encoder.c
+++ b/server/gstreamer-encoder.c
@@ -542,7 +542,7 @@ static void update_next_frame_mm_time(SpiceGstEncoder
*encoder)
uint64_t period_ns = NSEC_PER_SEC / get_source_fps(encoder);
uint64_t min_delay_ns = get_average_encoding_time(encoder);
if (min_delay_ns > period_ns) {
-spice_warning("your system seems to be too slow to encode this %dx%d
video in real time", encoder->width, encoder->height);
+spice_warning("your system seems to be too slow to encode this
%dx%d@%d video in real time", encoder->width, encoder->height,
get_source_fps(encoder));
}
min_delay_ns = MIN(min_delay_ns, SPICE_GST_MAX_PERIOD);
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice] gstreamer-encoder: Return the average frame size as a 32 bit int
On Thu, 16 May 2019, Frediano Ziglio wrote: [...] > > @@ -520,7 +520,7 @@ static uint32_t get_min_playback_delay(SpiceGstEncoder > > *encoder) > > * an I frame) and an average frame. This also takes into account the > > * frames dropped by the encoder bit rate control. > > */ > > -uint64_t size = get_maximum_frame_size(encoder) + > > get_average_frame_size(encoder); > > +uint32_t size = get_maximum_frame_size(encoder) + > > get_average_frame_size(encoder); > > uint32_t send_time = MSEC_PER_SEC * size * 8 / encoder->bit_rate; > > > > Here you have 8000 * 2 * frame_size so could overflow uint32_t with > frame_size >= ~256kb. Right. It's confusing that NSEC_PER_SEC and NSEC_PER_MILLISEC are LL constants but not NSEC_PER_MICROSEC and MSEC_PER_SEC. Should that be changed? Since they are all less than or equal to one billion, should they just be basic constants (which would avoid the %lld vs. %ld confusion whenever they are used in a trace) or should they all be LL constants to avoid unexpected overflows. > I agree get_average_frame_size can safely returns uint32_t but you should > change above line to > >uint32_t send_time = (uint32_t) ((uint64_t) (MSEC_PER_SEC * 8) * size / > encoder->bit_rate); > > or leave size uint64_t. I would be ok with that too. It's mostly having get_average_frame_size() return an uint64_t when get_maximum_frame_size() returns an uint32_t that was bothering me. -- Francois Gouget ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice] gstreamer-encoder: Return the average frame size as a 32 bit int
On Thu, 16 May 2019, Frediano Ziglio wrote: > > > > It makes no sense to expect average frame sizes anywhere close to 2GB. > > > > Signed-off-by: Francois Gouget > > Sure but 256 kb are possible. Even multiplied by two that fits pretty well in a 32 signed integer. -- Francois Gouget ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [spice] mjpeg: Pull more code in get_min_required_playback_delay()
This reduces code duplication and passing the MJpegEncoder object
makes it possible to modify the playback calculation without adding
more arguments.
Signed-off-by: Francois Gouget
---
server/mjpeg-encoder.c | 31 +--
1 file changed, 13 insertions(+), 18 deletions(-)
diff --git a/server/mjpeg-encoder.c b/server/mjpeg-encoder.c
index 1400519bb..15db38752 100644
--- a/server/mjpeg-encoder.c
+++ b/server/mjpeg-encoder.c
@@ -181,9 +181,8 @@ typedef struct MJpegEncoder {
} MJpegEncoder;
static void mjpeg_encoder_process_server_drops(MJpegEncoder *encoder);
-static uint32_t get_min_required_playback_delay(uint64_t frame_enc_size,
-uint64_t byte_rate,
-uint32_t latency);
+static uint32_t get_min_required_playback_delay(MJpegEncoder *encoder,
+uint64_t frame_enc_size);
static void mjpeg_video_buffer_free(VideoBuffer *video_buffer)
{
@@ -534,10 +533,7 @@ complete_sample:
spice_debug("MJpeg quality sample end %p: quality %d fps %d",
encoder, mjpeg_quality_samples[rate_control->quality_id],
rate_control->fps);
if (encoder->cbs.update_client_playback_delay) {
-uint32_t latency = mjpeg_encoder_get_latency(encoder);
-uint32_t min_delay =
get_min_required_playback_delay(final_quality_enc_size,
-
rate_control->byte_rate,
- latency);
+uint32_t min_delay = get_min_required_playback_delay(encoder,
final_quality_enc_size);
encoder->cbs.update_client_playback_delay(encoder->cbs.opaque,
min_delay);
}
@@ -1166,20 +1162,20 @@ static void
mjpeg_encoder_handle_positive_client_stream_report(MJpegEncoder *enc
* the video playback jitter buffer should be at least (send_time*2 +
net_latency) for
* preventing underflow
*/
-static uint32_t get_min_required_playback_delay(uint64_t frame_enc_size,
-uint64_t byte_rate,
-uint32_t latency)
+static uint32_t get_min_required_playback_delay(MJpegEncoder *encoder,
+uint64_t frame_enc_size)
{
-uint32_t one_frame_time;
-uint32_t min_delay;
+MJpegEncoderRateControl *rate_control = >rate_control;
+uint32_t latency, one_frame_time, min_delay;
-if (!frame_enc_size || !byte_rate) {
+latency = mjpeg_encoder_get_latency(encoder);
+if (!frame_enc_size || !rate_control->byte_rate) {
return latency;
}
-one_frame_time = (frame_enc_size * MSEC_PER_SEC) / byte_rate;
-min_delay = MIN(one_frame_time*2 + latency,
MJPEG_MAX_CLIENT_PLAYBACK_DELAY);
-return min_delay;
+one_frame_time = (frame_enc_size * MSEC_PER_SEC) / rate_control->byte_rate;
+min_delay = latency + 2 * one_frame_time;
+return MIN(min_delay, MJPEG_MAX_CLIENT_PLAYBACK_DELAY);
}
#define MJPEG_PLAYBACK_LATENCY_DECREASE_FACTOR 0.5
@@ -1219,8 +1215,7 @@ static void
mjpeg_encoder_client_stream_report(VideoEncoder *video_encoder,
rate_control->num_recent_enc_frames;
}
spice_debug("recent size avg %.2f (KB)", avg_enc_size / 1024.0);
-min_playback_delay = get_min_required_playback_delay(avg_enc_size,
rate_control->byte_rate,
-
mjpeg_encoder_get_latency(encoder));
+min_playback_delay = get_min_required_playback_delay(encoder,
avg_enc_size);
spice_debug("min-delay %u client-delay %d", min_playback_delay,
end_frame_delay);
if (min_playback_delay > end_frame_delay) {
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [spice] reds: Enable mm_time adjustments on startup
This reinstates the reds_enable_mm_time() call in do_spice_init()
that was removed by commit c541d7e29dc0.
We send mm_time adjustments to the client whenever there is no audio
playback. There is no audio playback on startup. Therefore
mm_time_enabled must be true on startup. QED.
This fixes adjusting the client mm_time whenever playing a silent
video (or full desktop stream) when no sound has been played before
such as when using Xspice, booting an OS with no startup or login
jingle, or possibly when migrating a VM (per commit 1c154ea5ecc3).
Signed-off-by: Francois Gouget
---
server/reds.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/server/reds.c b/server/reds.c
index d658103e6..792e98381 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -3570,6 +3570,11 @@ static int do_spice_init(RedsState *reds,
SpiceCoreInterface *core_interface)
if (!(reds->mig_timer = reds->core.timer_add(>core, migrate_timeout,
reds))) {
spice_error("migration timer create failed");
}
+/* Note that this will not actually send the mm_time to the client because
+ * the main channel is not connected yet. This would have been redundant
+ * with the RED_PIPE_ITEM_TYPE_MAIN_INIT message anyway.
+ */
+reds_enable_mm_time(reds);
if (reds_init_net(reds) < 0) {
spice_warning("Failed to open SPICE sockets");
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [spice] gstreamer-encoder: Return the average frame size as a 32 bit int
It makes no sense to expect average frame sizes anywhere close to 2GB.
Signed-off-by: Francois Gouget
---
server/gstreamer-encoder.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
index e319eea22..6130781da 100644
--- a/server/gstreamer-encoder.c
+++ b/server/gstreamer-encoder.c
@@ -396,7 +396,7 @@ static uint64_t get_average_encoding_time(SpiceGstEncoder
*encoder)
return encoder->stat_duration_sum / count;
}
-static uint64_t get_average_frame_size(SpiceGstEncoder *encoder)
+static uint32_t get_average_frame_size(SpiceGstEncoder *encoder)
{
uint32_t count = encoder->history_last +
(encoder->history_last < encoder->stat_first ? SPICE_GST_HISTORY_SIZE
: 0) -
@@ -520,7 +520,7 @@ static uint32_t get_min_playback_delay(SpiceGstEncoder
*encoder)
* an I frame) and an average frame. This also takes into account the
* frames dropped by the encoder bit rate control.
*/
-uint64_t size = get_maximum_frame_size(encoder) +
get_average_frame_size(encoder);
+uint32_t size = get_maximum_frame_size(encoder) +
get_average_frame_size(encoder);
uint32_t send_time = MSEC_PER_SEC * size * 8 / encoder->bit_rate;
/* Also factor in the network latency with a margin for jitter. */
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [spice] gstreamer-encoder: Include encoding time in get_min_playback_delay()
This way all the minimum delay calculation is in one place and this
makes gstreamer's implementation closer to the mjpeg one.
Signed-off-by: Francois Gouget
---
server/gstreamer-encoder.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
index 6130781da..17d9822c0 100644
--- a/server/gstreamer-encoder.c
+++ b/server/gstreamer-encoder.c
@@ -526,13 +526,13 @@ static uint32_t get_min_playback_delay(SpiceGstEncoder
*encoder)
/* Also factor in the network latency with a margin for jitter. */
uint32_t net_latency = get_network_latency(encoder) * (1.0 +
SPICE_GST_LATENCY_MARGIN);
-return send_time + net_latency;
+return send_time + net_latency + get_average_encoding_time(encoder) /
NSEC_PER_MILLISEC;
}
static void update_client_playback_delay(SpiceGstEncoder *encoder)
{
if (encoder->cbs.update_client_playback_delay) {
-uint32_t min_delay = get_min_playback_delay(encoder) +
get_average_encoding_time(encoder) / NSEC_PER_MILLISEC;
+uint32_t min_delay = get_min_playback_delay(encoder);
encoder->cbs.update_client_playback_delay(encoder->cbs.opaque,
min_delay);
}
}
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [spice] gstreamer-encoder: Show the source fps when the system is too slow
The source framerate is as important as the resolution when trying to
understand if the system should be fast enough to encode the video
stream in real time.
Signed-off-by: Francois Gouget
---
server/gstreamer-encoder.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
index 17d9822c0..3dfa2bae2 100644
--- a/server/gstreamer-encoder.c
+++ b/server/gstreamer-encoder.c
@@ -542,7 +542,7 @@ static void update_next_frame_mm_time(SpiceGstEncoder
*encoder)
uint64_t period_ns = NSEC_PER_SEC / get_source_fps(encoder);
uint64_t min_delay_ns = get_average_encoding_time(encoder);
if (min_delay_ns > period_ns) {
-spice_warning("your system seems to be too slow to encode this %dx%d
video in real time", encoder->width, encoder->height);
+spice_warning("your system seems to be too slow to encode this %dx%d
%d fps video in real time", encoder->width, encoder->height,
get_source_fps(encoder));
}
min_delay_ns = MIN(min_delay_ns, SPICE_GST_MAX_PERIOD);
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [spice] mjpeg: Remove the unused MJPEG_LOW_FPS_RATE_TH constant
Signed-off-by: Francois Gouget
---
server/mjpeg-encoder.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/server/mjpeg-encoder.c b/server/mjpeg-encoder.c
index b373e8b71..1400519bb 100644
--- a/server/mjpeg-encoder.c
+++ b/server/mjpeg-encoder.c
@@ -38,7 +38,6 @@ static const int
mjpeg_quality_samples[MJPEG_QUALITY_SAMPLE_NUM] = {20, 30, 40,
#define MJPEG_AVERAGE_SIZE_WINDOW 3
#define MJPEG_BIT_RATE_EVAL_MIN_NUM_FRAMES 3
-#define MJPEG_LOW_FPS_RATE_TH 3
#define MJPEG_SERVER_STATUS_EVAL_FPS_INTERVAL 1
#define MJPEG_SERVER_STATUS_DOWNGRADE_DROP_FACTOR_TH 0.1
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice] Enable mm_time adjustments on startup
work latency. So if the MJPEG encoder settles on such a high value it means something went seriously wrong. > > Decreasing the latency means a bunch of frames queued on the client may > > suddenly become late, causing them to be dropped. So more care must be > > taken. > > > > Is the problem of "care to take" something not avoidable or is it > something we create with our hands? I mean, I know that current > implementation will drop frames which we don't want but is it > not a workaround of a bad implementation? The answer depends in part on whether you consider the existing clients to be a given that the server has to deal with, or something that can be replaced to suit the server (or to not hold back the protocol). -- Francois Gouget , ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice] Enable mm_time adjustments on startup
On Fri, 26 Apr 2019, Frediano Ziglio wrote: > > > > We send mm_time adjustments to the client whenever there is no audio > > playback. There is no audio playback on startup. Therefore > > mm_time_enabled must be true on startup. QED. > > > > How did you demonstrated it ? Starting Xspice (so no Windows jingle or other being played on startup), playing a WebGL demo, and then trying to figure out why the server's latency adjustments had absolutely no effect on the client. -- Francois Gouget ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice] Enable mm_time adjustments on startup
On Wed, 17 Apr 2019, Snir Sheriber wrote: [...] > > The reason for that is that while a minimum 400 ms latency is fine when > > playing a YouTube video [1], it's very annoying when the whole desktop > > is being streamed, either through the streaming agent or through the > > future Virgl remote access, because then it translates into a 400 ms > > Are you working on something like that (remote virgl)? Yes. See (though I need to update refresh these GitHub branches): https://lists.freedesktop.org/archives/spice-devel/2019-January/047329.html > Notice that currently there's small hacky patch on client to ignore latency > when it's full > screen streaming and there is no audio playback. > > (d047b2fb7f5d492d6c49f589ba5ff862c6b115da) Right. It never actually had any effect in my tests. * When testing the fullscreen streaming in QEmu (i.e. when streaming_mode is true) I had to use the mjpeg encoding because there seems to be some conflict between QEmu and GStreamer on Debian. But the patch has no effect on the builtin mjpeg decoder because the latency in mjpeg_decoder_queue_frame() is to drop frames if it is negative. To determine when to display the frame it uses the frame's timestamp. * The rest of the time I'm testing by running an OpenGL application in a regular session and so streaming_mode is false so the GstVideoOverlay is not used and thus the patch has no effect. That's because spice_gst_decoder_queue_frame() uses latency to drop frames if it is negative; and to determine the deadline for decoding the frame. When not using the video overlay the decoded frames then get queued until it is time to display them, according to their mm_time timestamp which is not impacted by latency. That said I'm not sure ignoring the frames mm_time timestamp is a good idea as it means the network jitter will translate directly into framerate jitter (e.g. in OpenGL applications or in silent movies). > > Other steps are: > > * Reducing the default latency. > > What will be the default? what will happen to late video frames? Late frames will be dropped as has always been the case (at least in the mjpeg case otherwise it's a bit more complex). As before the latency must be adjusted, and particularly, increased, as required to avoid dropping frames. Of course when starting with a huge latency like 400 ms handling the latency correctly is much less important as it only become important when you have really a bad network connection or a very long encoding times. It's important to distinguish increasing and decreasing the latency. Increasing the latency means a frame queued on the client will be displayed even later. So the latency can be increased freely. Decreasing the latency means a bunch of frames queued on the client may suddenly become late, causing them to be dropped. So more care must be taken. -- Francois Gouget ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice] Enable mm_time adjustments on startup
On Thu, 11 Apr 2019, Victor Toso wrote: > Hi, > > On Wed, Apr 10, 2019 at 11:25:17AM +0200, Francois Gouget wrote: > > We send mm_time adjustments to the client whenever there is no audio > > playback. There is no audio playback on startup. Therefore > > mm_time_enabled must be true on startup. QED. > > > > Signed-off-by: Francois Gouget > > But what are you trying to fix/improve exactly? The goal is to reduce the video stream latency. The reason for that is that while a minimum 400 ms latency is fine when playing a YouTube video [1], it's very annoying when the whole desktop is being streamed, either through the streaming agent or through the future Virgl remote access, because then it translates into a 400 ms lag between every mouse click, keypress and the screen update. This patch is the first step in that without it adjusting the latency is impossible unless one has played some sound first. Other steps are: * Reducing the default latency. * Increasing the latency when needed. * Reducing the latency after network hiccups. * Ensuring latency adjustments don't interfere with the video stream bit rate control. [1] Even in that case the 400 ms latency shows up as the hiccup you get when Spice switches from sending frames as regular screen updates to the streaming code. -- Francois Gouget ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [spice] Consistently use the dcc local variable in update_client_playback_delay()
Signed-off-by: Francois Gouget
---
server/video-stream.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/server/video-stream.c b/server/video-stream.c
index 197950984..ba1a5adf2 100644
--- a/server/video-stream.c
+++ b/server/video-stream.c
@@ -675,7 +675,7 @@ static void update_client_playback_delay(void *opaque,
uint32_t delay_ms)
spice_debug("resetting client latency: %u",
dcc_get_max_stream_latency(dcc));
main_dispatcher_set_mm_time_latency(reds_get_main_dispatcher(reds),
client,
-
dcc_get_max_stream_latency(agent->dcc));
+dcc_get_max_stream_latency(dcc));
}
static void bitmap_ref(gpointer data)
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [spice] Enable mm_time adjustments on startup
We send mm_time adjustments to the client whenever there is no audio playback. There is no audio playback on startup. Therefore mm_time_enabled must be true on startup. QED. Signed-off-by: Francois Gouget --- server/reds.c | 1 + 1 file changed, 1 insertion(+) diff --git a/server/reds.c b/server/reds.c index 28542bd09..647039119 100644 --- a/server/reds.c +++ b/server/reds.c @@ -3590,6 +3590,7 @@ static int do_spice_init(RedsState *reds, SpiceCoreInterface *core_interface) reds->inputs_channel = inputs_channel_new(reds); reds->mouse_mode = SPICE_MOUSE_MODE_SERVER; +reds->mm_time_enabled = TRUE; spice_buffer_free(>client_monitors_config); -- 2.20.1 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [spice] Consistently check if drm_dma_buf_fd is greater or lower than zero
Based on a patch by Frediano Ziglio.
Signed-off-by: Francois Gouget
---
server/red-qxl.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/server/red-qxl.c b/server/red-qxl.c
index 0dd26b22c..789817f69 100644
--- a/server/red-qxl.c
+++ b/server/red-qxl.c
@@ -791,7 +791,7 @@ void spice_qxl_gl_scanout(QXLInstance *qxl,
pthread_mutex_lock(_state->scanout_mutex);
-if (qxl_state->scanout.drm_dma_buf_fd != -1) {
+if (qxl_state->scanout.drm_dma_buf_fd >= 0) {
close(qxl_state->scanout.drm_dma_buf_fd);
}
@@ -832,7 +832,7 @@ void spice_qxl_gl_draw_async(QXLInstance *qxl,
spice_return_if_fail(qxl != NULL);
qxl_state = qxl->st;
-if (qxl_state->scanout.drm_dma_buf_fd == -1) {
+if (qxl_state->scanout.drm_dma_buf_fd < 0) {
spice_warning("called spice_qxl_gl_draw_async without a buffer");
red_qxl_async_complete(qxl, cookie);
return;
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [spice] dcc: Remove a redundant NULL pointer check in dcc_create_surface()
dcc_create_surface() already returns immediately if dcc is NULL.
Signed-off-by: Francois Gouget
---
server/dcc.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/server/dcc.c b/server/dcc.c
index a05b6e59e..b6cd6b3f9 100644
--- a/server/dcc.c
+++ b/server/dcc.c
@@ -303,8 +303,7 @@ void dcc_create_surface(DisplayChannelClient *dcc, int
surface_id)
flags = is_primary_surface(display, surface_id) ?
SPICE_SURFACE_FLAGS_PRIMARY : 0;
/* don't send redundant create surface commands to client */
-if (!dcc ||
-
common_graphics_channel_get_during_target_migrate(COMMON_GRAPHICS_CHANNEL(display))
||
+if
(common_graphics_channel_get_during_target_migrate(COMMON_GRAPHICS_CHANNEL(display))
||
dcc->priv->surface_client_created[surface_id]) {
return;
}
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [spice] Use display_channel_surface_has_canvas() instead of free coding it
Signed-off-by: Francois Gouget
---
server/dcc.c | 2 +-
server/display-channel.c | 16
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/server/dcc.c b/server/dcc.c
index ae7b4380f..a05b6e59e 100644
--- a/server/dcc.c
+++ b/server/dcc.c
@@ -579,7 +579,7 @@ void dcc_start(DisplayChannelClient *dcc)
return;
red_channel_client_ack_zero_messages_window(rcc);
-if (display->priv->surfaces[0].context.canvas) {
+if (display_channel_surface_has_canvas(display, 0)) {
display_channel_current_flush(display, 0);
red_channel_client_pipe_add_type(rcc,
RED_PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE);
dcc_create_surface(dcc, 0);
diff --git a/server/display-channel.c b/server/display-channel.c
index e179abfd3..cf12e99bf 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -108,7 +108,7 @@ display_channel_finalize(GObject *object)
spice_assert(ring_is_empty(>priv->streams));
for (count = 0; count < NUM_SURFACES; ++count) {
-spice_assert(self->priv->surfaces[count].context.canvas == NULL);
+spice_assert(!display_channel_surface_has_canvas(self, count));
}
}
@@ -1498,7 +1498,7 @@ void display_channel_flush_all_surfaces(DisplayChannel
*display)
int x;
for (x = 0; x < NUM_SURFACES; ++x) {
-if (display->priv->surfaces[x].context.canvas) {
+if (display_channel_surface_has_canvas(display, x)) {
display_channel_current_flush(display, x);
}
}
@@ -2072,7 +2072,7 @@ void display_channel_destroy_surface_wait(DisplayChannel
*display, uint32_t surf
{
if (!display_channel_validate_surface(display, surface_id))
return;
-if (!display->priv->surfaces[surface_id].context.canvas)
+if (!display_channel_surface_has_canvas(display, surface_id))
return;
draw_depend_on_me(display, surface_id);
@@ -2092,12 +2092,12 @@ void display_channel_destroy_surfaces(DisplayChannel
*display)
spice_debug("trace");
//to handle better
for (i = 0; i < NUM_SURFACES; ++i) {
-if (display->priv->surfaces[i].context.canvas) {
+if (display_channel_surface_has_canvas(display, i)) {
display_channel_destroy_surface_wait(display, i);
-if (display->priv->surfaces[i].context.canvas) {
+if (display_channel_surface_has_canvas(display, i)) {
display_channel_surface_unref(display, i);
}
-spice_assert(!display->priv->surfaces[i].context.canvas);
+spice_assert(!display_channel_surface_has_canvas(display, i));
}
}
spice_warn_if_fail(ring_is_empty(>priv->streams));
@@ -2424,7 +2424,7 @@ gboolean display_channel_validate_surface(DisplayChannel
*display, uint32_t surf
spice_warning("invalid surface_id %u", surface_id);
return FALSE;
}
-if (!display->priv->surfaces[surface_id].context.canvas) {
+if (!display_channel_surface_has_canvas(display, surface_id)) {
spice_warning("canvas address is %p for %d (and is NULL)",
&(display->priv->surfaces[surface_id].context.canvas),
surface_id);
spice_warning("failed on %d", surface_id);
@@ -2462,7 +2462,7 @@ void
display_channel_set_monitors_config_to_primary(DisplayChannel *display)
QXLHead head = { 0, };
uint16_t old_max = 1;
-spice_return_if_fail(display->priv->surfaces[0].context.canvas);
+spice_return_if_fail(display_channel_surface_has_canvas(display, 0));
if (display->priv->monitors_config) {
old_max = display->priv->monitors_config->max_allowed;
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] Virgl remote support
Hi, I'm picking up the Virgl remote support patchset with the goal of adapting it for use with Xspice. If I'm not mistaken the latest version of this patchset is in the virgl-spice branches of the following repositories: https://cgit.freedesktop.org/~fziglio/spice-protocol/log/?h=virgl-spice https://cgit.freedesktop.org/~fziglio/spice-common/log/?h=virgl-spice https://cgit.freedesktop.org/~fziglio/spice-server/log/?h=virgl-spice This one also has a virgl_sort branch which is much more recent and has me quite curious. But it's not clear to me if it is really related. There is also a plain virgl branch but that one seems even older. https://cgit.freedesktop.org/~fziglio/qemu/log/?h=virgl-spice This one would not be needed for Xspice but I may use it for reference. And there has been no Virgl work on the Xspice side of things, right? -- Francois Gouget ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [client v2 5/5] streaming: Separate the network code from the display_stream management
On Tue, 9 May 2017, Christophe Fergeau wrote: [...] > > So I'd propose another variant where display_stream_create() takes a > > rect parameter but leaves clip to its default. See the attached patch. > > Feel free to pick any of the three variants. > > In the current code, clip is not really optional as it's unconditionally > set after calling display_stream_create(). It is in the sense that if you were to remove the code setting it on the caller side, the stream code would not behave erratically due to invalid or uninitialized data. > I'll push with my variant, and we can revise how this all works and > make this optional when/if the code you alude to gets public :) Sure. That totally works for me! -- Francois Gouget <fgou...@codeweavers.com> ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [client v2 5/5] streaming: Separate the network code from the display_stream management
On Mon, 24 Apr 2017, Christophe Fergeau wrote:
[...]
> > +c->streams[op->id] = display_stream_create(channel, op->surface_id,
> > op->flags, op->codec_type);
> > +if (c->streams[op->id] == NULL) {
> > +spice_printerr("could not create the %u video stream", op->id);
> > destroy_stream(channel, op->id);
> > report_invalid_stream(channel, op->id);
> > +} else {
> > +c->streams[op->id]->dest = op->dest;
> > +c->streams[op->id]->clip = op->clip;
>
> Any reason why you initialize everything in display_stream_create()
> except these SpiceRect/SpiceClip instances? I'd just squash this in:
There's no big reason.
* Concerning dest, setting it is required for the stream to work so
it could make sense to set it up in display_stream_create().
* But one does not need to set an explicit clip region to use the
stream and as is it defaults to SPICE_CLIP_TYPE_NONE which I think
makes sense. So I'd leave setting it up to the caller so it is
optional.
So I'd propose another variant where display_stream_create() takes a
rect parameter but leaves clip to its default. See the attached patch.
Feel free to pick any of the three variants.
I have a use case here where I need to create a stream before I have
received the rect and where I don't use clipping at all which is why I
naturally made these optional. But that code is not public (yet anyway)
and one could argue that the rect should be known at stream creation
time. In any case it would be trivial to initialize local variables to
dummy values and pass them to display_stream_create() so any variant
would be easy for me to adjust to.
(And I apologize for the delay, my Internet connection broke down last
week and then I was away for the extended week-end)
--
Francois Gouget <fgou...@codeweavers.com> commit 6a3bd17fa881c3d95c3c3e923161137e147660b6
Author: Francois Gouget <fgou...@codeweavers.com>
Date: Fri May 20 19:32:42 2016 +0200
streaming: Separate the network code from the display_stream management
This makes it easier to reuse display_streams for other types of
video streams should the need arise.
Signed-off-by: Francois Gouget <fgou...@codeweavers.com>
diff --git a/src/channel-display.c b/src/channel-display.c
index 00b54406..c129e3ba 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -109,7 +109,7 @@ static display_surface *find_surface(SpiceDisplayChannelPrivate *c, guint32 surf
static void spice_display_channel_reset(SpiceChannel *channel, gboolean migrating);
static void spice_display_channel_reset_capabilities(SpiceChannel *channel);
static void destroy_canvas(display_surface *surface);
-static void destroy_stream(SpiceChannel *channel, int id);
+static void destroy_display_stream(display_stream *st, int id);
static void display_session_mm_time_reset_cb(SpiceSession *session, gpointer data);
static SpiceGlScanout* spice_gl_scanout_copy(const SpiceGlScanout *scanout);
@@ -1168,11 +1168,59 @@ static display_stream *get_stream_by_id(SpiceChannel *channel, uint32_t id)
}
/* coroutine context */
+static display_stream *display_stream_create(SpiceChannel *channel, uint32_t surface_id, uint32_t flags, uint32_t codec_type, const SpiceRect *dest)
+{
+SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
+display_stream *st = g_new0(display_stream, 1);
+
+st->flags = flags;
+st->dest = *dest;
+/* st->clip defaults to SPICE_CLIP_TYPE_NONE */
+st->surface = find_surface(c, surface_id);
+st->channel = channel;
+st->drops_seqs_stats_arr = g_array_new(FALSE, FALSE, sizeof(drops_sequence_stats));
+
+region_init(>region);
+display_update_stream_region(st);
+
+switch (codec_type) {
+#ifdef HAVE_BUILTIN_MJPEG
+case SPICE_VIDEO_CODEC_TYPE_MJPEG:
+st->video_decoder = create_mjpeg_decoder(codec_type, st);
+break;
+#endif
+default:
+#ifdef HAVE_GSTVIDEO
+st->video_decoder = create_gstreamer_decoder(codec_type, st);
+#endif
+break;
+}
+if (st->video_decoder == NULL) {
+spice_printerr("could not create a video decoder for codec %u", codec_type);
+destroy_display_stream(st, 0);
+st = NULL;
+}
+return st;
+}
+
+static void destroy_stream(SpiceChannel *channel, int id)
+{
+SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
+
+g_return_if_fail(c != NULL);
+g_return_if_fail(c->streams != NULL);
+g_return_if_fail(c->nstreams > id);
+
+if (c->streams[id]) {
+destroy_display_stream(c->streams[id], id);
+c->streams[id] = NULL;
+}
+}
+
static void display_handle_stream_create(SpiceChannel *channel, SpiceMsgIn *in)
{
SpiceDisplayC
Re: [Spice-devel] [client v2 0/5] Abstract video streaming from the network details
On Thu, 6 Apr 2017, Francois Gouget wrote: > Currently the video decoders are directly passed network messages which > ties them pretty closely to the network code. Should the protocol switch > to a new type of messages the video decoding code will need to be > updated. Even worse, it will have to deal with two types of network > messages for backward compatiblity. > > So I feel it's better to try and separate the two which is what I've > done in the first two patches. > > The third one does not really change the code much, it just separates > the code that creates / destroys the display_stream objects from the > code that handles the network messages leading to their creation / > destruction. > > Changes from v1: > * Add a patch documenting the steps each frame goes through in the >GStreamer decoder, along with lifetime information. > * Rename SpiceMetaFrame to SpiceGstFrame. > * Separate renaming SpiceFrame to SpiceGstFrame from the patch removing >the video decoder's dependency on SpiceMsgIn messages. > * Tweaked that patch to minimize changes. > * Fixed the type of the SpiceGstFrame.sample field. Is this round better? -- Francois Gouget <fgou...@codeweavers.com> ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] vdagent: Spelling and case fixes in comments
On Fri, 7 Apr 2017, Christophe Fergeau wrote: [...] > > - pointing to a non existing window. */ > > + pointing to a nonexistent window. */ > > > Unsure about this one, I'll let a native speaker tell us what we should > use here. "nonexistent" is definitely a valid word: https://www.merriam-webster.com/thesaurus/nonexistent https://en.oxforddictionaries.com/definition/us/nonexistent It's also used as a suggested correction in Lintian's typo checker (Lintian is a Debian package QA tool): https://github.com/Debian/lintian/blob/master/data/spelling/corrections nonexistant||nonexistent "non-existent" exists too and would be the British spelling (though it might have lost its hyphen in a recent spelling revision that removed hyphens from some 16000 words. But I could not confirm that). So one could use that spelling if British English is the standard in Spice. https://en.oxforddictionaries.com/definition/non-existent But as far as I can tell the form with a space and the -ing ending is not valid at all. -- Francois Gouget <fgou...@codeweavers.com> ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] README: A couple of spelling fixes and slight rewording
On Fri, 7 Apr 2017, Christophe Fergeau wrote: [...] > > Note: I don't have commit access. > > Have you tried to request it? No. * I'm not interested in having commit access. * I don't think that someone should have commit access to be able to contribute. * But by now I've gotten used to my patches getting acked and then never getting pushed because people assume everyone has commit access (from what I gathered in the past). So now I prefer to remind reviewers in advance. * It's sad because it really discourages contributions from outside the tiny circle of core contributors. -- Francois Gouget <fgou...@codeweavers.com> ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] udscs: A spelling fix in a comment
Signed-off-by: Francois Gouget <fgou...@codeweavers.com> --- src/udscs.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/udscs.h b/src/udscs.h index 6160568..7b6bff0 100644 --- a/src/udscs.h +++ b/src/udscs.h @@ -53,7 +53,7 @@ typedef void (*udscs_read_callback)(struct udscs_connection **connp, *this callback has completed! * 2) This callback is always called, even if the disconnect is initiated *by the udscs user through returning -1 from a read callback, or - *by explictly calling udscs_destroy_connection. + *by explicitly calling udscs_destroy_connection. */ typedef void (*udscs_disconnect_callback)(struct udscs_connection *conn); -- 2.11.0 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] vdagentd: Some spelling fixes in comments
Signed-off-by: Francois Gouget <fgou...@codeweavers.com>
---
src/vdagentd/console-kit.c | 2 +-
src/vdagentd/vdagentd.c| 2 +-
src/vdagentd/virtio-port.h | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/vdagentd/console-kit.c b/src/vdagentd/console-kit.c
index a939f38..9e46ad5 100644
--- a/src/vdagentd/console-kit.c
+++ b/src/vdagentd/console-kit.c
@@ -168,7 +168,7 @@ si_dbus_read_signals(struct session_info *info)
dbus_message_iter_init(message, );
type = dbus_message_iter_get_arg_type();
/* Session should be an object path, but there is a bug in
- ConsoleKit where it sends a string rather then an object_path
+ ConsoleKit where it sends a string rather than an object_path
accept object_path too in case the bug ever gets fixed */
if (type == DBUS_TYPE_STRING || type == DBUS_TYPE_OBJECT_PATH) {
dbus_message_iter_get_basic(, );
diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
index aef5b96..0c54733 100644
--- a/src/vdagentd/vdagentd.c
+++ b/src/vdagentd/vdagentd.c
@@ -240,7 +240,7 @@ static void do_client_capabilities(struct
vdagent_virtio_port *vport,
}
memcpy(capabilities, caps->caps, capabilities_size * sizeof(uint32_t));
if (caps->request) {
-/* Report the previous client has disconneced. */
+/* Report the previous client has disconnected. */
do_client_disconnect();
if (debug)
syslog(LOG_DEBUG, "New client connected");
diff --git a/src/vdagentd/virtio-port.h b/src/vdagentd/virtio-port.h
index 810ea8a..f899e30 100644
--- a/src/vdagentd/virtio-port.h
+++ b/src/vdagentd/virtio-port.h
@@ -46,7 +46,7 @@ typedef int (*vdagent_virtio_port_read_callback)(
this callback has completed!
2) This callback is always called, even if the disconnect is initiated
by the vdagent_virtio_port user through returning -1 from a read
- callback, or by explictly calling vdagent_virtio_port_destroy */
+ callback, or by explicitly calling vdagent_virtio_port_destroy */
typedef void (*vdagent_virtio_port_disconnect_callback)(
struct vdagent_virtio_port *conn);
--
2.11.0
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] vdagentd: Spelling fixes for the man page and traces
Signed-off-by: Francois Gouget <fgou...@codeweavers.com>
---
data/spice-vdagentd.1.in | 2 +-
src/vdagentd/console-kit.c | 2 +-
src/vdagentd/vdagentd.c| 4 ++--
src/vdagentd/virtio-port.c | 2 +-
4 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/data/spice-vdagentd.1.in b/data/spice-vdagentd.1.in
index 045b36c..7a00ff1 100644
--- a/data/spice-vdagentd.1.in
+++ b/data/spice-vdagentd.1.in
@@ -20,7 +20,7 @@ resolution, to the number of client windows and their
resolution
Support of copy and paste (text and images) between the active X11 session
and the client, this supports both the primary selection and the clipboard
.P
-Support for transfering files from the client to the agent
+Support for transferring files from the client to the agent
.SH OPTIONS
.TP
\fB-h\fP
diff --git a/src/vdagentd/console-kit.c b/src/vdagentd/console-kit.c
index 9e46ad5..381b97e 100644
--- a/src/vdagentd/console-kit.c
+++ b/src/vdagentd/console-kit.c
@@ -443,7 +443,7 @@ char *session_info_session_for_pid(struct session_info
*info, uint32_t pid)
error.message);
dbus_error_free();
} else
-syslog(LOG_ERR, "GetSessionForUnixProces failed");
+syslog(LOG_ERR, "GetSessionForUnixProcess failed");
goto exit;
}
diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
index 0c54733..f3ac606 100644
--- a/src/vdagentd/vdagentd.c
+++ b/src/vdagentd/vdagentd.c
@@ -329,7 +329,7 @@ static void do_client_file_xfer(struct vdagent_virtio_port
*vport,
VDAgentFileXferStartMessage *s = (VDAgentFileXferStartMessage *)data;
if (!active_session_conn) {
send_file_xfer_status(vport,
- "Could not find an agent connnection belonging to the "
+ "Could not find an agent connection belonging to the "
"active session, cancelling client file-xfer request %u",
s->id, VD_AGENT_FILE_XFER_STATUS_CANCELLED);
return;
@@ -1152,7 +1152,7 @@ int main(int argc, char *argv[])
udscs_destroy_server(server);
if (unlink(vdagentd_socket) != 0)
syslog(LOG_ERR, "unlink %s: %s", vdagentd_socket, strerror(errno));
-syslog(LOG_INFO, "vdagentd quiting, returning status %d", retval);
+syslog(LOG_INFO, "vdagentd quitting, returning status %d", retval);
if (do_daemonize)
unlink(pidfilename);
diff --git a/src/vdagentd/virtio-port.c b/src/vdagentd/virtio-port.c
index ef3de90..3dc6f44 100644
--- a/src/vdagentd/virtio-port.c
+++ b/src/vdagentd/virtio-port.c
@@ -334,7 +334,7 @@ static void vdagent_virtio_port_do_chunk(struct
vdagent_virtio_port **vportp)
avail = vport->chunk_header.size - pos;
if (avail > read) {
-syslog(LOG_ERR, "chunk larger then message, lost sync?");
+syslog(LOG_ERR, "chunk larger than message, lost sync?");
vdagent_virtio_port_destroy(vportp);
return;
}
--
2.11.0
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] vdagent: Spelling and case fixes in comments
Use the official case for Xfce.
Signed-off-by: Francois Gouget <fgou...@codeweavers.com>
---
src/vdagent/vdagent.c | 2 +-
src/vdagent/x11-priv.h | 2 +-
src/vdagent/x11-randr.c | 6 +++---
src/vdagent/x11.c | 10 +-
4 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/src/vdagent/vdagent.c b/src/vdagent/vdagent.c
index 3d195b1..6f025e8 100644
--- a/src/vdagent/vdagent.c
+++ b/src/vdagent/vdagent.c
@@ -182,7 +182,7 @@ static void quit_handler(int sig)
wait to make sure the X connection worked. We wait up
to 10 seconds to get an 'all clear' from the child
before we exit. If we don't, we're able to exit with a
- status that indicates an error occured */
+ status that indicates an error occurred */
static void wait_and_exit(int s)
{
char buf[4];
diff --git a/src/vdagent/x11-priv.h b/src/vdagent/x11-priv.h
index d60cc07..677a44d 100644
--- a/src/vdagent/x11-priv.h
+++ b/src/vdagent/x11-priv.h
@@ -37,7 +37,7 @@ struct vdagent_x11_selection_request {
struct vdagent_x11_selection_request *next;
};
-/* A conversion request is X11 speak for asking an other app to give its
+/* A conversion request is X11 speak for asking another app to give its
clipboard data to us, we do these on behalf of the spice client to copy
data from the guest to the client. Like selection requests we process
these one at a time. */
diff --git a/src/vdagent/x11-randr.c b/src/vdagent/x11-randr.c
index bc480fe..0809f16 100644
--- a/src/vdagent/x11-randr.c
+++ b/src/vdagent/x11-randr.c
@@ -236,7 +236,7 @@ static void delete_mode(struct vdagent_x11 *x11, int
output_index,
vdagent_x11_restore_error_handler(x11);
}
-/* silly to update everytime for more then one monitor */
+/* silly to update every time for more than one monitor */
update_randr_res(x11, 0);
}
@@ -333,7 +333,7 @@ static XRRModeInfo *create_new_mode(struct vdagent_x11
*x11, int output_index,
// ignore race error, if mode is created by others
vdagent_x11_restore_error_handler(x11);
-/* silly to update everytime for more then one monitor */
+/* silly to update every time for more than one monitor */
update_randr_res(x11, 0);
return find_mode_by_name(x11, modename);
@@ -798,7 +798,7 @@ void vdagent_x11_set_monitor_config(struct vdagent_x11 *x11,
/* ... and disable the ones that would be bigger than
* the new RandR screen once it is resized. If they are enabled the
* XRRSetScreenSize call will fail with BadMatch. They will be
- * reenabled after hanging the screen size.
+ * re-enabled after hanging the screen size.
*/
for (i = 0; i < curr->num_of_monitors; ++i) {
int width, height;
diff --git a/src/vdagent/x11.c b/src/vdagent/x11.c
index 4dd1aa8..6e47ea1 100644
--- a/src/vdagent/x11.c
+++ b/src/vdagent/x11.c
@@ -123,7 +123,7 @@ static void vdagent_x11_get_wm_name(struct vdagent_x11 *x11)
this is when the display-manager (ie gdm) has set, and not cleared the
_NET_SUPPORTING_WM_CHECK property, and the window manager running in
the user session has not yet updated it to point to its window, so its
- pointing to a non existing window. */
+ pointing to a nonexistent window. */
vdagent_x11_set_error_handler(x11, vdagent_x11_ignore_bad_window_handler);
/* Get the window manager SUPPORTING_WM_CHECK window */
@@ -,7 +,7 @@ static void
vdagent_x11_handle_property_delete_notify(struct vdagent_x11 *x11,
x11->selection_req_data_pos += len;
-/* Note we must explictly send a 0 sized XChangeProperty to signal the
+/* Note we must explicitly send a 0 sized XChangeProperty to signal the
incr transfer is done. Hence we do not check if we've send all data
but instead check we've send the final 0 sized XChangeProperty. */
if (len == 0) {
@@ -1333,15 +1333,15 @@ void vdagent_x11_client_disconnected(struct vdagent_x11
*x11)
}
/* Function used to determine the default location to save file-xfers,
- xdg desktop dir or xdg download dir. We error on the save side and use a
- whitelist approach, so any unknown desktops will end up with saving
+ xdg desktop dir or xdg download dir. We err on the safe side and use a
+ whitelist approach, so any unknown desktop will end up with saving
file-xfers to the xdg download dir, and opening the xdg download dir with
xdg-open when the file-xfer completes. */
int vdagent_x11_has_icons_on_desktop(struct vdagent_x11 *x11)
{
const char * const wms_with_icons_on_desktop[] = {
"Metacity", /* GNOME-2 or GNOME-3 fallback */
-"Xfwm4",/* XFCE */
+"Xfwm4",/* Xfce */
"Marco",/* Mate */
NULL
};
--
2.11.0
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] vdagent: Spelling fixes for the man page and traces
Signed-off-by: Francois Gouget <fgou...@codeweavers.com>
---
data/spice-vdagent.1.in | 2 +-
src/vdagent/file-xfers.c | 4 ++--
src/vdagent/x11-randr.c | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/data/spice-vdagent.1.in b/data/spice-vdagent.1.in
index f8547d4..590efa5 100644
--- a/data/spice-vdagent.1.in
+++ b/data/spice-vdagent.1.in
@@ -21,7 +21,7 @@ resolution, to the number of client windows and their
resolution
Support of copy and paste (text and images) between the active X11 session
and the client, this supports both the primary selection and the clipboard
.P
-Support for transfering files from the client to the agent
+Support for transferring files from the client to the agent
.SH OPTIONS
.TP
\fB-h\fP
diff --git a/src/vdagent/file-xfers.c b/src/vdagent/file-xfers.c
index bfb2ce5..b3937a4 100644
--- a/src/vdagent/file-xfers.c
+++ b/src/vdagent/file-xfers.c
@@ -112,7 +112,7 @@ static AgentFileXferTask *vdagent_file_xfers_get_task(
task = g_hash_table_lookup(xfers->xfers, GUINT_TO_POINTER(id));
if (task == NULL)
-syslog(LOG_ERR, "file-xfer: error can not find task %u", id);
+syslog(LOG_ERR, "file-xfer: error cannot find task %u", id);
return task;
}
@@ -207,7 +207,7 @@ void vdagent_file_xfers_start(struct vdagent_file_xfers
*xfers,
g_free(task->file_name);
task->file_name = path;
if (i == 64) {
-syslog(LOG_ERR, "file-xfer: more then 63 copies of %s exist?",
+syslog(LOG_ERR, "file-xfer: more than 63 copies of %s exist?",
file_path);
goto error;
}
diff --git a/src/vdagent/x11-randr.c b/src/vdagent/x11-randr.c
index 0809f16..aade5ca 100644
--- a/src/vdagent/x11-randr.c
+++ b/src/vdagent/x11-randr.c
@@ -128,7 +128,7 @@ void vdagent_x11_randr_init(struct vdagent_x11 *x11)
int i;
if (x11->screen_count > 1) {
-syslog(LOG_WARNING, "X-server has more then 1 screen, "
+syslog(LOG_WARNING, "X-server has more than 1 screen, "
"disabling client -> guest resolution syncing");
return;
}
--
2.11.0
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] README: A couple of spelling fixes and slight rewording
Signed-off-by: Francois Gouget <fgou...@codeweavers.com> --- Note: I don't have commit access. README | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/README b/README index 0cd42d0..2eefbc4 100644 --- a/README +++ b/README @@ -20,12 +20,12 @@ Features: * Automatic adjustment of the X-session resolution to the client resolution * Support of copy and paste (text and images) between the active X-session and the client. This supports both the primary selection and the clipboard. -* Support for transfering files from the client to the agent +* Support for transferring files from the client to the agent * Full support for multiple displays using Xrandr, this requires a new enough xorg-x11-drv-qxl driver, as well as a new enough host. * Limited support for multiple displays using Xinerama, prerequisites: - * A new enough Xorg-server. For Fedora atleast Fedora-17, for RHEL-6 atleast -xorg-x11-server-1.10.4-6.el6_2.3 + * A new enough Xorg server: Fedora 17 or greater, for RHEL 6 +xorg-x11-server-1.10.4-6.el6_2.3 or greater. * A vm configured with multiple qxl devices * A guest running the latest spice-vdagent Then connect to the vm with the multiple monitor client which you want to -- 2.11.0 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [qxl] Spelling and typo fixes in some comments
On Tue, 4 Apr 2017, Jonathon Jongsma wrote:
> Acked-by: Jonathon Jongsma <jjong...@redhat.com>
Could you or someone else please commit this patch?
> On Tue, 2017-04-04 at 17:02 +0200, Francois Gouget wrote:
> > Signed-off-by: Francois Gouget <fgou...@codeweavers.com>
> > ---
> > src/mspace.c| 8
> > src/qxl_mem.c | 2 +-
> > src/spiceqxl_display.c | 2 +-
> > src/spiceqxl_io_port.c | 2 +-
> > src/spiceqxl_main_loop.c| 2 +-
> > src/spiceqxl_spice_server.c | 2 +-
> > src/uxa/uxa.h | 2 +-
> > 7 files changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/mspace.c b/src/mspace.c
> > index 822aade..60fc631 100644
> > --- a/src/mspace.c
> > +++ b/src/mspace.c
> > @@ -129,7 +129,7 @@ struct mallinfo {
> > #define SIZE_T_BITSIZE (sizeof(size_t) << 3)
> >
> > /* Some constants coerced to size_t */
> > -/* Annoying but necessary to avoid errors on some plaftorms */
> > +/* Annoying but necessary to avoid errors on some platforms */
> > #define SIZE_T_ZERO ((size_t)0)
> > #define SIZE_T_ONE ((size_t)1)
> > #define SIZE_T_TWO ((size_t)2)
> > @@ -286,7 +286,7 @@ struct mallinfo {
> > Each freshly allocated chunk must have both cinuse and pinuse set.
> > That is, each allocated chunk borders either a previously
> > allocated
> > and still in-use chunk, or the base of its memory arena. This is
> > - ensured by making all allocations from the the `lowest' part of
> > any
> > + ensured by making all allocations from the `lowest' part of any
> > found chunk. Further, no free chunk physically borders another
> > one,
> > so each free chunk is known to be preceded and followed by either
> > inuse chunks or the ends of memory.
> > @@ -480,12 +480,12 @@ nextchunk-> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-
> > +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> > of the same size are arranged in a circularly-linked list, with
> > only
> > the oldest chunk (the next to be used, in our FIFO ordering)
> > actually in the tree. (Tree members are distinguished by a non-
> > null
> > - parent pointer.) If a chunk with the same size an an existing
> > node
> > + parent pointer.) If a chunk with the same size as an existing
> > node
> > is inserted, it is linked off the existing node using pointers
> > that
> > work in the same way as fd/bk pointers of small chunks.
> >
> > Each tree contains a power of 2 sized range of chunk sizes (the
> > - smallest is 0x100 <= x < 0x180), which is is divided in half at
> > each
> > + smallest is 0x100 <= x < 0x180), which is divided in half at each
> > tree level, with the chunks in the smaller half of the range
> > (0x100
> > <= x < 0x140 for the top nose) in the left subtree and the larger
> > half (0x140 <= x < 0x180) in the right subtree. This is, of
> > course,
> > diff --git a/src/qxl_mem.c b/src/qxl_mem.c
> > index fde0976..bed0c1f 100644
> > --- a/src/qxl_mem.c
> > +++ b/src/qxl_mem.c
> > @@ -521,7 +521,7 @@ static void qxl_bo_output_bo_reloc(qxl_screen_t
> > *qxl, uint32_t dst_offset,
> > uint8_t slot_id;
> > uint64_t value;
> >
> > -/* take a refernce on the bo */
> > +/* take a reference on the bo */
> > src_bo->refcnt++;
> >
> > slot_id = src_bo->type == QXL_BO_SURF ? qxl->vram_mem_slot :
> > qxl->main_mem_slot;
> > diff --git a/src/spiceqxl_display.c b/src/spiceqxl_display.c
> > index c3609d8..4e1e382 100644
> > --- a/src/spiceqxl_display.c
> > +++ b/src/spiceqxl_display.c
> > @@ -163,7 +163,7 @@ static int interface_get_command(QXLInstance
> > *sin, struct QXLCommandExt *ext)
> > qxl_send_events(qxl, QXL_INTERRUPT_DISPLAY);
> > }
> > qxl->guest_primary.commands++;
> > -// TODO: reenable, useful
> > +// TODO: re-enable, useful
> > //qxl_track_command(qxl, ext);
> > //qxl_log_command(qxl, "cmd", ext);
> > return TRUE;
> > diff --git a/src/spiceqxl_io_port.c b/src/spiceqxl_io_port.c
> > index 6721d3f..4ebb50b 100644
> > --- a/src/spiceqxl_io_port.c
> > +++ b/src/spiceqxl_io_port.c
> > @@ -31,7 +31,7 @@
> > #include "qxl.h"
> > #include "spiceqxl_io_port.h"
> >
> > -/* TODO: taken from qemu qxl.c, try to remove dupplication */
> > +/* TODO: taken from qemu qxl.c, try to remo
Re: [Spice-devel] [qxl] Spelling fixes in the READMEs and configuration samples
On Tue, 4 Apr 2017, Jonathon Jongsma wrote:
> Acked-by: Jonathon Jongsma <jjong...@redhat.com>
Could you or someone else please commit this patch?
> On Tue, 2017-04-04 at 17:00 +0200, Francois Gouget wrote:
> > Signed-off-by: Francois Gouget <fgou...@codeweavers.com>
> > ---
> > README | 2 +-
> > README.xspice | 2 +-
> > src/spiceccid/spice.pcsc.conf.template | 2 +-
> > 3 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/README b/README
> > index 5ae3a59..92bb579 100644
> > --- a/README
> > +++ b/README
> > @@ -1,4 +1,4 @@
> > -The QXL virtual GPU is found in the RedHat Enterprise
> > +The QXL virtual GPU is found in the Red Hat Enterprise
> > Virtualisation system, and also in the spice project.
> >
> > All questions regarding this software should be directed at the
> > diff --git a/README.xspice b/README.xspice
> > index 803bc6c..9362f47 100644
> > --- a/README.xspice
> > +++ b/README.xspice
> > @@ -90,7 +90,7 @@ git clone
> > git://anongit.freedesktop.org/xorg/driver/xf86-video-qxl xspice
> > build and install into some non common prefix (not to overwrite
> > your existing server) - note that this is just for testing. This
> > should all work with the default server as well, but that server
> > -requires root generally and this is undesireable for testing (and
> > +requires root generally and this is undesirable for testing (and
> > running actually).
> >
> > export PKG_CONFIG_PATH=${TEST}/lib/pkgconfig:${TEST}/share/pkgconfig
> > diff --git a/src/spiceccid/spice.pcsc.conf.template
> > b/src/spiceccid/spice.pcsc.conf.template
> > index 345cdf5..a81a633 100644
> > --- a/src/spiceccid/spice.pcsc.conf.template
> > +++ b/src/spiceccid/spice.pcsc.conf.template
> > @@ -1,5 +1,5 @@
> > # Spice CCID Reader
> > -# This configuration file is the format required by the pcscd
> > deamon for
> > +# This configuration file is the format required by the pcscd
> > daemon for
> > # serial devices; so the qxl driver looks like a serial device to
> > pcscd.
> > FRIENDLYNAME "Spice ccid"
> > DEVICENAME/tmp/spice.pcsc.comm
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
--
Francois Gouget <fgou...@codeweavers.com>___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [client v2 4/5] streaming: Remove the video decoder's dependency on SpiceMsgIn messages
This improves the separation between networking and the video decoding
components.
It also makes it easier to reuse the latter should the client one day
receive video streams through other messages.
Signed-off-by: Francois Gouget <fgou...@codeweavers.com>
---
src/channel-display-gst.c | 79 +++--
src/channel-display-mjpeg.c | 78 ++--
src/channel-display-priv.h | 24 +++---
src/channel-display.c | 35 +++-
4 files changed, 113 insertions(+), 103 deletions(-)
diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 2c002eb0..9b79403e 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -90,23 +90,22 @@ G_STATIC_ASSERT(G_N_ELEMENTS(gst_opts) <=
SPICE_VIDEO_CODEC_TYPE_ENUM_END);
typedef struct SpiceGstFrame {
GstClockTime timestamp;
-SpiceMsgIn *msg;
+SpiceFrame *frame;
GstSample *sample;
} SpiceGstFrame;
-static SpiceGstFrame *create_gst_frame(GstBuffer *buffer, SpiceMsgIn *msg)
+static SpiceGstFrame *create_gst_frame(GstBuffer *buffer, SpiceFrame *frame)
{
SpiceGstFrame *gstframe = spice_new(SpiceGstFrame, 1);
gstframe->timestamp = GST_BUFFER_PTS(buffer);
-gstframe->msg = msg;
-spice_msg_in_ref(msg);
+gstframe->frame = frame;
gstframe->sample = NULL;
return gstframe;
}
static void free_gst_frame(SpiceGstFrame *gstframe)
{
-spice_msg_in_unref(gstframe->msg);
+gstframe->frame->free(gstframe->frame);
if (gstframe->sample) {
gst_sample_unref(gstframe->sample);
}
@@ -160,7 +159,7 @@ static gboolean display_frame(gpointer video_decoder)
goto error;
}
-stream_display_frame(decoder->base.stream, gstframe->msg,
+stream_display_frame(decoder->base.stream, gstframe->frame,
width, height, mapinfo.data);
gst_buffer_unmap(buffer, );
@@ -182,9 +181,8 @@ static void schedule_frame(SpiceGstDecoder *decoder)
break;
}
-SpiceStreamDataHeader *op = spice_msg_in_parsed(gstframe->msg);
-if (now < op->multi_media_time) {
-decoder->timer_id = g_timeout_add(op->multi_media_time - now,
+if (now < gstframe->frame->mm_time) {
+decoder->timer_id = g_timeout_add(gstframe->frame->mm_time - now,
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
@@ -193,8 +191,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 - op->multi_media_time,
-op->multi_media_time, now);
+__FUNCTION__, now - gstframe->frame->mm_time,
+gstframe->frame->mm_time, now);
stream_dropped_frame_on_playback(decoder->base.stream);
g_queue_pop_head(decoder->display_queue);
free_gst_frame(gstframe);
@@ -411,23 +409,17 @@ static void spice_gst_decoder_destroy(VideoDecoder
*video_decoder)
*/
}
-static void release_buffer_data(gpointer data)
-{
-SpiceMsgIn* frame_msg = (SpiceMsgIn*)data;
-spice_msg_in_unref(frame_msg);
-}
-/* spice_gst_decoder_queue_frame() queues the SpiceMsgIn message for decoding
- * and displaying. The steps it goes through are as follows:
+/* spice_gst_decoder_queue_frame() queues the SpiceFrame for decoding and
+ * displaying. The steps it goes through are as follows:
*
- * 1) A SpiceGstFrame is created to keep track of SpiceMsgIn and some
additional
- *metadata. SpiceMsgIn is reffed. The SpiceFrame is then pushed to the
- *decoding_queue.
- * 2) The data part of SpiceMsgIn, which contains the compressed frame data,
- *is wrapped in a GstBuffer and is pushed to the GStreamer pipeline for
- *decoding. SpiceMsgIn is reffed.
+ * 1) A SpiceGstFrame is created to keep track of SpiceFrame and some
additional
+ *metadata. The SpiceGstFrame is then pushed to the decoding_queue.
+ * 2) frame->data, which contains the compressed frame data, is reffed and
+ *wrapped in a GstBuffer which is pushed to the GStreamer pipeline for
+ *decoding.
* 3) As soon as the GStreamer pipeline no longer needs the compressed frame it
- *calls release_buffer_data() to unref SpiceMsgIn.
+ *will call frame->unref_data() to free it.
* 4) Once the decompressed frame is available the GStreamer pipeline calls
*new_sample() in the GStreamer thread.
* 5) new_sample() then matches the decompressed frame to a SpiceGstFrame
[Spice-devel] [client v2 5/5] streaming: Separate the network code from the display_stream management
This makes it easier to reuse display_streams for other types of
video streams should the need arise.
Signed-off-by: Francois Gouget <fgou...@codeweavers.com>
---
src/channel-display.c | 110 --
1 file changed, 62 insertions(+), 48 deletions(-)
diff --git a/src/channel-display.c b/src/channel-display.c
index 00b54406..6b0efaff 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -109,7 +109,7 @@ static display_surface
*find_surface(SpiceDisplayChannelPrivate *c, guint32 surf
static void spice_display_channel_reset(SpiceChannel *channel, gboolean
migrating);
static void spice_display_channel_reset_capabilities(SpiceChannel *channel);
static void destroy_canvas(display_surface *surface);
-static void destroy_stream(SpiceChannel *channel, int id);
+static void destroy_display_stream(display_stream *st, int id);
static void display_session_mm_time_reset_cb(SpiceSession *session, gpointer
data);
static SpiceGlScanout* spice_gl_scanout_copy(const SpiceGlScanout *scanout);
@@ -1168,11 +1168,57 @@ static display_stream *get_stream_by_id(SpiceChannel
*channel, uint32_t id)
}
/* coroutine context */
+static display_stream *display_stream_create(SpiceChannel *channel, uint32_t
surface_id, uint32_t flags, uint32_t codec_type)
+{
+SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
+display_stream *st = g_new0(display_stream, 1);
+
+st->flags = flags;
+st->surface = find_surface(c, surface_id);
+st->channel = channel;
+st->drops_seqs_stats_arr = g_array_new(FALSE, FALSE,
sizeof(drops_sequence_stats));
+
+region_init(>region);
+display_update_stream_region(st);
+
+switch (codec_type) {
+#ifdef HAVE_BUILTIN_MJPEG
+case SPICE_VIDEO_CODEC_TYPE_MJPEG:
+st->video_decoder = create_mjpeg_decoder(codec_type, st);
+break;
+#endif
+default:
+#ifdef HAVE_GSTVIDEO
+st->video_decoder = create_gstreamer_decoder(codec_type, st);
+#endif
+break;
+}
+if (st->video_decoder == NULL) {
+spice_printerr("could not create a video decoder for codec %u",
codec_type);
+destroy_display_stream(st, 0);
+st = NULL;
+}
+return st;
+}
+
+static void destroy_stream(SpiceChannel *channel, int id)
+{
+SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
+
+g_return_if_fail(c != NULL);
+g_return_if_fail(c->streams != NULL);
+g_return_if_fail(c->nstreams > id);
+
+if (c->streams[id]) {
+destroy_display_stream(c->streams[id], id);
+c->streams[id] = NULL;
+}
+}
+
static void display_handle_stream_create(SpiceChannel *channel, SpiceMsgIn *in)
{
SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
SpiceMsgDisplayStreamCreate *op = spice_msg_in_parsed(in);
-display_stream *st;
CHANNEL_DEBUG(channel, "%s: id %u", __FUNCTION__, op->id);
@@ -1188,36 +1234,15 @@ static void display_handle_stream_create(SpiceChannel
*channel, SpiceMsgIn *in)
memset(c->streams + n, 0, (c->nstreams - n) * sizeof(c->streams[0]));
}
g_return_if_fail(c->streams[op->id] == NULL);
-c->streams[op->id] = g_new0(display_stream, 1);
-st = c->streams[op->id];
-
-st->flags = op->flags;
-st->dest = op->dest;
-st->clip = op->clip;
-st->surface = find_surface(c, op->surface_id);
-st->channel = channel;
-st->drops_seqs_stats_arr = g_array_new(FALSE, FALSE,
sizeof(drops_sequence_stats));
-region_init(>region);
-display_update_stream_region(st);
-
-switch (op->codec_type) {
-#ifdef HAVE_BUILTIN_MJPEG
-case SPICE_VIDEO_CODEC_TYPE_MJPEG:
-st->video_decoder = create_mjpeg_decoder(op->codec_type, st);
-break;
-#endif
-default:
-#ifdef HAVE_GSTVIDEO
-st->video_decoder = create_gstreamer_decoder(op->codec_type, st);
-#else
-st->video_decoder = NULL;
-#endif
-}
-if (st->video_decoder == NULL) {
-spice_printerr("could not create a video decoder for codec %u",
op->codec_type);
+c->streams[op->id] = display_stream_create(channel, op->surface_id,
op->flags, op->codec_type);
+if (c->streams[op->id] == NULL) {
+spice_printerr("could not create the %u video stream", op->id);
destroy_stream(channel, op->id);
report_invalid_stream(channel, op->id);
+} else {
+c->streams[op->id]->dest = op->dest;
+c->streams[op->id]->clip = op->clip;
}
}
@@ -1503,24 +1528,14 @@ static void display_handle_stream_clip(SpiceChannel
*channel, SpiceMsgIn *in)
display_update_stream_region(st);
}
-static void destroy_stream(SpiceChannel *channel, int id)
+static void destroy_display_stream(display
[Spice-devel] [client v2 3/5] streaming: Rename SpiceFrame to SpiceGstFrame in the GStreamer decoder
This emphasizes that this structure is specific to the GStreamer
decoder.
Signed-off-by: Francois Gouget <fgou...@codeweavers.com>
---
I also removed the underscore in the SpiceFrame struct. It's not used
anywhere else so this has no impact. This name could probably be removed
altogether but naming the structs seems to be the custom so I'm keeping
it.
A side goal is to reduce the changes in the next patch which should make
it easier to review.
src/channel-display-gst.c | 92 +++
1 file changed, 46 insertions(+), 46 deletions(-)
diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 7e0ddde4..2c002eb0 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -86,31 +86,31 @@ G_STATIC_ASSERT(G_N_ELEMENTS(gst_opts) <=
SPICE_VIDEO_CODEC_TYPE_ENUM_END);
#define VALID_VIDEO_CODEC_TYPE(codec) \
(codec > 0 && codec < G_N_ELEMENTS(gst_opts))
-/* -- SpiceFrame -- */
+/* -- SpiceGstFrame -- */
-typedef struct _SpiceFrame {
+typedef struct SpiceGstFrame {
GstClockTime timestamp;
SpiceMsgIn *msg;
GstSample *sample;
-} SpiceFrame;
+} SpiceGstFrame;
-static SpiceFrame *create_frame(GstBuffer *buffer, SpiceMsgIn *msg)
+static SpiceGstFrame *create_gst_frame(GstBuffer *buffer, SpiceMsgIn *msg)
{
-SpiceFrame *frame = spice_new(SpiceFrame, 1);
-frame->timestamp = GST_BUFFER_PTS(buffer);
-frame->msg = msg;
+SpiceGstFrame *gstframe = spice_new(SpiceGstFrame, 1);
+gstframe->timestamp = GST_BUFFER_PTS(buffer);
+gstframe->msg = msg;
spice_msg_in_ref(msg);
-frame->sample = NULL;
-return frame;
+gstframe->sample = NULL;
+return gstframe;
}
-static void free_frame(SpiceFrame *frame)
+static void free_gst_frame(SpiceGstFrame *gstframe)
{
-spice_msg_in_unref(frame->msg);
-if (frame->sample) {
-gst_sample_unref(frame->sample);
+spice_msg_in_unref(gstframe->msg);
+if (gstframe->sample) {
+gst_sample_unref(gstframe->sample);
}
-free(frame);
+free(gstframe);
}
@@ -122,7 +122,7 @@ static void schedule_frame(SpiceGstDecoder *decoder);
static gboolean display_frame(gpointer video_decoder)
{
SpiceGstDecoder *decoder = (SpiceGstDecoder*)video_decoder;
-SpiceFrame *frame;
+SpiceGstFrame *gstframe;
GstCaps *caps;
gint width, height;
GstStructure *s;
@@ -131,17 +131,17 @@ static gboolean display_frame(gpointer video_decoder)
g_mutex_lock(>queues_mutex);
decoder->timer_id = 0;
-frame = g_queue_pop_head(decoder->display_queue);
+gstframe = g_queue_pop_head(decoder->display_queue);
g_mutex_unlock(>queues_mutex);
/* If the queue is empty we don't even need to reschedule */
-g_return_val_if_fail(frame, G_SOURCE_REMOVE);
+g_return_val_if_fail(gstframe, G_SOURCE_REMOVE);
-if (!frame->sample) {
+if (!gstframe->sample) {
spice_warning("got a frame without a sample!");
goto error;
}
-caps = gst_sample_get_caps(frame->sample);
+caps = gst_sample_get_caps(gstframe->sample);
if (!caps) {
spice_warning("GStreamer error: could not get the caps of the sample");
goto error;
@@ -154,18 +154,18 @@ static gboolean display_frame(gpointer video_decoder)
goto error;
}
-buffer = gst_sample_get_buffer(frame->sample);
+buffer = gst_sample_get_buffer(gstframe->sample);
if (!gst_buffer_map(buffer, , GST_MAP_READ)) {
spice_warning("GStreamer error: could not map the buffer");
goto error;
}
-stream_display_frame(decoder->base.stream, frame->msg,
+stream_display_frame(decoder->base.stream, gstframe->msg,
width, height, mapinfo.data);
gst_buffer_unmap(buffer, );
error:
-free_frame(frame);
+free_gst_frame(gstframe);
schedule_frame(decoder);
return G_SOURCE_REMOVE;
}
@@ -177,12 +177,12 @@ static void schedule_frame(SpiceGstDecoder *decoder)
g_mutex_lock(>queues_mutex);
while (!decoder->timer_id) {
-SpiceFrame *frame = g_queue_peek_head(decoder->display_queue);
-if (!frame) {
+SpiceGstFrame *gstframe = g_queue_peek_head(decoder->display_queue);
+if (!gstframe) {
break;
}
-SpiceStreamDataHeader *op = spice_msg_in_parsed(frame->msg);
+SpiceStreamDataHeader *op = spice_msg_in_parsed(gstframe->msg);
if (now < op->multi_media_time) {
decoder->timer_id = g_timeout_add(op->multi_media_time - now,
display_frame, decoder);
@@ -197,7 +197,7 @@ static void schedule_frame(SpiceGstDecoder *decoder)
op->multi_media_time, now);
stream_dropp
[Spice-devel] [client v2 2/5] streaming: Move SpiceMsgIn parsing to display_handle_stream_create()
This regroups all the parsing in one place and makes the rest of the
display_stream code independent from the network messaging details.
Signed-off-by: Francois Gouget <fgou...@codeweavers.com>
---
src/channel-display-priv.h | 7 +++
src/channel-display.c | 37 ++---
2 files changed, 13 insertions(+), 31 deletions(-)
diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
index b9c08a35..c5622f1a 100644
--- a/src/channel-display-priv.h
+++ b/src/channel-display-priv.h
@@ -99,12 +99,11 @@ typedef struct drops_sequence_stats {
} drops_sequence_stats;
struct display_stream {
-SpiceMsgIn *msg_create;
-SpiceMsgIn *msg_clip;
-
/* from messages */
+uint32_tflags;
+SpiceRect dest;
display_surface *surface;
-const SpiceClip *clip;
+SpiceClip clip;
QRegion region;
int have_region;
diff --git a/src/channel-display.c b/src/channel-display.c
index 7a5a23bb..2423fb0e 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -1118,11 +1118,11 @@ static void display_update_stream_region(display_stream
*st)
{
int i;
-switch (st->clip->type) {
+switch (st->clip.type) {
case SPICE_CLIP_TYPE_RECTS:
region_clear(>region);
-for (i = 0; i < st->clip->rects->num_rects; i++) {
-region_add(>region, >clip->rects->rects[i]);
+for (i = 0; i < st->clip.rects->num_rects; i++) {
+region_add(>region, >clip.rects->rects[i]);
}
st->have_region = true;
break;
@@ -1191,9 +1191,9 @@ static void display_handle_stream_create(SpiceChannel
*channel, SpiceMsgIn *in)
c->streams[op->id] = g_new0(display_stream, 1);
st = c->streams[op->id];
-st->msg_create = in;
-spice_msg_in_ref(in);
-st->clip = >clip;
+st->flags = op->flags;
+st->dest = op->dest;
+st->clip = op->clip;
st->surface = find_surface(c, op->surface_id);
st->channel = channel;
st->drops_seqs_stats_arr = g_array_new(FALSE, FALSE,
sizeof(drops_sequence_stats));
@@ -1225,9 +1225,7 @@ static const SpiceRect *stream_get_dest(display_stream
*st, SpiceMsgIn *frame_ms
{
if (frame_msg == NULL ||
spice_msg_in_type(frame_msg) != SPICE_MSG_DISPLAY_STREAM_DATA_SIZED) {
-SpiceMsgDisplayStreamCreate *info =
spice_msg_in_parsed(st->msg_create);
-
-return >dest;
+return >dest;
} else {
SpiceMsgDisplayStreamDataSized *op = spice_msg_in_parsed(frame_msg);
@@ -1236,13 +1234,6 @@ static const SpiceRect *stream_get_dest(display_stream
*st, SpiceMsgIn *frame_ms
}
-static uint32_t stream_get_flags(display_stream *st)
-{
-SpiceMsgDisplayStreamCreate *info = spice_msg_in_parsed(st->msg_create);
-
-return info->flags;
-}
-
G_GNUC_INTERNAL
uint32_t spice_msg_in_frame_data(SpiceMsgIn *frame_msg, uint8_t **data)
{
@@ -1288,7 +1279,7 @@ void stream_display_frame(display_stream *st, SpiceMsgIn
*frame_msg,
dest = stream_get_dest(st, frame_msg);
stride = width * sizeof(uint32_t);
-if (!(stream_get_flags(st) & SPICE_STREAM_FLAGS_TOP_DOWN)) {
+if (!(st->flags & SPICE_STREAM_FLAGS_TOP_DOWN)) {
data += stride * (height - 1);
stride = -stride;
}
@@ -1502,12 +1493,8 @@ static void display_handle_stream_clip(SpiceChannel
*channel, SpiceMsgIn *in)
display_stream *st = get_stream_by_id(channel, op->id);
g_return_if_fail(st != NULL);
-if (st->msg_clip) {
-spice_msg_in_unref(st->msg_clip);
-}
-spice_msg_in_ref(in);
-st->msg_clip = in;
-st->clip = >clip;
+
+st->clip = op->clip;
display_update_stream_region(st);
}
@@ -1561,10 +1548,6 @@ static void destroy_stream(SpiceChannel *channel, int id)
st->video_decoder->destroy(st->video_decoder);
}
-if (st->msg_clip)
-spice_msg_in_unref(st->msg_clip);
-spice_msg_in_unref(st->msg_create);
-
g_free(st);
c->streams[id] = NULL;
}
--
2.11.0
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [client v2 1/5] streaming: Document the GStreamer decoding process
Signed-off-by: Francois Gouget <fgou...@codeweavers.com> --- src/channel-display-gst.c | 26 ++ 1 file changed, 26 insertions(+) diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c index c4190b2d..7e0ddde4 100644 --- a/src/channel-display-gst.c +++ b/src/channel-display-gst.c @@ -417,6 +417,32 @@ static void release_buffer_data(gpointer data) spice_msg_in_unref(frame_msg); } +/* spice_gst_decoder_queue_frame() queues the SpiceMsgIn message for decoding + * and displaying. The steps it goes through are as follows: + * + * 1) A SpiceFrame is created to keep track of SpiceMsgIn and some additional + *metadata. SpiceMsgIn is reffed. The SpiceFrame is then pushed to the + *decoding_queue. + * 2) The data part of SpiceMsgIn, which contains the compressed frame data, + *is wrapped in a GstBuffer and is pushed to the GStreamer pipeline for + *decoding. SpiceMsgIn is reffed. + * 3) As soon as the GStreamer pipeline no longer needs the compressed frame it + *calls release_buffer_data() to unref SpiceMsgIn. + * 4) Once the decompressed frame is available the GStreamer pipeline calls + *new_sample() in the GStreamer thread. + * 5) new_sample() then matches the decompressed frame to a SpiceFrame from + *the decoding queue using the GStreamer timestamp information to deal with + *dropped frames. The SpiceFrame is popped from the decoding_queue. + * 6) new_sample() then attaches the decompressed frame to the SpiceFrame, + *pushes it to the display_queue and calls schedule_frame(). + * 7) schedule_frame() then uses the SpiceMsgIn's mm_time to arrange for + *display_frame() to be called, in the main thread, at the right time for + *the next frame. + * 8) display_frame() pops the first SpiceFrame from the display_queue and + *calls stream_display_frame(). + * 9) display_frame() then frees the SpiceFrame and the decompressed frame. + *SpiceMsgIn is unreffed. + */ static gboolean spice_gst_decoder_queue_frame(VideoDecoder *video_decoder, SpiceMsgIn *frame_msg, int32_t latency) -- 2.11.0 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [client v2 0/5] Abstract video streaming from the network details
Currently the video decoders are directly passed network messages which ties them pretty closely to the network code. Should the protocol switch to a new type of messages the video decoding code will need to be updated. Even worse, it will have to deal with two types of network messages for backward compatiblity. So I feel it's better to try and separate the two which is what I've done in the first two patches. The third one does not really change the code much, it just separates the code that creates / destroys the display_stream objects from the code that handles the network messages leading to their creation / destruction. Changes from v1: * Add a patch documenting the steps each frame goes through in the GStreamer decoder, along with lifetime information. * Rename SpiceMetaFrame to SpiceGstFrame. * Separate renaming SpiceFrame to SpiceGstFrame from the patch removing the video decoder's dependency on SpiceMsgIn messages. * Tweaked that patch to minimize changes. * Fixed the type of the SpiceGstFrame.sample field. * Remember: I don't have commit access * No patch depends on the ones that follow it. So if the first two patches are ok but not patch 3, please apply the first two so the next round has fewer patches. Francois Gouget (5): streaming: Document the GStreamer decoding process streaming: Move SpiceMsgIn parsing to display_handle_stream_create() streaming: Rename SpiceFrame to SpiceGstFrame in the GStreamer decoder streaming: Remove the video decoder's dependency on SpiceMsgIn messages streaming: Separate the network code from the display_stream management src/channel-display-gst.c | 149 - src/channel-display-mjpeg.c | 78 ++-- src/channel-display-priv.h | 31 +--- src/channel-display.c | 176 ++-- 4 files changed, 233 insertions(+), 201 deletions(-) -- 2.11.0 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [client 2/3] streaming: Remove the video decoder's dependency on SpiceMsgIn messages
d = 0; > > ->unref_data + ->free can probably be done in a helper for the mjpeg > decoder? Yes. It does mean freeing the network message a bit later than strictly necessary, that is after the frame has been displayed rather than before, but it probably does not matter. We're already keeping it around for much longer since we only decompress the frame at the last millisecond anyway. That is, the timeline is this: 1) Receive a SpiceMsgIn containing a compressed MJPEG frame 40 ms before it is due to be displayed. 2) Wrap it in a SpiceFrame for the MJPEG decoder. 3) mjpeg_decoder_queue_frame() puts the SpiceFrame in decoder->msgq and calls mjpeg_decoder_schedule(). 4) mjpeg_decoder_schedule() arranges for mjpeg_decoder_decode_frame() to be called in the main thread 40 ms from now. 5) 40 ms later it's time to display the frame. Oups it has not be decompressed yet. So do on the double now, blocking the main thread while we're at it. 6) 4 ms or so later the frame has been decompressed. We no longer need the SpiceMsgIn message any more but still keep it. 7) Call stream_display_frame() to display the frame, 4 ms late! 8) Microseconds later (presumably), call free_spice_frame() to free both the SpiceMsgIn message and the SpiceFrame structure. My main beef is with sitting on the compressed message for 40 ms doing nothing with it and then decompressing it only when it's essentially too late. But then MJPEG is really fast to decompress so it does not really matter and I understand why it's done this way (it's simpler, minimises memory usage). So freeing SpiceMsgIn in step 6 or 8 makes no significant difference. In contrast, with GStreamer, depending on the codec and the GStreamer version and bugs, decoding could take a lot more time, possibly into the tens of ms. So GStreamer decoder does things differently. It goes like this: 1) Receive a SpiceMsgIn containing a compressed frame 40 ms before it is due to be displayed. 2) Wrap it in a SpiceFrame for the decoder. 3) spice_gst_decoder_queue_frame() pushes the compressed frame proper (frame->data) to the GStreamer pipeline for decoding. But it still needs to know the where and when to display the decoded frame later to it create a SpiceMetaFrame and pushes that to the decoding_queue. And it returns control immediately to the caller. 4) At some unknown point GStreamer no longer needs the compressed frame data and frees it through frame->unref_data(). 5) Some colorspace conversions later GStreamer calls new_sample() with the decompressed frame. We attach it to the corresponding SpiceMetaFrame structure (which we pop) from the decoding_queue. 6) But it's not time to display it yet so we attach the decompressed frame to the SpiceMetaFrame, push it to the display_queue and call schedule_frame(). 7) schedule_frame() arranges for display_frame() to be called in what may now be only 30 ms away. 8) display_frame() pops the first SpiceMetaFrame from the display_queue and calls stream_display_frame() right on time. 9) display_frame() then frees the SpiceMetaFrame and the SpiceFrame and decompressed frame with it. So as I was saying earlier the compressed frame is freed in step 4 and SpiceGstFrame, SpiceFrame and GstSample in step 9, many milliseconds later. The advantages are that the main thread is not blocked while decoding, and displaying the frame is not delayed by the longer and less predictable decoding times. And the drawbacks are that the frames are queued in their decompressed form rather than the compressed one (increase memory usage), and the code is a bit more complex. -- Francois Gouget <fgou...@codeweavers.com> ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [client 2/3] streaming: Remove the video decoder's dependency on SpiceMsgIn messages
Code-wise this improves the separation between networking and the video
decoding.
It also makes it easier to reuse the latter should the client one day
receive video streams through other messages.
Signed-off-by: Francois Gouget <fgou...@codeweavers.com>
---
src/channel-display-gst.c | 128
src/channel-display-mjpeg.c | 74 +
src/channel-display-priv.h | 24 +++--
src/channel-display.c | 35 ++--
4 files changed, 134 insertions(+), 127 deletions(-)
diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index c4190b2d..93b61bab 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -86,31 +86,30 @@ G_STATIC_ASSERT(G_N_ELEMENTS(gst_opts) <=
SPICE_VIDEO_CODEC_TYPE_ENUM_END);
#define VALID_VIDEO_CODEC_TYPE(codec) \
(codec > 0 && codec < G_N_ELEMENTS(gst_opts))
-/* -- SpiceFrame -- */
+/* -- SpiceMetaFrame -- */
-typedef struct _SpiceFrame {
-GstClockTime timestamp;
-SpiceMsgIn *msg;
-GstSample *sample;
-} SpiceFrame;
+typedef struct SpiceMetaFrame {
+SpiceFrame *frame;
+GstClockTime pts;
+void *sample;
+} SpiceMetaFrame;
-static SpiceFrame *create_frame(GstBuffer *buffer, SpiceMsgIn *msg)
+static SpiceMetaFrame *create_meta_frame(SpiceFrame *frame, GstBuffer *buffer)
{
-SpiceFrame *frame = spice_new(SpiceFrame, 1);
-frame->timestamp = GST_BUFFER_PTS(buffer);
-frame->msg = msg;
-spice_msg_in_ref(msg);
-frame->sample = NULL;
-return frame;
+SpiceMetaFrame *meta = spice_new(SpiceMetaFrame, 1);
+meta->frame = frame;
+meta->pts = GST_BUFFER_PTS(buffer);
+meta->sample = NULL;
+return meta;
}
-static void free_frame(SpiceFrame *frame)
+static void free_meta_frame(SpiceMetaFrame *meta)
{
-spice_msg_in_unref(frame->msg);
-if (frame->sample) {
-gst_sample_unref(frame->sample);
+meta->frame->free(meta->frame);
+if (meta->sample) {
+gst_sample_unref(meta->sample);
}
-free(frame);
+free(meta);
}
@@ -122,7 +121,7 @@ static void schedule_frame(SpiceGstDecoder *decoder);
static gboolean display_frame(gpointer video_decoder)
{
SpiceGstDecoder *decoder = (SpiceGstDecoder*)video_decoder;
-SpiceFrame *frame;
+SpiceMetaFrame *meta;
GstCaps *caps;
gint width, height;
GstStructure *s;
@@ -131,17 +130,17 @@ static gboolean display_frame(gpointer video_decoder)
g_mutex_lock(>queues_mutex);
decoder->timer_id = 0;
-frame = g_queue_pop_head(decoder->display_queue);
+meta = g_queue_pop_head(decoder->display_queue);
g_mutex_unlock(>queues_mutex);
/* If the queue is empty we don't even need to reschedule */
-g_return_val_if_fail(frame, G_SOURCE_REMOVE);
+g_return_val_if_fail(meta, G_SOURCE_REMOVE);
-if (!frame->sample) {
+if (!meta->sample) {
spice_warning("got a frame without a sample!");
goto error;
}
-caps = gst_sample_get_caps(frame->sample);
+caps = gst_sample_get_caps(meta->sample);
if (!caps) {
spice_warning("GStreamer error: could not get the caps of the sample");
goto error;
@@ -154,18 +153,18 @@ static gboolean display_frame(gpointer video_decoder)
goto error;
}
-buffer = gst_sample_get_buffer(frame->sample);
+buffer = gst_sample_get_buffer(meta->sample);
if (!gst_buffer_map(buffer, , GST_MAP_READ)) {
spice_warning("GStreamer error: could not map the buffer");
goto error;
}
-stream_display_frame(decoder->base.stream, frame->msg,
+stream_display_frame(decoder->base.stream, meta->frame,
width, height, mapinfo.data);
gst_buffer_unmap(buffer, );
error:
-free_frame(frame);
+free_meta_frame(meta);
schedule_frame(decoder);
return G_SOURCE_REMOVE;
}
@@ -177,14 +176,13 @@ static void schedule_frame(SpiceGstDecoder *decoder)
g_mutex_lock(>queues_mutex);
while (!decoder->timer_id) {
-SpiceFrame *frame = g_queue_peek_head(decoder->display_queue);
-if (!frame) {
+SpiceMetaFrame *meta = g_queue_peek_head(decoder->display_queue);
+if (!meta) {
break;
}
-SpiceStreamDataHeader *op = spice_msg_in_parsed(frame->msg);
-if (now < op->multi_media_time) {
-decoder->timer_id = g_timeout_add(op->multi_media_time - now,
+if (now < meta->frame->mm_time) {
+decoder->timer_id = g_timeout_add(meta->frame->mm_time - now,
display_frame, decoder);
} else if (g_queue_get_length(decoder->display_queue) == 1) {
/* Still attempt to display the least ou
[Spice-devel] [client 1/3] streaming: Remove the display_stream dependency on SpiceMsgIn messages
Code-wise this improves the separation between networking and the video
decoding.
It also makes it easier to reuse the latter should the client one day
receive video streams through other messages.
Signed-off-by: Francois Gouget <fgou...@codeweavers.com>
---
src/channel-display-priv.h | 7 +++
src/channel-display.c | 37 ++---
2 files changed, 13 insertions(+), 31 deletions(-)
diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
index b9c08a35..60cc8efa 100644
--- a/src/channel-display-priv.h
+++ b/src/channel-display-priv.h
@@ -99,12 +99,11 @@ typedef struct drops_sequence_stats {
} drops_sequence_stats;
struct display_stream {
-SpiceMsgIn *msg_create;
-SpiceMsgIn *msg_clip;
-
/* from messages */
+uint32_t flags;
+SpiceRect dest;
display_surface *surface;
-const SpiceClip *clip;
+SpiceClip clip;
QRegion region;
int have_region;
diff --git a/src/channel-display.c b/src/channel-display.c
index 7a5a23bb..2423fb0e 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -1118,11 +1118,11 @@ static void display_update_stream_region(display_stream
*st)
{
int i;
-switch (st->clip->type) {
+switch (st->clip.type) {
case SPICE_CLIP_TYPE_RECTS:
region_clear(>region);
-for (i = 0; i < st->clip->rects->num_rects; i++) {
-region_add(>region, >clip->rects->rects[i]);
+for (i = 0; i < st->clip.rects->num_rects; i++) {
+region_add(>region, >clip.rects->rects[i]);
}
st->have_region = true;
break;
@@ -1191,9 +1191,9 @@ static void display_handle_stream_create(SpiceChannel
*channel, SpiceMsgIn *in)
c->streams[op->id] = g_new0(display_stream, 1);
st = c->streams[op->id];
-st->msg_create = in;
-spice_msg_in_ref(in);
-st->clip = >clip;
+st->flags = op->flags;
+st->dest = op->dest;
+st->clip = op->clip;
st->surface = find_surface(c, op->surface_id);
st->channel = channel;
st->drops_seqs_stats_arr = g_array_new(FALSE, FALSE,
sizeof(drops_sequence_stats));
@@ -1225,9 +1225,7 @@ static const SpiceRect *stream_get_dest(display_stream
*st, SpiceMsgIn *frame_ms
{
if (frame_msg == NULL ||
spice_msg_in_type(frame_msg) != SPICE_MSG_DISPLAY_STREAM_DATA_SIZED) {
-SpiceMsgDisplayStreamCreate *info =
spice_msg_in_parsed(st->msg_create);
-
-return >dest;
+return >dest;
} else {
SpiceMsgDisplayStreamDataSized *op = spice_msg_in_parsed(frame_msg);
@@ -1236,13 +1234,6 @@ static const SpiceRect *stream_get_dest(display_stream
*st, SpiceMsgIn *frame_ms
}
-static uint32_t stream_get_flags(display_stream *st)
-{
-SpiceMsgDisplayStreamCreate *info = spice_msg_in_parsed(st->msg_create);
-
-return info->flags;
-}
-
G_GNUC_INTERNAL
uint32_t spice_msg_in_frame_data(SpiceMsgIn *frame_msg, uint8_t **data)
{
@@ -1288,7 +1279,7 @@ void stream_display_frame(display_stream *st, SpiceMsgIn
*frame_msg,
dest = stream_get_dest(st, frame_msg);
stride = width * sizeof(uint32_t);
-if (!(stream_get_flags(st) & SPICE_STREAM_FLAGS_TOP_DOWN)) {
+if (!(st->flags & SPICE_STREAM_FLAGS_TOP_DOWN)) {
data += stride * (height - 1);
stride = -stride;
}
@@ -1502,12 +1493,8 @@ static void display_handle_stream_clip(SpiceChannel
*channel, SpiceMsgIn *in)
display_stream *st = get_stream_by_id(channel, op->id);
g_return_if_fail(st != NULL);
-if (st->msg_clip) {
-spice_msg_in_unref(st->msg_clip);
-}
-spice_msg_in_ref(in);
-st->msg_clip = in;
-st->clip = >clip;
+
+st->clip = op->clip;
display_update_stream_region(st);
}
@@ -1561,10 +1548,6 @@ static void destroy_stream(SpiceChannel *channel, int id)
st->video_decoder->destroy(st->video_decoder);
}
-if (st->msg_clip)
-spice_msg_in_unref(st->msg_clip);
-spice_msg_in_unref(st->msg_create);
-
g_free(st);
c->streams[id] = NULL;
}
--
2.11.0
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [client 3/3] streaming: Separate the network code from the display_stream management
This makes it easier to reuse display_streams for other types of
video streams should the need arise.
Signed-off-by: Francois Gouget <fgou...@codeweavers.com>
---
src/channel-display.c | 110 --
1 file changed, 62 insertions(+), 48 deletions(-)
diff --git a/src/channel-display.c b/src/channel-display.c
index 00b54406..6b0efaff 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -109,7 +109,7 @@ static display_surface
*find_surface(SpiceDisplayChannelPrivate *c, guint32 surf
static void spice_display_channel_reset(SpiceChannel *channel, gboolean
migrating);
static void spice_display_channel_reset_capabilities(SpiceChannel *channel);
static void destroy_canvas(display_surface *surface);
-static void destroy_stream(SpiceChannel *channel, int id);
+static void destroy_display_stream(display_stream *st, int id);
static void display_session_mm_time_reset_cb(SpiceSession *session, gpointer
data);
static SpiceGlScanout* spice_gl_scanout_copy(const SpiceGlScanout *scanout);
@@ -1168,11 +1168,57 @@ static display_stream *get_stream_by_id(SpiceChannel
*channel, uint32_t id)
}
/* coroutine context */
+static display_stream *display_stream_create(SpiceChannel *channel, uint32_t
surface_id, uint32_t flags, uint32_t codec_type)
+{
+SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
+display_stream *st = g_new0(display_stream, 1);
+
+st->flags = flags;
+st->surface = find_surface(c, surface_id);
+st->channel = channel;
+st->drops_seqs_stats_arr = g_array_new(FALSE, FALSE,
sizeof(drops_sequence_stats));
+
+region_init(>region);
+display_update_stream_region(st);
+
+switch (codec_type) {
+#ifdef HAVE_BUILTIN_MJPEG
+case SPICE_VIDEO_CODEC_TYPE_MJPEG:
+st->video_decoder = create_mjpeg_decoder(codec_type, st);
+break;
+#endif
+default:
+#ifdef HAVE_GSTVIDEO
+st->video_decoder = create_gstreamer_decoder(codec_type, st);
+#endif
+break;
+}
+if (st->video_decoder == NULL) {
+spice_printerr("could not create a video decoder for codec %u",
codec_type);
+destroy_display_stream(st, 0);
+st = NULL;
+}
+return st;
+}
+
+static void destroy_stream(SpiceChannel *channel, int id)
+{
+SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
+
+g_return_if_fail(c != NULL);
+g_return_if_fail(c->streams != NULL);
+g_return_if_fail(c->nstreams > id);
+
+if (c->streams[id]) {
+destroy_display_stream(c->streams[id], id);
+c->streams[id] = NULL;
+}
+}
+
static void display_handle_stream_create(SpiceChannel *channel, SpiceMsgIn *in)
{
SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
SpiceMsgDisplayStreamCreate *op = spice_msg_in_parsed(in);
-display_stream *st;
CHANNEL_DEBUG(channel, "%s: id %u", __FUNCTION__, op->id);
@@ -1188,36 +1234,15 @@ static void display_handle_stream_create(SpiceChannel
*channel, SpiceMsgIn *in)
memset(c->streams + n, 0, (c->nstreams - n) * sizeof(c->streams[0]));
}
g_return_if_fail(c->streams[op->id] == NULL);
-c->streams[op->id] = g_new0(display_stream, 1);
-st = c->streams[op->id];
-
-st->flags = op->flags;
-st->dest = op->dest;
-st->clip = op->clip;
-st->surface = find_surface(c, op->surface_id);
-st->channel = channel;
-st->drops_seqs_stats_arr = g_array_new(FALSE, FALSE,
sizeof(drops_sequence_stats));
-region_init(>region);
-display_update_stream_region(st);
-
-switch (op->codec_type) {
-#ifdef HAVE_BUILTIN_MJPEG
-case SPICE_VIDEO_CODEC_TYPE_MJPEG:
-st->video_decoder = create_mjpeg_decoder(op->codec_type, st);
-break;
-#endif
-default:
-#ifdef HAVE_GSTVIDEO
-st->video_decoder = create_gstreamer_decoder(op->codec_type, st);
-#else
-st->video_decoder = NULL;
-#endif
-}
-if (st->video_decoder == NULL) {
-spice_printerr("could not create a video decoder for codec %u",
op->codec_type);
+c->streams[op->id] = display_stream_create(channel, op->surface_id,
op->flags, op->codec_type);
+if (c->streams[op->id] == NULL) {
+spice_printerr("could not create the %u video stream", op->id);
destroy_stream(channel, op->id);
report_invalid_stream(channel, op->id);
+} else {
+c->streams[op->id]->dest = op->dest;
+c->streams[op->id]->clip = op->clip;
}
}
@@ -1503,24 +1528,14 @@ static void display_handle_stream_clip(SpiceChannel
*channel, SpiceMsgIn *in)
display_update_stream_region(st);
}
-static void destroy_stream(SpiceChannel *channel, int id)
+static void destroy_display_stream(display
[Spice-devel] [client 0/3] Abstract video streaming from the network details
Currently the video decoders are directly passed network messages which ties them pretty closely to the network code. Should the protocol switch to a new type of messages the video decoding code will need to be updated. Even worse, it will have to deal with two types of network messages for backward compatiblity. So I feel it's better to try and separate the two which is what I've done in the first two patches. The third one does not really change the code much, it just separates the code that creates / destroys the display_stream objects from the code that handles the network messages leading to their creation / destruction. Francois Gouget (3): streaming: Remove the display_stream dependency on SpiceMsgIn messages streaming: Remove the video decoder's dependency on SpiceMsgIn messages streaming: Separate the network code from the display_stream management src/channel-display-gst.c | 128 +++- src/channel-display-mjpeg.c | 74 +-- src/channel-display-priv.h | 31 +--- src/channel-display.c | 176 ++-- 4 files changed, 206 insertions(+), 203 deletions(-) -- 2.11.0 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [qxl] Spelling and typo fixes in some comments
Signed-off-by: Francois Gouget <fgou...@codeweavers.com>
---
src/mspace.c| 8
src/qxl_mem.c | 2 +-
src/spiceqxl_display.c | 2 +-
src/spiceqxl_io_port.c | 2 +-
src/spiceqxl_main_loop.c| 2 +-
src/spiceqxl_spice_server.c | 2 +-
src/uxa/uxa.h | 2 +-
7 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/src/mspace.c b/src/mspace.c
index 822aade..60fc631 100644
--- a/src/mspace.c
+++ b/src/mspace.c
@@ -129,7 +129,7 @@ struct mallinfo {
#define SIZE_T_BITSIZE (sizeof(size_t) << 3)
/* Some constants coerced to size_t */
-/* Annoying but necessary to avoid errors on some plaftorms */
+/* Annoying but necessary to avoid errors on some platforms */
#define SIZE_T_ZERO ((size_t)0)
#define SIZE_T_ONE ((size_t)1)
#define SIZE_T_TWO ((size_t)2)
@@ -286,7 +286,7 @@ struct mallinfo {
Each freshly allocated chunk must have both cinuse and pinuse set.
That is, each allocated chunk borders either a previously allocated
and still in-use chunk, or the base of its memory arena. This is
- ensured by making all allocations from the the `lowest' part of any
+ ensured by making all allocations from the `lowest' part of any
found chunk. Further, no free chunk physically borders another one,
so each free chunk is known to be preceded and followed by either
inuse chunks or the ends of memory.
@@ -480,12 +480,12 @@ nextchunk->
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
of the same size are arranged in a circularly-linked list, with only
the oldest chunk (the next to be used, in our FIFO ordering)
actually in the tree. (Tree members are distinguished by a non-null
- parent pointer.) If a chunk with the same size an an existing node
+ parent pointer.) If a chunk with the same size as an existing node
is inserted, it is linked off the existing node using pointers that
work in the same way as fd/bk pointers of small chunks.
Each tree contains a power of 2 sized range of chunk sizes (the
- smallest is 0x100 <= x < 0x180), which is is divided in half at each
+ smallest is 0x100 <= x < 0x180), which is divided in half at each
tree level, with the chunks in the smaller half of the range (0x100
<= x < 0x140 for the top nose) in the left subtree and the larger
half (0x140 <= x < 0x180) in the right subtree. This is, of course,
diff --git a/src/qxl_mem.c b/src/qxl_mem.c
index fde0976..bed0c1f 100644
--- a/src/qxl_mem.c
+++ b/src/qxl_mem.c
@@ -521,7 +521,7 @@ static void qxl_bo_output_bo_reloc(qxl_screen_t *qxl,
uint32_t dst_offset,
uint8_t slot_id;
uint64_t value;
-/* take a refernce on the bo */
+/* take a reference on the bo */
src_bo->refcnt++;
slot_id = src_bo->type == QXL_BO_SURF ? qxl->vram_mem_slot :
qxl->main_mem_slot;
diff --git a/src/spiceqxl_display.c b/src/spiceqxl_display.c
index c3609d8..4e1e382 100644
--- a/src/spiceqxl_display.c
+++ b/src/spiceqxl_display.c
@@ -163,7 +163,7 @@ static int interface_get_command(QXLInstance *sin, struct
QXLCommandExt *ext)
qxl_send_events(qxl, QXL_INTERRUPT_DISPLAY);
}
qxl->guest_primary.commands++;
-// TODO: reenable, useful
+// TODO: re-enable, useful
//qxl_track_command(qxl, ext);
//qxl_log_command(qxl, "cmd", ext);
return TRUE;
diff --git a/src/spiceqxl_io_port.c b/src/spiceqxl_io_port.c
index 6721d3f..4ebb50b 100644
--- a/src/spiceqxl_io_port.c
+++ b/src/spiceqxl_io_port.c
@@ -31,7 +31,7 @@
#include "qxl.h"
#include "spiceqxl_io_port.h"
-/* TODO: taken from qemu qxl.c, try to remove dupplication */
+/* TODO: taken from qemu qxl.c, try to remove duplication */
#undef SPICE_RING_PROD_ITEM
#define SPICE_RING_PROD_ITEM(r, ret) { \
typeof(r) start = r;\
diff --git a/src/spiceqxl_main_loop.c b/src/spiceqxl_main_loop.c
index 0213693..c2e8eef 100644
--- a/src/spiceqxl_main_loop.c
+++ b/src/spiceqxl_main_loop.c
@@ -423,7 +423,7 @@ SpiceCoreInterface *basic_event_loop_init(void)
#endif
bzero(, sizeof(core));
core.base.major_version = SPICE_INTERFACE_CORE_MAJOR;
-core.base.minor_version = SPICE_INTERFACE_CORE_MINOR; // anything less
then 3 and channel_event isn't called
+core.base.minor_version = SPICE_INTERFACE_CORE_MINOR; // anything less
than 3 and channel_event isn't called
core.timer_add = timer_add;
core.timer_start = timer_start;
core.timer_cancel = timer_cancel;
diff --git a/src/spiceqxl_spice_server.c b/src/spiceqxl_spice_server.c
index 38257d0..1a138b5 100644
--- a/src/spiceqxl_spice_server.c
+++ b/src/spiceqxl_spice_server.c
@@ -113,7 +113,7 @@ static const char *wan_compression_names[] = {
void xspice_set_spice_server_options(OptionInfoPtr options)
{
-/* environment variables take preceden
[Spice-devel] [qxl] Spelling fixes in the READMEs and configuration samples
Signed-off-by: Francois Gouget <fgou...@codeweavers.com>
---
README | 2 +-
README.xspice | 2 +-
src/spiceccid/spice.pcsc.conf.template | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/README b/README
index 5ae3a59..92bb579 100644
--- a/README
+++ b/README
@@ -1,4 +1,4 @@
-The QXL virtual GPU is found in the RedHat Enterprise
+The QXL virtual GPU is found in the Red Hat Enterprise
Virtualisation system, and also in the spice project.
All questions regarding this software should be directed at the
diff --git a/README.xspice b/README.xspice
index 803bc6c..9362f47 100644
--- a/README.xspice
+++ b/README.xspice
@@ -90,7 +90,7 @@ git clone
git://anongit.freedesktop.org/xorg/driver/xf86-video-qxl xspice
build and install into some non common prefix (not to overwrite
your existing server) - note that this is just for testing. This
should all work with the default server as well, but that server
-requires root generally and this is undesireable for testing (and
+requires root generally and this is undesirable for testing (and
running actually).
export PKG_CONFIG_PATH=${TEST}/lib/pkgconfig:${TEST}/share/pkgconfig
diff --git a/src/spiceccid/spice.pcsc.conf.template
b/src/spiceccid/spice.pcsc.conf.template
index 345cdf5..a81a633 100644
--- a/src/spiceccid/spice.pcsc.conf.template
+++ b/src/spiceccid/spice.pcsc.conf.template
@@ -1,5 +1,5 @@
# Spice CCID Reader
-# This configuration file is the format required by the pcscd deamon for
+# This configuration file is the format required by the pcscd daemon for
# serial devices; so the qxl driver looks like a serial device to pcscd.
FRIENDLYNAME "Spice ccid"
DEVICENAME/tmp/spice.pcsc.comm
--
2.11.0
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] GStreamer's zero-copy code is broken
On Thu, 2 Mar 2017, Frediano Ziglio wrote: [...] > Before I forgot this. > > Looks like GStreamer when you call gst_buffer_add_video_meta_full > assume that buffer is contiguous. The 8 pixel shift (more or less) > you can see are artifacts due to how the guest send the frames but > basically are bytes inside 2 chunks of data. > > Problems happens specifically in gst_video_frame_map_id. Did you report this bug to GStreamer? If not it would be nice if you could dump your current understanding into a bug for future reference. > (passing metadata is also required to pass texture directly to VAAPI). Interesting. Does it need the same type of GstVideoMeta data or some other type of metadata? Did you get the pipeline to work with VAAPI? -- Francois Gouget <fgou...@codeweavers.com> ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
