| Commit message (Collapse) | Author | Age | Files | Lines |
... | |
|
|
|
|
|
|
|
|
|
|
|
| |
playback
This bug results in the client dropping all the video frames after
migration in case that (1) the hosts involved in migration have different
mm-time; and that (2) there is no audio playback.
This is relvant only for the client that was connected during the
migration.
rhbz#958276
|
|
|
|
|
|
|
| |
When a client disconnects, red_channel_client_pipe_clear is called.
Releasing pipe items of type == MIGRATE||EMPTY_MSG||PING
wasn't handled, and was passed to channel_cbs.release_item.
There, an error occured since the pipe items were not recognized.
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
With a SPICE_DISPLAY_CAP_MONITORS_CONFIG capable client, the client needs to
know what part of the primary to use for each monitor. If the guest driver
does not support this, the server sends messages to the client for a
single monitor spanning the entire primary.
As soon as the guest calls spice_qxl_monitors_config_async once, we set
the red_worker driver_has_monitors_config flag and stop doing this.
This is a problem when the driver gets unloaded, for example after a reboot
or when switching to a text vc with usermode mode-setting under Linux.
To reproduce this start a multi-mon capable Linux guest which uses
usermode mode-setting and then once X has started switch to a text vc. Note
how the client window does not only not resize, if you try to resize it
manually you always keep blackborders since the aspect is wrong.
This patch is the spice-server side of fixing this, it adds a new
spice_qxl_driver_unload method which clears the driver_has_monitors_config
flag.
The other patch needed to fix this is in qemu, and will calls this new method
from qxl_enter_vga_mode.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
|
| |
|
| |
|
| |
|
|
|
|
|
| |
SPICE_BIT_RATE can be set for supplying red_worker the available
bandwidth (in Mbps).
|
|
|
|
|
|
|
|
| |
new streams
mjpeg_encoder modify the initial bit we supply it, according to the
client feedback. If it reaches a bit rate which is higher than the
initial one, we use the higher bit rate as the new bit rate estimation.
|
| |
|
|
|
|
|
|
|
|
| |
When there is no audio playback, we set the mm_time in the client to be older
than the one in the server by at least the requested latency (the delta is
actually bigger, due to the network latency).
When there is an audio playback, we adjust the mm_time in the client by
adjusting the playback buffer using SPICE_MSG_PLAYBACK_LATENCY.
|
|
|
|
| |
also update spice-common submodule
|
| |
|
|
|
|
|
|
| |
A frame can be dropped if a new frame was added during the same
call to red_process_command (we didn't attempt to send the older
frame). Such drops are ignored.
|
| |
|
|
|
|
|
|
| |
update mjpeg_encoder with reports from the client about
the playback quality.
The patch also updates the spice-common submodule.
|
|
|
|
|
|
|
|
|
|
| |
This patch only employs setting the stream parameters based on
the initial given bit-rate, the latency, and the encoding size.
Later patches will also employ mjpeg_encoder response to client reports,
and its control over frame drops.
The patch also removes old stream bit rate calculations that weren't
used.
|
| |
|
|
|
|
|
| |
Periodically calculate the rate of frames arriving from the guest to the
server.
|
| |
|
|
|
|
|
|
|
| |
spice_timer_queue
display channel - supplying timeouts interface to red_channel, in order to allow
periodic latency monitoring (see next patch).
|
|
|
|
|
| |
Each thread can create a spice_timer_queue, for managing its
own timers.
|
|
|
|
|
|
|
|
| |
The stream starts after lossless frames were sent to the client,
and without rate control (except for pipe congestion). Thus, on the beginning
of the stream, we might observe frame drops on the client and server side which
are not necessarily related to mis-estimation of the bit rate, and we would
like to wait till the stream stabilizes.
|
|
|
|
|
|
|
|
|
|
|
| |
The actual frames distribution does not necessarily fit the
condition "at least one frame every (1000/rate_contorl->fps)
milliseconds".
For keeping the average frame rate close to the defined fps, we
periodically measure the current average fps, and modify
rate_control->adjusted_fps accordingly. Then, we use
(1000/rate_control->adjusted_fps) as the interval between the
frames.
|
| |
|
|
|
|
|
|
|
|
|
|
| |
latency
The required client playback latency is assessed based on the current
estimation of the bit rate, the network latency, and the encoding size
of the frames. When the playback delay that is reported by the client
seems too small, or when the stream parameters change, we send the
client an updated playback latency estimation.
|
|
|
|
|
|
| |
Downgrading stream bit rate when the input frame rate in the server
exceeds the output frame rate, and frames are being dropped from the
output pipe.
|
|
|
|
|
|
|
|
| |
mjpeg_encoder can receive periodic reports about the playback status on
the client side. Then, mjpeg_encoder analyses the report and can
increase or decrease the stream bit rate, depending on the report.
When the bit rate is changed, the quality and frame rate of the stream
are re-evaluated.
|
|
|
|
|
|
|
| |
changes
If the encoding size seems to get smaller/bigger, re-evaluate the
stream quality and frame rate.
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
bit rate
Previously, the mjpeg quality was always 70. The frame rate was
tuned according to the frames' congestion in the pipe.
This patch sets the quality and frame rate according to
a given bit rate and the size of the first encoded frames.
The following patches will introduce an adaptive video streaming, in which
the bit rate, the quality, and the frame rate, change in response to
different parameters.
Patches that make red_worker adopt this feature will also follow.
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
The mjpeg_encoder should be client specific, and not shared between
different clients**, for the following reasons:
(1) Since we use abbreviated jpeg datastream for mjpeg, employing the same
mjpeg_encoder for different clients might cause errors when the
clients decode the jpeg data.
(2) The next patch introduces bit rate control to the mjpeg_encoder.
This feature depends on the bandwidth available, which is client
specific.
** at least till we change multi-clients not to re-encode the same
streams.
|
|
|
|
|
| |
Frames counting was skipped when the previous frame was already
sent completely to the client.
|
|
|
|
|
|
|
|
|
|
|
|
| |
My commit 71315b2e "snd_worker: Don't send empty audio-volume messages",
fixes only one case of sending an empty volume message, if the client connects
to a vm early during its boot sequence, while the snd hardware is being reset
by the guest driver, qemu will call spice_server_playback_set_volume() with
0 channels from the reset handler.
This patch also applies both fixes to the record channel.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
When qemu migration completes, we need to stop the streams, and to send
the corresponding upgrade_items to the client.
Otherwise, (1) the client might display lossy regions that we don't track
(streams are not part of the migration data).
(2) streams_timeout may occur after MSG_MIGRATE has been sent, leading
to messages being sent to the client after MSG_MIGRATE and before
MSG_MIGRATE_DATA (e.g., STREAM_CLIP, STREAM_DESTROY, DRAW_COPY).
No message besides MSG_MIGRATE_DATA should be sent after
MSG_MIGRATE.
When a msg other than MIGRATE_DATA reached spice-gtk after MSG_MIGRATE,
spice-gtk sent it to dest server as the migration data, and the dest
server crashed with a "bad message size" assert.
|
|
|
|
|
| |
In order not to confuse it with red_destroy_streams in the following
patch.
|
|
|
|
|
|
|
|
|
|
|
| |
If no volume has been set it, we end up sending a volume message with
audio-volume for 0 channels (iow an empty message). This is not useful
and triggers the following warning in spice-gtk:
(remote-viewer:8726): GSpice-WARNING **: set_sink_input_volume() failed:
Invalid argument
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
|
|
|
|
|
|
|
|
|
|
| |
2 closely related changes in one:
1) When leaving the read or write loop because the chardev has been stopped
active should not be updated. It has been set to FALSE by
spice_char_device_stop and should stay FALSE
2) The updating of dev->active should be done *before* unref-ing dev
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
|
|
|
|
|
|
|
| |
The write-retry timer should not be set when we're leaving
spice_char_device_write_to_device because the char-dev has been stopped.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
|
|
|
|
|
|
|
|
| |
Before this patch the write-loop in spice_char_device_write_to_device would
break on running becoming 0, after having written some data, without updating
the buffer status, causing the same data to be written *again* when started.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
|
|
|
|
|
|
|
| |
This fixes spice-gtk printing message like these on migration:
(remote-viewer:18402): GSpice-CRITICAL **: spice_channel_iterate_read: assertion `c->state != SPICE_CHANNEL_STATE_MIGRATING' failed
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
|
|
|
|
|
|
|
|
|
| |
This is clearly something which should be handled in the inputs_channel code,
rather then having a special case for it in the generic channel handling
code in reds.c. Moving it here also fixes the TODO we had on only sending
this message to new clients.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
|
|
|
|
|
|
|
| |
Sometimes we want to send a notify to a single client, rather then to
all of them.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
|
|
|
|
|
|
|
|
|
|
|
| |
Currently main_channel_push_notify only gets passed a static string, but
chances are in the future it may get passed dynamically allocated strings,
prepare it for this.
While at it also make clear that its argument is a string, and simplify
things a bit by making use of this knowledge (pushing the strlen call down).
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Client -> agent messages can spawn multiple VDIChunks. When this happens
the agent re-assembles the chunks into a complete VDAgentMessage before
processing it. The server only guarentees coherency at the chunk level,
so it is not possible for a partial chunk to get delivered to the agent.
But it is possible for some chunks of a VDAgentMessage to be delivered to
the agent followed by a client to disconnect without the rest of the
VDAgentMessage being delivered!
This will leave the agent in a wrong state, and the first messages send to it
by the next client to connect will get seen as the rest of the VDAgentMessage
from the previous client.
This patch sends the agent a new VD_AGENT_CLIENT_DISCONNECTED message from the
VDP_SERVER_PORT, on which the agent can then reset its VDP_CLIENT_PORT state.
Note that no capability check is done for this, since the capabilities are
something negotiated between client and agent. The server will simply always
send this message on client disconnect, relying on older agents discarding the
message since it has an unknown type (which both the windows and linux agents
already do).
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
|
|
|
|
|
|
|
| |
To allow the server to send agent messages without needing to wait for a
self-token. IE for sending VD_AGENT_CLIENT_DISCONNECTED messages.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
These messages are printed when the server tries to push a mouse event to
the agent before the previous one has been flushed. This is a normal condition
(which gets tracked by the reds->pending_mouse_event boolean), and as such
it should *not* trigger the printing of error messages.
I've seen these messages occasionally before, but with agent file-xfer they
are trivial to trigger, simply send a large file to the agent and while it
is transferring move the mouse over the client window. Note that due to the
client tokens not allowing the client to completely saturate the agent
channel mouse events do still get send to the agent, just with a slightly
larger interval. So everything is working as designed and this spice_printerr
is just leading to people chasing ghosts.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
1) This does not buy us much, as red_marshall_monitors_config() also
removes 0x0 sized monitors and does a much better job at it
(also removing intermediate ones, not only tailing ones)
2) The code is wrong, as it allocs space for real_count heads, where
real_count always <= monitors_config->count and then stores
monitors_config->count in worker->monitors_config->count, causing
red_marshall_monitors_config to potentially walk
worker->monitors_config->heads past its boundaries.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
red_worker.c:10968:red_push_monitors_config: condition `monitors_config != NULL' failed
During my dynamic monitor support testing today, I hit the following assert
in red_worker.c:
"red_push_monitors_config: condition `monitors_config != NULL' failed"
This is caused by the following scenario:
1) Guest causes handle_dev_monitors_config_async() to be called
2) handle_dev_monitors_config_async() calls worker_update_monitors_config()
3) handle_dev_monitors_config_async() pushes worker->monitors_config, this
takes a ref on the current monitors_config
4) Guest causes handle_dev_monitors_config_async() to be called *again*
5) handle_dev_monitors_config_async() calls worker_update_monitors_config()
6) worker_update_monitors_config() does a decref on worker->monitors_config,
releasing the workers reference, this monitor_config from step 2 is
not yet free-ed though as the pipe-item still holds a ref
7) worker_update_monitors_config() creates a new monitors_config with an
initial ref-count of 1 and stores that in worker->monitors_config
8) The pipe-item of the *first* monitors_config is send, upon completion
a decref is done on the monitors_config, and monitors_config_decref not
only frees the monitor_config, but *also* sets worker->monitors_config
to NULL, even though worker->monitors_config no longer refers to the
monitor_config being freed, it refers to the 2nd monitor_config!
9) The client which was connected when this all happened disconnects
10) A new client connects, leading to the assert:
at red_worker.c:9519
num_common_caps=1, common_caps=0x5555569b6f60, migrate=0,
stream=<optimized out>, client=<optimized out>, worker=<optimized out>)
at red_worker.c:10423
at red_worker.c:11301
Note that red_worker.c:9519 is:
red_push_monitors_config(dcc);
gdb does not point to the actual line of the assert because the function gets
inlined.
The fix is easy and obvious, don't set worker->monitors_config to NULL in
monitors_config_decref. I'm a bit baffled as to why that code is there in
the first place, the whole point of ref-counting is to not have one single
unique place to store the reference...
This fix should not have any adverse side-effects as the 4 callers of
monitors_config_decref fall into 2 categories:
1) Code which immediately after the decref replaces worker->monitors_config
with a new monitors_config:
worker_update_monitors_config()
set_monitors_config_to_primary()
2) pipe-item freeing code, which should not touch the worker state at all
to being with
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
|
|
|
|
|
|
|
|
|
|
|
| |
server/Makefile apparently forgot to link libspice-server
with -lm -lpthread, but it uses symbols from these libraries
directly. These libs are detected by configure and stored in
$(SPICE_NONPKGCONFIG_LIBS) make variable, but this variable
is never referenced at link time. Add it to server/Makefile.am,
to libspice_server_la_LIBADD variable.
Signed-off-By: Michael Tokarev <mjt@tls.msk.ru>
|
|
|
|
|
|
| |
The stream vis_region should be cleared after the stream region was sent
to the client losslessly. Otherwise, we might send redundant stream upgrades
if we process more drawables that are dependent on the stream region.
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
calling red_detach_streams_behind
resolves: rhbz#891326
Starting from commit 81fe00b08ad4f, red_detach_streams_behind can
trigger modifications in the current tree (by update_area calls). Thus,
after calling red_detach_streams_behind it is not safe to access tree
entries that were calculated before the call.
This patch inserts the drawable to the tree before the call to
red_detach_streams_behind. This change also requires making sure
that rendering operations that can be triggered by
red_detach_streams_behind will not include this drawable (which is now part of the tree).
|