Re: [Spice-devel] [PATCH] make celt to be optional

[Alexander Larsson]

On Thu, 2012-06-14 at 12:31 +0200, Christophe Fergeau wrote:
> On Thu, Jun 14, 2012 at 10:14:36AM +0200, Alexander Larsson wrote:
> > However, that is imho a different issue than the celt051 support. A new
> > release of spice client and server supporting opus does not magically
> > make old servers and client disappear, so it would still be the case
> > that e.g. debian spice client would get lame audio performance if
> > connecting to say a RHEV spice client, or if some old client connects to
> > a server running on debian. In time, it would perhaps make sense to drop
> > celt051 support, but its seems pretty bad to release a client binary
> > that doesn't do audio well with all currently existing deployed servers.
> 
> It all depends if we consider remote SPICE access with limited bandwidth and
> with audio needed will be an important use case that must run as good as
> possible. In my opinion, sound is most of the time not the most important
> thing if what you want is a remote desktop. It also won't be really
> noticeable on LAN, or in GNOME Boxes use case, ...
> 
> What I gather from this thread is that we don't want anyone to use the
> fallback PCM code, which means we should deprecate it if that's really what
> we want... Maybe the clients could be patched to stop advertising raw PCM
> support? I don't know if no audio at all is more acceptable than not doing
> audio well in some cases.

I don't know if that is true. If nothing else works then in some cases
PCM is a good approach. However, maybe the client should warn about this
and allow disabling audio?


___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] make celt to be optional

[Alexander Larsson]

On Tue, 2012-06-12 at 16:33 +0200, Christophe Fergeau wrote:
> On Tue, Jun 12, 2012 at 09:59:39AM -0400, Marc-André Lureau wrote:
> > 
> > 
> > - Mensaje original -
> > > On Tue, Jun 12, 2012 at 09:11:24AM -0400, Marc-André Lureau wrote:
> > > > As long as the bitstream is not frozen, we can't use opus, or we
> > > > will
> > > > have the same problems as with celt today.
> > >
> > > As I understand it, while the bitstream is not officially frozen yet,
> > > it's
> > > very unlikely to change before the real freeze (unless big last
> > > minute
> > > issues are fine), so an Opus mode (marked as "no compat guarantees,
> > > use at
> > > your own risk, ...") will probably not cause significant
> > > compatibilities issues.
> > 
> > That's just guesses. It's not about library API, but about bitstream.
> > There is no guarantee.
> 
> A guess supported by the slides at http://www.opus-codec.org/presentations/
> , by various mailing posts from opus developers, ...
> 
> > Sticking to celt051 is still a safer alternative.
> 
> Not suggesting dropping celt051 support upstream...
> 
> > Otherwise, how would you identify almost-frozen bitstream to frozen 
> > bitstream?
> > You would have to identify by library version (erk!)
> > and be compatible with the old and new bitstram (which might be complicated
> > depending on library design), or be incompatible with the intermediate 
> > version,
> > situation which we better avoid!
> 
> We would make no guarantee with interoperability between binaries using
> different opus versions until the format is officially frozen. I agree
> there's a bit of uncertainty in this move, but I think that at this point
> it's a reasonable assumption that things will work, even with different
> opus versions.

It seems like Opus is at a stage where we want to at least start adding
support for it so we can switch to it by default as early as possible.
Its not like this is a new idea, the plan was always to jump to a stable
bitstream format when one appeared.

However, that is imho a different issue than the celt051 support. A new
release of spice client and server supporting opus does not magically
make old servers and client disappear, so it would still be the case
that e.g. debian spice client would get lame audio performance if
connecting to say a RHEV spice client, or if some old client connects to
a server running on debian. In time, it would perhaps make sense to drop
celt051 support, but its seems pretty bad to release a client binary
that doesn't do audio well with all currently existing deployed servers.


___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] make celt to be optional

[Alexander Larsson]

On Tue, 2012-06-12 at 14:31 +0400, Michael Tokarev wrote:
> On 12.06.2012 13:15, Alexander Larsson wrote:
> []
> > Its not incompatible, like I i said in my "(Well, .." part above. But
> > what it means is that it will send audio as PCM instead of compressing
> > it. This is much larger, and will cause anyone using the debian spice
> > client to connect to a spice server (even one that supports celt) to use
> > much more bandwidth due to this. Its not very obvious that the reason
> 
> Why do you think it is really that bad?  When used in a LAN, bandwidth
> doesn't matter, and it will even work a tiny bit better due to less
> cpu usage for encoding/decoding.  It may matter for slow/long Internet
> links, but these are much less important for audio.  Also, audio does
> not come alone usually (except of a few windows sounds), it usually
> comes together with video (ie, movies etc), which has their own
> requiriments which are much stronger than even raw audio.
> 
> I'm not saying celt or other codec is bad or not needed, something
> is definitely worth to have to be able to transmit audio cheaply,
> but it is not the first priority thing for sure.
> 
> Much more important is to have good visual expirence, and this is what
> spice is all about, and this is something what is not changed by this
> patch.
> 
> Do you not agree?

I don't really agree. Its true that if you have unlimited bandwidth,
then bandwidth doesn't matter. But I don't think in practice this is
true, as bandwidth always has some limits in reality. Its possible that
it works fine in some situations, but its also quite possible that its a
deal-breaker in some others, and we have no data to substantiate the
claim that it doesn't matter (in fact, it seems unlikely since the
developers spent time and energy to implement audio compression).

Spice is very much about more than just pixels, that it does more than
graphics (usb, sound, audio-video sync, clipboard, etc) is an important
part of why Spice is better than other solutions. In fact, our support
of movies by using compressed video and audio is a pretty important
selling point of spice.

The main thing I disagree with this change is that you change how spice
works, making act differently than its creators intended, due to a
packaging technicallity. You decide that spice users are not interested
in low-bandwidh audio, without asking and without telling them. (I'm
sure it'll be mentioned somewhere, but its unlikely that most users will
see it.)


___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] make celt to be optional

[Alexander Larsson]

On Tue, 2012-06-12 at 12:54 +0400, Michael Tokarev wrote:
> On 12.06.2012 12:48, Alexander Larsson wrote:
> > On Sat, 2012-06-02 at 15:46 +0400, Michael Tokarev wrote:
> > 
> >> I plan to use this patch in the upcoming Debian
> >> release, codename wheezy, to get rid of celt
> >> codec library there, since we decided celt051 is
> >> not going to be included, but it is obviously not
> >> a good idea to drop spice entirely.
> > 
> > Isn't it better to drop spice completely rather than shipping a version
> > thats essentially protocol incompatible? (Well, it will fall back to no
> > audio or non-compressed audio, but that is untested and pretty bad for
> > actual use of spice.) 
> 
> It isn't incompatible.  It might be incompatible with older releases
> of spice where audio codec negotiation/fallback were not properly
> implemented (read: was buggy), but it is okay now.  Did you read
> the whole my email where I mention testing I performed?

Its not incompatible, like I i said in my "(Well, .." part above. But
what it means is that it will send audio as PCM instead of compressing
it. This is much larger, and will cause anyone using the debian spice
client to connect to a spice server (even one that supports celt) to use
much more bandwidth due to this. Its not very obvious that the reason
for this is that Debian decided to package Spice that way, but rather
the likely conclusion that any user would draw is that Spice just works
like that. Thats not exactly a good impression for Spice.

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] make celt to be optional

[Alexander Larsson]

On Sat, 2012-06-02 at 15:46 +0400, Michael Tokarev wrote:

> I plan to use this patch in the upcoming Debian
> release, codename wheezy, to get rid of celt
> codec library there, since we decided celt051 is
> not going to be included, but it is obviously not
> a good idea to drop spice entirely.

Isn't it better to drop spice completely rather than shipping a version
thats essentially protocol incompatible? (Well, it will fall back to no
audio or non-compressed audio, but that is untested and pretty bad for
actual use of spice.) 

Then users that want to use spice can get a working version somewhere
else instead of just thinking that spice sucks because of this change.

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [protocol RFC 0/2] RANDR support via QXLHead + SpiceHead

[Alexander Larsson]

On Mon, 2012-05-07 at 09:28 +0300, Alon Levy wrote:
> Hi,
> 
>  Currently we support multiple monitors by having:
>   single pci = single display channel = single client window
>  
>  The RANDR architecture doesn't lend itself to this, but on the other hand it
>  makes it very easy to have an alternative scheme:
>   single pci = single display channel = multiple client windows
>  
>  RANDR introduces a concept of a CRTC and an OUTPUT. The CRTC scansout a
>  portion of the framebuffer onto one or more OUTPUTs. I propose having a 1:1 
> CRTC:OUTPUT correspondence and introducing two new commands, one on PCI and 
> one for the protocol:
>  QXLHead = (id, x, y, width, height)
>  SpiceHead = (id, x, y, width, height)

I'd like to point out that while this is the current way things work the
plan for the furture is to implement Per-CRTC pixmaps[1] which will
allow clients to map a separate pixmap to each crtc rather than have all
the crtcs scan out from a separate one (the framebuffer).

This is important, because the current situation with multiple monitors
means the framebuffer can easily become much wider than the hardware
limitation on texture widths of modern GPUs which means you can't run a
3D compositor in such multi-monitor setups. So, instead the plan is to
have a separate pixmap per monitor and have the compositor draw the
windows to the right pixmap(s).

[1] http://lists.x.org/archives/xorg-devel/2010-May/008141.html

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] xf86-video-qxl performance

[Alexander Larsson]

On Wed, 2012-05-23 at 15:20 -0500, Jeremy White wrote:

> Also, as a crazy idea, has anyone considered implementing a pure
> streaming video driver?  That is, what if we had a frame buffer driver,
> and then a thread that fired 29 times a second to drive a theora or vp8
> encoder, simply feeding the current frame at those intervals.

Its not crazy at all. In fact, all the times I've been pondering how to
support modern (i.e. GPU-based) 3D acceleration to spice I've always
ended up at this. Its not really practical to support GPUs like a set of
remoted drawing operations, since:
a) They are infinitely flexible (turing complete)
b) Work on huge amounts on temporary data (textures, buffers, etc),
   often multiple times the size of what the final rendered screen
   size is.
c) Its impossible (comparable to halting problem) to figure out
   in general which part of the input sources some GPU code snippet
   uses, or to calculate the bounding box of the result.

So, what I think we want for GPU accelerated spice is an
eventually-losless video codec. By that I mean the fact that individual
frames might be lossy (so it can be effective for video and games), but
if the input is static we'd (quickly) converge on the lossless result
(so that you can read your word documents).

If additionally we could separate each toplevel window as its own stream
that would be even nicer, but It might be hard as these are composed to
the final screen using the GPU.

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [announce] alpha glib/gtk client library + app.

[Alexander Larsson]

On Thu, 2010-09-30 at 22:18 +0200, Gerd Hoffmann wrote:
> On 09/30/10 20:41, Alexander Larsson wrote:
> > On Thu, 2010-09-30 at 14:48 +0200, Alexander Larsson wrote:
> >
> >> More later...
> >
> > Bit more before I disappear:
> >
> > why do you have a spice_channel_destroy()? Its a gobject, just unref it.
> 
> spice_channel_destroy actually just does a unref (+ debug printf) now. 
> Used to be different.  I'm new to gobject programming, so I didn't got 
> everything right from start.
> 
> > I don't like that you have three libs. We've learned this lesson in
> > gnome by now: It seems that splitting things up into small separate
> > libraries is a good thing, but in practice its problematic because of
> > the per-library overhead in things like symbol resolving (one extra
> > lookup per item in the link list, for all symbols).
> >
> > So, at a minimum, combine libspice-client-pulse and libspice-client-gtk.
> > Although i'm not really sure we need the separate non-ui library
> > either...
> 
> gtk drags *tons* of stuff.
> 
> # ldd spicy | wc -l
> 64
> # ldd snappy | wc -l
> 23
> 
> So I think keeping the non-gui (actually protocol parsing and software 
> rendering)  bits separate looks useful to me.  Having gtk+sound separate 
> probably doesn't make that much sense as you likely either use both or 
> none of them ...

Yes, gtk+ drags in a lot. However, how many non-ui apps will link to
spice?

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a suicidal coffee-fuelled romance novelist with no name. She's a wealthy 
psychic bodyguard looking for love in all the wrong places. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [announce] alpha glib/gtk client library + app.

[Alexander Larsson]

On Thu, 2010-09-30 at 22:22 +0200, Gerd Hoffmann wrote:
> Hi,
> 
> >> If this is the plan we need to ensure that
> >> the APIs and implementations are not tied to tightly to unix.
> >
> > I have some X11 MIT-SHM bits in there for performance reasons, that
> > needs to be #ifdef'ed and have a alternative (generic cairo/pixman
> > based?) implementation for !x11 platforms. Other that that there should
> > be nothing major. glib helps alot here.
> 
> Oh, and while talking about windows:  The windows client has the gdi 
> renderer, how does that compare to the sw one performance-wise?

I don't actually know. I guess its hardware accelerated. On the other
hand, the operations spice does are hardly expensive on todays cpus.
Maybe its better to drop it in favour of a consistant behaviour and less
code to maintain. Needs some profiling though.

