Re: [Spice-devel] [PATCH spice-gtk 1/4] Avoid clang warnings on casts with stricter alignment requirements
>
>
> > On 11 May 2017, at 10:17, Frediano Ziglio wrote:
> >
> >>
> >> For example, something like this:
> >> uint8_t *p8;
> >> uint32_t *p32 = (uint32_t *) p8;
> >>
> >> generates a warning like this:
> >> spice-channel.c:1350:10: error: cast from 'uint8_t *' (aka 'unsigned char
> >> *') to
> >> 'uint32_t *' (aka 'unsigned int *') increases required alignment from
> >> 1
> >> to
> >> 4 [-Werror,-Wcast-align]
> >>
> >> Adding an intermediate cast to (void *) silences the warning.
> >>
> >> Signed-off-by: Christophe de Dinechin
> >> ---
> >> spice-common| 2 +-
> >> src/channel-cursor.c| 6 +++---
> >> src/channel-display-mjpeg.c | 2 +-
> >> src/decode-glz-tmpl.c | 2 +-
> >> src/spice-channel.c | 14 +-
> >> 5 files changed, 15 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/spice-common b/spice-common
> >> index af682b1..1239c82 16
> >> --- a/spice-common
> >> +++ b/spice-common
> >> @@ -1 +1 @@
> >> -Subproject commit af682b1b06dea55007d9aa7c37cd443e4349e43f
> >> +Subproject commit 1239c82c54dee0244cca262cca0aa21071b23e24
> >> diff --git a/src/channel-cursor.c b/src/channel-cursor.c
> >> index 609243b..51c6c2e 100644
> >> --- a/src/channel-cursor.c
> >> +++ b/src/channel-cursor.c
> >> @@ -340,7 +340,7 @@ static display_cursor *set_cursor(SpiceChannel
> >> *channel,
> >> SpiceCursor *scursor)
> >>memcpy(cursor->data, data, size);
> >>for (i = 0; i < hdr->width * hdr->height; i++) {
> >>pix_mask = get_pix_mask(data, size, i);
> >> -if (pix_mask && *((guint32*)data + i) == 0xff) {
> >> +if (pix_mask && *(SPICE_ALIGNED_CAST(guint32*, data) + i) ==
> >> 0xff) {
> >>cursor->data[i] = get_pix_hack(i, hdr->width);
> >>} else {
> >>cursor->data[i] |= (pix_mask ? 0 : 0xff00);
> >> @@ -350,7 +350,7 @@ static display_cursor *set_cursor(SpiceChannel
> >> *channel,
> >> SpiceCursor *scursor)
> >>case SPICE_CURSOR_TYPE_COLOR16:
> >>for (i = 0; i < hdr->width * hdr->height; i++) {
> >>pix_mask = get_pix_mask(data, size, i);
> >> -pix = *((guint16*)data + i);
> >> +pix = *(SPICE_ALIGNED_CAST(guint16*,data) + i);
> >>if (pix_mask && pix == 0x7fff) {
> >>cursor->data[i] = get_pix_hack(i, hdr->width);
> >>} else {
> >> @@ -364,7 +364,7 @@ static display_cursor *set_cursor(SpiceChannel
> >> *channel,
> >> SpiceCursor *scursor)
> >>for (i = 0; i < hdr->width * hdr->height; i++) {
> >>pix_mask = get_pix_mask(data, size + (sizeof(uint32_t) << 4),
> >>i);
> >>int idx = (i & 1) ? (data[i >> 1] & 0x0f) : ((data[i >> 1] &
> >>0xf0) >> 4);
> >> -pix = *((uint32_t*)(data + size) + idx);
> >> +pix = *(SPICE_ALIGNED_CAST(uint32_t*,(data + size)) + idx);
> >>if (pix_mask && pix == 0xff) {
> >>cursor->data[i] = get_pix_hack(i, hdr->width);
> >>} else {
> >
> > This is not fine. The palette at the end of the image (this is a 4 bit
> > image)
> > is not garantee to be 4 byte aligned.
>
> OK. This would show up as a warning in the logs, but I guess I would need to
> test color cursor changes to see it, right?
>
actually you need a 4 bit cursor, not sure if windows use these anymore...
surely with an old windows version you'll get these. I supposed is harder
to find on X.
>
> >
> >> diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
> >> index 3ae9d21..17c0f4f 100644
> >> --- a/src/channel-display-mjpeg.c
> >> +++ b/src/channel-display-mjpeg.c
> >> @@ -151,7 +151,7 @@ static gboolean mjpeg_decoder_decode_frame(gpointer
> >> video_decoder)
> >> #ifndef JCS_EXTENSIONS
> >>{
> >>uint8_t *s = lines[0];
> >> -uint32_t *d = (uint32_t *)s;
> >> +uint32_t *d = SPICE_ALIGNED_CAST(uint32_t *,s);
> >>
> >>if (back_compat) {
> >>for (unsigned int j = lines_read * width; j > 0; ) {
> >
> > Not really sure, lines are returned by jpeg_read_scanlines, not sure
> > the output alignment.
>
> I prefer to keep “aligned” by default. If there is a warning showing up in
> the logs, we can switch it to unaligned.
>
Well.. no as actually are just macros silencing a possible problem would
be better if this could be unaligned so a grep would help the real
porting (if needed) in the future.
> >
> >> diff --git a/src/decode-glz-tmpl.c b/src/decode-glz-tmpl.c
> >> index b337a8b..7695a28 100644
> >> --- a/src/decode-glz-tmpl.c
> >> +++ b/src/decode-glz-tmpl.c
> >> @@ -178,7 +178,7 @@ static size_t FNAME(decode)(SpiceGlzDecoderWindow
> >> *window,
> >>uint64_t image_id, SpicePalette *plt)
> >> {
> >>uint8_t *ip = in_buf;
> >> -OUT_PIXEL*out_pix_buf = (OUT_PIXEL *)out_buf;
> >> +OUT_PIXEL*out_pix_buf = SPICE_ALIGNE
Re: [Spice-devel] [PATCH spice-gtk 1/4] Avoid clang warnings on casts with stricter alignment requirements
> On 11 May 2017, at 10:37, Frediano Ziglio wrote:
>
>>
>>
>>> On 11 May 2017, at 10:17, Frediano Ziglio wrote:
>>>
For example, something like this:
uint8_t *p8;
uint32_t *p32 = (uint32_t *) p8;
generates a warning like this:
spice-channel.c:1350:10: error: cast from 'uint8_t *' (aka 'unsigned char
*') to
'uint32_t *' (aka 'unsigned int *') increases required alignment from
1
to
4 [-Werror,-Wcast-align]
Adding an intermediate cast to (void *) silences the warning.
Signed-off-by: Christophe de Dinechin
---
spice-common| 2 +-
src/channel-cursor.c| 6 +++---
src/channel-display-mjpeg.c | 2 +-
src/decode-glz-tmpl.c | 2 +-
src/spice-channel.c | 14 +-
5 files changed, 15 insertions(+), 11 deletions(-)
diff --git a/spice-common b/spice-common
index af682b1..1239c82 16
--- a/spice-common
+++ b/spice-common
@@ -1 +1 @@
-Subproject commit af682b1b06dea55007d9aa7c37cd443e4349e43f
+Subproject commit 1239c82c54dee0244cca262cca0aa21071b23e24
diff --git a/src/channel-cursor.c b/src/channel-cursor.c
index 609243b..51c6c2e 100644
--- a/src/channel-cursor.c
+++ b/src/channel-cursor.c
@@ -340,7 +340,7 @@ static display_cursor *set_cursor(SpiceChannel
*channel,
SpiceCursor *scursor)
memcpy(cursor->data, data, size);
for (i = 0; i < hdr->width * hdr->height; i++) {
pix_mask = get_pix_mask(data, size, i);
-if (pix_mask && *((guint32*)data + i) == 0xff) {
+if (pix_mask && *(SPICE_ALIGNED_CAST(guint32*, data) + i) ==
0xff) {
cursor->data[i] = get_pix_hack(i, hdr->width);
} else {
cursor->data[i] |= (pix_mask ? 0 : 0xff00);
@@ -350,7 +350,7 @@ static display_cursor *set_cursor(SpiceChannel
*channel,
SpiceCursor *scursor)
case SPICE_CURSOR_TYPE_COLOR16:
for (i = 0; i < hdr->width * hdr->height; i++) {
pix_mask = get_pix_mask(data, size, i);
-pix = *((guint16*)data + i);
+pix = *(SPICE_ALIGNED_CAST(guint16*,data) + i);
if (pix_mask && pix == 0x7fff) {
cursor->data[i] = get_pix_hack(i, hdr->width);
} else {
@@ -364,7 +364,7 @@ static display_cursor *set_cursor(SpiceChannel
*channel,
SpiceCursor *scursor)
for (i = 0; i < hdr->width * hdr->height; i++) {
pix_mask = get_pix_mask(data, size + (sizeof(uint32_t) << 4),
i);
int idx = (i & 1) ? (data[i >> 1] & 0x0f) : ((data[i >> 1] &
0xf0) >> 4);
-pix = *((uint32_t*)(data + size) + idx);
+pix = *(SPICE_ALIGNED_CAST(uint32_t*,(data + size)) + idx);
if (pix_mask && pix == 0xff) {
cursor->data[i] = get_pix_hack(i, hdr->width);
} else {
>>>
>>> This is not fine. The palette at the end of the image (this is a 4 bit
>>> image)
>>> is not garantee to be 4 byte aligned.
>>
>> OK. This would show up as a warning in the logs, but I guess I would need to
>> test color cursor changes to see it, right?
>>
>
> actually you need a 4 bit cursor, not sure if windows use these anymore...
> surely with an old windows version you'll get these. I supposed is harder
> to find on X.
>
>>
>>>
diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
index 3ae9d21..17c0f4f 100644
--- a/src/channel-display-mjpeg.c
+++ b/src/channel-display-mjpeg.c
@@ -151,7 +151,7 @@ static gboolean mjpeg_decoder_decode_frame(gpointer
video_decoder)
#ifndef JCS_EXTENSIONS
{
uint8_t *s = lines[0];
-uint32_t *d = (uint32_t *)s;
+uint32_t *d = SPICE_ALIGNED_CAST(uint32_t *,s);
if (back_compat) {
for (unsigned int j = lines_read * width; j > 0; ) {
>>>
>>> Not really sure, lines are returned by jpeg_read_scanlines, not sure
>>> the output alignment.
>>
>> I prefer to keep “aligned” by default. If there is a warning showing up in
>> the logs, we can switch it to unaligned.
>>
>
> Well.. no as actually are just macros silencing a possible problem would
> be better if this could be unaligned so a grep would help the real
> porting (if needed) in the future.
I updated the commit log, because I think there is a misunderstanding.
SPICE_ALIGNED_CAST means “I think this should be aligned”. If it is not
aligned, then a warning is emitted at runtime. So it is less “silencing” the
problem than SPICE_UNALIGNED_CAST, which states that you know this is
misaligned anyway.
As for being able to grep to identify code that needs rework, you are right
that it may be useful to revisit un
Re: [Spice-devel] [PATCH spice-gtk 1/4] Avoid clang warnings on casts with stricter alignment requirements
...
> > > > > g_array_set_size(c->remote_common_caps, num_common_caps);
> > > >
> > >
> >
>
> > > > > for (i = 0; i < num_common_caps; i++, caps++) {
> > > >
> > >
> >
>
> > I cannot find these macros defined in spice-gtk. Where are they?
>
> spice-common. I think I announced it in the envelope mail for the patch.
> That’s why there is a submodule update. I don’t know if this is the right
> way to do things for submodule required changes.
You should send the changeset with spice-common with the other changesets (so
the mail to the ML). Also the spice-gtk changes should include a change to
spice-common submodule.
Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk 5/5] display-gst: Use Playbin for GStreamer 1.9.0 onwards
Hi,
On Wed, May 10, 2017 at 03:41:40AM +, Daimon Wang wrote:
> Ah, glad to know the difference. I'm using 1.2.4-1, where vaapi is not
> the default.For versions before 1.9.0, maybe we can try with
> "autoplug-select" signal,
> from
> http://gstreamer-devel.966125.n4.nabble.com/Change-the-ranking-of-an-element-td4674437.html
That might work. If you want something simpler, just changing the
pipeline elements could work too:
(avdec_h264 -> vaapih264dec) or (decodebin -> vaapidecodebin)
I'm not planning to work on versions older then 1.9.0 but if you want
something like that upstream, patches are welcome ;)
>
> On Tuesday, May 9, 2017 7:08 PM, Victor Toso
> wrote:
>
>
> On Mon, May 08, 2017 at 03:41:59AM +, Daimon Wang wrote:
> > Hi,
> > Is there anything prevent playbin from working with previous versions
> > of Gstreamer? I'm trying to use palybin similarly on another program,
> > for the vaapi support.
>
> For versions 1.9.0 onwards and if you have supported hw with drivers and
> gstreamer1-vaapi (elements) installed, then playbin should be using
> either vaapidecodebin or vaapipostproc first, to verify if it can do hw
> decoding otherwise it fallback to sw decoding (with avdec_h264 for
> instance).
>
> For version before 1.9.0, as far as I understood, it'll not use the
> vaapielements first, so it should do sw decoding by default.
>
> I did not test with 1.9.0 or earlier versions to say it for sure.
>
> > Regards,Daimon
> >
> > On Thursday, May 4, 2017 9:44 PM, Victor Toso
> >wrote:
> >
> >
> > From: Victor Toso
> >
> > The Playbin can provide the full pipeline which reduces the
> > overall maintenance in the code as we don't need to track which
> > decoder can work with our stream type.
> >
> > We need to maintain the GstCaps per SPICE_VIDEO_CODEC_TYPE in order to
> > tell Playbin the type of data we expect. This much should be covered
> > by spice-protocol and very likely we will need to extend it in the
> > future to allow more settings that might not possible to verify at
> > runtime.
> >
> > This patch keeps previous code for compatibility reasons.
> >
> > Note that we have to wait Playbin to emit "source-setup" in order to
> > configure GstAppSrc with the capabilities of input stream. If in the
> > unlikely event of frames arriving while GstAppSrc is not setup, we
> > will drop those frames.
> >
> > Signed-off-by: Victor Toso
> > Signed-off-by: Victor Toso
> > ---
> > src/channel-display-gst.c | 88
> >+--
> > 1 file changed, 86 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > index 2a20763..2b14745 100644
> > --- a/src/channel-display-gst.c
> > +++ b/src/channel-display-gst.c
> > @@ -50,6 +50,9 @@ typedef struct SpiceGstDecoder {
> > guint timer_id;
> > } SpiceGstDecoder;
> >
> > +/* FIXME: With gstreamer version 1.9.0 and higher, we are using playbin to
> > + * create the pipeline for us and for that reason we don't need to keep
> > track of
> > + * decoder's name anymore. */
> > static struct {
> > const gchar *dec_name;
> > const gchar *dec_caps;
> > @@ -256,10 +259,13 @@ static GstFlowReturn new_sample(GstAppSink
> > *gstappsink, gpointer video_decoder)
> > }
> > l = l->next;
> > }
> > +#if !GST_CHECK_VERSION(1,9,0)
> > + /* We down own this sample when using playbin, no need to unref it
> > */
> > if (!l) {
> > spice_warning("got an unexpected decoded buffer!");
> > gst_sample_unref(sample);
> > }
> > +#endif
> >
> > g_mutex_unlock(&decoder->queues_mutex);
> > schedule_frame(decoder);
> > @@ -276,8 +282,11 @@ static void free_pipeline(SpiceGstDecoder *decoder)
> > }
> >
> > gst_element_set_state(decoder->pipeline, GST_STATE_NULL);
> > +#if !GST_CHECK_VERSION(1,9,0)
> > + /* Owned by playbin on 1.9.0 onwards */
> > gst_object_unref(decoder->appsrc);
> > gst_object_unref(decoder->appsink);
> > +#endif
> > gst_object_unref(decoder->pipeline);
> > gst_object_unref(decoder->clock);
> > decoder->pipeline = NULL;
> > @@ -305,13 +314,79 @@ static gboolean handle_pipeline_message(GstBus *bus,
> > GstMessage *msg, gpointer v
> > return TRUE;
> > }
> >
> > +#if GST_CHECK_VERSION(1,9,0)
> > +static void app_source_setup(GstElement *pipeline, GstElement *source,
> > SpiceGstDecoder *decoder)
> > +{
> > + GstCaps *caps;
> > +
> > + /* - We schedule the frame display ourselves so set sync=false on
> > appsink
> > + * so the pipeline decodes them as fast as possible. This will also
> > + * minimize the risk of frames getting lost when we rebuild the
> > + * pipeline.
> > + * - Set max-bytes=0 on appsrc so it does not drop frames that may be
> > + * needed by those that follow.
> > + */
> > + caps =
> > gst_caps_from_string(gst_opts[decoder->base.codec_type].dec_caps);
> >
Re: [Spice-devel] [spice-server v2 5/6] Make RedCharDeviceWriteBuffer::refs private
>
> On 28 Apr 2017, at 11:11, Christophe Fergeau wrote:
>
> Signed-off-by: Christophe Fergeau
> ---
> server/char-device.c | 11 ++-
> server/char-device.h | 1 -
> 2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/server/char-device.c b/server/char-device.c
> index 7e77b465b..481ea2a13 100644
> --- a/server/char-device.c
> +++ b/server/char-device.c
> @@ -36,6 +36,7 @@ struct RedCharDeviceWriteBufferPrivate {
> RedClient *client; /* The client that sent the message to the device.
> NULL if the server created the message */
> uint32_t token_price;
> +uint32_t refs;
> };
>
> typedef struct RedCharDeviceClient RedCharDeviceClient;
> @@ -165,7 +166,7 @@ static void write_buffers_queue_free(GQueue *write_queue)
> static void red_char_device_write_buffer_pool_add(RedCharDevice *dev,
> RedCharDeviceWriteBuffer
> *buf)
> {
> -if (buf->refs == 1 &&
> +if (buf->priv->refs == 1 &&
> dev->priv->cur_pool_size < MAX_POOL_SIZE) {
> buf->buf_used = 0;
> buf->priv->origin = WRITE_BUFFER_ORIGIN_NONE;
> @@ -584,7 +585,7 @@ static RedCharDeviceWriteBuffer
> *__red_char_device_write_buffer_get(
> }
>
> ret->priv->token_price = migrated_data_tokens ? migrated_data_tokens : 1;
> -ret->refs = 1;
> +ret->priv->refs = 1;
> return ret;
> error:
> dev->priv->cur_pool_size += ret->buf_size;
> @@ -612,7 +613,7 @@ static RedCharDeviceWriteBuffer
> *red_char_device_write_buffer_ref(RedCharDeviceW
> {
> spice_assert(write_buf);
>
> -write_buf->refs++;
> +write_buf->priv->refs++;
Increasing / decreasing refs is done in a non-atomic way. I assume this means
that we know we cannot enter red_char_device_write_buffer_ref from different
threads simultaneously for the same buffer. Just for my education, could
someone explain why? My own mental model is that marshalling is a very
sequential operation anyway, so it’s hard to think of a valid scenario where
we’d parallelize it. But that makes me wonder if there is some kind of
convention allowing us to easily identify that structures that are possibly
shared between threads.
> return write_buf;
> }
>
> @@ -620,8 +621,8 @@ static void
> red_char_device_write_buffer_unref(RedCharDeviceWriteBuffer *write_b
> {
> spice_assert(write_buf);
>
> -write_buf->refs--;
> -if (write_buf->refs == 0)
> +write_buf->priv->refs--;
> +if (write_buf->priv->refs == 0)
> red_char_device_write_buffer_free(write_buf);
> }
>
> diff --git a/server/char-device.h b/server/char-device.h
> index 13e625fe1..f66e2a158 100644
> --- a/server/char-device.h
> +++ b/server/char-device.h
> @@ -151,7 +151,6 @@ typedef struct RedCharDeviceWriteBuffer {
> uint8_t *buf;
> uint32_t buf_size;
> uint32_t buf_used;
> -uint32_t refs;
>
> RedCharDeviceWriteBufferPrivate *priv;
> } RedCharDeviceWriteBuffer;
> --
> 2.12.2
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk 1/4] Avoid clang warnings on casts with stricter alignment requirements
> On 11 May 2017, at 11:17, Frediano Ziglio wrote:
>
> ...
>
>
>
> g_array_set_size(c->remote_common_caps, num_common_caps);
> for (i = 0; i < num_common_caps; i++, caps++) {
>
>
> I cannot find these macros defined in spice-gtk. Where are they?
>
> spice-common. I think I announced it in the envelope mail for the patch.
> That’s why there is a submodule update. I don’t know if this is the right way
> to do things for submodule required changes.
> You should send the changeset with spice-common with the other changesets (so
> the mail to the ML).
You mean I should add a link for example
https://lists.freedesktop.org/archives/spice-devel/2017-May/037505.html
to
the description? Or make a single patch set with changes both in spice-common
and spice-gtk. If the latter, I don’t know how…
> Also the spice-gtk changes should include a change to spice-common submodule.
I see it in
https://lists.freedesktop.org/archives/spice-devel/2017-May/037509.html:
diff --git a/spice-common b/spice-common
index af682b1..1239c82 16
--- a/spice-common
+++ b/spice-common
@@ -1 +1 @@
-Subproject commit af682b1b06dea55007d9aa7c37cd443e4349e43f
+Subproject commit 1239c82c54dee0244cca262cca0aa21071b23e24
Or do you mean something else?
(BTW, I sent an updated series with your comments, still not showing up, so I
guess my mail setup is still not good enough)
Thanks
Christophe
>
> Frediano
>
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-gtk v2 0/4] Another set of macOS pre-enablement patches
From: Christophe de Dinechin This patch set eliminates warnings detected by clang with respect to type alignments. Vittorio Toso had submitted something similar. In this version, I took into account comments by Christophe Fergeau regarding how to know which casts were aligned and which ones were not aligned. I added some instrumentation to detect the two cases. The casts that are supposed to be aligned will issue a warning if data is actually misaligned. The casts that are known to be misaligned can still be checked by setting environment variable SPICE_DEBUG_ALIGNMENT=1 This requires an update to spice-common, see https://lists.freedesktop.org/archives/spice-devel/2017-May/037505.html. The reference to submodule in this patch serie is updated with a reference that can be fetched from https://github.com/c3d/spice-common.git. Version 2 takes into account comments by Frediano Ziglio and Pavel Grunt, specifically: - Clarify purpose and behavior of macros in patch commit message - Change one case to 'unaligned' - Improve way to avoid 'unused variable' warning Christophe de Dinechin (4): Avoid clang warnings on casts with stricter alignment requirements Avoid warning about snprintf on non-Linux platforms Remove warning about unused variable when building on macOS Add check for macOS, disable ucontext on macOS (deprecated) configure.ac| 14 ++ spice-common| 2 +- src/channel-cursor.c| 6 +++--- src/channel-display-mjpeg.c | 2 +- src/decode-glz-tmpl.c | 2 +- src/spice-channel.c | 14 +- src/spice-widget.c | 4 ++-- src/usbutil.c | 2 +- 8 files changed, 32 insertions(+), 14 deletions(-) -- 2.11.0 (Apple Git-81) ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-gtk v2 4/4] Add check for macOS, disable ucontext on macOS (deprecated)
From: Christophe de Dinechin Signed-off-by: Christophe de Dinechin --- configure.ac | 14 ++ 1 file changed, 14 insertions(+) diff --git a/configure.ac b/configure.ac index 74b5811..ecab365 100644 --- a/configure.ac +++ b/configure.ac @@ -62,6 +62,18 @@ esac AC_MSG_RESULT([$os_win32]) AM_CONDITIONAL([OS_WIN32],[test "$os_win32" = "yes"]) +AC_MSG_CHECKING([for native macOS]) +case "$host_os" in + *darwin*) +os_mac=yes +;; + *) +os_mac=no +;; +esac +AC_MSG_RESULT([$os_mac]) +AM_CONDITIONAL([OS_MAC],[test "$os_mac" = "yes"]) + AC_CHECK_HEADERS([sys/socket.h netinet/in.h arpa/inet.h]) AC_CHECK_HEADERS([termios.h]) AC_CHECK_HEADERS([epoxy/egl.h], @@ -468,6 +480,8 @@ esac if test "$with_coroutine" = "auto"; then if test "$os_win32" = "yes"; then with_coroutine=winfiber + elif test "$os_mac" = "yes"; then +with_coroutine=gthread else with_coroutine=ucontext fi -- 2.11.0 (Apple Git-81) ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-gtk v2 3/4] Remove warning about unused variable when building on macOS
From: Christophe de Dinechin
On macOS, neither of the two cases implemented in set_mouse_accel applies.
We get the following eror message:
CC spice-widget.lo
spice-widget.c:944:26: error: unused variable 'd' [-Werror,-Wunused-variable]
SpiceDisplayPrivate *d = display->priv;
Signed-off-by: Christophe de Dinechin
---
src/spice-widget.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/spice-widget.c b/src/spice-widget.c
index 8203d55..5542e85 100644
--- a/src/spice-widget.c
+++ b/src/spice-widget.c
@@ -941,9 +941,8 @@ static void update_keyboard_grab(SpiceDisplay *display)
static void set_mouse_accel(SpiceDisplay *display, gboolean enabled)
{
-SpiceDisplayPrivate *d = display->priv;
-
#if defined GDK_WINDOWING_X11
+SpiceDisplayPrivate *d = display->priv;
GdkWindow *w = GDK_WINDOW(gtk_widget_get_window(GTK_WIDGET(display)));
if (!GDK_IS_X11_DISPLAY(gdk_window_get_display(w))) {
@@ -965,6 +964,7 @@ static void set_mouse_accel(SpiceDisplay *display, gboolean
enabled)
d->x11_accel_numerator, d->x11_accel_denominator,
d->x11_threshold);
}
#elif defined GDK_WINDOWING_WIN32
+SpiceDisplayPrivate *d = display->priv;
if (enabled) {
g_return_if_fail(SystemParametersInfo(SPI_SETMOUSE, 0, &d->win_mouse,
0));
g_return_if_fail(SystemParametersInfo(SPI_SETMOUSESPEED, 0,
(PVOID)(INT_PTR)d->win_mouse_speed, 0));
--
2.11.0 (Apple Git-81)
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-gtk v2 2/4] Avoid warning about snprintf on non-Linux platforms
From: Christophe de Dinechin Without #include , calls to snprintf in the file cause a warning. The file is left aside on purpose, since src/usbutil.c may be compiled on Windows where this file does not exist. Signed-off-by: Christophe de Dinechin --- src/usbutil.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/usbutil.c b/src/usbutil.c index b68a2e1..e96ab11 100644 --- a/src/usbutil.c +++ b/src/usbutil.c @@ -27,8 +27,8 @@ #include #ifdef USE_USBREDIR -#ifdef __linux__ #include +#ifdef __linux__ #include #include #ifndef major /* major and minor macros were moved to sys/sysmacros.h from sys/types.h */ -- 2.11.0 (Apple Git-81) ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-gtk v2 1/4] Avoid clang warnings on casts with stricter alignment requirements
From: Christophe de Dinechin
For example, something like this:
uint8_t *p8;
uint32_t *p32 = (uint32_t *) p8;
generates a warning like this:
spice-channel.c:1350:10: error: cast from 'uint8_t *' (aka 'unsigned char *')
to
'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to
4 [-Werror,-Wcast-align]
The warning indicates that we end up with a pointer to data that
should be 4-byte aligned, but its value may be misaligned. On x86,
this does not make much of a difference, except a relatively minor
performance penalty. However, on platforms such as ARM, misaligned
accesses are emulated by the kernel, and support for them is optional.
So we may end up with a fault.
The intent of the fix here is to make it easy to identify and rework
places where actual mis-alignment occurs. Wherever casts raise the warning,
they are replaced with a macro:
- SPICE_ALIGNED_CAST(type, value) is the same as (type) value, but
silences the warning (using an intermediate cast to (void *)) and
adds a runtime check to verify that the pointer value is actually
aligned as expected. If that expectation is violated, a warning
is issued.
- SPICE_UNALIGNED_CAST(type, value) works like SPICE_ALIGNED_CAST,
but asssumes that the value is not known to be aligned. If there
is an alignment error, no warning is issued. It is still possible
to know if misalignment occurs by defining environment variable
SPICE_DEBUG_ALIGNMENT, in which case a debug message is issued for
every misaligned pointer.
Signed-off-by: Christophe de Dinechin
---
spice-common| 2 +-
src/channel-cursor.c| 6 +++---
src/channel-display-mjpeg.c | 2 +-
src/decode-glz-tmpl.c | 2 +-
src/spice-channel.c | 14 +-
5 files changed, 15 insertions(+), 11 deletions(-)
diff --git a/spice-common b/spice-common
index af682b1..1239c82 16
--- a/spice-common
+++ b/spice-common
@@ -1 +1 @@
-Subproject commit af682b1b06dea55007d9aa7c37cd443e4349e43f
+Subproject commit 1239c82c54dee0244cca262cca0aa21071b23e24
diff --git a/src/channel-cursor.c b/src/channel-cursor.c
index 609243b..50de5ce 100644
--- a/src/channel-cursor.c
+++ b/src/channel-cursor.c
@@ -340,7 +340,7 @@ static display_cursor *set_cursor(SpiceChannel *channel,
SpiceCursor *scursor)
memcpy(cursor->data, data, size);
for (i = 0; i < hdr->width * hdr->height; i++) {
pix_mask = get_pix_mask(data, size, i);
-if (pix_mask && *((guint32*)data + i) == 0xff) {
+if (pix_mask && *(SPICE_ALIGNED_CAST(guint32*, data) + i) ==
0xff) {
cursor->data[i] = get_pix_hack(i, hdr->width);
} else {
cursor->data[i] |= (pix_mask ? 0 : 0xff00);
@@ -350,7 +350,7 @@ static display_cursor *set_cursor(SpiceChannel *channel,
SpiceCursor *scursor)
case SPICE_CURSOR_TYPE_COLOR16:
for (i = 0; i < hdr->width * hdr->height; i++) {
pix_mask = get_pix_mask(data, size, i);
-pix = *((guint16*)data + i);
+pix = *(SPICE_ALIGNED_CAST(guint16*,data) + i);
if (pix_mask && pix == 0x7fff) {
cursor->data[i] = get_pix_hack(i, hdr->width);
} else {
@@ -364,7 +364,7 @@ static display_cursor *set_cursor(SpiceChannel *channel,
SpiceCursor *scursor)
for (i = 0; i < hdr->width * hdr->height; i++) {
pix_mask = get_pix_mask(data, size + (sizeof(uint32_t) << 4), i);
int idx = (i & 1) ? (data[i >> 1] & 0x0f) : ((data[i >> 1] & 0xf0)
>> 4);
-pix = *((uint32_t*)(data + size) + idx);
+pix = *(SPICE_UNALIGNED_CAST(uint32_t*,(data + size)) + idx);
if (pix_mask && pix == 0xff) {
cursor->data[i] = get_pix_hack(i, hdr->width);
} else {
diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
index 3ae9d21..17c0f4f 100644
--- a/src/channel-display-mjpeg.c
+++ b/src/channel-display-mjpeg.c
@@ -151,7 +151,7 @@ static gboolean mjpeg_decoder_decode_frame(gpointer
video_decoder)
#ifndef JCS_EXTENSIONS
{
uint8_t *s = lines[0];
-uint32_t *d = (uint32_t *)s;
+uint32_t *d = SPICE_ALIGNED_CAST(uint32_t *,s);
if (back_compat) {
for (unsigned int j = lines_read * width; j > 0; ) {
diff --git a/src/decode-glz-tmpl.c b/src/decode-glz-tmpl.c
index b337a8b..7695a28 100644
--- a/src/decode-glz-tmpl.c
+++ b/src/decode-glz-tmpl.c
@@ -178,7 +178,7 @@ static size_t FNAME(decode)(SpiceGlzDecoderWindow *window,
uint64_t image_id, SpicePalette *plt)
{
uint8_t *ip = in_buf;
-OUT_PIXEL*out_pix_buf = (OUT_PIXEL *)out_buf;
+OUT_PIXEL*out_pix_buf = SPICE_ALIGNED_CAST(OUT_PIXEL *,out_buf);
OUT_PIXEL*op = out_pix_buf;
OUT_PIXEL*op_limit = out_pix_buf + size;
diff --git a/src/spice-channel.c b/src/spice-channel.c
index 3b6231
Re: [Spice-devel] [PATCH spice-gtk v2 4/4] Add check for macOS, disable ucontext on macOS (deprecated)
On Thu, May 11, 2017 at 12:47:08PM +0200, Christophe de Dinechin wrote: > From: Christophe de Dinechin > > Signed-off-by: Christophe de Dinechin > --- > configure.ac | 14 ++ > 1 file changed, 14 insertions(+) > > diff --git a/configure.ac b/configure.ac > index 74b5811..ecab365 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -62,6 +62,18 @@ esac > AC_MSG_RESULT([$os_win32]) > AM_CONDITIONAL([OS_WIN32],[test "$os_win32" = "yes"]) > > +AC_MSG_CHECKING([for native macOS]) > +case "$host_os" in > + *darwin*) > +os_mac=yes > +;; > + *) > +os_mac=no > +;; > +esac > +AC_MSG_RESULT([$os_mac]) > +AM_CONDITIONAL([OS_MAC],[test "$os_mac" = "yes"]) > + > AC_CHECK_HEADERS([sys/socket.h netinet/in.h arpa/inet.h]) > AC_CHECK_HEADERS([termios.h]) > AC_CHECK_HEADERS([epoxy/egl.h], > @@ -468,6 +480,8 @@ esac > if test "$with_coroutine" = "auto"; then >if test "$os_win32" = "yes"; then > with_coroutine=winfiber > + elif test "$os_mac" = "yes"; then > +with_coroutine=gthread >else > with_coroutine=ucontext >fi Despite ucontext being deprecated we are still better off using that & ignoring the warnings, than using the gthread impl. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk v2 4/4] Add check for macOS, disable ucontext on macOS (deprecated)
> > On Thu, May 11, 2017 at 12:47:08PM +0200, Christophe de Dinechin wrote: > > From: Christophe de Dinechin > > > > Signed-off-by: Christophe de Dinechin > > --- > > configure.ac | 14 ++ > > 1 file changed, 14 insertions(+) > > > > diff --git a/configure.ac b/configure.ac > > index 74b5811..ecab365 100644 > > --- a/configure.ac > > +++ b/configure.ac > > @@ -62,6 +62,18 @@ esac > > AC_MSG_RESULT([$os_win32]) > > AM_CONDITIONAL([OS_WIN32],[test "$os_win32" = "yes"]) > > > > +AC_MSG_CHECKING([for native macOS]) > > +case "$host_os" in > > + *darwin*) > > +os_mac=yes > > +;; > > + *) > > +os_mac=no > > +;; > > +esac > > +AC_MSG_RESULT([$os_mac]) > > +AM_CONDITIONAL([OS_MAC],[test "$os_mac" = "yes"]) > > + > > AC_CHECK_HEADERS([sys/socket.h netinet/in.h arpa/inet.h]) > > AC_CHECK_HEADERS([termios.h]) > > AC_CHECK_HEADERS([epoxy/egl.h], > > @@ -468,6 +480,8 @@ esac > > if test "$with_coroutine" = "auto"; then > >if test "$os_win32" = "yes"; then > > with_coroutine=winfiber > > + elif test "$os_mac" = "yes"; then > > +with_coroutine=gthread > >else > > with_coroutine=ucontext > >fi > > Despite ucontext being deprecated we are still better off using that & > ignoring the warnings, than using the gthread impl. > > Regards, > Daniel But Christophe reported that this is not compiling at all. Did you manage to compile under OsX with ucontext ? Why ucontext is better? According to http://duriansoftware.com/joe/PSA:-avoiding-the-%22ucontext-routines-are-deprecated%22-error-on-Mac-OS-X-Snow-Leopard.html seems that defining _XOPEN_SOURCE should remove at least the error. Frediano ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk v2 4/4] Add check for macOS, disable ucontext on macOS (deprecated)
On Thu, May 11, 2017 at 07:08:13AM -0400, Frediano Ziglio wrote: > > > > On Thu, May 11, 2017 at 12:47:08PM +0200, Christophe de Dinechin wrote: > > > From: Christophe de Dinechin > > > > > > Signed-off-by: Christophe de Dinechin > > > --- > > > configure.ac | 14 ++ > > > 1 file changed, 14 insertions(+) > > > > > > diff --git a/configure.ac b/configure.ac > > > index 74b5811..ecab365 100644 > > > --- a/configure.ac > > > +++ b/configure.ac > > > @@ -62,6 +62,18 @@ esac > > > AC_MSG_RESULT([$os_win32]) > > > AM_CONDITIONAL([OS_WIN32],[test "$os_win32" = "yes"]) > > > > > > +AC_MSG_CHECKING([for native macOS]) > > > +case "$host_os" in > > > + *darwin*) > > > +os_mac=yes > > > +;; > > > + *) > > > +os_mac=no > > > +;; > > > +esac > > > +AC_MSG_RESULT([$os_mac]) > > > +AM_CONDITIONAL([OS_MAC],[test "$os_mac" = "yes"]) > > > + > > > AC_CHECK_HEADERS([sys/socket.h netinet/in.h arpa/inet.h]) > > > AC_CHECK_HEADERS([termios.h]) > > > AC_CHECK_HEADERS([epoxy/egl.h], > > > @@ -468,6 +480,8 @@ esac > > > if test "$with_coroutine" = "auto"; then > > >if test "$os_win32" = "yes"; then > > > with_coroutine=winfiber > > > + elif test "$os_mac" = "yes"; then > > > +with_coroutine=gthread > > >else > > > with_coroutine=ucontext > > >fi > > > > Despite ucontext being deprecated we are still better off using that & > > ignoring the warnings, than using the gthread impl. > > > > Regards, > > Daniel > > But Christophe reported that this is not compiling at all. > Did you manage to compile under OsX with ucontext ? Is that with or without -Werror though ? I understand if -Werror makes it fail due to the deprecation warnings, but AFAIK, the functionality still exists & should be compatible. > Why ucontext is better? Better performance essentially. > According to > http://duriansoftware.com/joe/PSA:-avoiding-the-%22ucontext-routines-are-deprecated%22-error-on-Mac-OS-X-Snow-Leopard.html > seems that defining _XOPEN_SOURCE should remove at least the error. Better is to just use a GCC pragma to silence the compile warning for that piece of code only. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] Windows 10 guest: 2D/3D Accel
On Tue, 9 May 2017 17:22:46 +0200 Oscar Segarra wrote: > Hi, Christophe, > > Thanks a lot again for the detailed explanation. > > I understand perfectly the difference between client, host and guest. > In my enviornment client is ferdora 25, host is Centos 7 and guest is > Windows 10. > > Regarding the 7th point: > > *7. The “client” will talk to that protocol and display that on your > screen. The client itself may use 3D acceleration to display things > on your screen, but it’s not the “same” 3D acceleration as used in > the guest. For example, if it gets a video stream from the server in > 6, now it’s using the video decoding capabilities of the 3D card, > even if your application in the guest is rendering 3D objects.* > > This means that the client (physical endpoint) will use the GPU just > for jpeg and mpeg decoding. It has no sense to buy an expensive card > for the endpoint, isn't it? It might be interesting project to support remote GL. There are some specific challenges to this: - assuming you connect the client from the very start the guest can connect to the client while booting and probe the client card features to load the correct driver, etc. If you want to connect during guest runtime you need a fallback SW card and GPU hotplug support in the guest. If you do not have perfect GPU hotplug support in guest you will probably need to intercept all GL calls in the guest driver to build a GL rendering state which you can then push to any client that connects. - the 3D applications typically expect instanteous roundtrip to the card and do not optimize for the case when pulling a buffer from the card and pushing it back is quite expensive (requiring network roundtrip between client and guest). They should given the roundtrip between CPU and GPU is typically relatively slow and costly even between directly connected components but that is not what the applications do in practice. So your 3D applications will probably run quite slow even if your GPU hotplug and remote rendering were perfect. Overall it would be interesting project to try but I expect some pieces might be quite technically challenging depending on the component mix you want to support and I do not expect that real world usability will be great. You will be probably able to run select applications reasonably well but most will fail or run too slow to be usable. Thanks Michal ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-server v4] sound: Remove sound channel global list
On Tue, 2017-05-09 at 19:50 +0100, Frediano Ziglio wrote:
> Use the channel list from RedState to iterate over all channels.
By the way, this should be RedsState rather than RedState.
> This removes one more global variable.
>
> Signed-off-by: Frediano Ziglio
> ---
> server/reds.c | 18 +--
> server/sound.c | 96 ++
>
> server/sound.h | 19 ++--
> 3 files changed, 63 insertions(+), 70 deletions(-)
>
> Changes since v3:
> - check class type in RedsState;
> - change function names.
>
> diff --git a/server/reds.c b/server/reds.c
> index 2a8f905..5d7f226 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -2614,7 +2614,14 @@ void reds_set_client_mm_time_latency(RedsState
> *reds, RedClient *client, uint32_
> latency, reds->mm_time_latency);
> }
> } else {
> -snd_set_playback_latency(client, latency);
> +GListIter it;
> +RedChannel *channel;
> +
> +GLIST_FOREACH(reds->channels, it, RedChannel, channel) {
> +if (IS_PLAYBACK_CHANNEL(channel)) {
> +playback_channel_set_latency(PLAYBACK_CHANNEL(channe
> l), client, latency);
> +}
> +}
It seems a bit strange to me that we would call
func(channel, client, latency)
when we don't know for sure whether client is actually related to
channel or not. To me it would make more sense to iterate through the
client's channels rather than the RedsState's channel list. Something
like:
GList *channel_clients = red_client_get_channel_clients(client);
GLIST_FOREACH(channel_clients, it, RedChannelClient, rcc) {
RedChannel *channel = red_channel_client_get_channel(rcc);
if (IS_PLAYBACK_CHANNEL(channel) {
playback_channel_set_latency(channel, latency);
}
}
Note that the function red_client_get_channel_clients() does not exist
at the moment. But it would be pretty simple to add. (Also note that
playback_channel_set_latency() would not require both a 'channel' and a
'client' argument in this scenario.)
Alternately, instead of adding a new method to retrieve the channel
clients from RedClient, we could simply add a red_client_set_latency()
function which would forward the request on to any playback channels it
owns. For example
void reds_set_client_mm_time_latency(RedsState *reds, RedClient
*client, uint32_t latency) {
...
red_client_set_latency(client, latency);
...
}
...
red_client_set_latency(RedClient *client, uint32_t latency) {
GListIter it;
GLIST_FOREACH(client->channels, it, RedChannelClient, rcc) {
RedChannel *channel = red_channel_client_get_channel(rcc);
if (IS_PLAYBACK_CHANNEL(channel) {
playback_channel_set_latency(channel, latency);
}
}
}
?
> }
> }
>
> @@ -4012,8 +4019,15 @@ static void reds_set_video_codecs(RedsState
> *reds, GArray *video_codecs)
>
> SPICE_GNUC_VISIBLE int
> spice_server_set_playback_compression(SpiceServer *reds, int enable)
> {
> +GListIter it;
> +RedChannel *channel;
> +
> reds->config->playback_compression = !!enable;
> -snd_set_playback_compression(enable);
> +GLIST_FOREACH(reds->channels, it, RedChannel, channel) {
> +if (IS_PLAYBACK_CHANNEL(channel)) {
> +playback_channel_set_compression(PLAYBACK_CHANNEL(channe
> l), !!enable);
> +}
> +}
> return 0;
> }
>
> diff --git a/server/sound.c b/server/sound.c
> index be7e607..7140854 100644
> --- a/server/sound.c
> +++ b/server/sound.c
> @@ -71,7 +71,6 @@ typedef struct PlaybackChannelClient
> PlaybackChannelClient;
> typedef struct RecordChannelClient RecordChannelClient;
> typedef struct AudioFrame AudioFrame;
> typedef struct AudioFrameContainer AudioFrameContainer;
> -typedef struct SpicePlaybackState PlaybackChannel;
> typedef struct SpiceRecordState RecordChannel;
>
> typedef void (*snd_channel_on_message_done_proc)(SndChannelClient
> *client);
> @@ -178,11 +177,6 @@ typedef struct SndChannelClass {
> G_DEFINE_TYPE(SndChannel, snd_channel, RED_TYPE_CHANNEL)
>
>
> -#define TYPE_PLAYBACK_CHANNEL playback_channel_get_type()
> -#define PLAYBACK_CHANNEL(obj) \
> -(G_TYPE_CHECK_INSTANCE_CAST((obj), TYPE_PLAYBACK_CHANNEL,
> PlaybackChannel))
> -GType playback_channel_get_type(void) G_GNUC_CONST;
> -
> struct SpicePlaybackState {
> SndChannel channel;
> };
> @@ -233,9 +227,6 @@ typedef struct RecordChannelClientClass {
> G_DEFINE_TYPE(RecordChannelClient, record_channel_client,
> TYPE_SND_CHANNEL_CLIENT)
>
>
> -/* A list of all Spice{Playback,Record}State objects */
> -static GList *snd_channels;
> -
> static void snd_send(SndChannelClient * client);
>
> /* sound channels only support a single client */
> @@ -976,28 +967,22 @@ SPICE_GNUC_VISIBLE void
> spice_server_playback_put_samples(SpicePlaybackInstance
> snd_send(SND_CHANNEL_CLIENT(playback_client));
> }
>
> -void snd_set_playback_latency(RedClient *client, uint32_t latency)
> +void playb
Re: [Spice-devel] [PATCH spice-gtk 0/5] Fix documentation strings
ACK series On Wed, 2017-05-10 at 11:33 +0200, Pavel Grunt wrote: > Hi, > > I noticed that some symbol were not displayed in the gtk-doc > documentation. This series should improve that. > > Thanks, > > Pavel Grunt (5): > doc: Set correct label > doc: Remove invalid parameter of property > doc: Describe spice_gl_scanout_free > doc: Describe spice_usb_device_manager_disconnect_device_async > doc: Display properties of spice-gtk classes > > doc/reference/spice-gtk-sections.txt | 6 ++ > src/channel-display.c| 11 +-- > src/spice-gtk-session.h | 11 +++ > src/spice-widget.h | 11 +++ > src/usb-device-manager.c | 14 ++ > src/usb-device-widget.h | 10 ++ > 6 files changed, 61 insertions(+), 2 deletions(-) > ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-server v2 5/6] Make RedCharDeviceWriteBuffer::refs private
On Thu, May 11, 2017 at 11:24:28AM +0200, Christophe de Dinechin wrote:
> >
> > On 28 Apr 2017, at 11:11, Christophe Fergeau wrote:
> >
> > Signed-off-by: Christophe Fergeau
> > ---
> > server/char-device.c | 11 ++-
> > server/char-device.h | 1 -
> > 2 files changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/server/char-device.c b/server/char-device.c
> > index 7e77b465b..481ea2a13 100644
> > --- a/server/char-device.c
> > +++ b/server/char-device.c
> > @@ -36,6 +36,7 @@ struct RedCharDeviceWriteBufferPrivate {
> > RedClient *client; /* The client that sent the message to the device.
> > NULL if the server created the message */
> > uint32_t token_price;
> > +uint32_t refs;
> > };
> >
> > typedef struct RedCharDeviceClient RedCharDeviceClient;
> > @@ -165,7 +166,7 @@ static void write_buffers_queue_free(GQueue
> > *write_queue)
> > static void red_char_device_write_buffer_pool_add(RedCharDevice *dev,
> > RedCharDeviceWriteBuffer
> > *buf)
> > {
> > -if (buf->refs == 1 &&
> > +if (buf->priv->refs == 1 &&
> > dev->priv->cur_pool_size < MAX_POOL_SIZE) {
> > buf->buf_used = 0;
> > buf->priv->origin = WRITE_BUFFER_ORIGIN_NONE;
> > @@ -584,7 +585,7 @@ static RedCharDeviceWriteBuffer
> > *__red_char_device_write_buffer_get(
> > }
> >
> > ret->priv->token_price = migrated_data_tokens ? migrated_data_tokens :
> > 1;
> > -ret->refs = 1;
> > +ret->priv->refs = 1;
> > return ret;
> > error:
> > dev->priv->cur_pool_size += ret->buf_size;
> > @@ -612,7 +613,7 @@ static RedCharDeviceWriteBuffer
> > *red_char_device_write_buffer_ref(RedCharDeviceW
> > {
> > spice_assert(write_buf);
> >
> > -write_buf->refs++;
> > +write_buf->priv->refs++;
>
> Increasing / decreasing refs is done in a non-atomic way. I assume
> this means that we know we cannot enter
> red_char_device_write_buffer_ref from different threads simultaneously
> for the same buffer. Just for my education, could someone explain why?
> My own mental model is that marshalling is a very sequential operation
> anyway, so it’s hard to think of a valid scenario where we’d
> parallelize it. But that makes me wonder if there is some kind of
> convention allowing us to easily identify that structures that are
> possibly shared between threads.
There are few threads being used, roughly most of the spice code runs in
the main QEMU thread, except for the display channel code (RedWorker),
each display channel has its own thread. The main thread and the worker
thread communicate through messages (dispatcher or reddispatcher, I
always mix these 2). There should be very few data structures which are
going to be shared between threads.
Christophe
signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-gtk v2 0/5] Always use Playbin to create the pipeline
From: Victor Toso -- v1-> v2: Adressing Frediano comments in v1 * Rebased * Removed caps="" for older version of GStreamer. No clear info on the issue that one had at that time. Still, video/x-h264 is the stream format that we receive and an error because of that should really be a bug in GStreamer. * Moved v1 02/04 to be v2 01/04 in order to fix more easily the "cap=%s" problem in v1 01/04 * Free GstSample even while using playbin * gst_element_ref() GstAppSrc and GstAppSink -- v1: Hi, First time using git-publish [0], sorry if anything goes wrong :) [0] https://github.com/stefanha/git-publish The main goal for this series is to have streaming working with hardware accelerated video decoding whenever is possible. The best way to achieve that is to let GStreamer do most of the work. Using Playbin [1] we can create the whole pipeline. We only need to work with GstAppSrc and GstAppSink to set the encoded data and gather the decoded data. [1] https://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-base-plugins/html/gst-plugins-base-plugins-playbin.html Trying my best to not break streaming with older versions of GStreamer. Based on a comment from ceyusa [2], since version 1.9.0 GStreamer is able to use Vaapi elements automaticaly and for that reason I'm wrapping those changes with GST_CHECK_VERSION(1,9,0). [2] https://lists.freedesktop.org/archives/spice-devel/2016-October/032825.html Cheers, toso Victor Toso (5): display-gst: include capabilities for h264 display-gst: move "caps=" from struct to pipeline display-gst: check GstRegistry for decoding elements display-gst: remove SPICE_GSTVIDEO_AUTO check display-gst: Use Playbin for GStreamer 1.9.0 onwards src/channel-display-gst.c | 184 +++--- 1 file changed, 156 insertions(+), 28 deletions(-) -- 2.12.2 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-gtk v2 3/5] display-gst: check GstRegistry for decoding elements
From: Victor Toso
With this patch, we can find all the elements in the registry that are
video decoders which can handle the predefined GstCaps.
Main benefits are:
- We don't rely on predefined list of GstElements. We don't need to
update them;
- debugging: It does help to know what the registry has at runtime;
Signed-off-by: Victor Toso
Signed-off-by: Victor Toso
---
src/channel-display-gst.c | 49 +++
1 file changed, 49 insertions(+)
diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 7323ce7..a9c45f6 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -535,6 +535,54 @@ VideoDecoder* create_gstreamer_decoder(int codec_type,
display_stream *stream)
G_GNUC_INTERNAL
gboolean gstvideo_has_codec(int codec_type)
{
+#if GST_CHECK_VERSION(1,9,0)
+GList *all_decoders, *codec_decoders;
+GstCaps *caps;
+GstElementFactoryListType type;
+
+g_return_val_if_fail(gstvideo_init(), FALSE);
+g_return_val_if_fail(VALID_VIDEO_CODEC_TYPE(codec_type), FALSE);
+
+type = GST_ELEMENT_FACTORY_TYPE_DECODER |
GST_ELEMENT_FACTORY_TYPE_MEDIA_VIDEO;
+all_decoders = gst_element_factory_list_get_elements(type, GST_RANK_NONE);
+if (all_decoders == NULL) {
+spice_warning("No video decoders from GStreamer were found");
+return FALSE;
+}
+
+caps = gst_caps_from_string(gst_opts[codec_type].dec_caps);
+codec_decoders = gst_element_factory_list_filter(all_decoders, caps,
GST_PAD_SINK, TRUE);
+gst_caps_unref(caps);
+
+if (codec_decoders == NULL) {
+spice_debug("From %u decoders, none can handle '%s'",
+g_list_length(all_decoders),
gst_opts[codec_type].dec_caps);
+gst_plugin_feature_list_free(all_decoders);
+return FALSE;
+}
+
+if (spice_util_get_debug()) {
+GList *l;
+GString *msg = g_string_new(NULL);
+/* Print list of available decoders to make debugging easier */
+g_string_printf(msg, "From %3u video decoder elements, %2u can handle
caps %12s: ",
+g_list_length(all_decoders),
g_list_length(codec_decoders),
+gst_opts[codec_type].dec_caps);
+
+for (l = codec_decoders; l != NULL; l = l->next) {
+GstPluginFeature *pfeat = GST_PLUGIN_FEATURE(l->data);
+g_string_append_printf(msg, "%s, ",
gst_plugin_feature_get_name(pfeat));
+}
+
+msg->str[msg->len - 2] = '\0';
+spice_debug("%s", msg->str);
+g_string_free(msg, TRUE);
+}
+
+gst_plugin_feature_list_free(codec_decoders);
+gst_plugin_feature_list_free(all_decoders);
+return TRUE;
+#else
gboolean has_codec = FALSE;
VideoDecoder *decoder = create_gstreamer_decoder(codec_type, NULL);
@@ -544,4 +592,5 @@ gboolean gstvideo_has_codec(int codec_type)
}
return has_codec;
+#endif
}
--
2.12.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-gtk v2 4/5] display-gst: remove SPICE_GSTVIDEO_AUTO check
From: Victor Toso
By using this environment variable, we could use decodebin to let
GStreamer automatically find the best elements to get the streaming
decoded. It was disable by default, in an attempt to have a easy way
to test it.
Follow up patch will use Playbin to create the pipeline which does the
similar behavior but with less work to maintain the pipeline.
Remove this in a separated patch to reduce the code changes.
Signed-off-by: Victor Toso
Signed-off-by: Victor Toso
---
src/channel-display-gst.c | 22 --
1 file changed, 4 insertions(+), 18 deletions(-)
diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index a9c45f6..dbdcef8 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -54,12 +54,8 @@ static struct {
const gchar *dec_name;
const gchar *dec_caps;
} gst_opts[] = {
-/* decodebin will use vaapi if installed, which for a time could
- * intentionally crash the application. So only use decodebin as a
- * fallback or when SPICE_GSTVIDEO_AUTO is set.
- * See: https://bugs.freedesktop.org/show_bug.cgi?id=90884
- */
-{ "decodebin", "" },
+/* Spice' video codec type starts on index 1 */
+{ NULL, NULL },
/* SPICE_VIDEO_CODEC_TYPE_MJPEG */
{ "jpegdec", "image/jpeg" },
@@ -304,21 +300,10 @@ static gboolean handle_pipeline_message(GstBus *bus,
GstMessage *msg, gpointer v
static gboolean create_pipeline(SpiceGstDecoder *decoder)
{
gchar *desc;
-gboolean auto_enabled;
-guint opt;
GstAppSinkCallbacks appsink_cbs = { NULL };
GError *err = NULL;
GstBus *bus;
-auto_enabled = (g_getenv("SPICE_GSTVIDEO_AUTO") != NULL);
-if (auto_enabled || !VALID_VIDEO_CODEC_TYPE(decoder->base.codec_type)) {
-SPICE_DEBUG("Trying %s for codec type %d %s",
-gst_opts[0].dec_name, decoder->base.codec_type,
-(auto_enabled) ? "(SPICE_GSTVIDEO_AUTO is set)" : "");
-opt = 0;
-} else {
-opt = decoder->base.codec_type;
-}
/* - We schedule the frame display ourselves so set sync=false on appsink
* so the pipeline decodes them as fast as possible. This will also
@@ -330,7 +315,8 @@ static gboolean create_pipeline(SpiceGstDecoder *decoder)
desc = g_strdup_printf("appsrc name=src is-live=true format=time
max-bytes=0 block=true "
"caps=%s ! %s ! videoconvert ! appsink name=sink "
"caps=video/x-raw,format=BGRx sync=false
drop=false",
- gst_opts[opt].dec_caps, gst_opts[opt].dec_name);
+ gst_opts[decoder->base.codec_type].dec_caps,
+ gst_opts[decoder->base.codec_type].dec_name);
SPICE_DEBUG("GStreamer pipeline: %s", desc);
decoder->pipeline = gst_parse_launch_full(desc, NULL,
GST_PARSE_FLAG_FATAL_ERRORS, &err);
--
2.12.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-gtk v2 5/5] display-gst: Use Playbin for GStreamer 1.9.0 onwards
From: Victor Toso
The Playbin can provide the full pipeline which reduces the
overall maintenance in the code as we don't need to track which
decoder can work with our stream type.
We need to maintain the GstCaps per SPICE_VIDEO_CODEC_TYPE in order to
tell Playbin the type of data we expect. This much should be covered
by spice-protocol and very likely we will need to extend it in the
future to allow more settings that might not possible to verify at
runtime.
This patch keeps previous code for compatibility reasons.
Note that we have to wait Playbin to emit "source-setup" in order to
configure GstAppSrc with the capabilities of input stream. If in the
unlikely event of frames arriving while GstAppSrc is not setup, we
will drop those frames.
Signed-off-by: Victor Toso
Signed-off-by: Victor Toso
---
src/channel-display-gst.c | 97 ++-
1 file changed, 95 insertions(+), 2 deletions(-)
diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index dbdcef8..7e2b195 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -50,6 +50,9 @@ typedef struct SpiceGstDecoder {
guint timer_id;
} SpiceGstDecoder;
+/* FIXME: With gstreamer version 1.9.0 and higher, we are using playbin to
+ * create the pipeline for us and for that reason we don't need to keep track
of
+ * decoder's name anymore. */
static struct {
const gchar *dec_name;
const gchar *dec_caps;
@@ -82,6 +85,21 @@ G_STATIC_ASSERT(G_N_ELEMENTS(gst_opts) <=
SPICE_VIDEO_CODEC_TYPE_ENUM_END);
#define VALID_VIDEO_CODEC_TYPE(codec) \
(codec > 0 && codec < G_N_ELEMENTS(gst_opts))
+typedef enum {
+ GST_PLAY_FLAG_VIDEO = (1 << 0),
+ GST_PLAY_FLAG_AUDIO = (1 << 1),
+ GST_PLAY_FLAG_TEXT = (1 << 2),
+ GST_PLAY_FLAG_VIS = (1 << 3),
+ GST_PLAY_FLAG_SOFT_VOLUME = (1 << 4),
+ GST_PLAY_FLAG_NATIVE_AUDIO = (1 << 5),
+ GST_PLAY_FLAG_NATIVE_VIDEO = (1 << 6),
+ GST_PLAY_FLAG_DOWNLOAD = (1 << 7),
+ GST_PLAY_FLAG_BUFFERING = (1 << 8),
+ GST_PLAY_FLAG_DEINTERLACE = (1 << 9),
+ GST_PLAY_FLAG_SOFT_COLORBALANCE = (1 << 10),
+ GST_PLAY_FLAG_FORCE_FILTERS = (1 << 11),
+} GstPlayFlags;
+
/* -- SpiceGstFrame -- */
typedef struct SpiceGstFrame {
@@ -297,13 +315,79 @@ static gboolean handle_pipeline_message(GstBus *bus,
GstMessage *msg, gpointer v
return TRUE;
}
+#if GST_CHECK_VERSION(1,9,0)
+static void app_source_setup(GstElement *pipeline, GstElement *source,
SpiceGstDecoder *decoder)
+{
+GstCaps *caps;
+
+/* - We schedule the frame display ourselves so set sync=false on appsink
+ * so the pipeline decodes them as fast as possible. This will also
+ * minimize the risk of frames getting lost when we rebuild the
+ * pipeline.
+ * - Set max-bytes=0 on appsrc so it does not drop frames that may be
+ * needed by those that follow.
+ */
+caps = gst_caps_from_string(gst_opts[decoder->base.codec_type].dec_caps);
+g_object_set(source,
+ "caps", caps,
+ "is-live", TRUE,
+ "format", GST_FORMAT_TIME,
+ "max-bytes", 0,
+ "block", TRUE,
+ NULL);
+gst_caps_unref(caps);
+decoder->appsrc = GST_APP_SRC(gst_object_ref(source));
+}
+#endif
+
static gboolean create_pipeline(SpiceGstDecoder *decoder)
{
-gchar *desc;
GstAppSinkCallbacks appsink_cbs = { NULL };
-GError *err = NULL;
GstBus *bus;
+#if GST_CHECK_VERSION(1,9,0)
+GstElement *playbin, *sink;
+gint flags;
+GstCaps *caps;
+playbin = gst_element_factory_make("playbin", "playbin");
+if (playbin == NULL) {
+spice_warning("error upon creation of 'playbin' element");
+return FALSE;
+}
+
+sink = gst_element_factory_make("appsink", "sink");
+if (sink == NULL) {
+spice_warning("error upon creation of 'appsink' element");
+gst_object_unref(playbin);
+return FALSE;
+}
+
+caps = gst_caps_from_string("video/x-raw,format=BGRx");
+g_object_set(sink,
+ "caps", caps,
+ "sync", FALSE,
+ "drop", FALSE,
+ NULL);
+gst_caps_unref(caps);
+
+g_object_set(playbin,
+ "uri", "appsrc://",
+ "video-sink", gst_object_ref(sink),
+ NULL);
+
+g_signal_connect(playbin, "source-setup", G_CALLBACK(app_source_setup),
decoder);
+
+/* Disable audio in playbin */
+g_object_get(playbin, "flags", &flags, NULL);
+flags &= ~(GST_PLAY_FLAG_AUDIO | GST_PLAY_FLAG_TEXT);
+g_object_set(playbin, "flags", flags, NULL);
+
+decoder->appsrc = NULL;
+decoder->appsink = GST_APP_SINK(sink);
+decoder->pipeline = playbin;
+#else
+gchar *desc;
+GError *err = NULL;
/* - We schedule the frame display ourselves so set sync=false on appsink
* so th
[Spice-devel] [PATCH spice-gtk v2 1/5] display-gst: include capabilities for h264
From: Victor Toso
Removing a comment in the code that says that incomplete GstCaps could
trigger errors as this seems to be working fine. We should include all
necessary parameters based on spice-protocol. At this time, the
information we have from protocol is the stream-type which this patch
includes.
Any other errors to h264 format should either be reported to
GStreamer or fixed by improving the spice-protocol.
The follow up patch will identify elements in GstRegistry based on
GstCaps so this is a necessary change to have.
This is also a preparatory patch to use Playbin element to create the
pipeline. Without this, Playbin or typefind will fail to recognize the
stream as H264.
Signed-off-by: Victor Toso
Signed-off-by: Victor Toso
---
src/channel-display-gst.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 9b79403..8546167 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -71,11 +71,11 @@ static struct {
*/
{ "vp8dec", "caps=video/x-vp8" },
-/* SPICE_VIDEO_CODEC_TYPE_H264
- * h264 streams detection works fine and setting an incomplete cap
- * causes errors. So let typefind do all the work.
- */
-{ "h264parse ! avdec_h264", "" },
+/* SPICE_VIDEO_CODEC_TYPE_H264 */
+/* We need the caps for Playbin. We could also add
"stream-format=byte-stream"
+ * as we set that in spice-server but we might want to have support to
+ * different stream-format */
+{ "h264parse ! avdec_h264", "caps=video/x-h264" },
/* SPICE_VIDEO_CODEC_TYPE_VP9 */
{ "vp9dec", "caps=video/x-vp9" },
--
2.12.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-gtk v2 2/5] display-gst: move "caps=" from struct to pipeline
From: Victor Toso
This way we have a map of the necessary GstCaps to a given
SPICE_VIDEO_CODEC_TYPE.
This patch is also a preparatory patch to:
- Identify which GstElements in GstRegistry can handle this GstCaps
- Use Playbin as wrapper to all elements beside GstAppSrc and
GstAppSink. In this case, we should rely on GstCaps to reduce
typefind errors as we should know what kind of data is expected
Signed-off-by: Victor Toso
Signed-off-by: Victor Toso
---
src/channel-display-gst.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 8546167..7323ce7 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -62,23 +62,23 @@ static struct {
{ "decodebin", "" },
/* SPICE_VIDEO_CODEC_TYPE_MJPEG */
-{ "jpegdec", "caps=image/jpeg" },
+{ "jpegdec", "image/jpeg" },
/* SPICE_VIDEO_CODEC_TYPE_VP8
*
* typefind is unable to identify VP8 streams by design.
* See: https://bugzilla.gnome.org/show_bug.cgi?id=756457
*/
-{ "vp8dec", "caps=video/x-vp8" },
+{ "vp8dec", "video/x-vp8" },
/* SPICE_VIDEO_CODEC_TYPE_H264 */
/* We need the caps for Playbin. We could also add
"stream-format=byte-stream"
* as we set that in spice-server but we might want to have support to
* different stream-format */
-{ "h264parse ! avdec_h264", "caps=video/x-h264" },
+{ "h264parse ! avdec_h264", "video/x-h264" },
/* SPICE_VIDEO_CODEC_TYPE_VP9 */
-{ "vp9dec", "caps=video/x-vp9" },
+{ "vp9dec", "video/x-vp9" },
};
G_STATIC_ASSERT(G_N_ELEMENTS(gst_opts) <= SPICE_VIDEO_CODEC_TYPE_ENUM_END);
@@ -328,7 +328,7 @@ static gboolean create_pipeline(SpiceGstDecoder *decoder)
* needed by those that follow.
*/
desc = g_strdup_printf("appsrc name=src is-live=true format=time
max-bytes=0 block=true "
- "%s ! %s ! videoconvert ! appsink name=sink "
+ "caps=%s ! %s ! videoconvert ! appsink name=sink "
"caps=video/x-raw,format=BGRx sync=false
drop=false",
gst_opts[opt].dec_caps, gst_opts[opt].dec_name);
SPICE_DEBUG("GStreamer pipeline: %s", desc);
--
2.12.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk v2 4/4] Add check for macOS, disable ucontext on macOS (deprecated)
> On 11 May 2017, at 12:56, Daniel P. Berrange wrote: > > On Thu, May 11, 2017 at 12:47:08PM +0200, Christophe de Dinechin wrote: >> From: Christophe de Dinechin >> >> Signed-off-by: Christophe de Dinechin >> --- >> configure.ac | 14 ++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/configure.ac b/configure.ac >> index 74b5811..ecab365 100644 >> --- a/configure.ac >> +++ b/configure.ac >> @@ -62,6 +62,18 @@ esac >> AC_MSG_RESULT([$os_win32]) >> AM_CONDITIONAL([OS_WIN32],[test "$os_win32" = "yes"]) >> >> +AC_MSG_CHECKING([for native macOS]) >> +case "$host_os" in >> + *darwin*) >> +os_mac=yes >> +;; >> + *) >> +os_mac=no >> +;; >> +esac >> +AC_MSG_RESULT([$os_mac]) >> +AM_CONDITIONAL([OS_MAC],[test "$os_mac" = "yes"]) >> + >> AC_CHECK_HEADERS([sys/socket.h netinet/in.h arpa/inet.h]) >> AC_CHECK_HEADERS([termios.h]) >> AC_CHECK_HEADERS([epoxy/egl.h], >> @@ -468,6 +480,8 @@ esac >> if test "$with_coroutine" = "auto"; then >> if test "$os_win32" = "yes"; then >> with_coroutine=winfiber >> + elif test "$os_mac" = "yes"; then >> +with_coroutine=gthread >> else >> with_coroutine=ucontext >> fi > > Despite ucontext being deprecated we are still better off using that & > ignoring the warnings, than using the gthread impl. Yes, I remember you explained the benefits of keeping ucontext. But for the moment at least, on macOS, it is not a warning: /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/usr/include/ucontext.h:43:2: error: The deprecated ucontext routines require _XOPEN_SOURCE to be defined #error The deprecated ucontext routines require _XOPEN_SOURCE to be defined ^ 1 error generated. So I can: - Add the error in the patch description - Attempt to define _XOPEN_SOURCE in the configuration. I’m concerned about side effects. The latter leads to another can of worms. Notably, the macro container_of triggers the alignment warning for container_of, so I have a set of alignment warnings, and a set of deprecation warnings. But it builds. The incremental patch would be something like: diff --git a/configure.ac b/configure.ac index ecab365..8b433ba 100644 --- a/configure.ac +++ b/configure.ac @@ -481,7 +481,8 @@ if test "$with_coroutine" = "auto"; then if test "$os_win32" = "yes"; then with_coroutine=winfiber elif test "$os_mac" = "yes"; then -with_coroutine=gthread +with_coroutine=ucontext +AC_DEFINE([_XOPEN_SOURCE], [1], [Define _XOPEN_SOURCE on macOS for ucontext]) else with_coroutine=ucontext fi diff --git a/src/continuation.h b/src/continuation.h index 675a257..cbca06e 100644 --- a/src/continuation.h +++ b/src/continuation.h @@ -49,7 +49,7 @@ int cc_swap(struct continuation *from, struct continuation *to); #define offset_of(type, member) ((unsigned long)(&((type *)0)->member)) #define container_of(obj, type, member) \ -(type *)(((char *)obj) - offset_of(type, member)) +(type *)(void *)(((char *)obj) - offset_of(type, member)) #endif /* Would that be better in your opinion? Christophe > > Regards, > Daniel > -- > |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o-https://fstop138.berrange.com :| > |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| > ___ > Spice-devel mailing list > Spice-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/spice-devel ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
