That means RedClient tracks a ring of channels. Right now there will be only
a single client because of the disconnection mechanism - whenever a new
client comes we disconnect all existing clients. But this patch adds already
a ring of clients to reds.c (stored in RedServer).
There is a known problem handling many connections and disconnections at the
same time, trigerrable easily by the following script:
export NEW_DISPLAY=:3.0
Xephyr $NEW_DISPLAY -noreset &
for ((i = 0 ; i < 5; ++i)); do
for ((j = 0 ; j < 10; ++j)); do
DISPLAY=$NEW_DISPLAY c_win7x86_qxl_tests &
done
sleep 2;
done
I fixed a few of the problems resulting from this in the same patch. This
required already introducing a few other changes:
* make sure all removal of channels happens in the main thread, for that
two additional dispatcher calls are added to remove a specific channel
client (RED_WORKER_MESSAGE_CURSOR_DISCONNECT_CLIENT and
RED_WORKER_MESSAGE_DISPLAY_DISCONNECT_CLIENT).
* change some asserts in input channel.
* make main channel disconnect not recursive
* introduce disconnect call back to red_channel_create_parser
The remaining abort is from a double free in the main channel, still can't
find it (doesn't happen when running under valgrind - probably due to the
slowness resulting from that), but is easy to see when running under gdb.
use MainChannel* instead of Channel* for a many functions in main_channel.h
(affects main_channel.c and reds.c).
some one liner fixes are hidden in here too.
Another cleanup patch, no change to behavior (still one client, and it
disconnects previous client if any).
The implementation for multiple client is straightforward: the pipe
remains per (channel,client) pair, so it needs to move from the RedChannel
that to RedChannelClient. Implementation using a single pipe with multiple
consumers (to reflect different latencies) doesn't fit well with pipe rewriting
that is used by the display channel. Additionally this approach is much simpler
to verify. Lastly it doesn't add considerable overhead (but see the display
channel changes in a later patch for a real place to rethink).
This patch is just technical, changing signatures to reflect the first
argument (oop style) so red_channel becomes red_channel_client. Some places
may seem odd but they should be fixed with later comits where the channels
grow to support multiple clients.
Sound (playback/record) channels are the only ones not touched - this is
consistent with previous patches, since they have been left out of the
RedChannel refactoring. That is left as future work. (note that they don't use
a pipe, which was the reason for not refactoring).
This commit adds a RedChannelClient that now owns the stream connection,
but still doesn't own the pipe. There is only a single RCC per RC
right now (and RC still means RedChannel, RedClient will be introduced
later). All internal api changes are in server/red_channel.h, hence
the need to update all channels. red_worker.c is affected the most because
it makes use of direct access to some of RedChannel still.
API changes:
1. red_channel_client_create added.
rec_channel_create -> (red_channel_create, red_channel_client_create)
2. two way connection: rcc->channel, channel->rcc (later channel will
hold a list, and there will be a RedClient to hold the list of channels
per client)
3. seperation of channel disconnect and channel_client_disconnect
TODO:
usbredir added untested.
The only difference between them being that the later also does a push.
I don't believe that to be a problem, but if it does I can always introduce
a push'less version.
rename types - we use _proc suffix mostly to indicate function pointer types,
use it for some function pointers that were missing it.
s/channel_handle_migrate_flush_mark/channel_handle_migrate_flush_mark_proc/
s/channel_handle_migrate_data_get_serial/channel_handle_migrate_data_get_serial_proc/
s/channel_handle_migrate_data/channel_handle_migrate_data_proc/
and spice_server_playback_put_samples. The former retrieves a buffer from a free
list with spice_server_playback_get_buffer, and should be used once via
spice_server_playback_put_samples. The tester previously reused the same buffer
a number of times.
Adds the required options to provide a vdagent to the guest in both source and target qemu
instances.
This will be the last update of the in spice git tests directory, I've moved those tests
to the repository spice-tests. The longer term goal remains autotest integration, but since
this test (and some minor others for qemu) need a home it is:
http://cgit.freedesktop.org/~alon/spice-tests/
(I'm reluctant to put it under spice/ because of my wish to go to autotest, but still, there
they are. Nothing as permanent as the temporary).
Independent (of external modules, i.e. qemu) tests (server/tests) should remain in tree.
To be able to enable/disable keyboard leds on X11, we need to query
the X server for which mask correspond to which led (NumLock,
CapsLock). So far this was done using XKeysymToKeycode and iterating
over X modifier mapping.
Xkb provides XkbKeysymToModifiers for this purpose, and since
we're using Xkb anyway, it makes more sense to use it.
At some point, on my Fedora 15 box, XKeysymToKeycode was returning
NoSymbol for CapsLock and NumLock leading to spicec not being able
to change the keyboard leds when qemu tells it to. However, I couldn't
reproduce this when I tried again :-/
InputsChannel::handle_modifiers converts _modifiers which is a
bitflag of SPICE_KEYBOARD_MODIFIER_FLAGS_* to a Platform::*_MODIFIER
bitflag, which is what Platform::set_keyboard_lock_modifiers expects.
However, it's called with _modifiers, and the bitflag that this
function computes is never used. Pass the computed bitflag to
::set_keyboard_lock_modifiers since _modifiers format is meaningless
for ::set_keyboard_lock_modifiers.
This bug was harmless because the two different set of modifier
flags actually use the same values, so _modifiers and modifiers could
be used interchangeably. However it's more future-proof to use the
right format here.
Even if VDAgentDisplayConfig::depth will be unused if the
VD_AGENT_DISPLAY_CONFIG_FLAG_SET_COLOR_DEPTH isn't set, it's
better to initialize it anyway to avoid warnings from valgrind.
After the changes to add libjpeg-turbo support to spice-server mjpeg
compression code, it's relatively easy to hit an assertion from
libjpeg in spice-server about "too few scanlines transferred" when
the mjpeg streaming code triggers. This assertion brings down qemu,
which is bad :)
This is because when we first initialize the mjpeg encoder, we do:
stream_width = SPICE_ALIGN(src_rect->right - src_rect->left, 2);
stream_height = SPICE_ALIGN(src_rect->bottom - src_rect->top, 2);
stream->mjpeg_encoder = mjpeg_encoder_new(stream_width, stream_height);
and then when we iterate over the image scanlines to feed them to
libjpeg, we do:
const int image_height = src->bottom - src->top;
const int image_width = src->right - src->left;
for (i = 0; i < image_height; i++) {
mjpeg_encoder_encode_scanline(...);
}
mjpeg_encoder_end_frame(...);
When stream_height is odd, the mjpeg_encoder will be created with
an height that is 1 more than the number of lines we encode. Then
libjpeg asserts when we tell it we're done with the compression
while it's still waiting for one more scanline.
Looking through git history, this rounding seems to be an artifact
from when we were using ffmpeg for the mjpeg encoding. Since
spicec and spicy (the latter needs a few fixes) can handle streams
with odd height/width, the best way to solve this issue is to stop
rounding up the height and width of the streams we create. This
even saves some bandwidth :)
when changing resolutions due to the new async code paths the surface
creation command was kept by reference, and later, when the red_worker
signaled completion by calling async_complete the mouse mode was updated
using the reference. This caused the wrong values to be read resulting in wrong
resolutions set and a non working mouse pointer. Fix this by keeping a copy of
the surface creation command instead of a reference.
No bz. Found in testing.
Changelog from Arnon Gilboa, patch from me:
Commit eb6f554094 caused the following regression:
When client runs without the auto-conf or disable-effects options
(either from CLI or controller), which is the case when using Spice
from Admin Portal, the client will unecessarily wait for 30sec before
connecting to a Windows guest with an agent running (this won't happen
with linux guests or without an agent running).
The mentioned patch assumed that on_agent_reply() of
VD_AGENT_DISPLAY_CONFIG will call send_main_attach_channels() and
connect. However, when auto-conf or disable-effects are not used,
on_agent_reply() will ignore the reply and not call
send_main_attach_channels(). Therefore, send_main_attach_channels()
will only be called on agent timeout.
The solution is to activate agent timer only if auto-conf or
disable-effects. Otherwise, simply call send_main_attach_channels().
Fixes rhbz #726441
They were trying to convert the destination pointer to an integer before
trying to dereference it. The initial conversion was meant to be a cast
to a pointer of the right size, not to an integer.
It is a bit early to bump, since a 0.9.1 release is not happening yet,
but this allows me to test if the vdagent SpiceCharInterface state callback
fixes are present or not in qemu code, and thus disabling the ugly vdagent
specific workaround from spice-qemu-char.c when compiling against a new
enough spice-server.
It was sending the wrong data, the memory right after the VCSMsgHeader
which was actually not where the data was.
Fixed by having the header and data (VSCError, 4 bytes of the error code)
embedded in the ErrorItem pipe item.
It's not used when we use jpeg-turbo colorspaces, so it's better
to allocate it when we know we'll need it rather than always
allocating it even if it won't be used.
After the refactoring to optionally use libjpeg-turbo, some
of the functions that mjpeg-encoder used to provide are now no
longer used. This commit removes them.
When libjpeg-turbo is available, we can use the BGR and BGRX
colorspaces that it provides to avoid extra conversions of the
data we want to compress to mjpeg
The main point is to move the pixel conversion code into
the MjpegEncoder class to be able to make use libjpeg-turbo
additional pixel formats without the reds_worker code noticing.
This API is meant to allow us to move the pixel format conversion
into MjpegEncoder. This will allow us to be able to use the
additional pixel formats from libjpeg-turbo when available.
It takes a lot of arguments, "id" is unused, "frame" and
"frame_size" can be obtained from the "stream" argument, so
can get rid of 3 arguments to make things more readable.