Actually i think all GDI stuff is software only in vista, but i think
they added back some acceleration in win7.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a hate-fuelled flyboy cop on the run. She's a radical red-headed research 
scientist from aristocratic European stock. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [announce] alpha glib/gtk client library + app.

[Alexander Larsson]

On Thu, 2010-09-30 at 22:08 +0200, Gerd Hoffmann wrote:

> > "spice_watch", "spice_msg_out" etc is a pretty uncommon capitalization
> > for Gtk+/GObject APIs. Generally all (public) types are CamelCase.
> 
> spice_msg_out I want keep internal.  Right now it isn't but I want 
> figure a way to change that.
> 
> Need to look at spice_watch again, not sure how much sense this makes, 
> the original idea was to allow different os-specific implementations 
> there.  But when we require glib anyway we can just depend on the glib 
> main loop unconditionally.

Oh, ignore the specifics, i'm just talking about the captialization of
the type names.


> > Also, i don't like the audio integration, as it exposes the backend
> > implementation (at least by name) in the API. If we have multiple sound
> > implementations then each client must have code to integrate with each
> > one. So, I'd rather have a public SpiceAudioSink type that has different
> > implementations under the hood (which one gets picked might be solely a
> > build-time thing, or we could have an env var to choose between them).
> 
> Hmm.  The idea was to allow for SpicePulse and SpiceAlsa and probably 
> something else on windows and macos.  There isn't much interfacing 
> needed, the only thing the client app must do is creating a object 
> instance and hand it over a SpiceSesson object.  So having a convenience 
> helper function which picks one implementation should be easy to do.

I understand that, and underneath the covers there obviously have to be
these implementations. However, there is no need to expose these in the
public api. All the API the user needs is to create some type of object
that can handle the playing. I guess what you want is a base class which
is derived by each implementation, and then a factory function that gets
you the default implementation.

Also:

SpicePulse *spice_pulse_new(SpiceSession *session, GMainLoop *mainloop,
const char *name);


The name should probably just be taken from g_get_prgname() or
g_get_application_name().

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's an uncontrollable drug-addicted barbarian who hides his scarred face 
behind a mask. She's a man-hating green-skinned pearl diver on the trail of a 
serial killer. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [announce] alpha glib/gtk client library + app.

[Alexander Larsson]

On Thu, 2010-09-30 at 14:48 +0200, Alexander Larsson wrote:

> More later...

Bit more before I disappear:

why do you have a spice_channel_destroy()? Its a gobject, just unref it.

I don't like that you have three libs. We've learned this lesson in
gnome by now: It seems that splitting things up into small separate
libraries is a good thing, but in practice its problematic because of
the per-library overhead in things like symbol resolving (one extra
lookup per item in the link list, for all symbols).

So, at a minimum, combine libspice-client-pulse and libspice-client-gtk.
Although i'm not really sure we need the separate non-ui library
either...

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a benighted arachnophobic hairdresser haunted by memories of 'Nam. She's 
a cold-hearted gold-digging archaeologist in the witness protection program. 
They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [announce] alpha glib/gtk client library + app.

[Alexander Larsson]

On Wed, 2010-09-29 at 12:07 +0200, Gerd Hoffmann wrote:
> Hi folks,
> 
> /me proudly presents some early glib/gtk bits for the client side of the 
> spice protocol.  The code is available in the gtk.v2 branch in my 
> personal spice repo @ freedesktop.org:
> 
>http://cgit.freedesktop.org/~kraxel/spice/log/?h=gtk.v2
> 
> The README is here:
> 
>http://cgit.freedesktop.org/~kraxel/spice/tree/gtk/README?h=gtk.v2
> 
> Good enougth to start playing with it.
> Far from being stable enougth for production use.
> Tested with winxp guest only so far.
> 
> Reviews are very welcome.  Especially looking at the design and 
> interfaces of the glib/gtk objects would be very helpful.  Looking at 
> the actual code isn't that useful (yet) as there are plenty of known 
> issues anyway.

I had a quick look at this. It seems this is a re-write of a client
implementation (based on the existing code i guess). I don't necessarily
disagree with this (as the current code isn't exactly well suited for
librarification), but this is a pretty big chunk of work that will take
a while to stabilize. Also, unless we completely replace the old client
implementation we will have to maintain two independent client
implementations. I guess if we make the gtk+ one work on windows too
then we can drop the old one. If this is the plan we need to ensure that
the APIs and implementations are not tied to tightly to unix.

A few random API comments:

"spice_watch", "spice_msg_out" etc is a pretty uncommon capitalization
for Gtk+/GObject APIs. Generally all (public) types are CamelCase.

spice_watch_put => spice_watch_free is a more typical Gtk+ naming
convention (or possibly _destroy). Or _ref/_unref for refcounted types.

Also, i don't like the audio integration, as it exposes the backend
implementation (at least by name) in the API. If we have multiple sound
implementations then each client must have code to integrate with each
one. So, I'd rather have a public SpiceAudioSink type that has different
implementations under the hood (which one gets picked might be solely a
build-time thing, or we could have an env var to choose between them).


Things like this is a bit uncommon/lowlevel in a gtk+ api (and, binds
badly to other languages):
int spice_session_get_channels(SpiceSession *session, SpiceChannel
**channels, int max);
A more typical Gtk+ style call is:
SpiceChannel ** spice_session_get_channels(SpiceSession *session);
returning a NULL-terminated array of items. Or possibly a GList *.

More later...

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's an uncontrollable guitar-strumming paranormal investigator with acid for 
blood. She's a sharp-shooting gold-digging Valkyrie from aristocratic European 
stock. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] Announcing Spice 0.6.1

[Alexander Larsson]

Today we released the first bugfix update to the stable spice 0.6
series.

Updated sources and windows binaries are availible at:
http://spice-space.org/download.html

We hope to have packages in Fedora 14 shortly.

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] client: port for Mac OS X

[Alexander Larsson]

On Sun, 2010-09-26 at 14:57 +0200, Attila Sukosd wrote:
> Hi All,
> 
> I finally had a bit of time to gather the changes to the spice client
> in order to get it working under Mac.

Good stuff. I landed some of the cleanup from this patch as:
http://lists.freedesktop.org/archives/spice-devel/2010-September/001249.html

Also, I did a new version of the select patch that further simplifies
things:
http://lists.freedesktop.org/archives/spice-devel/2010-September/001264.html
Waiting for review on it...

Further review:

Why the GPL change? All other headers are LGPL 2.1 anyway, and we want
the client LGPL anyway so it can be librariefied.

+#if defined(__APPLE__) || defined(__MACH__)
+#ifndef MSG_NOSIGNAL
+#define MSG_NOSIGNAL SO_NOSIGPIPE
+#endif
+#endif

This is not right. SO_NOSIGPIPE is not used in the same way as
MSG_NOSIGNAL.
See e.g.
http://lists.policyd.org/pipermail/devel/2007-September/000468.html

In general we want to avoid sprinkling __LINUX__ or __APPLE__ defines in
the "unixy" code (its hard to avoid for win32 though). Instead we want
to check things in configure and use proper HAVE_FOO checks, etc. 

So, for instance we'd have HAVE_ALSA (or maybe USE_ALSA in this case
since we might have multiple detected sound system in any particular
system), rather than __LINUX__ checks. And similar for the thread stuff.

Anyway, if you rebase your patch based on the stuff thats landed things
should look much easier for you to handle.

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] Replace epoll with select in X client

[Alexander Larsson]

On Wed, 2010-09-29 at 17:28 +0200, Alexander Larsson wrote:

I'm commiting this part:

> diff --git a/client/x11/platform.cpp b/client/x11/platform.cpp
> index 1133414..ca5f1d5 100644
> --- a/client/x11/platform.cpp
> +++ b/client/x11/platform.cpp
> @@ -3024,8 +3024,12 @@ void 
> Platform::set_clipboard_listener(ClipboardListener* listener)
>  return;
>  }
>  clipboard_listener = listener;
> -XConvertSelection(x_display, XA_PRIMARY, utf8_atom, clipboard_prop,
> -platform_win, CurrentTime);
> +/* Seems platform_win can be NULL, we'll just ignore that for now.
> +   This will be fixed when the rest of cut and paste lands */
> +if (platform_win) {
> +  XConvertSelection(x_display, XA_PRIMARY, utf8_atom, clipboard_prop,
> + platform_win, CurrentTime);
> +}
>  }
>  

As it works around crashes here. Then i'll do the 0.6.1 release and we
can land this post release.

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] Replace epoll with select in X client

[Alexander Larsson]

The use of epoll in the client is totally overkill in terms of
scalability, and its a problem for portability. The OSX port converts
this to use select, but keeps some of the old complexities in the code.
This new patch makes it simpler and look much more like the windows
code.

Review?

diff --git a/client/x11/event_sources_p.h b/client/x11/event_sources_p.h
index 09703c0..959460c 100644
--- a/client/x11/event_sources_p.h
+++ b/client/x11/event_sources_p.h
@@ -21,24 +21,18 @@
 #include "common.h"
 #include "threads.h"
 
-#if __GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 8)
-#define USING_EVENT_FD
-#endif
-
 #define INFINITE -1
 
-class EventWrapper;
+class EventSource;
 
 class EventSources_p {
-public:
-void remove_wrapper(EventWrapper*);
+protected:
+void add_event(int fd, EventSource* source);
+void remove_event(EventSource* source);
 
 public:
-int _epoll;
-typedef std::list Events;
-Events _events;
-
-friend class EventWrapper;
+std::vector _events;
+std::vector _fds;
 };
 
 class Trigger_p {
@@ -49,9 +43,7 @@ public:
 
 public:
 int _event_fd;
-#ifndef USING_EVENT_FD
 int _event_write_fd;
-#endif
 bool _pending_int;
 Mutex _lock;
 };
diff --git a/client/x11/event_sources_p.cpp b/client/x11/event_sources_p.cpp
index d59bffd..54d950f 100644
--- a/client/x11/event_sources_p.cpp
+++ b/client/x11/event_sources_p.cpp
@@ -15,189 +15,141 @@
License along with this library; if not, see .
 */
 
-#include 
+#include 
 #include 
 
 #include "event_sources.h"
 #include "debug.h"
 #include "utils.h"
 
-#ifdef USING_EVENT_FD
-#include 
-#endif
-
-#define NUM_EPOLL_EVENTS 10
-
-#ifdef USING_EVENT_FD
-#define WRITE_FD _event_fd
-#define EVENT_DATA_TYPE eventfd_t
-#else
-#define WRITE_FD _event_write_fd
-#define EVENT_DATA_TYPE uint8_t
-#endif
-
-class EventWrapper {
-public:
-EventWrapper(EventSources& owner, EventSource& event)
-: _owner (owner)
-, _event (&event)
-, _refs (1)
-{
-}
-
-EventWrapper* ref()
-{
-_refs++;
-return this;
+static void set_non_blocking(int fd)
+{
+int flags;
+if ((flags = ::fcntl(fd, F_GETFL)) == -1) {
+THROW("failed to set socket non block: %s", strerror(errno));
 }
 
-void unref()
-{
-if (!--_refs) {
-_owner.remove_wrapper(this);
-delete this;
-}
+if (::fcntl(fd, F_SETFL, flags | O_NONBLOCK) == -1) {
+THROW("failed to set socket non block: %s", strerror(errno));
 }
+}
 
-EventSource* get_event()
-{
-return _event;
+static void set_blocking(int fd)
+{
+int flags;
+if ((flags = ::fcntl(fd, F_GETFL)) == -1) {
+THROW("failed to clear socket non block: %s", strerror(errno));
 }
 
-void invalidate()
-{
-_event = NULL;
+if (::fcntl(fd, F_SETFL, flags & ~O_NONBLOCK) == -1) {
+THROW("failed to clear socket non block: %s", strerror(errno));
 }
-
-private:
-EventSources& _owner;
-EventSource* _event;
-int _refs;
-};
+}
 
 EventSources::EventSources()
 {
-_epoll = epoll_create(NUM_EPOLL_EVENTS);
-if (_epoll == -1) {
-THROW("create epool failed");
-}
 }
 
 EventSources::~EventSources()
 {
-Events::iterator iter = _events.begin();
-for (; iter != _events.end(); iter++) {
-delete *iter;
-}
-close(_epoll);
 }
 
-bool EventSources::wait_events(int timeout_ms)
+void EventSources_p::add_event(int fd, EventSource* source)
 {
-struct epoll_event events[NUM_EPOLL_EVENTS];
-int num_events = epoll_wait(_epoll, events, NUM_EPOLL_EVENTS, timeout_ms);
+int size = _events.size();
+_events.resize(size + 1);
+_fds.resize(size + 1);
+_events[size] = source;
+_fds[size] = fd;
+}
 
-if (num_events == -1) {
-if (errno == EINTR) {
-return false;
+void EventSources_p::remove_event(EventSource* source)
+{
+int size = _events.size();
+for (int i = 0; i < size; i++) {
+if (_events[i] == source) {
+for (i++; i < size; i++) {
+_events[i - 1] = _events[i];
+_fds[i - 1] = _fds[i];
+}
+_events.resize(size - 1);
+_fds.resize(size - 1);
+return;
 }
-THROW("wait error eventfd failed");
+}
+THROW("event not found");
+}
+
+bool EventSources::wait_events(int timeout_msec)
+{
+int maxfd = 0;
+fd_set rfds;
+struct timeval tv;
+struct timeval *tvp;
+int ready;
+
+FD_ZERO(&rfds);
+
+int size = _events.size();
+for (int i = 0; i < size; i++) {
+maxfd = MAX(maxfd, _fds[i]);
+FD_SET(_fds[i], &rfds);
 }
 
-for (int i = 0; i < num_events; i++) {
-((EventWrapper*)events[i].data.ptr)->ref();
+if (timeout_msec == INFINITE) {
+tvp = NULL;
+} else {
+tv.tv_sec = timeout_msec / 1000;
+tv.tv_usec = (timeout_msec % 1000) *

Re: [Spice-devel] [PATCH] client: support clipboard/selection-owner model (v2)

[Alexander Larsson]

On Wed, 2010-09-29 at 14:17 +0200, Arnon Gilboa wrote:

> -enable USE_XRANDR_1_2

I'm commiting this right away so we get it for 0.6.1.

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] 0.6.1 release and cut and paste

[Alexander Larsson]

On Mon, 2010-09-27 at 16:45 +0200, Arnon Gilboa wrote:
> +1 to your "0.6.1 now". what's the cons of c&p only in 0.6.2 (in a week 
> or 2!) ?

I'm for this too, and will do a 0.6.1 release today.

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] vd_agent: add protocol messages for clipboard/selection-owner model

[Alexander Larsson]

On Sun, 2010-09-26 at 10:27 +0200, Arnon Gilboa wrote:
> Alexander Larsson wrote:
> > On Wed, 2010-09-22 at 11:47 +0200, Arnon Gilboa wrote:
> >   
> >> with the current msgs we can use multiple GRAB(type)s & REQUEST(type)s ;)
> >> 
> >
> > I don't think that works. You'd need to know when to reset the list of
> > types and when to add to it.
> >
> >   
> You are right. Should we vectorize the type in the grab msg?
> e.g.
> typedef struct SPICE_ATTR_PACKED VDAgentClipboardGrab {
> uint32_t type[0];
> } VDAgentClipboardGrab;
> 
> In the request & data we will keep type as is, which is enough for our 
> needs.

Yeah, might as well do this for future possibilities.

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] spice 6.0

[Alexander Larsson]

On Thu, 2010-09-23 at 22:46 +0200, len...@opensourcemedia.de wrote:
> On 22.09.2010 15:25, Alexander Larsson wrote:
> > On Wed, 2010-09-22 at 11:27 +0200, len...@opensourcemedia.de wrote:
> >> On 22.09.2010 10:23, Alexander Larsson wrote:
> >> Thanks for the quick reply. It worked with the
> >>
> >> -global qxl.revision=2 switch
> >>
> >> but performance ist still really slow, task-manager in xp shows 100% 
> >> cpu most of the time whether I use '-enable-kvm' or not.
> >> My server is a quadcore intel with 8G ram.
> >> Is this a qemu issue?
> >> I remember playing around with spice on fedora 13 in version 0.4.x 
> >> and back then I was impressed with the performance of audio and video 
> >> (flash) over lan.
> > Did you install the drivers at
> >
> > http://spice-space.org/download/binaries/qxl-win32-0.6.0.zip
> >
> 
> Yes, drivers and agent are installed in the newest version.

Weird. A few things have changed since 0.4.0, but you should be getting
approximately the same kind of performance.

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] vd_agent: add protocol messages for clipboard/selection-owner model

[Alexander Larsson]

On Wed, 2010-09-22 at 11:47 +0200, Arnon Gilboa wrote:
> with the current msgs we can use multiple GRAB(type)s & REQUEST(type)s ;)

I don't think that works. You'd need to know when to reset the list of
types and when to add to it.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's an immortal hunchbacked card sharp with a robot buddy named Sparky. She's 
a high-kicking hypochondriac mechanic who hides her beauty behind a pair of 
thick-framed spectacles. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] client: support clipboard/selection-owner model

[Alexander Larsson]

On Wed, 2010-09-22 at 14:16 +0200, Arnon Gilboa wrote:
> 
>  utf8_atom = XInternAtom(x_display, "UTF8_STRING", False);
> +
> +Platform::clipboard_formats[0].format = utf8_atom;
> +Platform::clipboard_formats[0].type =
> Platform::CLIPBOARD_UTF8_TEXT;

How common is UTF8_STRING support these days? Can we rely on all
interesting apps supporting it, or should we also try to handle the
older non-utf8 X text formats by doing the conversion ourselves?

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a sword-wielding native American gentleman spy haunted by an iconic dead 
American confidante She's a cold-hearted hypochondriac angel from aristocratic 
European stock. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] vd_agent: support clipboard/selection-owner model

[Alexander Larsson]

On Wed, 2010-09-22 at 11:45 +0200, Arnon Gilboa wrote:
> MSDN is saying: "WM_RENDERALLFORMATS Message is sent to the clipboard 
> owner before it is destroyed, if the clipboard owner has delayed 
> rendering one or more clipboard formats. For the content of the 
> clipboard to remain available to other applications, the clipboard owner 
> must render data in all the formats it is capable of generating, and 
> place the data on the clipboard by calling the SetClipboardData function. "
> 
> When our agent is killed, do we really want to send a REQUEST to the 
> client? I see no reason to render in this case or keep the clipboard 
> available for other apps.

That indeed seems a bit silly. Isn't windows waiting for you to render
something though? You could just set the data to a zero size block.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's an unconventional amnesiac ex-con in drag. She's a radical green-skinned 
soap star with a knack for trouble. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 0/5] Fix bugs found by WHQL

[Alexander Larsson]

On Tue, 2010-09-21 at 20:19 +0200, al...@redhat.com wrote:
> From: Alexander Larsson 
> 
> I've been playing with an old WHQL display test running various
> rendering tests to see how we stack up. During this testing
> I found a bunch of crashes and leaks, fixed in this series.
> 
> Alexander Larsson (5):
>   Fix crash when resetting pixman image transform
>   server: Don't leak QUIC image chunks
>   server: Use the right image size for self_bitmap
>   server: Handle NULL image in red_update_streamable
>   server: Handle self_image in localize_bitmap
> 
>  common/canvas_base.c   |3 ++-
>  server/red_parse_qxl.c |3 +++
>  server/red_worker.c|   32 
>  3 files changed, 25 insertions(+), 13 deletions(-)

Found another one:

commit c425ed626c19ac5c2401cb8765033dd3e9d558b8
Author: Alexander Larsson 
Date:   Wed Sep 22 14:36:40 2010 +0200

Handle surface images in DrawOpaque

diff --git a/common/canvas_base.c b/common/canvas_base.c
index acf9ae5..c2763bc 100644
--- a/common/canvas_base.c
+++ b/common/canvas_base.c
@@ -2414,6 +2414,7 @@ static void canvas_draw_opaque(SpiceCanvas *spice_canvas, 
SpiceRect *bbox, Spice
 CanvasBase *canvas = (CanvasBase *)spice_canvas;
 pixman_image_t *src_image;
 pixman_region32_t dest_region;
+SpiceCanvas *surface_canvas;
 SpiceROP rop;
 
 pixman_region32_init_rect(&dest_region,
@@ -2436,30 +2437,52 @@ static void canvas_draw_opaque(SpiceCanvas 
*spice_canvas, SpiceRect *bbox, Spice
 return;
 }
 
-src_image = canvas_get_image(canvas, opaque->src_bitmap, FALSE);
-
-if (rect_is_same_size(bbox, &opaque->src_area)) {
-spice_canvas->ops->blit_image(spice_canvas, &dest_region,
-  src_image,
-  bbox->left - opaque->src_area.left,
-  bbox->top - opaque->src_area.top);
+surface_canvas = canvas_get_surface(canvas, opaque->src_bitmap);
+if (surface_canvas) {
+if (rect_is_same_size(bbox, &opaque->src_area)) {
+spice_canvas->ops->blit_image_from_surface(spice_canvas, 
&dest_region,
+   surface_canvas,
+   bbox->left - 
opaque->src_area.left,
+   bbox->top - 
opaque->src_area.top);
+} else {
+spice_canvas->ops->scale_image_from_surface(spice_canvas, 
&dest_region,
+surface_canvas,
+opaque->src_area.left,
+opaque->src_area.top,
+opaque->src_area.right 
- opaque->src_area.left,
+
opaque->src_area.bottom - opaque->src_area.top,
+bbox->left,
+bbox->top,
+bbox->right - 
bbox->left,
+bbox->bottom - 
bbox->top,
+opaque->scale_mode);
+}
 } else {
-spice_canvas->ops->scale_image(spice_canvas, &dest_region,
-   src_image,
-   opaque->src_area.left,
-   opaque->src_area.top,
-   opaque->src_area.right - 
opaque->src_area.left,
-   opaque->src_area.bottom - 
opaque->src_area.top,
-   bbox->left,
-   bbox->top,
-   bbox->right - bbox->left,
-   bbox->bottom - bbox->top,
-   opaque->scale_mode);
+src_image = canvas_get_image(canvas, opaque->src_bitmap, FALSE);
+
+if (rect_is_same_size(bbox, &opaque->src_area)) {
+spice_canvas->ops->blit_image(spice_canvas, &dest_region,
+  src_image,
+  bbox->left - opaque->src_area.left,
+  bbox->top - opaque->src_area.top);
+} else {
+spice_canvas->ops->scale_image(spice_canvas, &dest_region,
+   src_image,
+   opaq

[Spice-devel] Dynamic and high resolutions

[Alexander Larsson]

The way resolution and modesetting work right now is static. We have
two static items:
1) A static (atm 8 meg) framebuffer location, where the primary
   surface is always allocated at offset 0. The memory not used
   by the framebuffer is wasted.
2) A static list of supported resolutions in the qxl device rom.

Changing any of these is an incompatible change for device migration.
Additionally there can only be one primary surface per device, which
is also less dynamic than we want.

This is no good, we want to be able to support resolutions larger than
fits in 8 megs without wasting ram, and we want to support resolutions
that the client can support without being in some hardcoded list, and
we want to support multiple monitors on a single device.

We've discussed this internally and on bugs before, and I'd like to
write down in public some of the ideas we've come up with.

First of all, the list of resolutions in the rom is required for
compatibility with drivers that use it, but the actual operations that
set the mode doesn't take this list into account at all. All they pass
is the resolution and the width/height. So, while the current windows
driver does limit itself to this set of resolution we can easily fix
this in a driver update, with no spice changes needed.

To make this work nicely we could have a static small set of
resolutions, then have the client report further resolutions. Maybe we
could also have modesetting working in the driver to any resolution
(even those not listed in the drivers mode list) so that the agent can
change to any resolution, thus matching whatever resoltion the client
currently has. In fact, this way we could probably support resizeing
the client window changing the resolution.

Secondly, the 8 meg ram region that is currently used for the fixed
framebuffer (and as vga ram in vga mode) isn't really implicit in
modesetting. When modesetting all we do is pass the physical memory
address of the framebuffer and its stride to spice. At the moment
we always pass in the start of the 8 meg block, but there is nothing
that requires this.

So, for framebuffers larger than 8 megabytes we should try to allocate
the framebuffer from the vram, which is where we allocate offscreen
surfaces. This is currently 64 meg, so should be enough for very high
resolutions. It may be that the allocation fails due to fragmentation
or lots of offscreens, but if so we can just migrate some offscreens
into normal software surfaces.

I don't think there is anything in the current spice server that makes
this not work, but only experimenting can verify this.

The one last thing is support for multiple primary surfaces. This is
something that the X driver need for true Xrandr. Theoretically this
should work by creating multiple primaries, some in the vram. However,
this is unlikely to work with current spice as there are assumptions
at various places in the code that there is only one primary
surface. So, this requires changes in all of spice.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a fiendish alcoholic werewolf for the 21st century. She's a brilliant 
kleptomaniac journalist with an evil twin sister. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] vd_agent: add protocol messages for clipboard/selection-owner model

[Alexander Larsson]

On Tue, 2010-09-21 at 13:54 +0200, Hans de Goede wrote:
> Hi,
> 
> On 09/21/2010 09:31 AM, Arnon Gilboa wrote:
> > pasting clipboard data is now "only-by-demand" from both sides (client and 
> > agent), whose behavior is symmetric.
> >
> > -VD_AGENT_CLIPBOARD_GRAB(type) - tell the other side that an application in 
> > our side ("we") got ownership of the clipboard.
> > -VD_AGENT_CLIPBOARD_REQUEST(type) - after we know the other side owns the 
> > clipboard (GRAB received), we notify the os we are the owner. when we are 
> > asked by the os/app for the clipboard data, we send this REQUEST msg to the 
> > other side.
> > -VD_AGENT_CLIPBOARD(type, data) - the existing message for sending the 
> > clipboard, is now sent only in response to REQUEST.
> > -VD_AGENT_CLIPBOARD_RELEASE - tell the other side that we are no longer the 
> > owner of the clipboard (e.g. the owner app was closed).
> >
> > this patch will be followed by agent&  client patches handling the above 
> > messages.
> > ---
> >   spice/vd_agent.h |   13 -
> >   1 files changed, 12 insertions(+), 1 deletions(-)
> >
> > diff --git a/spice/vd_agent.h b/spice/vd_agent.h
> > index 9e5adec..0da23aa 100644
> > --- a/spice/vd_agent.h
> > +++ b/spice/vd_agent.h
> > @@ -65,6 +65,9 @@ enum {
> >   VD_AGENT_CLIPBOARD,
> >   VD_AGENT_DISPLAY_CONFIG,
> >   VD_AGENT_ANNOUNCE_CAPABILITIES,
> > +VD_AGENT_CLIPBOARD_GRAB,
> > +VD_AGENT_CLIPBOARD_REQUEST,
> > +VD_AGENT_CLIPBOARD_RELEASE,
> >   VD_AGENT_END_MESSAGE,
> >   };
> >
> > @@ -121,7 +124,6 @@ enum {
> >   VD_AGENT_ERROR,
> >   };
> >
> > -//FIXME: size required?
> >   typedef struct SPICE_ATTR_PACKED VDAgentClipboard {
> >   uint32_t type;
> >   uint8_t data[0];
> > @@ -129,8 +131,17 @@ typedef struct SPICE_ATTR_PACKED VDAgentClipboard {
> 
> >   enum {
> >   VD_AGENT_CLIPBOARD_UTF8_TEXT = 1,
> > +VD_AGENT_CLIPBOARD_BITMAP,
> >   };
> >
> 
> Hmm, I'm not sure about this bit, wouldn't it be better to
> use a string with a mimetype in there, that brings us much
> larger flexibility. I think this is esp. usefull when the
> guest as and the client os are the same os, then
> copying of all kind of things could be .

Not sure about this, if we want full compatibility with same-os cut and
paste we need more features anyway, like multiple formats. I think its
more important to allow some common subset of clipboard types to work
across os types than supporting all native types.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a shy Amish sorceror trapped in a world he never made. She's a 
disco-crazy Buddhist former first lady from a secret island of warrior women. 
They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] vd_agent: add protocol messages for clipboard/selection-owner model

[Alexander Larsson]

On Tue, 2010-09-21 at 09:31 +0200, Arnon Gilboa wrote:
> pasting clipboard data is now "only-by-demand" from both sides (client and 
> agent), whose behavior is symmetric.
> 
> -VD_AGENT_CLIPBOARD_GRAB(type) - tell the other side that an application in 
> our side ("we") got ownership of the clipboard.
> -VD_AGENT_CLIPBOARD_REQUEST(type) - after we know the other side owns the 
> clipboard (GRAB received), we notify the os we are the owner. when we are 
> asked by the os/app for the clipboard data, we send this REQUEST msg to the 
> other side.
> -VD_AGENT_CLIPBOARD(type, data) - the existing message for sending the 
> clipboard, is now sent only in response to REQUEST.
> -VD_AGENT_CLIPBOARD_RELEASE - tell the other side that we are no longer the 
> owner of the clipboard (e.g. the owner app was closed).
> 
> this patch will be followed by agent & client patches handling the above 
> messages.
> ---

One possible issue i see with this is that this only allows us to
specify one type for the clipboard context. Whereas in both win32 and X
you can claim several formats and then the consumer side can pick which
one to use (i.e. a web browser would support copy as html or copy as
text, etc).

I'm not sure we want to really support this, as it increases the
complexity of cut and paste quite a bit, and is perhaps not as important
as just getting text cut and paste working.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a suicidal coffee-fuelled gangster on the run. She's a high-kicking 
psychic hooker descended from a line of powerful witches. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] vdservice: use "com.redhat.spice.0" symbolic link as virtio-serial port path

[Alexander Larsson]

On Wed, 2010-09-22 at 08:53 +0200, Arnon Gilboa wrote:
> remove get_device_path() by GUID & dependency on setupapi.lib
> ---

What exactly is setupapi.lib and why did you remove it here and then add
it in some places in a later patch?

Otherwise this looks good.



-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a war-weary hunchbacked filmmaker haunted by memories of 'Nam. She's a 
psychotic belly-dancing stripper from out of town. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] vd_agent: support clipboard/selection-owner model

[Alexander Larsson]

On Tue, 2010-09-21 at 19:16 +0200, Arnon Gilboa wrote:
> -#endif // CLIPBOARD_ENABLED
> +case WM_RENDERFORMAT:
> +a->on_clipboard_render((UINT)wparam);
> +break;
> +case WM_RENDERALLFORMATS:
> +vd_printf("WM_RENDERALLFORMATS");
> +break;

I think you need to handle RENDERALLFORMATS too.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a suicidal shark-wrestling sorceror with a secret. She's a disco-crazy 
hip-hop barmaid in the witness protection program. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] vdservice: replace VDAgentDataChunk with VDIChunkHeader

[Alexander Larsson]

On Tue, 2010-09-21 at 19:16 +0200, Arnon Gilboa wrote:
> ---
>  vdservice/vdi_port.cpp  |2 +-
>  vdservice/vdservice.cpp |   12 +++-
>  2 files changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/vds

Ack

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a jaded drug-addicted assassin who knows the secret of the alien 
invasion. She's a mentally unstable mute college professor who inherited a 
spooky stately manor from her late maiden aunt. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] vdservice: cleanup vcprojs

[Alexander Larsson]

On Tue, 2010-09-21 at 19:16 +0200, Arnon Gilboa wrote:
> -remove deprecated Detect64BitPortabilityProblems
> -add setupapi.lib to AdditionalDependencies in x64

Not sure exactly what these are but they look like safe changes, and if
they make things work for you: ack.


-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a suave playboy gangster haunted by memories of 'Nam. She's a 
high-kicking renegade safe cracker living homeless in New York's sewers. They 
fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] Spice roadmap

[Alexander Larsson]

On Mon, 2010-09-20 at 13:27 -0700, Stephen Duse-Anthony wrote:


> HI,
> I would like to know if you have a roadmap with an ETA for the
> following features:
> Network tunneling:
> - usb device redirection
> - CD-ROM redirection
> - Printer redirection

There is some initial experimental support for network tunneling, but
its not enabled by default and we don't know if it is the right
approach.

For the others, no ETA exists, but they're all things we eventually want
to have.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat,
Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a globe-trotting coffee-fuelled rock star from a doomed world.
She's a 
scantily clad tempestuous fairy princess with a song in her heart and a
spring 
in her step. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] spice 6.0

[Alexander Larsson]

On Wed, 2010-09-22 at 09:10 +0200, len...@opensourcemedia.de wrote:
> Hi there,
> 
> I am experimenting with the newest version of Spice (6.0) on Fedora14 
> alpha. I managed to get everything installed fine via yum but my qemu 
> doesn't seem to work with the qxl device.
> 
> If I start it like this:
> 
> qemu xp.img -usbdevice tablet -soundhw ac97 -vga qxl -m 3200 -spice 
> port=5930,disable-ticketing -enable-kvm
> 
> and start the client like this:
> 
> spicec -h localhost -p 5930
> 
> i get eventually a tiny little window for the client which I cannot 
> maximize or toggle fullscreen.

Yes, this is a bug in the qemu spice integration (bug
https://bugzilla.redhat.com/show_bug.cgi?id=634535 waiting for update to
reach repos).

For now, to work around it, pass "-global qxl.revision=2" on the qemu
command line.

Sorry about this.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a globe-trotting moralistic paranormal investigator searching for his 
wife's true killer. She's a mentally unstable Bolivian schoolgirl fleeing from 
a Satanic cult. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 00/17] Win32 driver thread safety

[Alexander Larsson]

On Tue, 2010-09-14 at 21:08 +0200, al...@redhat.com wrote:
> From: Alexander Larsson 

Izik reviewed all of these and found no issues, and i've been running it
for a while without issues, so i'm pushing it now.

However, if anyone has time please do review this anyway as there is a
non-neglible risk we missed some case.


-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a superhumanly strong white trash gentleman spy on the run. She's a 
vivacious green-skinned snake charmer with a knack for trouble. They fight 
crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] 0.6.1 release schedule

[Alexander Larsson]

I added a 0.6.1 release to the schedule at 
http://spice-space.org/page/Releases/SpiceZeroPointSix

This will include the current commited bugfixes, and possibly the cut
and paste support if that is ready in time. Additionally I want to get
the win32 driver threadsafety fixes in.

Are there other outstanding issues we should try to get in?

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's an ungodly albino master criminal with acid for blood. She's a 
bloodthirsty African-American widow from out of town. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] set initial depth

[Alexander Larsson]

On Fri, 2010-09-17 at 18:15 -0400, bwellsnc wrote:
> I am running 0.6.0 and I have a quick question.  Is there a way to set
> the initial depth.  The -g doesn't seem to work.  I am trying to run
> Ubuntu 10.04 and it will not boot because it doesn't seem to know the
> initial screen resolution.  It I am able to set it to 640x480, then
> this should work or if I know what spice defaults to then I can set
> that in the grub configuration.

I'm not sure what you mean by initial depth. The guest OS will boot and
see the hardware as a generic vga compatible card, and use whatever
resolutions it would have used on any other vga card. Then when the qxl
driver is loaded it can support more resolutions and depths, but which
ones are used depends on how the guest os/drivers are setup.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's an oversexed misogynist senator who must take medication to keep him 
sane. She's a brilliant paranoid schoolgirl living homeless in New York's 
sewers. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 02/17] Drop DevResDynamic

[Alexander Larsson]

On Tue, 2010-09-14 at 21:08 +0200, al...@redhat.com wrote:
> From: Alexander Larsson 
> 
> Now that all DevRes are dynamic we can just put the data in DevRes.
> ---

Got an ack from izik on irc, pushing.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's an impetuous amnesiac barbarian on a mission from God. She's a 
mistrustful bisexual doctor in the wrong place at the wrong time. They fight 
crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 01/17] Make global_res be an array of pointers and Res a pointer

[Alexander Larsson]

On Tue, 2010-09-14 at 21:08 +0200, al...@redhat.com wrote:
> From: Alexander Larsson 
> 
> Instead of allocating an array of DevRes we allocate each DevRes
> dynamically for each element.
> 
> Also make PDEV->Res be a pointer instead of copying the global pdev.
> This also drops the needto Sync it when the pdev is made inactive,
> as all pdevs share the same DevRes.
> 
> This way shared things like semaphores are really shared.
> ---

Got an ack from izik on irc. Pushed.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a notorious flyboy Green Beret on the run. She's a sarcastic paranoid 
soap star from a family of eight older brothers. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] Memory Leak on spicec

[Alexander Larsson]

On Tue, 2010-09-14 at 16:02 +0200, Alex Peter wrote:
> I referred "Target is the one running the spicec client"

We had a shared memory leak in the X client, but that was fixed before
0.6.0 was released. I have not personally seen any other client leaks.

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] smartcard: add channel

[Alexander Larsson]

On Tue, 2010-09-14 at 14:50 -0400, Alon Levy wrote:
> - "Gerd Hoffmann"  wrote:

> > I don't think this is a good idea.  This is just a chardev
> > passthrough, 
> > right?  If so, then we should just make it that.  Name it 'chardev' or
> > 
> > 'datapipe' or something simliar.  Have a additional 'init' message to
> > 
> > specify the kind of chardev.  Then we can just reuse it when we'll
> > have 
> > more simliar users in the future.
> > 
> 
> Do you think this is a better direction then adding the actual messages? (see 
> below)

I don't think its better.

> > Or we could make this a real interface definition where each smartcard
> > 
> > message gets its own message type.
> > 
> 
> Yes, I was lazy - actually let me back up a little. For the smartcard reader
> device to be viable on it's own, it has to talk some protocol that is defined
> outside of spice. That protocol is also a header "type/size" protocol, so it 
> is
> easy to write the spice.proto channel definition for it - that's were I was 
> lazy
> and haven't done it. I guess it's time to be less lazy.

Sounds good to me. Then you'd get automatic (de)marshalling support for
it in the client too.

> > Oh, and shouldn't the channel-specific messages better start with
> > '101' 
> > like all other channels do?
> 
> Yeah. Anyone know why they start at that particular number?

Its because all channels derive from BaseChannel, adding some unused
numbers so that it can be extended in the future.


___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] smartcard: add channel

[Alexander Larsson]

On Tue, 2010-09-14 at 14:46 -0400, Alon Levy wrote:
> - "Alexander Larsson"  wrote:
> 
> > On Mon, 2010-09-13 at 13:58 -0400, Alon Levy wrote:
> > > ---
> > >  spice/enums.h |   13 +
> > >  1 files changed, 13 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/spice/enums.h b/spice/enums.h
> > 
> > Is this change generated? If not, just update the spice.proto and run
> > 
> 
> yes, it is.

Hmm? Then, is your channel derived from BaseChannel? It doesn't look so
since the messages start at 1.

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 3/3] fix palette handling for 0.4 compat

[Alexander Larsson]

On Tue, 2010-09-14 at 10:45 +0200, Gerd Hoffmann wrote:
> spice 0.4 guests pass 16bpp palette colors when running in
> a 16bpp video mode.  Convert them to 32bpp.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

ack

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's an otherworldly Catholic dog-catcher on a mission from God. She's a 
sarcastic thirtysomething queen of the dead from a family of eight older 
brothers. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 2/3] fix brush handling for 0.4 compat

[Alexander Larsson]

On Tue, 2010-09-14 at 10:45 +0200, Gerd Hoffmann wrote:
> spice 0.4 guests pass 16bpp colors for brushes when running in
> a 16bpp video mode.  Convert them to 32bpp.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

ack

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a short-sighted native American romance novelist with a secret. She's a 
manipulative snooty cab driver with a flame-thrower. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 1/3] move command flags handling to the qxl parser

[Alexander Larsson]

On Tue, 2010-09-14 at 10:45 +0200, Gerd Hoffmann wrote:
> Pass through command flags to the qxl parser, so we can hide all
> compat bits for spice 0.4 within the qxl parser.

ack
-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a suicidal hunchbacked hairdresser fleeing from a secret government 
programme. She's a ditzy junkie vampire with the soul of a mighty warrior. 
They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] smartcard: add channel

[Alexander Larsson]

On Mon, 2010-09-13 at 13:58 -0400, Alon Levy wrote:
> ---
>  spice/enums.h |   13 +
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/spice/enums.h b/spice/enums.h

Is this change generated? If not, just update the spice.proto and run

 ./spice_codegen.py -e spice.proto ../spice-proto/spice/enums.h

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's an unconventional white trash paramedic with no name. She's a scantily 
clad French-Canadian mercenary with an evil twin sister. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] add compat flag for 16bpp mode

[Alexander Larsson]

On Tue, 2010-09-14 at 10:12 +0200, Gerd Hoffmann wrote:
> spice 0.6 uses 32bpp values unconditionally for brush and palette
> colors.
> 
> spice 0.4 used to use 16 bpp or 32 bpp depending on the video mode.
> The qxl parser needs to know the guest video mode depth to correctly
> interpret these values in spice 0.4 compat mode.  Add a flag to pass
> on this informartion.
> 
> Signed-off-by: Gerd Hoffmann 

Ack

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's an all-American ninja boxer on the run. She's a cold-hearted impetuous 
single mother looking for love in all the wrong places. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] Memory Leak on spicec

[Alexander Larsson]

On Tue, 2010-09-14 at 04:13 +0200, Alex Peter wrote:
> Dear All,
> 
> 
> I have crossed compiled spice client for target platform
> (spicec, 0.6.0 version)  and server &  qemu (git version) on x86_64
> system.
> 
> XP SP3 was installed under qemulator.  both server and client are in
> same network.  target platform is able to display XP GUI on top X
> server.  
> 
> As per my observation, Target system's memory was keep reducing for
> each activities like window opening.  At some point of time, 
> 
> out of memory error happens on the Target system. Any of you guys
> observed this behavior ?.

By target, do you mean the guest, or the one running the client?


> Additional Info:
> 
> 
> 1. Cursor movement was not at all smooth, i.e pointer is
> moving around.. 

Try using a tablet instead of a mouse ( -usbdevice tablet in qemu).


-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's an uncontrollable chivalrous waffle chef She's an elegant mute vampire 
fleeing from a Satanic cult. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH xf86-video-qxl 1/2] Change default virtual size to 1920x1080

[Alexander Larsson]

On Mon, 2010-09-13 at 22:14 +0200, Hans de Goede wrote:
> With the old default virtual size of 1024x768, using higher resolutions
> is not possible without an xorg.conf. Since the default now a days is
> to not have an xorg.conf, this is sort of unfortunate.
> 
> This patch makes these higher resolutions available by making the
> default virtual size 1920x1080, while keeping the default resolution
> used when none is specified through xorg.conf at 1024x786, so that
> the spice client window won't be way too large for smaller screens
> by default.
> 
> This change does come at the prize of using 5MB more memory, but that
> seems like a reasonable price to pay to give us parity wrt supported
> resolutions with the windows driver. Also this is a must have to allow
> the to be written Linux agent to change the guest resolution to match
> the client machines one when running in auto fullscreen mode.

Does this really use 5 megs more memory? I don't think we ever use the
framebuffer part of the qxl device for anything but the primary surface,
so the 5 megs are unused right now. (unless i'm missing something.)


-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a lounge-singing crooked sorceror who hangs with the wrong crowd. She's a 
sarcastic mutant schoolgirl from a different time and place. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] Are there 64 bit Windows 7 QXL drivers?

[Alexander Larsson]

On Thu, 2010-09-09 at 10:23 -0400, Matt Feinberg wrote:
> I apologize if this information is on the list somewhere.  I could not find 
> it.
> 
> Are there binaries for 64 bit Windows 7 QXL drivers?  I have a 64 bit Windows 
> 7
> guest which I'm trying to access.

I'm not sure if the current qxl sources build in 64bit. You'd have to
try.

> Alternatively, are there instructions on how to build the drivers for 
> Windows?  The information
> in the user manual appears incomplete for this task.

Download the windows driver kit, start a build console from it,
set SPICE_COMMON_DIR to point to spice-protocol, then go to the qxl dir
and run "build".

This is all pretty standard windows driver building.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's an uncontrollable shark-wrestling librarian with nothing left to lose. 
She's a time-travelling Bolivian bounty hunter looking for love in all the 
wrong places. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 00/35] *** SUBJECT HERE ***

[Alexander Larsson]

On Thu, 2010-09-09 at 19:15 +0200, al...@redhat.com wrote:
> From: Alexander Larsson 
> 
> *** BLURB HERE ***

Sorry. Didn't mean to send this one...


-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's an obese guitar-strumming dog-catcher in a wheelchair. She's a radical 
paranoid queen of the dead on her way to prison for a murder she didn't 
commit. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] leaking drawables

[Alexander Larsson]

On Thu, 2010-09-09 at 09:31 +0200, Gerd Hoffmann wrote:
> 
> There is one special thing in the release ring usage though.  As you 
> know qxl builds up a linked lists there, where the heads are in the 
> ring.  qxl does also store the head of the list which it currently 
> builds into the ring, but doesn't bump prod (yet) so the guest will not 
> grab it.  Which needs one extra ring entry.  Because of that the IS_FULL 
> macro simply doesn't work for the release ring and the check is open coded.
> 
> So, the old code checks whenever prod - cons + 1 != num_items.  Lets 
> assume the guest didn't take out any entries yet, so cons is 0.  When 
> prod is 7 the check fails and qxl will stop pushing stuff to the ring 
> due to being full.  Lets have a look at the prod == 6 case:
> 
>* The extra entry (ring[7%8]) holds the head for the list qxl
>  is building).
>* qxl pushes that ring item to the guest (SPICE_RING_PUSH),
>  prod is 7 now.
>* qxl gets a pointer to the next entry (SPICE_RING_PROD_ITEM),
>  which is ring[8%8].
>* qxl zero-initializes the ring entry as new list head, thereby
>  overriding ring[0], which isn't yet consumed by the guest.
> 
> Correct?

No, that doesn't seem correct to me.

Initially, both prod an cons are zero, and has 
 num items = prod - cons = 0 - 0 = 0

Then we release an item (interface_release_resource). This starts
writing to the current producer item SPICE_RING_PROD_ITEM(ring, item),
either initializing it or appending to the list. In this case prod is
zero, so we're writing to ring->items[0].

Then we push this, which increases prod to 1 and then initializes the
new temp item (at 1).

Eventually we reach the case where prod is 6 (and lets say cons is still
0). We put stuff in item 6, then SPICE_RING_PUSH increasing prod to 7,
initialize item 7. No overwrite here. Then when prod is 7 we keep
appending to the item 7 list, but never push it because prod - cons + 1
= 7 - 0 + 1 = 8 == num_items.





___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] leaking drawables

[Alexander Larsson]

On Tue, 2010-09-07 at 17:08 +0200, Gerd Hoffmann wrote:
> Hi,
> 
> > This happens for several consecutive resource releases. Looking at what
> > actually gets released by the driver we see that the resources are freed
> > in the same order as they were release, its just that a chunk of
> > resources are missing here and there.
> 
> Try the attached patch.  I'm pretty sure qxl just overruns the release 
> ring, making complete release lists disappear now and then.

This doesn't seem right. The ring doesn't wrap cons and prod, as they
are incremented. Instead they are % num_items when used as indexes. This
means that its ok to have the ring be completely full (and indeed, the
IS_FULL macro just checks prod - cons == num_items).

Thus, prod - cons means we're pushing to the last item in the ring.

And, testing with this patch I still see the leak.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's an otherworldly white trash Green Beret fleeing from a secret government 
programme. She's a transdimensional winged wrestler from out of town. They 
fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] leaking drawables

[Alexander Larsson]

I've been tracking the "out of memory" issue. i.e. we seem to be getting
these to often. And it seems we might be leaking resources.

I modified the driver and the server to print Drawables as they are
freed. In one run there were several instances of the server releasing a
drawable. I.E. in free_red_drawable we call release_resource, but the
corresponding resource is never seen in ReleaseOutput.

This happens for several consecutive resource releases. Looking at what
actually gets released by the driver we see that the resources are freed
in the same order as they were release, its just that a chunk of
resources are missing here and there.

To me this sounds like a race, and we know there are threadsafety issues
in the win32 driver. For instance, we might be racing over reading the
release ring, thus missing one list of items to free.

Unfortunately I don't have time to look into this until thursday. Maybe
someone else want to take a look before then?

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a one-legged hunchbacked cyborg fleeing from a secret government 
programme. She's a green-fingered Buddhist research scientist trying to make a 
difference in a man's world. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] server: avoid creating a stream from traces more than once for the same drawable

[Alexander Larsson]

On Thu, 2010-09-02 at 15:24 +0200, Alexander Larsson wrote:
> On Wed, 2010-09-01 at 12:10 +0300, Yonit Halperin wrote:
> > could have caused ASSERT(!drawable->stream) in red_create_stream
> > ---
> >  server/red_worker.c |   15 ++-
> >  1 files changed, 10 insertions(+), 5 deletions(-)
> 
> ack

Pushing this to master.

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] asking help to implement spice

[Alexander Larsson]

On Wed, 2010-09-01 at 11:05 +0530, mohan Doss wrote:
> hello sir i am mohan dass i am doing BE final year , i like doing
> project in desktop virtulization .can you help me to implement your
> protocol in my project.

"Implement a protocol" is a very vague term. You need to describe more
what you want to do. Reimplement spice? If so, which part? server?
client? drivers?


___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] server: avoid creating a stream from traces more than once for the same drawable

[Alexander Larsson]

On Wed, 2010-09-01 at 12:10 +0300, Yonit Halperin wrote:
> could have caused ASSERT(!drawable->stream) in red_create_stream
> ---
>  server/red_worker.c |   15 ++-
>  1 files changed, 10 insertions(+), 5 deletions(-)

ack


___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] spicec-x11: Fix going into a never ending loop upon xrandr event (#628573)

[Alexander Larsson]

On Thu, 2010-09-02 at 10:16 +0200, Hans de Goede wrote:
> Hi,
> 
> On 09/02/2010 09:51 AM, Alexander Larsson wrote:
> > On Wed, 2010-09-01 at 20:15 +0200, Hans de Goede wrote:
> >> ---
> >>   client/x11/platform.cpp |5 +
> >>   1 files changed, 5 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/client/x11/platform.cpp b/client/x11/platform.cpp
> >> index 8292d44..1f25a7f 100644
> >> --- a/client/x11/platform.cpp
> >> +++ b/client/x11/platform.cpp
> >> @@ -778,6 +778,11 @@ DynamicScreen::DynamicScreen(Display* display, int 
> >> screen, int&  next_mon_id)
> >>   platform_win = XCreateSimpleWindow(display, RootWindow(display, 
> >> screen), 0, 0, 1, 1, 0, 0, 0);
> >>   XSelectInput(display, platform_win, StructureNotifyMask);
> >>   XRRSelectInput(display, platform_win, RRScreenChangeNotifyMask);
> >> +
> >> +Monitor::self_monitors_change++;
> >> +process_monitor_configure_events(platform_win);
> >> +Monitor::self_monitors_change--;
> >> +
> >>   XPlatform::set_win_proc(platform_win, root_win_proc);
> >>   intern_clipboard_atoms();
> >>   X_DEBUG_SYNC(display);
> >
> > Does this really work? I guess the events come from the XRRSelectInput
> > call, but there is no guarantee that this has even been sent to the
> > xserver (XFlush), even less so gotten the reply into the client event
> > queue. Shouldn't we have some XSync thing here?
> >
> 
> There is an XSync call as the first thing in process_monitor_configure_events

Ah, missed that. Then:

Ack


___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] spicec-x11: Fix going into a never ending loop upon xrandr event (#628573)

[Alexander Larsson]

On Wed, 2010-09-01 at 20:15 +0200, Hans de Goede wrote:
> ---
>  client/x11/platform.cpp |5 +
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/client/x11/platform.cpp b/client/x11/platform.cpp
> index 8292d44..1f25a7f 100644
> --- a/client/x11/platform.cpp
> +++ b/client/x11/platform.cpp
> @@ -778,6 +778,11 @@ DynamicScreen::DynamicScreen(Display* display, int 
> screen, int& next_mon_id)
>  platform_win = XCreateSimpleWindow(display, RootWindow(display, screen), 
> 0, 0, 1, 1, 0, 0, 0);
>  XSelectInput(display, platform_win, StructureNotifyMask);
>  XRRSelectInput(display, platform_win, RRScreenChangeNotifyMask);
> +
> +Monitor::self_monitors_change++;
> +process_monitor_configure_events(platform_win);
> +Monitor::self_monitors_change--;
> +
>  XPlatform::set_win_proc(platform_win, root_win_proc);
>  intern_clipboard_atoms();
>  X_DEBUG_SYNC(display);

Does this really work? I guess the events come from the XRRSelectInput
call, but there is no guarantee that this has even been sent to the
xserver (XFlush), even less so gotten the reply into the client event
queue. Shouldn't we have some XSync thing here?



___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] Announcing Spice 0.6.0

[Alexander Larsson]

After some delays due to holidays we today proudly released Spice 0.6.0.
This is the beginning of a new stable series based on the new spice
network protocol major version. It has some new exciting features like
offscreen surfaces and WAN mode, as well as a more solid codebase and a
saner qemu integration API.

Updated sources and windows binaries are availible at:
http://spice-space.org/download.html

I'm currently working on updating the packages in Fedora 14.


-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's an oversexed ninja paranormal investigator for the 21st century. She's an 
artistic cat-loving magician's assistant with the power to bend men's minds. 
They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] server: red_current_add_equal - don't push a drawable to the middle of the pipe if it depends on surfaces.

[Alexander Larsson]

On Tue, 2010-08-31 at 10:29 +0300, Yonit Halperin wrote:
> This will prevent: 1) rendering problems (commands sent to the client in the 
> wrong order)
> 2) sending commands for surfaces that haven't been created yet on the client 
> side.
> ---

Ack.


-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a Nobel prize-winning coffee-fuelled cat burglar haunted by memories of 
'Nam. She's a mentally unstable mute detective with a birthmark shaped like 
Liberty's torch. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] qxl parser: complete parsing of QXLCompatDrawable structs

[Alexander Larsson]

On Tue, 2010-08-31 at 11:59 +0200, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann 
> ---
>  server/red_parse_qxl.c |   14 ++
>  1 files changed, 14 insertions(+), 0 deletions(-)

Ack

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's an obese small-town photographer on the hunt for the last specimen of a 
great and near-mythical creature. She's a plucky mutant mercenary with a knack 
for trouble. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] delay 0.6.0 to tomorrow

[Alexander Larsson]

I want to get the vdagent versioning patch in to future-proof the agent
protocol, so lets delay 0.6.0 to tomorrow.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a benighted dishevelled shaman with no name. She's a tortured bisexual 
doctor with the soul of a mighty warrior. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] server: fix red_current_flush to flush only the surface that was given as parameter

[Alexander Larsson]

On Mon, 2010-08-30 at 14:03 +0300, Yonit Halperin wrote:
> A side effect of the previous red_current_flush, which flushed all the 
> surfaces, and was called on a new display channel connection, was
> that red_handle_drawable_surfaces_client_synced sent the most updated 
> surfaces images when needed. However, now, it should
> explicitly call red_current_flush.
> Moreover, since red_current_flush was called on a new display channel 
> connection only if there was a primary surface,
> if the connection of the display channel occurred at the moment of no primary 
> surface, red_handle_drawable_surfaces_client_synced was buggy.
> ---

Ack

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a shy vegetarian stage actor with no name. She's a radical bisexual 
Valkyrie in the wrong place at the wrong time. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] Bump versions.

[Alexander Larsson]

On Mon, 2010-08-30 at 12:50 +0200, Gerd Hoffmann wrote:
> Update #define in server/spice.h in preparation for the 0.6.0 release.
> We also got some new functions, thus we have to increate the shared
> lib minor number for spice-server.

Ack

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a fiendish dishevelled photographer from the 'hood. She's a pregnant 
bisexual femme fatale from out of town. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] add vd_agent announce capabilities message

[Alexander Larsson]

On Sun, 2010-08-29 at 13:22 -0400, Alon Levy wrote:
> This adds a 256 bits capabilities bitmap message to spice-protocol.
> 
> 
> ---
>  spice/vd_agent.h |   17 +
>  1 files changed, 17 insertions(+), 0 deletions(-)
> 
> diff --git a/spice/vd_agent.h b/spice/vd_agent.h
> index 8e1e6ca..9007bee 100644
> --- a/spice/vd_agent.h
> +++ b/spice/vd_agent.h
> @@ -54,6 +54,7 @@ enum {
>  VD_AGENT_REPLY,
>  VD_AGENT_CLIPBOARD,
>  VD_AGENT_DISPLAY_CONFIG,
> +VD_AGENT_ANNOUNCE_CAPABILITIES,
>  };
>  
>  typedef struct SPICE_ATTR_PACKED VDAgentMonConfig {
> @@ -119,6 +120,22 @@ enum {
>  VD_AGENT_CLIPBOARD_UTF8_TEXT = 1,
>  };
>  
> +enum {
> +// supports DisplayConfig message
> +VD_AGENT_CAP_DISPLAY_CONFIG=0,
> +
> +VD_AGENT_CAP_MAX=256,
> +};
> +
> +typedef struct SPICE_ATTR_PACKED VDAgentAnnounceCapabilities {
> +uint32_t caps[256/(sizeof(uint32_t)*8)];
> +} VDAgentAnnounceCapabilities;

Why do you use a fixed size for this? Just send the size of the array,
then we can send a smaller size now and expand it dynamically. Just
assume every capability bit not sent is zero if you get a short array in
the client.


-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's an ungodly amnesiac filmmaker with a passion for fast cars. She's a 
provocative foul-mouthed socialite from Mars. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] Add config functions.

[Alexander Larsson]

On Fri, 2010-08-27 at 15:37 +0200, Gerd Hoffmann wrote:
> A bunch of configuration functions where never ported forward from
> rhel-6 to upstream.  Add them so we can add qemu config options for
> these settings.
> ---
>  server/reds.c  |   27 +++
>  server/spice.h |   11 +++
>  2 files changed, 38 insertions(+), 0 deletions(-)

Ack

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a world-famous native American master criminal who hangs with the wrong 
crowd. She's a disco-crazy Buddhist journalist from beyond the grave. They 
fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 1/1] Add API to turn on backwards compatibility mode

[Alexander Larsson]

On Fri, 2010-08-27 at 17:28 +0200, Gerd Hoffmann wrote:
> Hi,
> 
> > Having a more fine-grained command line feature selection just causes
> > complexity and risk that sysadmins get things wrong. I don't see any
> > gain in it.
> 
> Point.
> 
> > I'll note that this switch is really only about migration compatibility.
> 
> Any reason to add this now?  We could delay it until we actually hit a 
> migration compatibility issue.

Don't we have any migration incompatibilities with 0.4? Anyway, its
probably ok to delay this until we need to call it from qemu.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a lonely bohemian cyborg whom everyone believes is mad. She's a 
high-kicking nymphomaniac opera singer prone to fits of savage, blood-crazed 
rage. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 1/1] Add API to turn on backwards compatibility mode

[Alexander Larsson]

On Fri, 2010-08-27 at 15:08 +0200, Gerd Hoffmann wrote:
> >> As you are talking about "set of features" already ...
> >>
> >> I think we should use a feature bitmask instead of a version number in
> >> the API.
> >
> > How would you use this in qemu though? Say you link to spice 0.10.0,
> > which has a set of new features not in 0.8.0. Why would you want to make
> > a spice instance that doesn't use all features in 0.10.0, yet is not
> > compatible with 0.8.0, so you can't migrate between them.
> 
> I would only enable features qemu knows about, i.e. something like
> 
>  features  = spice_get_features()
>  features &= qemu_supported_features;
>  spice_server_enable_features(features);
> 
> Where qemu_supported_features is the set of features the qemu qxl code 
> knows about and can handle.  Have options to turn off specific features.
> 
> In case a new release adds multiple new features users / admins might 
> want to selectively enable/disable them depending on how they affect 
> compatibility.  qemu live migration compatibility is only affected in 
> case the new feature requires additional state to be saved.  client 
> compatibility is only affected in case the wire protocol changes.  So it 
> might make sense to enable a subset of the new features, although that 
> most likely isn't the common case.

I don't think the onus should be on the sysadmin to know what features
is compatible with what version of spice. Thus, in my mind you'll
specify "compat with 0.6" which in 0.8 will enable all the features in
0.8 that are possible to migrate to/from 0.6 (including some that may
not be in 0.6). 

Having a more fine-grained command line feature selection just causes
complexity and risk that sysadmins get things wrong. I don't see any
gain in it.

I'll note that this switch is really only about migration compatibility.
For the client side i think in the future we should either:

* Add a similar switch for client compat
or
* Make sure that we always support connecting with old clients in
  future releases.

I was expecting to do the second. But maybe thats not always possible?

> > Right now we don't do any compat with 0.4, but the plan is that
> > eventually we want to add a qemu mode where it doesn't use offscreen
> > surfaces, nor the new qxl pci device, and allows migration to/from spice
> > 0.4.0 instances.
> 
> It's there.  The new qxl device handles old guest drivers just fine. 
> With the spice.v17 branch even live migration should work.  At least new 
> qxl parses old qxl state just fine when switched into compatibility 
> mode.  Not fully tested yet though due to some non-spice-related 
> migration issues.

Cool.


-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's an immortal native American gangster whom everyone believes is mad. She's 
a wealthy green-skinned fairy princess from Mars. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 1/2] server: Don't send stream frames that are identical to the last one

[Alexander Larsson]

On Fri, 2010-08-27 at 10:49 +0300, Yaniv Kaul wrote:
> On 2010-08-26 18:33, Yonit Halperin wrote:
> > On 08/25/2010 11:49 AM, al...@redhat.com wrote:
> >> From: Alexander Larsson
> >>
> >> This happens e.g. with the vlc player when paused, as it still draws
> >> a continuous stream of frames (bug #28817). It may also help slightly
> >> if a video contains identical frames in a row for other reasons.
> >>
> >> We detect frame equality using the adler32 checksum. There is a small
> >> chance of checksum collisions, but that will just cause one frame to
> >> be skipped, or if it is the last frame it will be fixed when the stream
> >> ends and we get the lossless data for the region.
> >> ---
> >
> > Ack, but I'm not sure we should add efficiency overhead for all 
> > streams (audio and video wise) if only VLC behaves in such manner.
> > Maybe we can add it only for low bandwidth,  and/or preform sampling 
> > of every X frames, and then if they are equal check if all the frames 
> > are equal.
> 
> I totally agree - right now, it's a corner case (and we should file a BZ 
> on VLC to fix their behaviour!)
> In addition, IIRC, adler32 is pretty weak for short messages (has a 
> greater chance of collisions), making it sub-optimal for the audio 
> stream (perhaps fine for the video stream).

adler32 is not used for the audio stream. We check for silence by seeing
if all samples are zero.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's an all-American sweet-toothed cop looking for a cure to the poison 
coursing through his veins. She's a chain-smoking gypsy college professor from 
aristocratic European stock. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 1/1] Add API to turn on backwards compatibility mode

[Alexander Larsson]

On Fri, 2010-08-27 at 09:16 +0200, Gerd Hoffmann wrote:
> Hi,
> 
> > This API allows qemu to limit the set of features that spice uses to
> > those compatible with an older version, in order to do an upgrade like
> > this. Right now it doesn't really do much, since we don't keep compat
> > with 0.4.0 atm (although that may be added later).
> 
> As you are talking about "set of features" already ...
> 
> I think we should use a feature bitmask instead of a version number in 
> the API.

How would you use this in qemu though? Say you link to spice 0.10.0,
which has a set of new features not in 0.8.0. Why would you want to make
a spice instance that doesn't use all features in 0.10.0, yet is not
compatible with 0.8.0, so you can't migrate between them. 

I don't see the point really.

> > +__visible__ spice_compat_version_t spice_get_current_compat_version(void)
> 
> uint64_t spice_get_supported_features();
> 
> > +__visible__ int spice_server_set_compat_version(SpiceServer *s,
> > +spice_compat_version_t 
> > version)
> 
> spice_server_enable_features(SpiceServer *s, uint64_t features);
> 
> > +/* Don't use features incompatible with a specific spice
> > +   version, so that migration to/from that version works. */
> > +typedef enum {
> > +SPICE_COMPAT_VERSION_0_4 = 0,
> > +SPICE_COMPAT_VERSION_0_6 = 1,
> > +} spice_compat_version_t;
> 
> /* 0.6 */
> #define SPICE_FEATURE_PROTOCOL_MAJOR2  (1<<0)
> #define SPICE_FEATURE_SURFACES (1<<1)
> 
> /* 0.8 */
> #define SPICE_FEATURE_MULTIMONITOR (1<<2)
> 
> /* for convinience */
> #define SPICE_FEATURES_MASK_V06 \
>  (SPICE_FEATURE_PROTOCOL_MAJOR2 | SPICE_FEATURE_SURFACES)
> #define SPICE_FEATURES_MASK_V08 \
>  (SPICE_FEATURES_MASK_V06 | SPICE_FEATURE_MULTIMONITOR)
> 
> Another question is whenever we want prepare for 0.4 compatibility.
> I think the only use case would be to allow 0.4 clients connect to the 
> 0.6 spice server.  Wasn't the plan to upgrade the clients instead?

Right now we don't do any compat with 0.4, but the plan is that
eventually we want to add a qemu mode where it doesn't use offscreen
surfaces, nor the new qxl pci device, and allows migration to/from spice
0.4.0 instances. 

It will still produce a major 2 protocol stream though, so clients must
be >= 0.6.0. I don't think this is a big problem, as clients can be
upgraded before a cluster in an upgrade situation anyway.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a suicidal shark-wrestling househusband fleeing from a secret government 
programme. She's a plucky tomboy lawyer on the trail of a serial killer. They 
fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 3/5] zap dead qxl chunk code

[Alexander Larsson]

On Fri, 2010-08-27 at 08:49 +0200, Gerd Hoffmann wrote:
> On 08/27/10 08:41, Alexander Larsson wrote:
> > On Fri, 2010-08-27 at 00:09 +0200, Gerd Hoffmann wrote:
> >> Signed-off-by: Gerd Hoffmann
> >> ---
> >>   server/red_worker.c |   29 -
> >>   1 files changed, 0 insertions(+), 29 deletions(-)
> >
> > Actually, you can get rid of BufDescriptor->type, slot_id, and group_id
> > too.
> 
> I suspect with only minor marshaller adjustments we can get rid of 
> BufDescriptor altogether ...

Possibly, but if you're removing unused stuff, might as well remove all
unused stuff.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's an impetuous ninja boxer with no name. She's a violent red-headed 
magician's assistant with only herself to blame. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 2/5] qxl parser: add cursor parsing

[Alexander Larsson]

On Fri, 2010-08-27 at 08:51 +0200, Gerd Hoffmann wrote:
> On 08/27/10 08:38, Alexander Larsson wrote:
> > On Fri, 2010-08-27 at 00:09 +0200, Gerd Hoffmann wrote:
> >>
> >> +red->data_size = qxl->data_size;
> >> +size = red_get_data_chunks_ptr(slots, group_id,
> >> +   get_memslot_id(slots, addr),
> >> +&chunks,&qxl->chunk);
> >> +data = red_linearize_chunk(&chunks, size,&free_data);
> >> +red_put_data_chunks(&chunks);
> >> +red->data = spice_malloc(size);
> >> +memcpy(red->data, data, size);
> >> +
> >> +if (free_data) {
> >> +free(data);
> >> +}
> >
> > Ack, but this part could be more efficient. In the n_chunks>  1 case the
> > red_linearize_chunk part will already malloc and copy the data so we
> > don't need to do it then.
> 
> Right.  Incremental patch attached.

Ack

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a hate-fuelled zombie barbarian looking for a cure to the poison coursing 
through his veins. She's a scantily clad cat-loving politician who hides her 
beauty behind a pair of thick-framed spectacles. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 3/5] zap dead qxl chunk code

[Alexander Larsson]

On Fri, 2010-08-27 at 00:09 +0200, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann 
> ---
>  server/red_worker.c |   29 -
>  1 files changed, 0 insertions(+), 29 deletions(-)

Actually, you can get rid of BufDescriptor->type, slot_id, and group_id
too.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a war-weary overambitious inventor on a search for his missing sister. 
She's an elegant winged politician with a flame-thrower. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 5/5] fix red_cursur_flush segfault

[Alexander Larsson]

On Fri, 2010-08-27 at 00:09 +0200, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann 
> ---
>  server/red_worker.c |4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)

Ack

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a scarfaced zombie dog-catcher with a robot buddy named Sparky. She's a 
hard-bitten hypochondriac mercenary with only herself to blame. They fight 
crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 4/5] zap dead typedefs

[Alexander Larsson]

On Fri, 2010-08-27 at 00:09 +0200, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann 
> ---
>  server/red_worker.c |5 -
>  1 files changed, 0 insertions(+), 5 deletions(-)
> 
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 17bbf6f..1975597 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -522,11 +522,6 @@ typedef struct FreeList {
>  
>  typedef struct DisplayChannel DisplayChannel;
>  
> -typedef void *(*enc_get_virt_fn_t)(void *get_virt_opaque, unsigned long 
> addr, uint32_t add_size,
> -   uint32_t group_id);
> -typedef void (*enc_validate_virt_fn_t)(void *validate_virt_opaque, unsigned 
> long virt,
> -   unsigned long from_addr, uint32_t 
> add_size,
> -   uint32_t group_id);
>  typedef struct  {
>  DisplayChannel *display_channel;
>  RedCompressBuf *bufs_head;

Ack
-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's an unconventional Jewish hairdresser haunted by memories of 'Nam. She's a 
wealthy antique-collecting cab driver living on borrowed time. They fight 
crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 3/5] zap dead qxl chunk code

[Alexander Larsson]

On Fri, 2010-08-27 at 00:09 +0200, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann 
> ---
>  server/red_worker.c |   29 -
>  1 files changed, 0 insertions(+), 29 deletions(-)

Ack

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a shy pirate firefighter on the run. She's a strong-willed 
African-American mechanic from a family of eight older brothers. They fight 
crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 2/5] qxl parser: add cursor parsing

[Alexander Larsson]

On Fri, 2010-08-27 at 00:09 +0200, Gerd Hoffmann wrote:
> 
> +red->data_size = qxl->data_size;
> +size = red_get_data_chunks_ptr(slots, group_id,
> +   get_memslot_id(slots, addr),
> +   &chunks, &qxl->chunk);
> +data = red_linearize_chunk(&chunks, size, &free_data);
> +red_put_data_chunks(&chunks);
> +red->data = spice_malloc(size);
> +memcpy(red->data, data, size);
> +
> +if (free_data) {
> +free(data);
> +} 

Ack, but this part could be more efficient. In the n_chunks > 1 case the
red_linearize_chunk part will already malloc and copy the data so we
don't need to do it then. We should have a linearlize_chunks variant
that always copies and use that.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a time-tossed flyboy boxer with a winning smile and a way with the 
ladies. She's a wealthy junkie advertising executive from aristocratic 
European stock. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 1/5] qxl parser: complete QXL_SURFACE_CMD_CREATE parsing

[Alexander Larsson]

On Fri, 2010-08-27 at 00:09 +0200, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann 
> ---
>  server/red_parse_qxl.c |5 -
>  server/red_parse_qxl.h |2 +-
>  server/red_worker.c|3 +--
>  3 files changed, 6 insertions(+), 4 deletions(-)

Ack

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a globe-trotting pirate gangster on the wrong side of the law. She's a 
blind blonde single mother from beyond the grave. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 2/3] server: really wait for a surface to be destroyed, when calling destroy_surface_wait

[Alexander Larsson]

On Thu, 2010-08-26 at 17:28 +0300, Yonit Halperin wrote:
> On 08/26/2010 05:25 PM, Alexander Larsson wrote:
> red_push doesn't send one item at a time, but rather pushes till it 
> blocks (due to socket block or missing acks). So if the item is still in 
> the pipe, it means that channel->send_data.blocked
> >> +red_receive(channel);
> >> +red_send_data(channel, NULL);
> >> +red_push(channel->worker);
> >> +}
> >
> > Also, I'm generally worried a bit about the red_receive calls. While
> > waiting for sending the stuff in the pipe, handling the
> > destroy_surface_wait we may handle an incoming message from the client
> > which sort of reenters the existing call stack. Isn't there a risk that
> > such reentrancy causes problems? like deadlocks or weird crashes?
> >
> >
> First, we must call red_receive, since if we are missing acks, we won't 
> get out of the blocking mode. Second, I don't think that any message 
> from the client can cause re-entering to this call stack.

Yeah, i don't know of any existing codepath where this is a problem. I
just generally fear reentrancy. Its bitten me in the ass way to many
times before. Otoh, its not like we don't do this already in other ways.

So, ack from me.


___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 1/3] server: consider also PIPE_ITEM_UPGRADE when searching for drawables in red_clear_surface_drawables_from_pipe

[Alexander Larsson]

On Thu, 2010-08-26 at 17:00 +0300, Yonit Halperin wrote:
> On 08/26/2010 04:58 PM, Alexander Larsson wrote:
> > On Thu, 2010-08-26 at 12:41 +0300, Yonit Halperin wrote:
> >> ---
> >>   server/red_worker.c |   36 +++-
> >>   1 files changed, 19 insertions(+), 17 deletions(-)
> >
> >
> > Don't you also need to check for ImageItems with the same surface_id?
> >
> >
> >
> We can, but actually it is not necessary, since the image is copied 
> while processing and not sending, so it has no reference to the surface.
> But it will be nicer to remove those as well.

Ack then

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v3] spice-{vmc,vdi}: implement subtype

[Alexander Larsson]

On Thu, 2010-08-26 at 10:28 -0400, Alon Levy wrote:
> v3 changes:
>  subtype is now a field of SpiceCharDeviceInstance
> 
> btw, there shouldn't be a newline added before vdi_port_interface,
> but I can't get git diff to admit there isn't one there. I'll make
> sure there isn't one in the committed version.
Ack


___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v3] server: add subtype to SpiceCharDeviceInterface, use for vdagent

[Alexander Larsson]

On Thu, 2010-08-26 at 10:12 -0400, Alon Levy wrote:
> v3 changes:
>  subtype now part of SpiceCharDeviceInstance, not a method of S.C.D.Interface

Ack


___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 3/3] server: cleanups in destorying surfaces code

[Alexander Larsson]

On Thu, 2010-08-26 at 12:41 +0300, Yonit Halperin wrote:
> ---
>  server/red_worker.c |   51 
> ++-
>  1 files changed, 22 insertions(+), 29 deletions(-)


Ack


___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 2/3] server: really wait for a surface to be destroyed, when calling destroy_surface_wait

[Alexander Larsson]

On Thu, 2010-08-26 at 12:41 +0300, Yonit Halperin wrote:
> Waiting till all the pipe items that are dependent on the surface will be 
> sent.
> This was probably the cause for freedesktop bug #29750.
> ---
>  server/red_worker.c |   84 +-
>  1 files changed, 75 insertions(+), 9 deletions(-)
> 
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 1a3f755..e688971 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c

> +red_printf("");

Leftover?

> +red_ref_channel(channel);
> +channel->hold_item(item);
> +
> +end_time = red_now() + CHANNEL_PUSH_TIMEOUT;
> +
> +if (channel->send_data.blocked) {
> +red_receive(channel);
> +red_send_data(channel, NULL);
> +}
> +// todo: different push for each channel
> +red_push(channel->worker);

Why do this outside the loop, and why the check on send_data.blocked. Is
it not better to start with red_push (which will do nothing if
send_data.blocked), then receive+send, always in the loop. Then you can
something loop like: push, receive, send, if blocked and item_in_pipe
usleep.

> +
> +while((item_in_pipe = ring_item_is_linked(&item->link)) && (red_now() < 
> end_time)) {
> +usleep(CHANNEL_PUSH_SLEEP_DURATION);

Why are you sleeping between each item sent? We should only need to
sleep when channel->send_data.blocked is true.

> +red_receive(channel);
> +red_send_data(channel, NULL);
> +red_push(channel->worker);
> +}

Also, I'm generally worried a bit about the red_receive calls. While
waiting for sending the stuff in the pipe, handling the
destroy_surface_wait we may handle an incoming message from the client
which sort of reenters the existing call stack. Isn't there a risk that
such reentrancy causes problems? like deadlocks or weird crashes?


___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 1/3] server: consider also PIPE_ITEM_UPGRADE when searching for drawables in red_clear_surface_drawables_from_pipe

[Alexander Larsson]

On Thu, 2010-08-26 at 12:41 +0300, Yonit Halperin wrote:
> ---
>  server/red_worker.c |   36 +++-
>  1 files changed, 19 insertions(+), 17 deletions(-)


Don't you also need to check for ImageItems with the same surface_id?



___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 2/2] spice-{vmc,vdi}: implement subtype

[Alexander Larsson]

On Thu, 2010-08-26 at 04:46 -0400, Alon Levy wrote:
> ---
>  hw/spice-vdi.c |6 ++
>  hw/spice-vmc.c |   47 +++
>  2 files changed, 53 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/spice-vdi.c b/hw/spice-vdi.c
> index 415932b..f5fda4d 100644
> --- a/hw/spice-vdi.c
> +++ b/hw/spice-vdi.c
> @@ -290,12 +290,18 @@ static int 
> vdi_port_interface_read(SpiceCharDeviceInstance *sin,
>  return actual_read;
>  }
>  
> +static const char* vdi_port_subtype(SpiceCharDeviceInstance* sin)
> +{
> +return "vdagent";
> +}
> +
>  static SpiceCharDeviceInterface vdi_port_interface = {
>  .base.type  = SPICE_INTERFACE_CHAR_DEVICE,
>  .base.description   = "vdi port",
>  .base.major_version = SPICE_INTERFACE_CHAR_DEVICE_MAJOR,
>  .base.minor_version = SPICE_INTERFACE_CHAR_DEVICE_MINOR,
>  
> +.subtype= vdi_port_subtype,

Should be just 
   .subtype= "vdagent",

>  .write  = vdi_port_interface_write,
>  .read   = vdi_port_interface_read,
>  };


___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 1/2] spice-{vdi, vmc}: rename SpiceVDIPort* to SpiceCharDevice*

[Alexander Larsson]

On Thu, 2010-08-26 at 04:46 -0400, Alon Levy wrote:
> ---
>  hw/spice-vdi.c |   16 
>  hw/spice-vmc.c |   22 +++---
>  2 files changed, 19 insertions(+), 19 deletions(-)

Ack



___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 1/2] server: rename SpiceVDIPort* to SpiceCharDevice*

[Alexander Larsson]

On Thu, 2010-08-26 at 04:44 -0400, Alon Levy wrote:
> ---
>  server/reds.c   |   48 +-
>  server/spice-experimental.h |   28 
>  2 files changed, 38 insertions(+), 38 deletions(-)

Ack

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 2/2] server: add subtype to SpiceCharDeviceInterface, use for vdagent

[Alexander Larsson]

On Thu, 2010-08-26 at 04:45 -0400, Alon Levy wrote:

> diff --git a/server/spice-experimental.h b/server/spice-experimental.h
> index e40b3ec..fd8ef67 100644
> --- a/server/spice-experimental.h
> +++ b/server/spice-experimental.h
> @@ -10,6 +10,7 @@ typedef struct SpiceCharDeviceState SpiceCharDeviceState;
>  struct SpiceCharDeviceInterface {
>  SpiceBaseInterface base;
>  
> +const char* (*subtype)(SpiceCharDeviceInstance *sin);
>  void (*state)(SpiceCharDeviceInstance *sin, int connected);
>  int (*write)(SpiceCharDeviceInstance *sin, const uint8_t *buf, int len);
>  int (*read)(SpiceCharDeviceInstance *sin, uint8_t *buf, int len);
> @@ -21,6 +22,7 @@ struct SpiceCharDeviceInstance {
>  };

I think you should move this from a method on the interface to a const
char * on the instance.

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 1/1] Add API to turn on backwards compatibility mode

[Alexander Larsson]

On Thu, 2010-08-26 at 09:26 -0400, Alon Levy wrote:
> - al...@redhat.com wrote:
> 
> > From: Alexander Larsson 
> > 
> > When upgrading a cluster of machines you typically do this by
> > upgrading a set of machines at a time, making the new machines run
> > the new software version, but in a fashion compatible with the old
> > versions (in terms of e.g. migration). Then when all machines are
> 
> missing word here: migrated?

Upgraded.

> > 
> 
> ACK

Cool. Gerd, are you ok with this too, for the qemu side?


___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [RFC 1/2] spice, server: introduce SpiceCharDevice

[Alexander Larsson]

On Wed, 2010-08-25 at 11:19 -0400, Alon Levy wrote:
> - "Alexander Larsson"  wrote:
> > > +const char* (*subtype)(SpiceCharDeviceInstance *sin);
> > 
> > I don't think we need a callback here. Just make this a const char *.
> > 
> 
> No, that doesn't work - the subtype is not a feature of the interface, it's a 
> feature
> of the specific instance:
> 
> typedef struct SpiceVirtualChannel {
> VirtIOSerialPort  port;
> VMChangeStateEntry*vmstate;
> SpiceCharDeviceInstance  sin;
> ...
> }
> 
> and then:
> 
> static const char* vmc_subtype(SpiceCharDeviceInstance* sin)
> {
> SpiceVirtualChannel *scd = container_of(sin, SpiceVirtualChannel, sin);
> 
> dprintf(scd, 2, "%s\n", __func__);
> return scd->subtype;
> }

I see. So, make the const char *subtype member be a member of
SpiceCharDeviceInstance then. Just like "int id" is in QXLInstance.

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [RFC 1/2] spice, server: introduce SpiceCharDevice

[Alexander Larsson]

On Wed, 2010-08-25 at 06:53 -0400, Alon Levy wrote:
> index aede4ce..6c02afb 100644
> --- a/server/spice-experimental.h
> +++ b/server/spice-experimental.h
> @@ -1,26 +1,31 @@
> -/* vdi port interface */
> +/* char device interfaces */
>  
> -#define SPICE_INTERFACE_VDI_PORT "vdi_port"
> -#define SPICE_INTERFACE_VDI_PORT_MAJOR 1
> -#define SPICE_INTERFACE_VDI_PORT_MINOR 1
> -typedef struct SpiceVDIPortInterface SpiceVDIPortInterface;
> -typedef struct SpiceVDIPortInstance SpiceVDIPortInstance;
> -typedef struct SpiceVDIPortState SpiceVDIPortState;
> +#define SPICE_INTERFACE_CHAR_DEVICE "char_device"
> +#define SPICE_INTERFACE_CHAR_DEVICE_MAJOR 1
> +#define SPICE_INTERFACE_CHAR_DEVICE_MINOR 1
> +typedef struct SpiceCharDeviceInterface SpiceCharDeviceInterface;
> +typedef struct SpiceCharDeviceInstance SpiceCharDeviceInstance;
> +typedef struct SpiceCharDeviceState SpiceCharDeviceState;
>  
> -struct SpiceVDIPortInterface {
> +struct SpiceCharDeviceInterface {
>  SpiceBaseInterface base;
>  
> -void (*state)(SpiceVDIPortInstance *sin, int connected);
> -int (*write)(SpiceVDIPortInstance *sin, const uint8_t *buf, int len);
> -int (*read)(SpiceVDIPortInstance *sin, uint8_t *buf, int len);
> +/* called by spice */
> +const char* (*subtype)(SpiceCharDeviceInstance *sin);

I don't think we need a callback here. Just make this a const char *.

> +void (*state)(SpiceCharDeviceInstance *sin, int connected);
> +int (*write)(SpiceCharDeviceInstance *sin, const uint8_t *buf, int len);
> +int (*read)(SpiceCharDeviceInstance *sin, uint8_t *buf, int len);
>  };
>  
> -struct SpiceVDIPortInstance {
> -SpiceBaseInstance base;
> -SpiceVDIPortState *st;
> +struct SpiceCharDeviceInstance {
> +SpiceBaseInstance   base;
> +SpiceCharDeviceState*st;
> +/* called by spice embedder */
> +void (*wakeup)(SpiceCharDeviceInstance *sin);
>  };
>
> -void spice_server_vdi_port_wakeup(SpiceVDIPortInstance *sin);

We've generally avoided having ways to call into spice like this.
Instead have a spice_server_char_device_wakeup() call which internally
could e.g. call via a function pointer in SpiceCharDeviceState.

> +int spice_server_char_device_recognized_subtype(const char* subtype);
> +const char* spice_server_char_device_supported_subtypes(void);

I think its better to have a single call that returns a strv, i.e. a
NULL terminated char *[]. This can be used to handle both of these
functions and is a nicer api to support.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a shy Republican assassin with a robot buddy named Sparky. She's an 
artistic thirtysomething Hell's Angel descended from a line of powerful 
witches. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] Fwd: [PATCH 2/3] server: fix race when data arrives from guest through vdi interface

[Alexander Larsson]

On Wed, 2010-08-25 at 06:28 -0400, Alon Levy wrote:
> - Forwarded Message -
> From: "Alon Levy" 
> To: al...@redhat.com
> Sent: Sunday, August 22, 2010 10:28:37 PM (GMT+0200) Auto-Detected
> Subject: [PATCH 2/3] server: fix race when data arrives from guest through 
> vdi interface
> 
> The call chains that could lead to write_to_vdi_port from two threads:
> 
> guest paste:
>  per cpu thread:
>  kvm_main_loop_cpu..vmc_have_data..spice_server_vdi_port_wakeup
>   ..write_to_vdi_port

Is this enough though?

It seems to me that read_from_vdi_port() is not threadsafe either. It
changes some state and does not seem to have any locking or anything.


___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] Fwd: [PATCH 1/3] server: bugfix - make vdi_port_write_retry reiterate if write_queue still not empty

[Alexander Larsson]

On Wed, 2010-08-25 at 06:27 -0400, Alon Levy wrote:
> - Forwarded Message -
> From: "Alon Levy" 
> To: al...@redhat.com
> Sent: Sunday, August 22, 2010 10:28:36 PM (GMT+0200) Auto-Detected
> Subject: [PATCH 1/3] server: bugfix - make vdi_port_write_retry reiterate if 
> write_queue still not empty

ack

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a world-famous devious vagrant with a secret. She's a cosmopolitan 
renegade detective fleeing from a Satanic cult. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] server: clean glz drawables when reseting qxl

[Alexander Larsson]

On Wed, 2010-08-25 at 10:06 +0300, Yonit Halperin wrote:
> When the we reset qxl, we destroy all srufaces. Since surfaces and glz
> drawables are no longer dependent, we need to call 
> red_display_clear_glz_drawables explicitly
> in order to clear all our drawables references in the server.
> ---

Ack

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a lonely Republican card sharp on a mission from God. She's a hard-bitten 
hip-hop journalist from a secret island of warrior women. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] Pushed removal of STREAM_TRACE and USE_EXCLUDE_RGN

[Alexander Larsson]

I pushed two trivial patches to remove the STREAM_TRACE and
USE_EXCLUDE_RGN defines. We didn't really maintain the else cases
anyway, and there is no need to use the old untested implementations.

red_worker.c is compex as is without #ifdef spaghetti.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a lounge-singing chivalrous jungle king on the edge. She's a blind 
cat-loving stripper fleeing from a Satanic cult. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] client: disable clipboard for now (defined out)

[Alexander Larsson]

On Mon, 2010-08-23 at 10:34 -0400, Alon Levy wrote:
> ---
>  client/application.cpp |2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/client/application.cpp b/client/application.cpp
> index 490cd8c..8eac8f6 100644
> --- a/client/application.cpp
> +++ b/client/application.cpp
> @@ -1325,7 +1325,9 @@ void Application::on_app_activated()
>  {
>  _active = true;
>  _key_handler->on_focus_in();
> +#ifdef CLIPBOARD_ENABLED
>  Platform::set_clipboard_listener(this);
> +#endif
>  }
>  
>  void Application::on_clipboard_change()

Ack

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a war-weary guitar-strumming stage actor looking for 'the Big One.' She's 
a transdimensional blonde opera singer from a secret island of warrior women. 
They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] Fwd: [PATCH 0/8] vdagent update to virtio and clipboard, take 2

[Alexander Larsson]

I looked this over a bit, but I don't really know the vdagent codebase.
The split up and the general layout of these patches look good to me,
and is something we need. So, go ahead and commit this, then we'll at
least have a working agent for 0.6.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a witless neurotic assassin looking for 'the Big One.' She's a foxy 
foul-mouthed nun from a different time and place. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] Limiting video streaming to the primary surface. freedesktop bug #28088.

[Alexander Larsson]

On Sun, 2010-08-22 at 09:17 +0300, Yonit Halperin wrote:

> @@ -3262,10 +3271,12 @@ static inline int red_current_add(RedWorker *worker, 
> Ring *ring, Drawable *drawa
>  red_streams_update_clip(worker, drawable);
>  } else {
>  #ifdef STREAM_TRACE
> -red_detach_streams_behind(worker, &drawable->tree_item.base.rgn);
> +if (drawable->surface_id == 0) {
> +red_detach_streams_behind(worker, &drawable->tree_item.base.rgn);
>  #else
> -red_stop_streams_behind(worker, &drawable->tree_item.base.rgn);
> +red_stop_streams_behind(worker, &drawable->tree_item.base.rgn);
>  #endif
> +}

The if is inside the ifdef, but the } is outside it, will not build
with !STREAM_TRACE.

Otherwise ack.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a fast talking chivalrous werewolf living undercover at Ringling Bros. 
Circus. She's an orphaned paranoid college professor with her own daytime 
radio talk show. They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 7/8] Store surfaces_used in a bit-array

[Alexander Larsson]

On Sun, 2010-08-22 at 15:56 +0300, Yonit Halperin wrote:
> On 08/20/2010 09:54 PM, al...@redhat.com wrote:
> > From: Alexander Larsson
> >
> > This is smaller than a byte array, and allows us to skip full
> > blocks of 32 ids in one check.
> > ---
> 
> Hi,
> why not use a linked list, for free surfaces, in a static 
> UINT32[n_surfaces] array? Any reason besides space, which is 4k?

In fact, the best way to do this would probably be to chain the free
surfaces together into a linked list using e.g. SurfaceInfo->pdev which
are unused for free surface_infos. This way we use no extra memory.

> shouldn't it be ~(0x8000 >> (surface_id % 32))?

> > +if (used_mask == 0x) {
> shouldn't it be used_mask != 0x?

Yeah, Good catches.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc 
   al...@redhat.comalexander.lars...@gmail.com 
He's a notorious sweet-toothed cat burglar moving from town to town, helping 
folk in trouble. She's a supernatural tempestuous doctor married to the Mob. 
They fight crime! 

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel