Re: [Spice-devel] [PATCH spice-gtk] gst: Fix detection of h264 stream decoder

[Victor Toso]

A new day, a new thread

On Wed, Jul 19, 2017 at 02:59:00PM +0200, Pavel Grunt wrote:
> GStreamer's avdec_h264 needs h264parse to be able to process H264 video
> streams. However the check for elements through GstRegistry does not
> take into account the possible pipeline of elements (like "h264parse ! 
> avdec_h264").
>
> Fix that by checking for the elements by their name.

By reading your commit log, your are fixing a single use case by
checking element's names that are hard coded to our codebase.

The reason for the following two patches were to remove the usage of
hard coded names to check and play the stream.

Shortlog: display-gst: Use Playbin for GStreamer 1.9.0 onwards
Commit  : ed697dd7fa165632bd0cbdb34c971c8001c433fa
Victor Toso on Mon, 10 Jul 2017 17:34:23 +0200

Shortlog: display-gst: check GstRegistry for decoding elements
Commit  : c9209aef946b3ad517e7794d2e5648caf5ee2bf9
Victor Toso on Mon, 10 Jul 2017 17:33:10 +0200

So, this patch is going backwards in regard to one of the objectives of
playbin's introducing besides the fact that it does not guarantee a
solution for 3rd party plugins that might be filtered out.

So, Let's try to be clear about what's going on:

The regression introduced by c9209aef946b3 is that avdec_h264 is filter
out in gstvideo_has_codec() and for that reason, for systems that do not
have any other decoder, our caps for decoding h264 will be disabled,
which is a regression.

To fix the issue in a way that follows the objective of current code, we
should simply fix the filter as I've suggested at [0]. By the way, I
went to the GStreamer folks about this filtering issue and I understood
that my code in regard to the filtering is wrong due the assumption of
what is a subset. That's a bug alright.

https://lists.freedesktop.org/archives/spice-devel/2017-July/038634.html

Pavel, you saying that patch is incorrect is wrong. If you want, you can
say _incomplete_, but that's a fix.

Incomplete you may say, because you think it is important to check for
h264parse element. I'm not against that but I do think we, again, need
to approach #gstreamer folks and check with them that we want a more
robust check for enabling or not a streaming given a video-codec. If we
go this way and checking for parses seem important, we should do it in a
more generic fashion.

Again, I would go with my patch that fixes an issue and see from now on
how to make our check more robust and I would not be surprise if there
is no way to guarantee 100% that checking every single element in a
pipeline would in fact decode a stream and that's because we don't have
the parameters for the stream itself embedded in our protocol besides
other issues that may occur.

So, yes, if you remove h264parse from registry, we will say that the
stream can be decoded with avdec_h264 while it can't. I think that's a
weak argument in favor of your patch and as I'm saying a few times now,
we should consider failure an option, fallback to image compression or
different streams with different video-codec types if the fallback
does not work, it is a bug even with your patch.

Cheers,


> ---
>  src/channel-display-gst.c | 25 +
>  1 file changed, 25 insertions(+)
> 
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index 5042fc8..a9a044a 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -666,6 +666,31 @@ gboolean gstvideo_has_codec(int codec_type)
>  codec_decoders = gst_element_factory_list_filter(all_decoders, caps, 
> GST_PAD_SINK, TRUE);
>  gst_caps_unref(caps);
>  
> +/* Check for the presence of decoding elements that could be filter out.
> + * TODO: Improve filtering to avoid this...
> + */
> +{
> +GList *decoder_elements = NULL;
> +gchar **decoders_by_names;
> +guint i = 0;
> +decoders_by_names = g_strsplit(gst_opts[codec_type].dec_name, "!", 
> -1);
> +for (i = 0; decoders_by_names[i] != NULL; i++) {
> +GstElementFactory *element = 
> gst_element_factory_find(g_strstrip(decoders_by_names[i]));
> +if (element == NULL) {
> +gst_plugin_feature_list_free(decoder_elements);
> +decoder_elements = NULL;
> +break;
> +}
> +if (g_list_find(codec_decoders, element)) {
> +gst_object_unref(element);
> +} else {
> +decoder_elements = g_list_append(decoder_elements, element);
> +}
> +}
> +codec_decoders = g_list_concat(codec_decoders, decoder_elements);
> +g_strfreev(decoders_by_names);
> +}
> +
>  if (codec_decoders == NULL) {
>  spice_debug("From %u decoders, none can handle '%s'",
>  g_list_length(all_decoders), 
> gst_opts[codec_type].dec_caps);
> -- 
> 2.13.3
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> 

Re: [Spice-devel] [PATCH linux vdagent] Add systemd socket activation

[Fabiano Fidêncio]

On Thu, Jul 13, 2017 at 10:35 PM, Jonathon Jongsma  wrote:
> If we are configured to use the systemd init script, also add support
> for systemd socket activation. systemd will listen on the socket that is
> used to communicate between the session agent and the system daemon.
> When the session agent connects, the system daemon will automatically be
> started. The socket will be enabled only if the required virtio-port
> device exists. The socket is disabled when the device is removed.
>
> This has a couple minor advantages to the previous approach:
>   - For VMS that are not running a graphical desktop (and thus no
> session agents are running), the system vdagent daemon won't get
> started at all even if the spice virtio port is configured. Only the
> socket will be enabled. In the previous approach, the system daemon
> was started when the virtio device was added regardless of whether
> it was needed or not.
>   - Solves issues related to switching between systemd targets. With the
> previous approach, when a user switches to a different target
> ("systemctl isolate multi-user.target"), spice-vdagentd.target was
> stopped by systemd (since "isolate" by definition stops all targets
> except the one specified). This meant that if the user subsequently
> switched back to graphical.target, the spice-vdagentd.target would
> still be disabled and vdagent functionality would not work. With
> this change, the socket will still be listening after switching to a
> different target, so as soon as a session agent tries to connect to
> the socket, the system daemon will get restarted.
>
> Fixes: rhbz#1340160
>
> Signed-off-by: Jonathon Jongsma 
> ---
> NOTES:
>  - this patch piggybacks onto the --with-init-script configure option to
>determine whether to use systemd socket activation. I'm not sure whether
>that's the correct choice or not, but it seems to work ok. If the init
>script is specified as "systemd" or "systemd+redhat" and we have a new
>enough libsystemd, we attempt to use socket activation.
>  - Even if the agent is configured with systemd socket activation, it should
>still fall back to opening its own socket if it fails to get a socket from
>systemd (for example, if it is running under a different init system).
>  - In theory, we could use socket activation with systemd < 209, but then we'd
>have to link against the separate libsystemd-daemon library. Since this is 
> a
>new feature, I decided to only support more recent versions of systemd. But
>support for older versions could be added pretty easily.
>
>
>  Makefile.am  |  5 +++-
>  configure.ac |  8 +++
>  data/70-spice-vdagentd.rules |  2 +-
>  data/spice-vdagentd.service  |  8 +--
>  data/spice-vdagentd.socket   | 10 
>  data/spice-vdagentd.target   |  2 --
>  src/udscs.c  | 45 
>  src/udscs.h  | 11 +
>  src/vdagentd/vdagentd.c  | 55 
> +++-
>  9 files changed, 109 insertions(+), 37 deletions(-)
>  create mode 100644 data/spice-vdagentd.socket
>  delete mode 100644 data/spice-vdagentd.target
>
> diff --git a/Makefile.am b/Makefile.am
> index 7755f09..f70d514 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -42,6 +42,7 @@ src_spice_vdagent_SOURCES =   \
>
>  src_spice_vdagentd_CFLAGS =\
> $(DBUS_CFLAGS)  \
> +   $(LIBSYSTEMD_DAEMON_CFLAGS) \
> $(LIBSYSTEMD_LOGIN_CFLAGS)  \
> $(PCIACCESS_CFLAGS) \
> $(SPICE_CFLAGS) \
> @@ -52,6 +53,7 @@ src_spice_vdagentd_CFLAGS =   \
>
>  src_spice_vdagentd_LDADD = \
> $(DBUS_LIBS)\
> +   $(LIBSYSTEMD_DAEMON_LIBS)   \
> $(LIBSYSTEMD_LOGIN_LIBS)\
> $(PCIACCESS_LIBS)   \
> $(SPICE_LIBS)   \
> @@ -102,7 +104,7 @@ if INIT_SCRIPT_SYSTEMD
>  systemdunitdir = $(SYSTEMDSYSTEMUNITDIR)
>  systemdunit_DATA = \
> $(top_srcdir)/data/spice-vdagentd.service \
> -   $(top_srcdir)/data/spice-vdagentd.target
> +   $(top_srcdir)/data/spice-vdagentd.socket
>
>  udevrulesdir = /lib/udev/rules.d
>  udevrules_DATA = $(top_srcdir)/data/70-spice-vdagentd.rules
> @@ -124,6 +126,7 @@ EXTRA_DIST =\
> data/spice-vdagent.desktop  \
> data/spice-vdagentd \
> data/spice-vdagentd.service \
> +   data/spice-vdagentd.socket  \
> data/spice-vdagentd.target  \
> data/tmpfiles.d/spice-vdagentd.conf \
> data/xorg.conf.RHEL-5   \
> 

Re: [Spice-devel] [PATCH linux vdagent] Add systemd socket activation

[Jonathon Jongsma]

So, Victor told me on IRC that he was having trouble getting the patch
to work and asked me to follow up on the mailing list about how to test
 the patch. I tested it on a Fedora25 vm as follows:

- Set up a Fedora 25 vm
- uninstall system spice-vdagent
- build spice-vdagent from git and install in /usr
  $ ./configure --with-init-script=systemd --prefix=/usr
  $ make
  # make install
- follow test instructions at https://bugzilla.redhat.com/show_bug.cgi?
id=1340160
- also test some additional scenarios such as:
- boot into a different runlevel (e.g. add systemd.unit=multi-
user.target to kernel commandline) and see whether the service is
running. Switch to a different target (e.g. systemctl isolate
graphical.target) and make sure things still work
- boot the vm without a vdagent channel device and then add it (e.g.
with virt-manager) after the guest is already running. Does the
agent service get started after logging into the desktop?
- remove the vdagent channel device from a running vm. Then add it
back. Does it behave as expected?
- anything else you can think of.

You should have the following files installed:
  /usr/lib/systemd/system/spice-vdagentd.service
  /usr/lib/systemd/system/spice-vdagentd.socket

It worked fine for me, but I'd definitely like to know if it doesn't
work for others. 

Jonathon



On Thu, 2017-07-13 at 15:35 -0500, Jonathon Jongsma wrote:
> If we are configured to use the systemd init script, also add support
> for systemd socket activation. systemd will listen on the socket that
> is
> used to communicate between the session agent and the system daemon.
> When the session agent connects, the system daemon will automatically
> be
> started. The socket will be enabled only if the required virtio-port
> device exists. The socket is disabled when the device is removed.
> 
> This has a couple minor advantages to the previous approach:
>   - For VMS that are not running a graphical desktop (and thus no
> session agents are running), the system vdagent daemon won't get
> started at all even if the spice virtio port is configured. Only
> the
> socket will be enabled. In the previous approach, the system
> daemon
> was started when the virtio device was added regardless of
> whether
> it was needed or not.
>   - Solves issues related to switching between systemd targets. With
> the
> previous approach, when a user switches to a different target
> ("systemctl isolate multi-user.target"), spice-vdagentd.target
> was
> stopped by systemd (since "isolate" by definition stops all
> targets
> except the one specified). This meant that if the user
> subsequently
> switched back to graphical.target, the spice-vdagentd.target
> would
> still be disabled and vdagent functionality would not work. With
> this change, the socket will still be listening after switching
> to a
> different target, so as soon as a session agent tries to connect
> to
> the socket, the system daemon will get restarted.
> 
> Fixes: rhbz#1340160
> 
> Signed-off-by: Jonathon Jongsma 
> ---
> NOTES:
>  - this patch piggybacks onto the --with-init-script configure option
> to
>    determine whether to use systemd socket activation. I'm not sure
> whether
>    that's the correct choice or not, but it seems to work ok. If the
> init
>    script is specified as "systemd" or "systemd+redhat" and we have a
> new
>    enough libsystemd, we attempt to use socket activation.
>  - Even if the agent is configured with systemd socket activation, it
> should
>    still fall back to opening its own socket if it fails to get a
> socket from
>    systemd (for example, if it is running under a different init
> system).
>  - In theory, we could use socket activation with systemd < 209, but
> then we'd
>    have to link against the separate libsystemd-daemon library. Since
> this is a
>    new feature, I decided to only support more recent versions of
> systemd. But
>    support for older versions could be added pretty easily.
> 
> 
>  Makefile.am  |  5 +++-
>  configure.ac |  8 +++
>  data/70-spice-vdagentd.rules |  2 +-
>  data/spice-vdagentd.service  |  8 +--
>  data/spice-vdagentd.socket   | 10 
>  data/spice-vdagentd.target   |  2 --
>  src/udscs.c  | 45 
> 
>  src/udscs.h  | 11 +
>  src/vdagentd/vdagentd.c  | 55
> +++-
>  9 files changed, 109 insertions(+), 37 deletions(-)
>  create mode 100644 data/spice-vdagentd.socket
>  delete mode 100644 data/spice-vdagentd.target
> 
> diff --git a/Makefile.am b/Makefile.am
> index 7755f09..f70d514 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -42,6 +42,7 @@ src_spice_vdagent_SOURCES = 
> \
>  
>  src_spice_vdagentd_CFLAGS =  \
>   $(DBUS_CFLAGS)  \
> +  

Re: [Spice-devel] [vdagent-win PATCH v6 2/5] Initial rewrite of image conversion code

[Frediano Ziglio]

> 
> On Wed, Jul 19, 2017 at 08:03:49AM -0400, Frediano Ziglio wrote:
> > > 
> > > On Wed, Jul 19, 2017 at 12:09:23PM +0200, Christophe de Dinechin wrote:
> > > > 
> > > > > On 19 Jul 2017, at 11:21, Christophe Fergeau 
> > > > > wrote:
> > > > > 
> > > > > On Wed, Jul 19, 2017 at 10:23:30AM +0200, Christophe de Dinechin
> > > > > wrote:
> > > > >> 
> > > > >>> On 18 Jul 2017, at 17:28, Christophe Fergeau 
> > > > >>> wrote:
> > > > >>> 
> > > > >>> On Mon, Jul 17, 2017 at 11:01:22AM +0100, Frediano Ziglio wrote:
> > > >  Remove CxImage linking.
> > > >  Support Windows BMP format.
> > > > >>> 
> > > > >>> Too bad there is no small/maintained library which would do that
> > > > >>> for us
> > > > >>> :-/ From a quick glance, looks ok.
> > > > >>> 
> > > > >>> 
> > > >  
> > > >  +static inline size_t compute_dib_stride(unsigned width, unsigned
> > > >  bit_count)
> > > > >>> 
> > > > >>> Can you use full type names, unsigned int?
> > > > >> 
> > > > >> No. Really, no ;-) Otherwise, for consistency, you should replace
> > > > >> ‘int’
> > > > >> with ‘signed int’,
> > > > > 
> > > > > The way I see it, 'signed'/'unsigned' are type modifiers, 'int' is an
> > > > > actual type name.
> > > > 
> > > > Yes. But ‘long’ is not. It is also a modifier. So why allow “long” or
> > > > “short" but not “unsigned”?
> > > > Or are you also writing “long int” and “short int”?
> > > 
> > > long/short are enough to make the storage size of the integer obvious,
> > > even if you don't know that long means long int.
> > > "unsigned" does not make this obvious unless you know that "unsigned"
> > > means "unsigned int"
> > > 
> > 
> > Section 6.7.2 of C99 standard specified "unsigned" as type.
> > The fact you are not familiar with this is an opinion I don't
> > personally share. "long" does not specify a type as "unsigned"
> > doesn't.
> > 
> 
> [...]
> 
> > 
> > So let's write "long int" for anything. "unsigned" is not less typing,
> > it's a type specified by the language.
> 
> I never said "unsigned" is not standard compliant, so I don't know why
> you keep coming back to that.
> I previously said that just because something is standard-compliant does
> not mean it's a good idea to do it, [insert your favourite obfuscated C
> contest example here].
> 
> In this particular case, since you feel strongly about it, feel free to
> ignore my comment, but I'll nonetheless keep thinking it makes things
> less readable ;)
> 
> Christophe
> 

I moved to "unsigned int" 2 versions ago.

But still think that is a useful discussion. But honestly I think
in this case the readability is quite an opinion and for me
unsigned is like long, perfectly readable and I saw lot of code
using just unsigned.

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

Re: [Spice-devel] [spice-server PATCH 1/3] red_get_surface_cmd: avoid overflow

[Uri Lublin]

On 07/17/2017 11:22 AM, Frediano Ziglio wrote:


Although unlikely, theoretically, multiplying two 32-bit
numbers may overflow.

Found by coverity.

Signed-off-by: Uri Lublin 
---
  server/red-parse-qxl.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
index 33f36923a..0ffa5f7d4 100644
--- a/server/red-parse-qxl.c
+++ b/server/red-parse-qxl.c
@@ -1397,7 +1397,7 @@ bool red_get_surface_cmd(RedMemSlotInfo *slots, int
group_id,
  return false;
  }
  
-size = red->u.surface_create.height *

abs(red->u.surface_create.stride);
+size = red->u.surface_create.height *
(uint64_t)abs(red->u.surface_create.stride);
  red->u.surface_create.data =
  (uint8_t*)memslot_get_virt(slots, qxl->u.surface_create.data,
  size, group_id, );
  if (error) {


This overflow is already handled by red_validate_surface call.


Indeed.
I'll drop this patch.



Also note that silently the uint64_t size is converted to 32 bit calling
memslot_get_virt so maybe would be better to change size to uint32_t.

Frediano



Thanks,
Uri.

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

Re: [Spice-devel] [PATCH spice-gtk] gst: Fix detection of h264 stream decoder

[Pavel Grunt]

On Wed, 2017-07-19 at 18:00 +0200, Christophe Fergeau wrote:
> On Wed, Jul 19, 2017 at 05:46:51PM +0200, Victor Toso wrote:
> > What I'm trying to stress is that we are trying to not check *specific*
> > plugins related to decode/parse a stream. We don't know what our users
> > will be using and we should not enforce them to use avdec_h264 for
> > instance. You are doing this in this patch and that's why I'm against
> > it.
> 

Hi Christophe,

this patch is really not enforcing usage of any plugin. It's just making work
what used to work before c9209aef946b3ad517e7794d2e5648caf5ee2bf9. It does not
switch from GstCaps to checking by names. The checking by names is just an extra
addition to make sure that we don't forget about elements that could be filter
out due to *our* usage of GstCaps & gst_element_factory_list_filter()

> Yes, actually, on fedora you won't get avdec_h264 unless you enable RPM
> Fusion, and you might prefer to go the Cisco openh264 road instead
> ( https://ausil.us/wordpress/?p=126 - assuming this is still the
> recommended way to go with Workstation these days).

openh264 can be used only for WebRTC (at least for now). In general you need to
enable the 3rd party repo to get h246 - even for Cisco's openh264

https://fedoraproject.org/wiki/OpenH264

> So yeah my feeling
> would be to avoid hardcoding plugin names, assuming we can fallback
> properly if the decoding fails. 

we got this streaming features thanks to contribution from community, I don't
see a reason for breaking it and doing a release. Let's break (= improve) it
after that (removing configure checks for plugins, Victor's FIXME and my TODO).

> We could also try to make use of the
> 'missing-codec' GStreamer infrastructure if we want to get fancy.
> 
Yeah, that would be a nice RFE for a newcomer to learn about SPICE protocol.
Whether it is useful or annoying, it's a different question :)

Thanks,
Pavel

P.S.: In fact for gstreamer < 1.9 we should check for the plugin only by their
names (or by launching the pipeline). Because with the current approach it can
happen that an element for hw decoding is present (and the capability is
reported to the server) however a different (possibly missing sw decoding)
element is used in the gstreamer's decoding pipeline.


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

Re: [Spice-devel] [spice-server PATCH 3/3] init ssl connection: return quickly if link is null

[Uri Lublin]

On 07/17/2017 11:17 AM, Frediano Ziglio wrote:


On Sun, 2017-07-16 at 18:47 +0300, Uri Lublin wrote:

Under error: 'link' fields are being accessed, so it's
wrong to goto error with link == NULL.

Instead, return immediately.

Found by coverity.

Signed-off-by: Uri Lublin 

Acked-by: Pavel Grunt 

---
  server/reds.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/server/reds.c b/server/reds.c
index f412a8486..46d33fb5f 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -2462,7 +2462,7 @@ static RedLinkInfo
*reds_init_client_ssl_connection(RedsState *reds, int socket)
  
  link = reds_init_client_connection(reds, socket);

  if (link == NULL)
-goto error;
+return NULL;
  
  ssl_status = reds_stream_enable_ssl(link->stream, reds->ctx);

  switch (ssl_status) {


Patch is fine. Could you update the style (brackets) ?


I did and pushed it.

Thanks,
Uri.
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk] gst: Fix detection of h264 stream decoder

[Pavel Grunt]

On Wed, 2017-07-19 at 18:13 +0200, Victor Toso wrote:
> On Wed, Jul 19, 2017 at 06:01:43PM +0200, Pavel Grunt wrote:
> > On Wed, 2017-07-19 at 17:46 +0200, Victor Toso wrote:
> > > Hi,
> > > 
> > > On Wed, Jul 19, 2017 at 05:27:43PM +0200, Pavel Grunt wrote:
> > > > > Stream decoding will fail, server will know about it and sever should
> > > > > fallback to image compression or a different video codec format for
> > > > > streaming.
> > > > 
> > > > millions of criticals in the debug log and some resources wasted
> > > > (Server will keep trying to create streams - it does not know whether
> > > > it is a temporary issue or permanent).
> > > 
> > > With *host* creating the stream or are you testing something else? :)
> > 
> > Only the host (spice-server) creates the stream.
> > 
> > > 
> > > With the following diff, I can test an error in the pipeline which
> > > should remove the stream, see:
> > > 
> > > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > > index 3f51361..eb6a86b 100644
> > > --- a/src/channel-display-gst.c
> > > +++ b/src/channel-display-gst.c
> > > @@ -511,6 +511,9 @@ static gboolean
> > > spice_gst_decoder_queue_frame(VideoDecoder
> > > *video_decoder,
> > > return TRUE;
> > > }
> > > 
> > > +if (video_decoder->codec_type == SPICE_VIDEO_CODEC_TYPE_H264)
> > > +  return FALSE;
> > > +
> > >  if (spice_mmtime_diff(frame->mm_time, decoder->last_mm_time) < 0) {
> > >   SPICE_DEBUG("new-frame-time < last-frame-time (%u < %u):"
> > >" resetting stream",
> > > 
> > > channel-display.c:1241 display-2:0: display_handle_stream_create: id 49
> > > channel-display.c:1497 display-2:0: video latency: 350
> > > channel-display.c:1563 display-2:0: destroy_display_stream: id=49 #in-
> > > frames=1 
> > > out/in=1.00 #drops-on-receive=0 avg-late-time(ms)=0.00 #drops-on-
> > > playback=0
> > > spice-channel.c:2913 test cap 4 in 0x1052: yes report_invalid_stream:
> > > notify
> > > the server that stream 49 does not exist
> > > spice-channel.c:2913 test cap 4 in 0x1052: yes report_invalid_stream:
> > > notify
> > > the server that stream 49 does not exist
> > > 
> > > GSpice-CRITICAL **: display_handle_stream_data: assertion 'st != NULL'
> > > failed
> > > decode-glz.c:352 decode_header: 1920x946, id 51, ref 49
> > > decode-glz.c:352 decode_header: 1920x946, id 52, ref 50
> > > decode-glz.c:352 decode_header: 1920x946, id 53, ref 51
> > > decode-glz.c:352 decode_header: 1920x946, id 54, ref 52
> > > decode-glz.c:352 decode_header: 1920x946, id 55, ref 53
> > > 
> > > Just one critical, which is expected because we might have frames to
> > > decode while we already remove the decoder.
> > 
> > one critical per created stream, in case of youtube it may try to create
> > more
> > streams
> > > 
> > > > > 
> > > > > We cannot guarantee that the pipeline will work smooth in all systems,
> > > > > all possible combinations.
> > > > 
> > > > Victor, that is a different and unrelated issue
> > > 
> > > Its not, because your patch is trying to make avdec_h264 work because of
> > > h264parse and that means you are trying to guarantee h264 to succeed if
> > > this two elements are present
> > > 
> > 
> > no, I am trying to keep compatibility while not reporting 
> 
> Did not understand.. You are trying to fix a regression but only for
> avdec_h264 case while our code is more generic now...

Yes, I am trying to fix the regression and avoid future regressions just before
the release

> 
> > 
> > 
> > > > 
> > > > > 
> > > > > IMHO, we should improve debug log to provide the information of what
> > > > > is
> > > > > lacking on GStreamer point of view instead of trying to guarantee that
> > > > > it will work.
> > > > 
> > > > unrelated
> > > 
> > > It is unrelated.
> > > 
> > > > 
> > > > > 
> > > > > > >  You shouldn't do that in the
> > > > > > > first place.
> > > > > > > 
> > > > > > > My point is to check for decoders only. If you want to check for
> > > > > > > parsers, this patch of yours will do it solely for hard coded
> > > > > > > elements.
> > > > > > 
> > > > > > This patch is just try to keep working what used to work in the
> > > > > > previous release and prevent breaking it for the next release (which
> > > > > > seems to happen soon).
> > > > > 
> > > > > My patch should work as well and possibly for other plugins like
> > > > > avdec_h264 that don't show up due bad filtering.
> > > > > 
> > > > 
> > > > Have you tested that? They are not working (I'm using playbin).
> > > 
> > > Have you checked the commit log of that patch?
> > 
> > Where do you mention testing of those plugins
> 
> Maybe we are talking about something different. I'm talking about
> testing my patch, which shows avdec_h264 with a simple line to fix the
> filtering...
> 
By *they are not working* I am talking about "the other plugins" - does nvdec
work on your system, avdec_mjpeg? on my system it also shows openh264dec. I
tested these 

Re: [Spice-devel] [PATCH spice-gtk] gst: Fix detection of h264 stream decoder

[Victor Toso]

On Wed, Jul 19, 2017 at 06:01:43PM +0200, Pavel Grunt wrote:
> On Wed, 2017-07-19 at 17:46 +0200, Victor Toso wrote:
> > Hi,
> > 
> > On Wed, Jul 19, 2017 at 05:27:43PM +0200, Pavel Grunt wrote:
> > > > Stream decoding will fail, server will know about it and sever should
> > > > fallback to image compression or a different video codec format for
> > > > streaming.
> > > 
> > > millions of criticals in the debug log and some resources wasted
> > > (Server will keep trying to create streams - it does not know whether
> > > it is a temporary issue or permanent).
> > 
> > With *host* creating the stream or are you testing something else? :)
> 
> Only the host (spice-server) creates the stream.
> 
> > 
> > With the following diff, I can test an error in the pipeline which
> > should remove the stream, see:
> > 
> > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > index 3f51361..eb6a86b 100644
> > --- a/src/channel-display-gst.c
> > +++ b/src/channel-display-gst.c
> > @@ -511,6 +511,9 @@ static gboolean 
> > spice_gst_decoder_queue_frame(VideoDecoder
> > *video_decoder,
> > return TRUE;
> > }
> > 
> > +if (video_decoder->codec_type == SPICE_VIDEO_CODEC_TYPE_H264)
> > +  return FALSE;
> > +
> >  if (spice_mmtime_diff(frame->mm_time, decoder->last_mm_time) < 0) {
> >   SPICE_DEBUG("new-frame-time < last-frame-time (%u < %u):"
> >" resetting stream",
> > 
> > channel-display.c:1241 display-2:0: display_handle_stream_create: id 49
> > channel-display.c:1497 display-2:0: video latency: 350
> > channel-display.c:1563 display-2:0: destroy_display_stream: id=49 
> > #in-frames=1 
> > out/in=1.00 #drops-on-receive=0 avg-late-time(ms)=0.00 #drops-on-playback=0
> > spice-channel.c:2913 test cap 4 in 0x1052: yes report_invalid_stream: notify
> > the server that stream 49 does not exist
> > spice-channel.c:2913 test cap 4 in 0x1052: yes report_invalid_stream: notify
> > the server that stream 49 does not exist
> > 
> > GSpice-CRITICAL **: display_handle_stream_data: assertion 'st != NULL' 
> > failed
> > decode-glz.c:352 decode_header: 1920x946, id 51, ref 49
> > decode-glz.c:352 decode_header: 1920x946, id 52, ref 50
> > decode-glz.c:352 decode_header: 1920x946, id 53, ref 51
> > decode-glz.c:352 decode_header: 1920x946, id 54, ref 52
> > decode-glz.c:352 decode_header: 1920x946, id 55, ref 53
> > 
> > Just one critical, which is expected because we might have frames to
> > decode while we already remove the decoder.
> 
> one critical per created stream, in case of youtube it may try to create more
> streams
> > 
> > > > 
> > > > We cannot guarantee that the pipeline will work smooth in all systems,
> > > > all possible combinations.
> > > 
> > > Victor, that is a different and unrelated issue
> > 
> > Its not, because your patch is trying to make avdec_h264 work because of
> > h264parse and that means you are trying to guarantee h264 to succeed if
> > this two elements are present
> > 
> no, I am trying to keep compatibility while not reporting 

Did not understand.. You are trying to fix a regression but only for
avdec_h264 case while our code is more generic now...

> 
> 
> > > 
> > > > 
> > > > IMHO, we should improve debug log to provide the information of what is
> > > > lacking on GStreamer point of view instead of trying to guarantee that
> > > > it will work.
> > > 
> > > unrelated
> > 
> > It is unrelated.
> > 
> > > 
> > > > 
> > > > > >  You shouldn't do that in the
> > > > > > first place.
> > > > > > 
> > > > > > My point is to check for decoders only. If you want to check for
> > > > > > parsers, this patch of yours will do it solely for hard coded
> > > > > > elements.
> > > > > 
> > > > > This patch is just try to keep working what used to work in the
> > > > > previous release and prevent breaking it for the next release (which
> > > > > seems to happen soon).
> > > > 
> > > > My patch should work as well and possibly for other plugins like
> > > > avdec_h264 that don't show up due bad filtering.
> > > > 
> > > 
> > > Have you tested that? They are not working (I'm using playbin).
> > 
> > Have you checked the commit log of that patch?
> Where do you mention testing of those plugins

Maybe we are talking about something different. I'm talking about
testing my patch, which shows avdec_h264 with a simple line to fix the
filtering...


In my system, our debug shows:
.. gstvideo_debug_available_decoders: From 228 video decoder elements,
- 4 can handle caps   image/jpeg: jpegdec, nvdec, avdec_mjpeg, vaapijpegdec
- 3 can handle caps  video/x-vp8: vaapidecodebin, vp8dec, avdec_vp8
- 4 can handle caps video/x-h264: vaapidecodebin, avdec_h264, nvdec, 
vaapih264dec
- 3 can handle caps  video/x-vp9: vaapidecodebin, vp9dec, avdec_vp9


>
> 
> > 
> > > We do have the check for the same elements in configure, I really
> > > don't understand what is wrong about doing the same check in runtime
> > > (considerring TODO and 

Re: [Spice-devel] [PATCH spice-gtk] gst: Fix detection of h264 stream decoder

[Pavel Grunt]

On Wed, 2017-07-19 at 17:46 +0200, Victor Toso wrote:
> Hi,
> 
> On Wed, Jul 19, 2017 at 05:27:43PM +0200, Pavel Grunt wrote:
> > > Stream decoding will fail, server will know about it and sever should
> > > fallback to image compression or a different video codec format for
> > > streaming.
> > 
> > millions of criticals in the debug log and some resources wasted
> > (Server will keep trying to create streams - it does not know whether
> > it is a temporary issue or permanent).
> 
> With *host* creating the stream or are you testing something else? :)

Only the host (spice-server) creates the stream.

> 
> With the following diff, I can test an error in the pipeline which
> should remove the stream, see:
> 
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index 3f51361..eb6a86b 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -511,6 +511,9 @@ static gboolean spice_gst_decoder_queue_frame(VideoDecoder
> *video_decoder,
> return TRUE;
> }
> 
> +if (video_decoder->codec_type == SPICE_VIDEO_CODEC_TYPE_H264)
> +  return FALSE;
> +
>  if (spice_mmtime_diff(frame->mm_time, decoder->last_mm_time) < 0) {
>   SPICE_DEBUG("new-frame-time < last-frame-time (%u < %u):"
>" resetting stream",
> 
> channel-display.c:1241 display-2:0: display_handle_stream_create: id 49
> channel-display.c:1497 display-2:0: video latency: 350
> channel-display.c:1563 display-2:0: destroy_display_stream: id=49 
> #in-frames=1 
> out/in=1.00 #drops-on-receive=0 avg-late-time(ms)=0.00 #drops-on-playback=0
> spice-channel.c:2913 test cap 4 in 0x1052: yes report_invalid_stream: notify
> the server that stream 49 does not exist
> spice-channel.c:2913 test cap 4 in 0x1052: yes report_invalid_stream: notify
> the server that stream 49 does not exist
> 
> GSpice-CRITICAL **: display_handle_stream_data: assertion 'st != NULL' failed
> decode-glz.c:352 decode_header: 1920x946, id 51, ref 49
> decode-glz.c:352 decode_header: 1920x946, id 52, ref 50
> decode-glz.c:352 decode_header: 1920x946, id 53, ref 51
> decode-glz.c:352 decode_header: 1920x946, id 54, ref 52
> decode-glz.c:352 decode_header: 1920x946, id 55, ref 53
> 
> Just one critical, which is expected because we might have frames to
> decode while we already remove the decoder.

one critical per created stream, in case of youtube it may try to create more
streams
> 
> > > 
> > > We cannot guarantee that the pipeline will work smooth in all systems,
> > > all possible combinations.
> > 
> > Victor, that is a different and unrelated issue
> 
> Its not, because your patch is trying to make avdec_h264 work because of
> h264parse and that means you are trying to guarantee h264 to succeed if
> this two elements are present
> 
no, I am trying to keep compatibility while not reporting 


> > 
> > > 
> > > IMHO, we should improve debug log to provide the information of what is
> > > lacking on GStreamer point of view instead of trying to guarantee that
> > > it will work.
> > 
> > unrelated
> 
> It is unrelated.
> 
> > 
> > > 
> > > > >  You shouldn't do that in the
> > > > > first place.
> > > > > 
> > > > > My point is to check for decoders only. If you want to check for
> > > > > parsers, this patch of yours will do it solely for hard coded
> > > > > elements.
> > > > 
> > > > This patch is just try to keep working what used to work in the
> > > > previous release and prevent breaking it for the next release (which
> > > > seems to happen soon).
> > > 
> > > My patch should work as well and possibly for other plugins like
> > > avdec_h264 that don't show up due bad filtering.
> > > 
> > 
> > Have you tested that? They are not working (I'm using playbin).
> 
> Have you checked the commit log of that patch?
Where do you mention testing of those plugins


> 
> > We do have the check for the same elements in configure, I really
> > don't understand what is wrong about doing the same check in runtime
> > (considerring TODO and keeping the compatibility with the last
> > release).
> > 
> > Pavel
> 
> What I'm trying to stress is that we are trying to not check *specific*
> plugins related to decode/parse a stream. We don't know what our users
> will be using and we should not enforce them to use avdec_h264 for
> instance.

I am not enforcing anything in this patch... 

>  You are doing this in this patch and that's why I'm against
> it.
Where am I enforcing it?

> 
> I could be wrong that's why I'm waiting for someone else's feedback. I
> stated that we will not agree on this.
> 
> I would take the patch to include parsers if it is agnostic to plugins
> as a workaround but I still don't think is the right way...

No, like in your patch it may lead to incorrect results.

> 
> Cheers,
> 
> 
> > 
> > > > 
> > > > > You would need to do something nicer using GStreamer API
> > > > 
> > > > That is the reason for TODO
> > > > 
> > > > >  to be agnostic
> > > > > of specific parser 

Re: [Spice-devel] [PATCH spice-gtk] gst: Fix detection of h264 stream decoder

[Christophe Fergeau]

On Wed, Jul 19, 2017 at 05:46:51PM +0200, Victor Toso wrote:
> What I'm trying to stress is that we are trying to not check *specific*
> plugins related to decode/parse a stream. We don't know what our users
> will be using and we should not enforce them to use avdec_h264 for
> instance. You are doing this in this patch and that's why I'm against
> it.

Yes, actually, on fedora you won't get avdec_h264 unless you enable RPM
Fusion, and you might prefer to go the Cisco openh264 road instead
( https://ausil.us/wordpress/?p=126 - assuming this is still the
recommended way to go with Workstation these days). So yeah my feeling
would be to avoid hardcoding plugin names, assuming we can fallback
properly if the decoding fails. We could also try to make use of the
'missing-codec' GStreamer infrastructure if we want to get fancy.

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk] gst: Fix detection of h264 stream decoder

[Victor Toso]

Hi,

On Wed, Jul 19, 2017 at 05:27:43PM +0200, Pavel Grunt wrote:
> > Stream decoding will fail, server will know about it and sever should
> > fallback to image compression or a different video codec format for
> > streaming.
>
> millions of criticals in the debug log and some resources wasted
> (Server will keep trying to create streams - it does not know whether
> it is a temporary issue or permanent).

With *host* creating the stream or are you testing something else? :)

With the following diff, I can test an error in the pipeline which
should remove the stream, see:

diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 3f51361..eb6a86b 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -511,6 +511,9 @@ static gboolean spice_gst_decoder_queue_frame(VideoDecoder 
*video_decoder,
return TRUE;
}

+if (video_decoder->codec_type == SPICE_VIDEO_CODEC_TYPE_H264)
+  return FALSE;
+
 if (spice_mmtime_diff(frame->mm_time, decoder->last_mm_time) < 0) {
  SPICE_DEBUG("new-frame-time < last-frame-time (%u < %u):"
   " resetting stream",

channel-display.c:1241 display-2:0: display_handle_stream_create: id 49
channel-display.c:1497 display-2:0: video latency: 350
channel-display.c:1563 display-2:0: destroy_display_stream: id=49 #in-frames=1 
out/in=1.00 #drops-on-receive=0 avg-late-time(ms)=0.00 #drops-on-playback=0
spice-channel.c:2913 test cap 4 in 0x1052: yes report_invalid_stream: notify 
the server that stream 49 does not exist
spice-channel.c:2913 test cap 4 in 0x1052: yes report_invalid_stream: notify 
the server that stream 49 does not exist

GSpice-CRITICAL **: display_handle_stream_data: assertion 'st != NULL' failed
decode-glz.c:352 decode_header: 1920x946, id 51, ref 49
decode-glz.c:352 decode_header: 1920x946, id 52, ref 50
decode-glz.c:352 decode_header: 1920x946, id 53, ref 51
decode-glz.c:352 decode_header: 1920x946, id 54, ref 52
decode-glz.c:352 decode_header: 1920x946, id 55, ref 53

Just one critical, which is expected because we might have frames to
decode while we already remove the decoder.

> >
> > We cannot guarantee that the pipeline will work smooth in all systems,
> > all possible combinations.
>
> Victor, that is a different and unrelated issue

Its not, because your patch is trying to make avdec_h264 work because of
h264parse and that means you are trying to guarantee h264 to succeed if
this two elements are present

> 
> > 
> > IMHO, we should improve debug log to provide the information of what is
> > lacking on GStreamer point of view instead of trying to guarantee that
> > it will work.
> 
> unrelated

It is unrelated.

> 
> > 
> > > >  You shouldn't do that in the
> > > > first place.
> > > > 
> > > > My point is to check for decoders only. If you want to check for
> > > > parsers, this patch of yours will do it solely for hard coded elements.
> > > 
> > > This patch is just try to keep working what used to work in the
> > > previous release and prevent breaking it for the next release (which
> > > seems to happen soon).
> > 
> > My patch should work as well and possibly for other plugins like
> > avdec_h264 that don't show up due bad filtering.
> > 
> Have you tested that? They are not working (I'm using playbin).

Have you checked the commit log of that patch?

> We do have the check for the same elements in configure, I really
> don't understand what is wrong about doing the same check in runtime
> (considerring TODO and keeping the compatibility with the last
> release).
>
> Pavel

What I'm trying to stress is that we are trying to not check *specific*
plugins related to decode/parse a stream. We don't know what our users
will be using and we should not enforce them to use avdec_h264 for
instance. You are doing this in this patch and that's why I'm against
it.

I could be wrong that's why I'm waiting for someone else's feedback. I
stated that we will not agree on this.

I would take the patch to include parsers if it is agnostic to plugins
as a workaround but I still don't think is the right way...

Cheers,


> 
> > > 
> > > > You would need to do something nicer using GStreamer API
> > > 
> > > That is the reason for TODO
> > > 
> > > >  to be agnostic
> > > > of specific parser elements.
> > > > 
> > > > And yet, we could miss something else now or in one year.
> > > > 
> > > > Yes, my take is to check for decoders and avoid this workarounds.
> > > > 
> > > > > 
> > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > I do think the right approach is as I mentioned before, fix
> > > > > > > > > > the
> > > > > > > > > > filtering to show all decoders for h264 and that's it. If 
> > > > > > > > > > the
> > > > > > > > > > pipeline
> > > > > > > > > > fails, that's fine because it is *hard* to *ensure* that 
> > > > > > > > > > it'll
> > > > > > > > > > work
> > > > > > > > > > anyway without an actual video stream.
> > > > > > > > > > 
> > > 

Re: [Spice-devel] Consult:how to install Spice in Openstack?

[Christophe Fergeau]

Hey,

On Thu, Jul 13, 2017 at 02:11:46PM +0800, 李比 wrote:
> Hello guys!Sorry to interrupt you.I want to remote access to VM through
> spice protocol instead of RDP protocol .I have read the offical website
> about how to install SPICE throuth virt-manager or libvirt or QEMU.But the
> content doesn`t refer to how to integrate SPICE with Openstack.Do you know
> how to install Spice protocol in Openstack?

This needs work on the openstack side if you want to be able to access
one of its VM using remote-viewer/virt-viewer or virt-manager. 
This question comes up once in a while, I know of
https://github.com/vladikr/spicetcpproxy which was working towards that,
but I don't think this ever landed in openstack upstream :-(

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk] gst: Fix detection of h264 stream decoder

[Pavel Grunt]

On Wed, 2017-07-19 at 16:43 +0200, Victor Toso wrote:
> Hi,
> 
> On Wed, Jul 19, 2017 at 04:25:19PM +0200, Pavel Grunt wrote:
> > On Wed, 2017-07-19 at 16:15 +0200, Victor Toso wrote:
> > > Hi,
> > > 
> > > On Wed, Jul 19, 2017 at 04:01:21PM +0200, Pavel Grunt wrote:
> > > > On Wed, 2017-07-19 at 15:52 +0200, Victor Toso wrote:
> > > > > On Wed, Jul 19, 2017 at 03:46:36PM +0200, Pavel Grunt wrote:
> > > > > > On Wed, 2017-07-19 at 15:40 +0200, Victor Toso wrote:
> > > > > > > On Wed, Jul 19, 2017 at 03:30:49PM +0200, Pavel Grunt wrote:
> > > > > > > > On Wed, 2017-07-19 at 15:23 +0200, Victor Toso wrote:
> > > > > > > > > Hi,
> > > > > > > > > 
> > > > > > > > > On Wed, Jul 19, 2017 at 02:59:00PM +0200, Pavel Grunt wrote:
> > > > > > > > > > GStreamer's avdec_h264 needs h264parse to be able to process
> > > > > > > > > > H264
> > > > > > > > > > video
> > > > > > > > > > streams. However the check for elements through GstRegistry
> > > > > > > > > > does
> > > > > > > > > > not
> > > > > > > > > > take into account the possible pipeline of elements (like
> > > > > > > > > > "h264parse
> > > > > > > > > > !
> > > > > > > > > > avdec_h264").
> > > > > > > > > > 
> > > > > > > > > > Fix that by checking for the elements by their name.
> > > > > > > > > 
> > > > > > > > > As I mentioned before, given the extra complexity that this is
> > > > > > > > > requiring, I'm not convinced that it is a good fix because it
> > > > > > > > > relies
> > > > > > > > > on
> > > > > > > > > the fact that we know the elements that we need (hard coded).
> > > > > > > > > 
> > > > > > > > > 1-) An user might have a different subset of plugins, like the
> > > > > > > > > fluendo
> > > > > > > > > ones or a different 3rd party ones. Do that work when we
> > > > > > > > > check
> > > > > > > > > for
> > > > > > > > > h264parse avdec_h264? I don't think so as it should be
> > > > > > > > > different
> > > > > > > > > plugins (one that people pay to have in their system)
> > > > > > > > 
> > > > > > > > This checks for presence of all the requirred plugins, for both
> > > > > > > > h264parse and avdec_h264. the user must have both to detect it.
> > > > > > > 
> > > > > > > I can have h264 stream decoded without avdec_h264 -> Not required
> > > > > > > 
> > > > > > > I _might_ _still_ have failures due lack of plugins on runtime
> > > > > > > even
> > > > > > > with
> > > > > > > h264parse -> Not enough
> > > > > > > 
> > > > > > > > It fixes the "regression" caused by the GstRegistry patch by
> > > > > > > > checking
> > > > > > > > for
> > > > > > > > the
> > > > > > > > elements like it was done before (but without parsing the full
> > > > > > > > pipeline)
> > > > > > > 
> > > > > > > It does fix for this case. I would prefer to be agnostic about
> > > > > > > plugins,
> > > > > > > really.
> > > > > > > 
> > > > > > > > > 
> > > > > > > > > 2-) If playbin just needs an extra plugin, we don't check it
> > > > > > > > > and
> > > > > > > > > that
> > > > > > > > > means the pipeline will fail even if the caps were
> > > > > > > > > enabled.
> > > > > > > > 
> > > > > > > > It is about the plugin combinations already tested in the past.
> > > > > > > > We
> > > > > > > > have
> > > > > > > > their
> > > > > > > > name (and their usage in a pipeline) in the header file
> > > > > > > 
> > > > > > > That's correct but it doesn't mean that with playbin it'll be
> > > > > > > always
> > > > > > > correct as we are trying to increase the possibilities (i.e not
> > > > > > > depend
> > > > > > > solely on avdec_h264)
> > > > > > > 
> > > > > > > I don't think we will agree because you want to check for
> > > > > > > h264parse
> > > > > > > and
> > > > > > > that's one thing I want to avoid.
> > > > > > > 
> > > > > > 
> > > > > > I do the check for the plugins defined in the header, if you remove
> > > > > > h264parse
> > > > > > from the header, it won't check for it
> > > > > 
> > > > > Which would mean that [0] is much simpler, no?
> > > > > 
> > > > > [0] https://lists.freedesktop.org/archives/spice-devel/2017-July/03863
> > > > > 4.ht
> > > > > ml
> > > > > 
> > > > 
> > > > It is simple, because it is incorrect. Remove h264parse and vaapi
> > > > plugins
> > > > and
> > > > keep avdec_h264, then try to connect to h264 streaming vm...
> > > 
> > > The pipeline will fail and that's fine.
> > 
> > yeah, but server will be sending a stream in h264 because the client
> > incorrectly
> > reported h264 cap. and client will render just a black screen, imho very bad
> > UX.
> 
> Stream decoding will fail, server will know about it and sever should
> fallback to image compression or a different video codec format for
> streaming.

millions of criticals in the debug log and some resources wasted (Server will
keep trying to create streams - it does not know whether it is a temporary issue
or permanent).

> 
> We cannot guarantee that the pipeline will work smooth in all systems,
> all possible combinations.

Victor, that is a 

Re: [Spice-devel] [vdagent-win PATCH v6 2/5] Initial rewrite of image conversion code

[Christophe Fergeau]

On Wed, Jul 19, 2017 at 08:03:49AM -0400, Frediano Ziglio wrote:
> > 
> > On Wed, Jul 19, 2017 at 12:09:23PM +0200, Christophe de Dinechin wrote:
> > > 
> > > > On 19 Jul 2017, at 11:21, Christophe Fergeau  
> > > > wrote:
> > > > 
> > > > On Wed, Jul 19, 2017 at 10:23:30AM +0200, Christophe de Dinechin wrote:
> > > >> 
> > > >>> On 18 Jul 2017, at 17:28, Christophe Fergeau 
> > > >>> wrote:
> > > >>> 
> > > >>> On Mon, Jul 17, 2017 at 11:01:22AM +0100, Frediano Ziglio wrote:
> > >  Remove CxImage linking.
> > >  Support Windows BMP format.
> > > >>> 
> > > >>> Too bad there is no small/maintained library which would do that for 
> > > >>> us
> > > >>> :-/ From a quick glance, looks ok.
> > > >>> 
> > > >>> 
> > >  
> > >  +static inline size_t compute_dib_stride(unsigned width, unsigned
> > >  bit_count)
> > > >>> 
> > > >>> Can you use full type names, unsigned int?
> > > >> 
> > > >> No. Really, no ;-) Otherwise, for consistency, you should replace ‘int’
> > > >> with ‘signed int’,
> > > > 
> > > > The way I see it, 'signed'/'unsigned' are type modifiers, 'int' is an
> > > > actual type name.
> > > 
> > > Yes. But ‘long’ is not. It is also a modifier. So why allow “long” or
> > > “short" but not “unsigned”?
> > > Or are you also writing “long int” and “short int”?
> > 
> > long/short are enough to make the storage size of the integer obvious,
> > even if you don't know that long means long int.
> > "unsigned" does not make this obvious unless you know that "unsigned"
> > means "unsigned int"
> > 
> 
> Section 6.7.2 of C99 standard specified "unsigned" as type.
> The fact you are not familiar with this is an opinion I don't
> personally share. "long" does not specify a type as "unsigned"
> doesn't.
> 

[...]

> 
> So let's write "long int" for anything. "unsigned" is not less typing,
> it's a type specified by the language.

I never said "unsigned" is not standard compliant, so I don't know why
you keep coming back to that.
I previously said that just because something is standard-compliant does
not mean it's a good idea to do it, [insert your favourite obfuscated C
contest example here].

In this particular case, since you feel strongly about it, feel free to
ignore my comment, but I'll nonetheless keep thinking it makes things
less readable ;)

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk] gst: Fix detection of h264 stream decoder

[Victor Toso]

Hi,

On Wed, Jul 19, 2017 at 04:25:19PM +0200, Pavel Grunt wrote:
> On Wed, 2017-07-19 at 16:15 +0200, Victor Toso wrote:
> > Hi,
> > 
> > On Wed, Jul 19, 2017 at 04:01:21PM +0200, Pavel Grunt wrote:
> > > On Wed, 2017-07-19 at 15:52 +0200, Victor Toso wrote:
> > > > On Wed, Jul 19, 2017 at 03:46:36PM +0200, Pavel Grunt wrote:
> > > > > On Wed, 2017-07-19 at 15:40 +0200, Victor Toso wrote:
> > > > > > On Wed, Jul 19, 2017 at 03:30:49PM +0200, Pavel Grunt wrote:
> > > > > > > On Wed, 2017-07-19 at 15:23 +0200, Victor Toso wrote:
> > > > > > > > Hi,
> > > > > > > > 
> > > > > > > > On Wed, Jul 19, 2017 at 02:59:00PM +0200, Pavel Grunt wrote:
> > > > > > > > > GStreamer's avdec_h264 needs h264parse to be able to process
> > > > > > > > > H264
> > > > > > > > > video
> > > > > > > > > streams. However the check for elements through GstRegistry 
> > > > > > > > > does
> > > > > > > > > not
> > > > > > > > > take into account the possible pipeline of elements (like
> > > > > > > > > "h264parse
> > > > > > > > > !
> > > > > > > > > avdec_h264").
> > > > > > > > > 
> > > > > > > > > Fix that by checking for the elements by their name.
> > > > > > > > 
> > > > > > > > As I mentioned before, given the extra complexity that this is
> > > > > > > > requiring, I'm not convinced that it is a good fix because it
> > > > > > > > relies
> > > > > > > > on
> > > > > > > > the fact that we know the elements that we need (hard coded).
> > > > > > > > 
> > > > > > > > 1-) An user might have a different subset of plugins, like the
> > > > > > > > fluendo
> > > > > > > > ones or a different 3rd party ones. Do that work when we 
> > > > > > > > check
> > > > > > > > for
> > > > > > > > h264parse avdec_h264? I don't think so as it should be
> > > > > > > > different
> > > > > > > > plugins (one that people pay to have in their system)
> > > > > > > 
> > > > > > > This checks for presence of all the requirred plugins, for both
> > > > > > > h264parse and avdec_h264. the user must have both to detect it.
> > > > > > 
> > > > > > I can have h264 stream decoded without avdec_h264 -> Not required
> > > > > > 
> > > > > > I _might_ _still_ have failures due lack of plugins on runtime even
> > > > > > with
> > > > > > h264parse -> Not enough
> > > > > > 
> > > > > > > It fixes the "regression" caused by the GstRegistry patch by
> > > > > > > checking
> > > > > > > for
> > > > > > > the
> > > > > > > elements like it was done before (but without parsing the full
> > > > > > > pipeline)
> > > > > > 
> > > > > > It does fix for this case. I would prefer to be agnostic about
> > > > > > plugins,
> > > > > > really.
> > > > > > 
> > > > > > > > 
> > > > > > > > 2-) If playbin just needs an extra plugin, we don't check it and
> > > > > > > > that
> > > > > > > > means the pipeline will fail even if the caps were enabled.
> > > > > > > 
> > > > > > > It is about the plugin combinations already tested in the past. We
> > > > > > > have
> > > > > > > their
> > > > > > > name (and their usage in a pipeline) in the header file
> > > > > > 
> > > > > > That's correct but it doesn't mean that with playbin it'll be always
> > > > > > correct as we are trying to increase the possibilities (i.e not 
> > > > > > depend
> > > > > > solely on avdec_h264)
> > > > > > 
> > > > > > I don't think we will agree because you want to check for h264parse
> > > > > > and
> > > > > > that's one thing I want to avoid.
> > > > > > 
> > > > > 
> > > > > I do the check for the plugins defined in the header, if you remove
> > > > > h264parse
> > > > > from the header, it won't check for it
> > > > 
> > > > Which would mean that [0] is much simpler, no?
> > > > 
> > > > [0] 
> > > > https://lists.freedesktop.org/archives/spice-devel/2017-July/038634.ht
> > > > ml
> > > > 
> > > 
> > > It is simple, because it is incorrect. Remove h264parse and vaapi plugins
> > > and
> > > keep avdec_h264, then try to connect to h264 streaming vm...
> > 
> > The pipeline will fail and that's fine.
> yeah, but server will be sending a stream in h264 because the client 
> incorrectly
> reported h264 cap. and client will render just a black screen, imho very bad 
> UX.

Stream decoding will fail, server will know about it and sever should
fallback to image compression or a different video codec format for
streaming.

We cannot guarantee that the pipeline will work smooth in all systems,
all possible combinations.

IMHO, we should improve debug log to provide the information of what is
lacking on GStreamer point of view instead of trying to guarantee that
it will work.

> >  You shouldn't do that in the
> > first place.
>
> >
> > My point is to check for decoders only. If you want to check for
> > parsers, this patch of yours will do it solely for hard coded elements.
>
> This patch is just try to keep working what used to work in the
> previous release and prevent breaking it for the next release (which
> seems to happen soon).

My patch should work as well and possibly for 

Re: [Spice-devel] [PATCH spice-gtk] gst: Fix detection of h264 stream decoder

[Pavel Grunt]

On Wed, 2017-07-19 at 16:15 +0200, Victor Toso wrote:
> Hi,
> 
> On Wed, Jul 19, 2017 at 04:01:21PM +0200, Pavel Grunt wrote:
> > On Wed, 2017-07-19 at 15:52 +0200, Victor Toso wrote:
> > > On Wed, Jul 19, 2017 at 03:46:36PM +0200, Pavel Grunt wrote:
> > > > On Wed, 2017-07-19 at 15:40 +0200, Victor Toso wrote:
> > > > > On Wed, Jul 19, 2017 at 03:30:49PM +0200, Pavel Grunt wrote:
> > > > > > On Wed, 2017-07-19 at 15:23 +0200, Victor Toso wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > On Wed, Jul 19, 2017 at 02:59:00PM +0200, Pavel Grunt wrote:
> > > > > > > > GStreamer's avdec_h264 needs h264parse to be able to process
> > > > > > > > H264
> > > > > > > > video
> > > > > > > > streams. However the check for elements through GstRegistry does
> > > > > > > > not
> > > > > > > > take into account the possible pipeline of elements (like
> > > > > > > > "h264parse
> > > > > > > > !
> > > > > > > > avdec_h264").
> > > > > > > > 
> > > > > > > > Fix that by checking for the elements by their name.
> > > > > > > 
> > > > > > > As I mentioned before, given the extra complexity that this is
> > > > > > > requiring, I'm not convinced that it is a good fix because it
> > > > > > > relies
> > > > > > > on
> > > > > > > the fact that we know the elements that we need (hard coded).
> > > > > > > 
> > > > > > > 1-) An user might have a different subset of plugins, like the
> > > > > > > fluendo
> > > > > > > ones or a different 3rd party ones. Do that work when we check
> > > > > > > for
> > > > > > > h264parse avdec_h264? I don't think so as it should be
> > > > > > > different
> > > > > > > plugins (one that people pay to have in their system)
> > > > > > 
> > > > > > This checks for presence of all the requirred plugins, for both
> > > > > > h264parse and avdec_h264. the user must have both to detect it.
> > > > > 
> > > > > I can have h264 stream decoded without avdec_h264 -> Not required
> > > > > 
> > > > > I _might_ _still_ have failures due lack of plugins on runtime even
> > > > > with
> > > > > h264parse -> Not enough
> > > > > 
> > > > > > It fixes the "regression" caused by the GstRegistry patch by
> > > > > > checking
> > > > > > for
> > > > > > the
> > > > > > elements like it was done before (but without parsing the full
> > > > > > pipeline)
> > > > > 
> > > > > It does fix for this case. I would prefer to be agnostic about
> > > > > plugins,
> > > > > really.
> > > > > 
> > > > > > > 
> > > > > > > 2-) If playbin just needs an extra plugin, we don't check it and
> > > > > > > that
> > > > > > > means the pipeline will fail even if the caps were enabled.
> > > > > > 
> > > > > > It is about the plugin combinations already tested in the past. We
> > > > > > have
> > > > > > their
> > > > > > name (and their usage in a pipeline) in the header file
> > > > > 
> > > > > That's correct but it doesn't mean that with playbin it'll be always
> > > > > correct as we are trying to increase the possibilities (i.e not depend
> > > > > solely on avdec_h264)
> > > > > 
> > > > > I don't think we will agree because you want to check for h264parse
> > > > > and
> > > > > that's one thing I want to avoid.
> > > > > 
> > > > 
> > > > I do the check for the plugins defined in the header, if you remove
> > > > h264parse
> > > > from the header, it won't check for it
> > > 
> > > Which would mean that [0] is much simpler, no?
> > > 
> > > [0] https://lists.freedesktop.org/archives/spice-devel/2017-July/038634.ht
> > > ml
> > > 
> > 
> > It is simple, because it is incorrect. Remove h264parse and vaapi plugins
> > and
> > keep avdec_h264, then try to connect to h264 streaming vm...
> 
> The pipeline will fail and that's fine.
yeah, but server will be sending a stream in h264 because the client incorrectly
reported h264 cap. and client will render just a black screen, imho very bad UX.

>  You shouldn't do that in the
> first place.


> 
> My point is to check for decoders only. If you want to check for
> parsers, this patch of yours will do it solely for hard coded elements.

This patch is just try to keep working what used to work in the previous release
and prevent breaking it for the next release (which seems to happen soon).

> You would need to do something nicer using GStreamer API

That is the reason for TODO

>  to be agnostic
> of specific parser elements.
> 
> And yet, we could miss something else now or in one year.
> 
> Yes, my take is to check for decoders and avoid this workarounds.
> 
> > 
> > 
> > > > 
> > > > 
> > > > > > > 
> > > > > > > I do think the right approach is as I mentioned before, fix the
> > > > > > > filtering to show all decoders for h264 and that's it. If the
> > > > > > > pipeline
> > > > > > > fails, that's fine because it is *hard* to *ensure* that it'll
> > > > > > > work
> > > > > > > anyway without an actual video stream.
> > > > > > > 
> > > > > > > > ---
> > > > > > > >  src/channel-display-gst.c | 25 +
> > > > > > > >  1 file changed, 25 

Re: [Spice-devel] [PATCH spice-gtk] gst: Fix detection of h264 stream decoder

[Victor Toso]

Hi,

On Wed, Jul 19, 2017 at 04:01:21PM +0200, Pavel Grunt wrote:
> On Wed, 2017-07-19 at 15:52 +0200, Victor Toso wrote:
> > On Wed, Jul 19, 2017 at 03:46:36PM +0200, Pavel Grunt wrote:
> > > On Wed, 2017-07-19 at 15:40 +0200, Victor Toso wrote:
> > > > On Wed, Jul 19, 2017 at 03:30:49PM +0200, Pavel Grunt wrote:
> > > > > On Wed, 2017-07-19 at 15:23 +0200, Victor Toso wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > On Wed, Jul 19, 2017 at 02:59:00PM +0200, Pavel Grunt wrote:
> > > > > > > GStreamer's avdec_h264 needs h264parse to be able to process H264
> > > > > > > video
> > > > > > > streams. However the check for elements through GstRegistry does 
> > > > > > > not
> > > > > > > take into account the possible pipeline of elements (like 
> > > > > > > "h264parse
> > > > > > > !
> > > > > > > avdec_h264").
> > > > > > > 
> > > > > > > Fix that by checking for the elements by their name.
> > > > > > 
> > > > > > As I mentioned before, given the extra complexity that this is
> > > > > > requiring, I'm not convinced that it is a good fix because it relies
> > > > > > on
> > > > > > the fact that we know the elements that we need (hard coded).
> > > > > > 
> > > > > > 1-) An user might have a different subset of plugins, like the 
> > > > > > fluendo
> > > > > > ones or a different 3rd party ones. Do that work when we check 
> > > > > > for
> > > > > > h264parse avdec_h264? I don't think so as it should be different
> > > > > > plugins (one that people pay to have in their system)
> > > > > 
> > > > > This checks for presence of all the requirred plugins, for both
> > > > > h264parse and avdec_h264. the user must have both to detect it.
> > > > 
> > > > I can have h264 stream decoded without avdec_h264 -> Not required
> > > > 
> > > > I _might_ _still_ have failures due lack of plugins on runtime even with
> > > > h264parse -> Not enough
> > > > 
> > > > > It fixes the "regression" caused by the GstRegistry patch by checking
> > > > > for
> > > > > the
> > > > > elements like it was done before (but without parsing the full 
> > > > > pipeline)
> > > > 
> > > > It does fix for this case. I would prefer to be agnostic about plugins,
> > > > really.
> > > > 
> > > > > > 
> > > > > > 2-) If playbin just needs an extra plugin, we don't check it and 
> > > > > > that
> > > > > > means the pipeline will fail even if the caps were enabled.
> > > > > 
> > > > > It is about the plugin combinations already tested in the past. We 
> > > > > have
> > > > > their
> > > > > name (and their usage in a pipeline) in the header file
> > > > 
> > > > That's correct but it doesn't mean that with playbin it'll be always
> > > > correct as we are trying to increase the possibilities (i.e not depend
> > > > solely on avdec_h264)
> > > > 
> > > > I don't think we will agree because you want to check for h264parse and
> > > > that's one thing I want to avoid.
> > > > 
> > > 
> > > I do the check for the plugins defined in the header, if you remove
> > > h264parse
> > > from the header, it won't check for it
> > 
> > Which would mean that [0] is much simpler, no?
> > 
> > [0] https://lists.freedesktop.org/archives/spice-devel/2017-July/038634.html
> > 
> It is simple, because it is incorrect. Remove h264parse and vaapi plugins and
> keep avdec_h264, then try to connect to h264 streaming vm...

The pipeline will fail and that's fine. You shouldn't do that in the
first place.

My point is to check for decoders only. If you want to check for
parsers, this patch of yours will do it solely for hard coded elements.
You would need to do something nicer using GStreamer API to be agnostic
of specific parser elements.

And yet, we could miss something else now or in one year.

Yes, my take is to check for decoders and avoid this workarounds.

>
>
> > > 
> > > 
> > > > > > 
> > > > > > I do think the right approach is as I mentioned before, fix the
> > > > > > filtering to show all decoders for h264 and that's it. If the 
> > > > > > pipeline
> > > > > > fails, that's fine because it is *hard* to *ensure* that it'll work
> > > > > > anyway without an actual video stream.
> > > > > > 
> > > > > > > ---
> > > > > > >  src/channel-display-gst.c | 25 +
> > > > > > >  1 file changed, 25 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > > > > > > index 5042fc8..a9a044a 100644
> > > > > > > --- a/src/channel-display-gst.c
> > > > > > > +++ b/src/channel-display-gst.c
> > > > > > > @@ -666,6 +666,31 @@ gboolean gstvideo_has_codec(int codec_type)
> > > > > > >  codec_decoders = 
> > > > > > > gst_element_factory_list_filter(all_decoders,
> > > > > > > caps,
> > > > > > > GST_PAD_SINK, TRUE);
> > > > > > >  gst_caps_unref(caps);
> > > > > > > 
> > > > > > > +/* Check for the presence of decoding elements that could be
> > > > > > > filter
> > > > > > > out.
> > > > > > > + * TODO: Improve filtering to avoid this...
> > > > > > > +

Re: [Spice-devel] [PATCH spice-gtk] gst: Fix detection of h264 stream decoder

[Pavel Grunt]

On Wed, 2017-07-19 at 15:52 +0200, Victor Toso wrote:
> On Wed, Jul 19, 2017 at 03:46:36PM +0200, Pavel Grunt wrote:
> > On Wed, 2017-07-19 at 15:40 +0200, Victor Toso wrote:
> > > On Wed, Jul 19, 2017 at 03:30:49PM +0200, Pavel Grunt wrote:
> > > > On Wed, 2017-07-19 at 15:23 +0200, Victor Toso wrote:
> > > > > Hi,
> > > > > 
> > > > > On Wed, Jul 19, 2017 at 02:59:00PM +0200, Pavel Grunt wrote:
> > > > > > GStreamer's avdec_h264 needs h264parse to be able to process H264
> > > > > > video
> > > > > > streams. However the check for elements through GstRegistry does not
> > > > > > take into account the possible pipeline of elements (like "h264parse
> > > > > > !
> > > > > > avdec_h264").
> > > > > > 
> > > > > > Fix that by checking for the elements by their name.
> > > > > 
> > > > > As I mentioned before, given the extra complexity that this is
> > > > > requiring, I'm not convinced that it is a good fix because it relies
> > > > > on
> > > > > the fact that we know the elements that we need (hard coded).
> > > > > 
> > > > > 1-) An user might have a different subset of plugins, like the fluendo
> > > > > ones or a different 3rd party ones. Do that work when we check for
> > > > > h264parse avdec_h264? I don't think so as it should be different
> > > > > plugins (one that people pay to have in their system)
> > > > 
> > > > This checks for presence of all the requirred plugins, for both
> > > > h264parse and avdec_h264. the user must have both to detect it.
> > > 
> > > I can have h264 stream decoded without avdec_h264 -> Not required
> > > 
> > > I _might_ _still_ have failures due lack of plugins on runtime even with
> > > h264parse -> Not enough
> > > 
> > > > It fixes the "regression" caused by the GstRegistry patch by checking
> > > > for
> > > > the
> > > > elements like it was done before (but without parsing the full pipeline)
> > > 
> > > It does fix for this case. I would prefer to be agnostic about plugins,
> > > really.
> > > 
> > > > > 
> > > > > 2-) If playbin just needs an extra plugin, we don't check it and that
> > > > > means the pipeline will fail even if the caps were enabled.
> > > > 
> > > > It is about the plugin combinations already tested in the past. We have
> > > > their
> > > > name (and their usage in a pipeline) in the header file
> > > 
> > > That's correct but it doesn't mean that with playbin it'll be always
> > > correct as we are trying to increase the possibilities (i.e not depend
> > > solely on avdec_h264)
> > > 
> > > I don't think we will agree because you want to check for h264parse and
> > > that's one thing I want to avoid.
> > > 
> > 
> > I do the check for the plugins defined in the header, if you remove
> > h264parse
> > from the header, it won't check for it
> 
> Which would mean that [0] is much simpler, no?
> 
> [0] https://lists.freedesktop.org/archives/spice-devel/2017-July/038634.html
> 
It is simple, because it is incorrect. Remove h264parse and vaapi plugins and
keep avdec_h264, then try to connect to h264 streaming vm...


> > 
> > 
> > > > > 
> > > > > I do think the right approach is as I mentioned before, fix the
> > > > > filtering to show all decoders for h264 and that's it. If the pipeline
> > > > > fails, that's fine because it is *hard* to *ensure* that it'll work
> > > > > anyway without an actual video stream.
> > > > > 
> > > > > > ---
> > > > > >  src/channel-display-gst.c | 25 +
> > > > > >  1 file changed, 25 insertions(+)
> > > > > > 
> > > > > > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > > > > > index 5042fc8..a9a044a 100644
> > > > > > --- a/src/channel-display-gst.c
> > > > > > +++ b/src/channel-display-gst.c
> > > > > > @@ -666,6 +666,31 @@ gboolean gstvideo_has_codec(int codec_type)
> > > > > >  codec_decoders = gst_element_factory_list_filter(all_decoders,
> > > > > > caps,
> > > > > > GST_PAD_SINK, TRUE);
> > > > > >  gst_caps_unref(caps);
> > > > > > 
> > > > > > +/* Check for the presence of decoding elements that could be
> > > > > > filter
> > > > > > out.
> > > > > > + * TODO: Improve filtering to avoid this...
> > > > > > + */
> > > > > > +{
> > > > > > +GList *decoder_elements = NULL;
> > > > > > +gchar **decoders_by_names;
> > > > > > +guint i = 0;
> > > > > > +decoders_by_names =
> > > > > > g_strsplit(gst_opts[codec_type].dec_name,
> > > > > > "!",
> > > > > > -1);
> > > > > > +for (i = 0; decoders_by_names[i] != NULL; i++) {
> > > > > > +GstElementFactory *element =
> > > > > > gst_element_factory_find(g_strstrip(decoders_by_names[i]));
> > > > > > +if (element == NULL) {
> > > > > > +gst_plugin_feature_list_free(decoder_elements);
> > > > > > +decoder_elements = NULL;
> > > > > > +break;
> > > > > > +}
> > > > > > +if (g_list_find(codec_decoders, element)) {
> > > > > > +

Re: [Spice-devel] [PATCH spice-gtk] gst: Fix detection of h264 stream decoder

[Victor Toso]

On Wed, Jul 19, 2017 at 03:46:36PM +0200, Pavel Grunt wrote:
> On Wed, 2017-07-19 at 15:40 +0200, Victor Toso wrote:
> > On Wed, Jul 19, 2017 at 03:30:49PM +0200, Pavel Grunt wrote:
> > > On Wed, 2017-07-19 at 15:23 +0200, Victor Toso wrote:
> > > > Hi,
> > > > 
> > > > On Wed, Jul 19, 2017 at 02:59:00PM +0200, Pavel Grunt wrote:
> > > > > GStreamer's avdec_h264 needs h264parse to be able to process H264 
> > > > > video
> > > > > streams. However the check for elements through GstRegistry does not
> > > > > take into account the possible pipeline of elements (like "h264parse !
> > > > > avdec_h264").
> > > > > 
> > > > > Fix that by checking for the elements by their name.
> > > > 
> > > > As I mentioned before, given the extra complexity that this is
> > > > requiring, I'm not convinced that it is a good fix because it relies on
> > > > the fact that we know the elements that we need (hard coded).
> > > > 
> > > > 1-) An user might have a different subset of plugins, like the fluendo
> > > > ones or a different 3rd party ones. Do that work when we check for
> > > > h264parse avdec_h264? I don't think so as it should be different
> > > > plugins (one that people pay to have in their system)
> > > 
> > > This checks for presence of all the requirred plugins, for both
> > > h264parse and avdec_h264. the user must have both to detect it.
> > 
> > I can have h264 stream decoded without avdec_h264 -> Not required
> > 
> > I _might_ _still_ have failures due lack of plugins on runtime even with
> > h264parse -> Not enough
> > 
> > > It fixes the "regression" caused by the GstRegistry patch by checking for
> > > the
> > > elements like it was done before (but without parsing the full pipeline)
> > 
> > It does fix for this case. I would prefer to be agnostic about plugins,
> > really.
> > 
> > > > 
> > > > 2-) If playbin just needs an extra plugin, we don't check it and that
> > > > means the pipeline will fail even if the caps were enabled.
> > > 
> > > It is about the plugin combinations already tested in the past. We have
> > > their
> > > name (and their usage in a pipeline) in the header file
> > 
> > That's correct but it doesn't mean that with playbin it'll be always
> > correct as we are trying to increase the possibilities (i.e not depend
> > solely on avdec_h264)
> > 
> > I don't think we will agree because you want to check for h264parse and
> > that's one thing I want to avoid.
> > 
> I do the check for the plugins defined in the header, if you remove h264parse
> from the header, it won't check for it

Which would mean that [0] is much simpler, no?

[0] https://lists.freedesktop.org/archives/spice-devel/2017-July/038634.html

> 
> 
> > > > 
> > > > I do think the right approach is as I mentioned before, fix the
> > > > filtering to show all decoders for h264 and that's it. If the pipeline
> > > > fails, that's fine because it is *hard* to *ensure* that it'll work
> > > > anyway without an actual video stream.
> > > > 
> > > > > ---
> > > > >  src/channel-display-gst.c | 25 +
> > > > >  1 file changed, 25 insertions(+)
> > > > > 
> > > > > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > > > > index 5042fc8..a9a044a 100644
> > > > > --- a/src/channel-display-gst.c
> > > > > +++ b/src/channel-display-gst.c
> > > > > @@ -666,6 +666,31 @@ gboolean gstvideo_has_codec(int codec_type)
> > > > >  codec_decoders = gst_element_factory_list_filter(all_decoders,
> > > > > caps,
> > > > > GST_PAD_SINK, TRUE);
> > > > >  gst_caps_unref(caps);
> > > > > 
> > > > > +/* Check for the presence of decoding elements that could be 
> > > > > filter
> > > > > out.
> > > > > + * TODO: Improve filtering to avoid this...
> > > > > + */
> > > > > +{
> > > > > +GList *decoder_elements = NULL;
> > > > > +gchar **decoders_by_names;
> > > > > +guint i = 0;
> > > > > +decoders_by_names = g_strsplit(gst_opts[codec_type].dec_name,
> > > > > "!",
> > > > > -1);
> > > > > +for (i = 0; decoders_by_names[i] != NULL; i++) {
> > > > > +GstElementFactory *element =
> > > > > gst_element_factory_find(g_strstrip(decoders_by_names[i]));
> > > > > +if (element == NULL) {
> > > > > +gst_plugin_feature_list_free(decoder_elements);
> > > > > +decoder_elements = NULL;
> > > > > +break;
> > > > > +}
> > > > > +if (g_list_find(codec_decoders, element)) {
> > > > > +gst_object_unref(element);
> > > > > +} else {
> > > > > +decoder_elements = g_list_append(decoder_elements,
> > > > > element);
> > > > > +}
> > > > > +}
> > > > > +codec_decoders = g_list_concat(codec_decoders,
> > > > > decoder_elements);
> > > > > +g_strfreev(decoders_by_names);
> > > > > +}
> > > > > +
> > > > >  if (codec_decoders == NULL) {
> > > > >  spice_debug("From 

Re: [Spice-devel] [PATCH spice-gtk] gst: Fix detection of h264 stream decoder

[Pavel Grunt]

On Wed, 2017-07-19 at 15:40 +0200, Victor Toso wrote:
> On Wed, Jul 19, 2017 at 03:30:49PM +0200, Pavel Grunt wrote:
> > On Wed, 2017-07-19 at 15:23 +0200, Victor Toso wrote:
> > > Hi,
> > > 
> > > On Wed, Jul 19, 2017 at 02:59:00PM +0200, Pavel Grunt wrote:
> > > > GStreamer's avdec_h264 needs h264parse to be able to process H264 video
> > > > streams. However the check for elements through GstRegistry does not
> > > > take into account the possible pipeline of elements (like "h264parse !
> > > > avdec_h264").
> > > > 
> > > > Fix that by checking for the elements by their name.
> > > 
> > > As I mentioned before, given the extra complexity that this is
> > > requiring, I'm not convinced that it is a good fix because it relies on
> > > the fact that we know the elements that we need (hard coded).
> > > 
> > > 1-) An user might have a different subset of plugins, like the fluendo
> > > ones or a different 3rd party ones. Do that work when we check for
> > > h264parse avdec_h264? I don't think so as it should be different
> > > plugins (one that people pay to have in their system)
> > 
> > This checks for presence of all the requirred plugins, for both
> > h264parse and avdec_h264. the user must have both to detect it.
> 
> I can have h264 stream decoded without avdec_h264 -> Not required
> 
> I _might_ _still_ have failures due lack of plugins on runtime even with
> h264parse -> Not enough
> 
> > It fixes the "regression" caused by the GstRegistry patch by checking for
> > the
> > elements like it was done before (but without parsing the full pipeline)
> 
> It does fix for this case. I would prefer to be agnostic about plugins,
> really.
> 
> > > 
> > > 2-) If playbin just needs an extra plugin, we don't check it and that
> > > means the pipeline will fail even if the caps were enabled.
> > 
> > It is about the plugin combinations already tested in the past. We have
> > their
> > name (and their usage in a pipeline) in the header file
> 
> That's correct but it doesn't mean that with playbin it'll be always
> correct as we are trying to increase the possibilities (i.e not depend
> solely on avdec_h264)
> 
> I don't think we will agree because you want to check for h264parse and
> that's one thing I want to avoid.
> 
I do the check for the plugins defined in the header, if you remove h264parse
from the header, it won't check for it


> > > 
> > > I do think the right approach is as I mentioned before, fix the
> > > filtering to show all decoders for h264 and that's it. If the pipeline
> > > fails, that's fine because it is *hard* to *ensure* that it'll work
> > > anyway without an actual video stream.
> > > 
> > > > ---
> > > >  src/channel-display-gst.c | 25 +
> > > >  1 file changed, 25 insertions(+)
> > > > 
> > > > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > > > index 5042fc8..a9a044a 100644
> > > > --- a/src/channel-display-gst.c
> > > > +++ b/src/channel-display-gst.c
> > > > @@ -666,6 +666,31 @@ gboolean gstvideo_has_codec(int codec_type)
> > > >  codec_decoders = gst_element_factory_list_filter(all_decoders,
> > > > caps,
> > > > GST_PAD_SINK, TRUE);
> > > >  gst_caps_unref(caps);
> > > > 
> > > > +/* Check for the presence of decoding elements that could be filter
> > > > out.
> > > > + * TODO: Improve filtering to avoid this...
> > > > + */
> > > > +{
> > > > +GList *decoder_elements = NULL;
> > > > +gchar **decoders_by_names;
> > > > +guint i = 0;
> > > > +decoders_by_names = g_strsplit(gst_opts[codec_type].dec_name,
> > > > "!",
> > > > -1);
> > > > +for (i = 0; decoders_by_names[i] != NULL; i++) {
> > > > +GstElementFactory *element =
> > > > gst_element_factory_find(g_strstrip(decoders_by_names[i]));
> > > > +if (element == NULL) {
> > > > +gst_plugin_feature_list_free(decoder_elements);
> > > > +decoder_elements = NULL;
> > > > +break;
> > > > +}
> > > > +if (g_list_find(codec_decoders, element)) {
> > > > +gst_object_unref(element);
> > > > +} else {
> > > > +decoder_elements = g_list_append(decoder_elements,
> > > > element);
> > > > +}
> > > > +}
> > > > +codec_decoders = g_list_concat(codec_decoders,
> > > > decoder_elements);
> > > > +g_strfreev(decoders_by_names);
> > > > +}
> > > > +
> > > >  if (codec_decoders == NULL) {
> > > >  spice_debug("From %u decoders, none can handle '%s'",
> > > >  g_list_length(all_decoders),
> > > > gst_opts[codec_type].dec_caps);
> > > > -- 
> > > > 2.13.3
> > > > 
> > > > ___
> > > > Spice-devel mailing list
> > > > Spice-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
___
Spice-devel 

Re: [Spice-devel] [PATCH spice-gtk] gst: Fix detection of h264 stream decoder

[Victor Toso]

On Wed, Jul 19, 2017 at 03:30:49PM +0200, Pavel Grunt wrote:
> On Wed, 2017-07-19 at 15:23 +0200, Victor Toso wrote:
> > Hi,
> > 
> > On Wed, Jul 19, 2017 at 02:59:00PM +0200, Pavel Grunt wrote:
> > > GStreamer's avdec_h264 needs h264parse to be able to process H264 video
> > > streams. However the check for elements through GstRegistry does not
> > > take into account the possible pipeline of elements (like "h264parse !
> > > avdec_h264").
> > > 
> > > Fix that by checking for the elements by their name.
> > 
> > As I mentioned before, given the extra complexity that this is
> > requiring, I'm not convinced that it is a good fix because it relies on
> > the fact that we know the elements that we need (hard coded).
> > 
> > 1-) An user might have a different subset of plugins, like the fluendo
> > ones or a different 3rd party ones. Do that work when we check for
> > h264parse avdec_h264? I don't think so as it should be different
> > plugins (one that people pay to have in their system)
>
> This checks for presence of all the requirred plugins, for both
> h264parse and avdec_h264. the user must have both to detect it.

I can have h264 stream decoded without avdec_h264 -> Not required

I _might_ _still_ have failures due lack of plugins on runtime even with
h264parse -> Not enough

> It fixes the "regression" caused by the GstRegistry patch by checking for the
> elements like it was done before (but without parsing the full pipeline)

It does fix for this case. I would prefer to be agnostic about plugins,
really.

> >
> > 2-) If playbin just needs an extra plugin, we don't check it and that
> > means the pipeline will fail even if the caps were enabled.
>
> It is about the plugin combinations already tested in the past. We have their
> name (and their usage in a pipeline) in the header file

That's correct but it doesn't mean that with playbin it'll be always
correct as we are trying to increase the possibilities (i.e not depend
solely on avdec_h264)

I don't think we will agree because you want to check for h264parse and
that's one thing I want to avoid.

> >
> > I do think the right approach is as I mentioned before, fix the
> > filtering to show all decoders for h264 and that's it. If the pipeline
> > fails, that's fine because it is *hard* to *ensure* that it'll work
> > anyway without an actual video stream.
> > 
> > > ---
> > >  src/channel-display-gst.c | 25 +
> > >  1 file changed, 25 insertions(+)
> > > 
> > > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > > index 5042fc8..a9a044a 100644
> > > --- a/src/channel-display-gst.c
> > > +++ b/src/channel-display-gst.c
> > > @@ -666,6 +666,31 @@ gboolean gstvideo_has_codec(int codec_type)
> > >  codec_decoders = gst_element_factory_list_filter(all_decoders, caps,
> > > GST_PAD_SINK, TRUE);
> > >  gst_caps_unref(caps);
> > > 
> > > +/* Check for the presence of decoding elements that could be filter
> > > out.
> > > + * TODO: Improve filtering to avoid this...
> > > + */
> > > +{
> > > +GList *decoder_elements = NULL;
> > > +gchar **decoders_by_names;
> > > +guint i = 0;
> > > +decoders_by_names = g_strsplit(gst_opts[codec_type].dec_name, 
> > > "!",
> > > -1);
> > > +for (i = 0; decoders_by_names[i] != NULL; i++) {
> > > +GstElementFactory *element =
> > > gst_element_factory_find(g_strstrip(decoders_by_names[i]));
> > > +if (element == NULL) {
> > > +gst_plugin_feature_list_free(decoder_elements);
> > > +decoder_elements = NULL;
> > > +break;
> > > +}
> > > +if (g_list_find(codec_decoders, element)) {
> > > +gst_object_unref(element);
> > > +} else {
> > > +decoder_elements = g_list_append(decoder_elements,
> > > element);
> > > +}
> > > +}
> > > +codec_decoders = g_list_concat(codec_decoders, decoder_elements);
> > > +g_strfreev(decoders_by_names);
> > > +}
> > > +
> > >  if (codec_decoders == NULL) {
> > >  spice_debug("From %u decoders, none can handle '%s'",
> > >  g_list_length(all_decoders),
> > > gst_opts[codec_type].dec_caps);
> > > -- 
> > > 2.13.3
> > > 
> > > ___
> > > Spice-devel mailing list
> > > Spice-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk] gst: Fix detection of h264 stream decoder

[Pavel Grunt]

On Wed, 2017-07-19 at 15:23 +0200, Victor Toso wrote:
> Hi,
> 
> On Wed, Jul 19, 2017 at 02:59:00PM +0200, Pavel Grunt wrote:
> > GStreamer's avdec_h264 needs h264parse to be able to process H264 video
> > streams. However the check for elements through GstRegistry does not
> > take into account the possible pipeline of elements (like "h264parse !
> > avdec_h264").
> > 
> > Fix that by checking for the elements by their name.
> 
> As I mentioned before, given the extra complexity that this is
> requiring, I'm not convinced that it is a good fix because it relies on
> the fact that we know the elements that we need (hard coded).
> 
> 1-) An user might have a different subset of plugins, like the fluendo
> ones or a different 3rd party ones. Do that work when we check for
> h264parse avdec_h264? I don't think so as it should be different
> plugins (one that people pay to have in their system)

This checks for presence of all the requirred plugins, for both h264parse and
avdec_h264. the user must have both to detect it.

It fixes the "regression" caused by the GstRegistry patch by checking for the
elements like it was done before (but without parsing the full pipeline)

> 
> 2-) If playbin just needs an extra plugin, we don't check it and that
> means the pipeline will fail even if the caps were enabled.

It is about the plugin combinations already tested in the past. We have their
name (and their usage in a pipeline) in the header file

> 
> I do think the right approach is as I mentioned before, fix the
> filtering to show all decoders for h264 and that's it. If the pipeline
> fails, that's fine because it is *hard* to *ensure* that it'll work
> anyway without an actual video stream.
> 
> > ---
> >  src/channel-display-gst.c | 25 +
> >  1 file changed, 25 insertions(+)
> > 
> > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > index 5042fc8..a9a044a 100644
> > --- a/src/channel-display-gst.c
> > +++ b/src/channel-display-gst.c
> > @@ -666,6 +666,31 @@ gboolean gstvideo_has_codec(int codec_type)
> >  codec_decoders = gst_element_factory_list_filter(all_decoders, caps,
> > GST_PAD_SINK, TRUE);
> >  gst_caps_unref(caps);
> > 
> > +/* Check for the presence of decoding elements that could be filter
> > out.
> > + * TODO: Improve filtering to avoid this...
> > + */
> > +{
> > +GList *decoder_elements = NULL;
> > +gchar **decoders_by_names;
> > +guint i = 0;
> > +decoders_by_names = g_strsplit(gst_opts[codec_type].dec_name, "!",
> > -1);
> > +for (i = 0; decoders_by_names[i] != NULL; i++) {
> > +GstElementFactory *element =
> > gst_element_factory_find(g_strstrip(decoders_by_names[i]));
> > +if (element == NULL) {
> > +gst_plugin_feature_list_free(decoder_elements);
> > +decoder_elements = NULL;
> > +break;
> > +}
> > +if (g_list_find(codec_decoders, element)) {
> > +gst_object_unref(element);
> > +} else {
> > +decoder_elements = g_list_append(decoder_elements,
> > element);
> > +}
> > +}
> > +codec_decoders = g_list_concat(codec_decoders, decoder_elements);
> > +g_strfreev(decoders_by_names);
> > +}
> > +
> >  if (codec_decoders == NULL) {
> >  spice_debug("From %u decoders, none can handle '%s'",
> >  g_list_length(all_decoders),
> > gst_opts[codec_type].dec_caps);
> > -- 
> > 2.13.3
> > 
> > ___
> > 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] gst: Fix detection of h264 stream decoder

[Victor Toso]

Hi,

On Wed, Jul 19, 2017 at 02:59:00PM +0200, Pavel Grunt wrote:
> GStreamer's avdec_h264 needs h264parse to be able to process H264 video
> streams. However the check for elements through GstRegistry does not
> take into account the possible pipeline of elements (like "h264parse ! 
> avdec_h264").
>
> Fix that by checking for the elements by their name.

As I mentioned before, given the extra complexity that this is
requiring, I'm not convinced that it is a good fix because it relies on
the fact that we know the elements that we need (hard coded).

1-) An user might have a different subset of plugins, like the fluendo
ones or a different 3rd party ones. Do that work when we check for
h264parse avdec_h264? I don't think so as it should be different
plugins (one that people pay to have in their system)

2-) If playbin just needs an extra plugin, we don't check it and that
means the pipeline will fail even if the caps were enabled.

I do think the right approach is as I mentioned before, fix the
filtering to show all decoders for h264 and that's it. If the pipeline
fails, that's fine because it is *hard* to *ensure* that it'll work
anyway without an actual video stream.

> ---
>  src/channel-display-gst.c | 25 +
>  1 file changed, 25 insertions(+)
>
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index 5042fc8..a9a044a 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -666,6 +666,31 @@ gboolean gstvideo_has_codec(int codec_type)
>  codec_decoders = gst_element_factory_list_filter(all_decoders, caps, 
> GST_PAD_SINK, TRUE);
>  gst_caps_unref(caps);
>
> +/* Check for the presence of decoding elements that could be filter out.
> + * TODO: Improve filtering to avoid this...
> + */
> +{
> +GList *decoder_elements = NULL;
> +gchar **decoders_by_names;
> +guint i = 0;
> +decoders_by_names = g_strsplit(gst_opts[codec_type].dec_name, "!", 
> -1);
> +for (i = 0; decoders_by_names[i] != NULL; i++) {
> +GstElementFactory *element = 
> gst_element_factory_find(g_strstrip(decoders_by_names[i]));
> +if (element == NULL) {
> +gst_plugin_feature_list_free(decoder_elements);
> +decoder_elements = NULL;
> +break;
> +}
> +if (g_list_find(codec_decoders, element)) {
> +gst_object_unref(element);
> +} else {
> +decoder_elements = g_list_append(decoder_elements, element);
> +}
> +}
> +codec_decoders = g_list_concat(codec_decoders, decoder_elements);
> +g_strfreev(decoders_by_names);
> +}
> +
>  if (codec_decoders == NULL) {
>  spice_debug("From %u decoders, none can handle '%s'",
>  g_list_length(all_decoders), 
> gst_opts[codec_type].dec_caps);
> -- 
> 2.13.3
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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] gst: Fix detection of h264 stream decoder

[Pavel Grunt]

GStreamer's avdec_h264 needs h264parse to be able to process H264 video
streams. However the check for elements through GstRegistry does not
take into account the possible pipeline of elements (like "h264parse ! 
avdec_h264").

Fix that by checking for the elements by their name.
---
 src/channel-display-gst.c | 25 +
 1 file changed, 25 insertions(+)

diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 5042fc8..a9a044a 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -666,6 +666,31 @@ gboolean gstvideo_has_codec(int codec_type)
 codec_decoders = gst_element_factory_list_filter(all_decoders, caps, 
GST_PAD_SINK, TRUE);
 gst_caps_unref(caps);
 
+/* Check for the presence of decoding elements that could be filter out.
+ * TODO: Improve filtering to avoid this...
+ */
+{
+GList *decoder_elements = NULL;
+gchar **decoders_by_names;
+guint i = 0;
+decoders_by_names = g_strsplit(gst_opts[codec_type].dec_name, "!", -1);
+for (i = 0; decoders_by_names[i] != NULL; i++) {
+GstElementFactory *element = 
gst_element_factory_find(g_strstrip(decoders_by_names[i]));
+if (element == NULL) {
+gst_plugin_feature_list_free(decoder_elements);
+decoder_elements = NULL;
+break;
+}
+if (g_list_find(codec_decoders, element)) {
+gst_object_unref(element);
+} else {
+decoder_elements = g_list_append(decoder_elements, element);
+}
+}
+codec_decoders = g_list_concat(codec_decoders, decoder_elements);
+g_strfreev(decoders_by_names);
+}
+
 if (codec_decoders == NULL) {
 spice_debug("From %u decoders, none can handle '%s'",
 g_list_length(all_decoders), 
gst_opts[codec_type].dec_caps);
-- 
2.13.3

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

Re: [Spice-devel] [vdagent-win PATCH v6 2/5] Initial rewrite of image conversion code

[Frediano Ziglio]

> 
> On Wed, Jul 19, 2017 at 12:09:23PM +0200, Christophe de Dinechin wrote:
> > 
> > > On 19 Jul 2017, at 11:21, Christophe Fergeau  wrote:
> > > 
> > > On Wed, Jul 19, 2017 at 10:23:30AM +0200, Christophe de Dinechin wrote:
> > >> 
> > >>> On 18 Jul 2017, at 17:28, Christophe Fergeau 
> > >>> wrote:
> > >>> 
> > >>> On Mon, Jul 17, 2017 at 11:01:22AM +0100, Frediano Ziglio wrote:
> >  Remove CxImage linking.
> >  Support Windows BMP format.
> > >>> 
> > >>> Too bad there is no small/maintained library which would do that for us
> > >>> :-/ From a quick glance, looks ok.
> > >>> 
> > >>> 
> >  
> >  +static inline size_t compute_dib_stride(unsigned width, unsigned
> >  bit_count)
> > >>> 
> > >>> Can you use full type names, unsigned int?
> > >> 
> > >> No. Really, no ;-) Otherwise, for consistency, you should replace ‘int’
> > >> with ‘signed int’,
> > > 
> > > The way I see it, 'signed'/'unsigned' are type modifiers, 'int' is an
> > > actual type name.
> > 
> > Yes. But ‘long’ is not. It is also a modifier. So why allow “long” or
> > “short" but not “unsigned”?
> > Or are you also writing “long int” and “short int”?
> 
> long/short are enough to make the storage size of the integer obvious,
> even if you don't know that long means long int.
> "unsigned" does not make this obvious unless you know that "unsigned"
> means "unsigned int"
> 

Section 6.7.2 of C99 standard specified "unsigned" as type.
The fact you are not familiar with this is an opinion I don't
personally share. "long" does not specify a type as "unsigned"
doesn't.

> > 
> > > Huge difference to me.
> > 
> > No, really not, at least as far as C and usage are concerned. It’s
> > just a personal preference.  So if Frediano prefers to write
> > ‘unsigned’, I think it’s OK, and I will most likely write the same
> > way.
> 
> I'll byte here, in general (not necessarily in this case) "it's just
> personal preference" is not a good argument at all, we want a consistent
> codebase, ideally one which is readable. Just because something is valid with
> the C standard does not mean saying "personal preference" is a good
> justification for using it (and still generally speaking, less typing
> often means something less readable, and you read code more than you
> write it).
> 

So let's write "long int" for anything. "unsigned" is not less typing,
it's a type specified by the language.

> > 
> > 
> > > you cannot guess the range of the values you
> > > can store in there. So let's just be specific.
> > 
> > The language is quite clear that ‘unsigned’ means ‘unsigned int’.
> > There is no guessing involved whatsoever. The guessing about
> > the size is because C is not specific about the size of ‘int’ in bits,
> > which is why we have .
> 
> Yes, the spec is clear about it. The proportion of C coders who will
> know when reading "unsigned" that it means "unsigned int" is what I'm
> questioning here, and why I'm pushing for a more specific type.
> 
> Christophe
> 

I personally have seen tons of "unsigned" reading code.
If some C coders don't know that syntax they can learn it.

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

Re: [Spice-devel] [PATCH spice-gtk] gst-audio: Do not update mmtime without real audio channel

[Victor Toso]

Hi,

On Tue, Jul 18, 2017 at 06:55:12PM +0200, Christophe de Dinechin wrote:
> Interestingly, I just discovered something I did not really expect: a
> hot CPU is more than enough to make the spice client lack “oxygen”,
> even if nothing else is running.
>
> If I start under light load and keep running that way, it can run a
> long time without a problem. The queue length typically runs up and
> down a little, but it does not diverge. See
> http://blackbox.dinechin.org/images/170718-NotDiverging.png.
>
> There is even plenty of CPU left to converge back if we temporarily
> accumulate things in the queue:
> http://blackbox.dinechin.org/images/170718-Reconverge.png.
>
> However, if the machine gets hot, then it does not reconverge, because
> the machine spends over 50% “in the kernel” just trying to cool down.
> Starting the client under these conditions, it rapidly diverges:
> http://blackbox.dinechin.org/images/170718-WithHotCPU.png.

Isn't it expected as the CPU lost 50% of its processing time in order to
cool down? We can think about different ways to workaround this kind of
issue. Dropping the queue as you did in a different patch is a good
approach for peaks like this, I think.

If those situations happens too often it would be a good signal that
current stream is too much for the client to handle and we could lower
quality/fps, increase I-FRAME, etc. in order to make it less demanding
for the client.

Still, it should be much better if you were doing hw decoding, I guess.

> Also, without enough I-frames to recover, when this starts happening,
> we are a bit stuck.  This is all documented in more details here:
> http://blackbox.dinechin.org/170718.html.

Cool charts

> Tons of messages that look like:
>
> channel-display-gst.c:266: [783802 1768.364819] spice_warning: got an
> unexpected decoded buffer!

First time seeing it, cool!

> So I think we will need the mechanism we discussed with Uri, where we
> send something back to tell the server we need an I-frame. Or we
> close/reopen the channel.

Definitely! We should start discussing about it... I think it will
require a new/improved protocol.

Cheers,



signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [vdagent-win PATCH v6 2/5] Initial rewrite of image conversion code

[Christophe Fergeau]

On Wed, Jul 19, 2017 at 12:09:23PM +0200, Christophe de Dinechin wrote:
> 
> > On 19 Jul 2017, at 11:21, Christophe Fergeau  wrote:
> > 
> > On Wed, Jul 19, 2017 at 10:23:30AM +0200, Christophe de Dinechin wrote:
> >> 
> >>> On 18 Jul 2017, at 17:28, Christophe Fergeau  wrote:
> >>> 
> >>> On Mon, Jul 17, 2017 at 11:01:22AM +0100, Frediano Ziglio wrote:
>  Remove CxImage linking.
>  Support Windows BMP format.
> >>> 
> >>> Too bad there is no small/maintained library which would do that for us
> >>> :-/ From a quick glance, looks ok.
> >>> 
> >>> 
>  
>  +static inline size_t compute_dib_stride(unsigned width, unsigned 
>  bit_count)
> >>> 
> >>> Can you use full type names, unsigned int?
> >> 
> >> No. Really, no ;-) Otherwise, for consistency, you should replace ‘int’ 
> >> with ‘signed int’, 
> > 
> > The way I see it, 'signed'/'unsigned' are type modifiers, 'int' is an 
> > actual type name.
> 
> Yes. But ‘long’ is not. It is also a modifier. So why allow “long” or “short" 
> but not “unsigned”?
> Or are you also writing “long int” and “short int”?

long/short are enough to make the storage size of the integer obvious,
even if you don't know that long means long int.
"unsigned" does not make this obvious unless you know that "unsigned"
means "unsigned int"

> 
> > Huge difference to me.
> 
> No, really not, at least as far as C and usage are concerned. It’s
> just a personal preference.  So if Frediano prefers to write
> ‘unsigned’, I think it’s OK, and I will most likely write the same
> way.

I'll byte here, in general (not necessarily in this case) "it's just
personal preference" is not a good argument at all, we want a consistent
codebase, ideally one which is readable. Just because something is valid with
the C standard does not mean saying "personal preference" is a good
justification for using it (and still generally speaking, less typing
often means something less readable, and you read code more than you
write it).

> 
> 
> > you cannot guess the range of the values you
> > can store in there. So let's just be specific.
> 
> The language is quite clear that ‘unsigned’ means ‘unsigned int’.
> There is no guessing involved whatsoever. The guessing about
> the size is because C is not specific about the size of ‘int’ in bits,
> which is why we have .

Yes, the spec is clear about it. The proportion of C coders who will
know when reading "unsigned" that it means "unsigned int" is what I'm
questioning here, and why I'm pushing for a more specific type.

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [vdagent-win PATCH v6 2/5] Initial rewrite of image conversion code

[Christophe de Dinechin]

> On 19 Jul 2017, at 11:21, Christophe Fergeau  wrote:
> 
> On Wed, Jul 19, 2017 at 10:23:30AM +0200, Christophe de Dinechin wrote:
>> 
>>> On 18 Jul 2017, at 17:28, Christophe Fergeau  wrote:
>>> 
>>> On Mon, Jul 17, 2017 at 11:01:22AM +0100, Frediano Ziglio wrote:
 Remove CxImage linking.
 Support Windows BMP format.
>>> 
>>> Too bad there is no small/maintained library which would do that for us
>>> :-/ From a quick glance, looks ok.
>>> 
>>> 
 
 +static inline size_t compute_dib_stride(unsigned width, unsigned 
 bit_count)
>>> 
>>> Can you use full type names, unsigned int?
>> 
>> No. Really, no ;-) Otherwise, for consistency, you should replace ‘int’ with 
>> ‘signed int’, 
> 
> The way I see it, 'signed'/'unsigned' are type modifiers, 'int' is an actual 
> type name.

Yes. But ‘long’ is not. It is also a modifier. So why allow “long” or “short" 
but not “unsigned”?
Or are you also writing “long int” and “short int”?

> Huge difference to me.

No, really not, at least as far as C and usage are concerned. It’s just a 
personal preference.
So if Frediano prefers to write ‘unsigned’, I think it’s OK, and I will most 
likely write the same way.


> With just "unsigned", which is unusual in the
> code I've seen in the past,

It is as customary to write ‘unsigned’ alone as it is to write ‘long’.
In the Linux source code ‘arch’ directory, I find over 2200 occurrences
of ‘unsigned’ not followed by any type. Granted, there are
about 10x as many ‘unsigned int’ and 20 times as many ‘unsigned long’.
But 2200 occurrences show it’s certainly not “unusual".


> you cannot guess the range of the values you
> can store in there. So let's just be specific.

The language is quite clear that ‘unsigned’ means ‘unsigned int’.
There is no guessing involved whatsoever. The guessing about
the size is because C is not specific about the size of ‘int’ in bits,
which is why we have .

I think that we can agree that this particular aspect of the C syntax is messy.
That does not mean we have to invent / enforce rules that don’t exist.
Feel free to write ‘unsigned int’ if you want. No reason to force your
personal preference on people who prefer the shorter ‘unsigned’ form.


Christophe

> 
> Christophe
> ___
> 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] Revert "Ignore modifiers messages if no modifiers changed"

[Victor Toso]

Hi,

On Mon, Jul 17, 2017 at 01:17:27PM +0200, Victor Toso wrote:
> Hi Pavel,
> 
> On Tue, Jul 11, 2017 at 01:34:12PM +0200, Pavel Grunt wrote:
> > This reverts commit 73cd553fb0fbd213b64d72f8b4289ed8a17fc6c0.
> 
> Which is:
> 
>  "This avoid keep sending modifiers changes if guest is not
>   synchronising the changes.
> 
>   I consider this as an improving as this avoids client to try again
>   and again to force synchronisation however this does not prevent
>   every unwanted keystroke insertion which possibly can be a real
>   problem on some configurations.
> 
>   For instance if guest do not handle caps lock as the client do
>   if client uses another modifiers (as num lock) this can force
>   inserting virtual caps keypress."
> 
> > It may be an optimization, but it complicates turning off
> > the capslock once it is enabled.
> >
> > https://bugs.freedesktop.org/show_bug.cgi?id=101344
> 
> It feels like the original patch was trying to fix a bug, not an
> optimization. At the same time, above bug is definitely a regression
> somewhere.
>
> CC'ing, Frediano for comments

Acked-by: Victor Toso 

Feel free to push if no one has any further comments.

>
>
> > ---
> >  src/channel-inputs.c | 6 ++
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/channel-inputs.c b/src/channel-inputs.c
> > index f79bc38..7572bff 100644
> > --- a/src/channel-inputs.c
> > +++ b/src/channel-inputs.c
> > @@ -242,10 +242,8 @@ static void inputs_handle_modifiers(SpiceChannel 
> > *channel, SpiceMsgIn *in)
> >  SpiceInputsChannelPrivate *c = SPICE_INPUTS_CHANNEL(channel)->priv;
> >  SpiceMsgInputsKeyModifiers *modifiers = spice_msg_in_parsed(in);
> >  
> > -if (c->modifiers != modifiers->modifiers) {
> > -c->modifiers = modifiers->modifiers;
> > -g_coroutine_signal_emit(channel, signals[SPICE_INPUTS_MODIFIERS], 
> > 0);
> > -}
> > +c->modifiers = modifiers->modifiers;
> > +g_coroutine_signal_emit(channel, signals[SPICE_INPUTS_MODIFIERS], 0);
> >  }
> >  
> >  /* coroutine context */
> > -- 
> > 2.13.0
> > 
> > ___
> > 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



signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [vdagent-win PATCH v6 2/5] Initial rewrite of image conversion code

[Frediano Ziglio]

> 
> On Wed, Jul 19, 2017 at 10:23:30AM +0200, Christophe de Dinechin wrote:
> > 
> > > On 18 Jul 2017, at 17:28, Christophe Fergeau  wrote:
> > > 
> > > On Mon, Jul 17, 2017 at 11:01:22AM +0100, Frediano Ziglio wrote:
> > >> Remove CxImage linking.
> > >> Support Windows BMP format.
> > > 
> > > Too bad there is no small/maintained library which would do that for us
> > > :-/ From a quick glance, looks ok.
> > > 
> > > 
> > >> 
> > >> +static inline size_t compute_dib_stride(unsigned width, unsigned
> > >> bit_count)
> > > 
> > > Can you use full type names, unsigned int?
> > 
> > No. Really, no ;-) Otherwise, for consistency, you should replace ‘int’
> > with ‘signed int’,
> 
> The way I see it, 'signed'/'unsigned' are type modifiers, 'int' is an actual
> type name.
> Huge difference to me. With just "unsigned", which is unusual in the
> code I've seen in the past, you cannot guess the range of the values you
> can store in there. So let's just be specific.
> 
> Christophe
> 

Even long is a modifier, however in spice-server:

$ cgrep -w 'long' | wc -l
579
$ cgrep 'long int' | wc -l
3

So... why not for long?

And by the way, is standard C and in C++ is even normal to just see
unsigned without int.

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

Re: [Spice-devel] [vdagent-win PATCH] Split long if statement

[Uri Lublin]

On 07/19/2017 11:54 AM, Frediano Ziglio wrote:

Instead of having a long if statement with checks and assignment
split in two.
The conditions where used to compute 2 different path, this does
not help much with code readability.

Signed-off-by: Frediano Ziglio 


Acked-by: Uri Lublin 


---
  vdservice/vdservice.cpp | 11 ++-
  1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/vdservice/vdservice.cpp b/vdservice/vdservice.cpp
index 1eff380..7f3a5a3 100644
--- a/vdservice/vdservice.cpp
+++ b/vdservice/vdservice.cpp
@@ -306,19 +306,20 @@ VOID WINAPI VDService::main(DWORD argc, TCHAR* argv[])
  {
  VDService* s = new VDService;
  SERVICE_STATUS* status;
-TCHAR log_path[MAX_PATH];
  TCHAR full_path[MAX_PATH];
  TCHAR temp_path[MAX_PATH];
  TCHAR* slash;
  
  ASSERT(s);

-if (GetModuleFileName(NULL, full_path, MAX_PATH) && (slash = wcsrchr(full_path, 
TCHAR('\\'))) &&
-GetTempPath(MAX_PATH, temp_path)) {
-*slash = TCHAR('\0');
-swprintf_s(s->_agent_path, MAX_PATH, VD_AGENT_PATH, full_path);
+if (GetTempPath(MAX_PATH, temp_path)) {
+TCHAR log_path[MAX_PATH];
  swprintf_s(log_path, MAX_PATH, VD_SERVICE_LOG_PATH, temp_path);
  s->_log = VDLog::get(log_path);
  }
+if (GetModuleFileName(NULL, full_path, MAX_PATH) && (slash = 
wcsrchr(full_path, TCHAR('\\' {
+*slash = TCHAR('\0');
+swprintf_s(s->_agent_path, MAX_PATH, VD_AGENT_PATH, full_path);
+}
  vd_printf("***Service started***");
  log_version();
  if (!SetPriorityClass(GetCurrentProcess(), ABOVE_NORMAL_PRIORITY_CLASS)) {



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

Re: [Spice-devel] [spice-gtk v1] display-gst: Improve h264 elements filtering

[Victor Toso]

Hi,

On Thu, Jul 13, 2017 at 02:33:48PM +0200, Victor Toso wrote:
> From: Victor Toso 
>
> This patch fixes the avdec_h264 element not being present on
> gstvideo_has_codec() which get all decoder elements from GstRegistry
> and filter them on our GstCaps in order to get the ones for given
> codec.
>
> The issue is around the filtering. The current GstCaps for h264 is not
> consider a subset of avdec_h264's capabilites and that will fiter this
> element out of the list.
>
> The proposed solution for that is to set `subsetonly` parameter from
> gst_element_factory_list_filter() to false.
>
> While at it, the cap "stream-format=byte-stream" is less useful now
> because it isn't needed to play the stream - see 6fe88871240c53b8
>
> In my system, our debug shows:
> .. gstvideo_debug_available_decoders: From 228 video decoder elements,
> - 4 can handle caps   image/jpeg: jpegdec, nvdec, avdec_mjpeg, vaapijpegdec
> - 3 can handle caps  video/x-vp8: vaapidecodebin, vp8dec, avdec_vp8
> - 4 can handle caps video/x-h264: vaapidecodebin, avdec_h264, nvdec, 
> vaapih264dec
> - 3 can handle caps  video/x-vp9: vaapidecodebin, vp9dec, avdec_vp9
>
> Signed-off-by: Victor Toso 

I consider this one a blocker to the release as a client that had only
the avdec_h264 decoder for h264, would be able to decode h264 streams
but without this patch it can't as we will not find any available
decoder due wrong filtering which will disable the capability for h264
decoding.

Any comments?

> ---
>  src/channel-display-gst.c  | 2 +-
>  src/channel-display-priv.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index 20d236a..1bd7df1 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -647,7 +647,7 @@ gboolean gstvideo_has_codec(int codec_type)
>  }
>  
>  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);
> +codec_decoders = gst_element_factory_list_filter(all_decoders, caps, 
> GST_PAD_SINK, FALSE);
>  gst_caps_unref(caps);
>  
>  if (codec_decoders == NULL) {
> diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
> index 04cb4d1..9bfd4ac 100644
> --- a/src/channel-display-priv.h
> +++ b/src/channel-display-priv.h
> @@ -181,7 +181,7 @@ static const struct {
>   * (hardcoded in spice-server), let's add it here to avoid the warning.
>   */
>  { SPICE_DISPLAY_CAP_CODEC_H264, "h264",
> -  "h264parse ! avdec_h264", "video/x-h264,stream-format=byte-stream" },
> +  "h264parse ! avdec_h264", "video/x-h264" },
>  
>  /* SPICE_VIDEO_CODEC_TYPE_VP9 */
>  { SPICE_DISPLAY_CAP_CODEC_VP9, "vp9",
> -- 
> 2.13.0
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [vdagent-win PATCH v6 2/5] Initial rewrite of image conversion code

[Christophe Fergeau]

On Wed, Jul 19, 2017 at 10:23:30AM +0200, Christophe de Dinechin wrote:
> 
> > On 18 Jul 2017, at 17:28, Christophe Fergeau  wrote:
> > 
> > On Mon, Jul 17, 2017 at 11:01:22AM +0100, Frediano Ziglio wrote:
> >> Remove CxImage linking.
> >> Support Windows BMP format.
> > 
> > Too bad there is no small/maintained library which would do that for us
> > :-/ From a quick glance, looks ok.
> > 
> > 
> >> 
> >> +static inline size_t compute_dib_stride(unsigned width, unsigned 
> >> bit_count)
> > 
> > Can you use full type names, unsigned int?
> 
> No. Really, no ;-) Otherwise, for consistency, you should replace ‘int’ with 
> ‘signed int’, 

The way I see it, 'signed'/'unsigned' are type modifiers, 'int' is an actual 
type name.
Huge difference to me. With just "unsigned", which is unusual in the
code I've seen in the past, you cannot guess the range of the values you
can store in there. So let's just be specific.

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [vdagent-win PATCH v4 3/5] Write code to decode PNG format

[Christophe Fergeau]

On Tue, Jul 18, 2017 at 01:29:24PM -0400, Frediano Ziglio wrote:
> > On Mon, Jul 17, 2017 at 09:54:04AM +0100, Frediano Ziglio wrote:
> > > +}
> > > +break;
> > > +case PNG_COLOR_TYPE_PALETTE:
> > > +// should return 1, 2, 4 and 8, BMP does not support 2
> > 
> > Is the first '2' unwanted? Since you say later 2 is unsupported
> > 
> 
> 2 is supported by PNG but not by BMP so the line_fixup_2bpp_to_4bpp
> function.
> 
> Maybe the comment should be "should return 1, 4 and 8, BMP does not support 
> 2" ?

Yes, I was asking about the comment, but did not manage to express this clearly 
;) 

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] [vdagent-win PATCH] Split long if statement

[Frediano Ziglio]

Instead of having a long if statement with checks and assignment
split in two.
The conditions where used to compute 2 different path, this does
not help much with code readability.

Signed-off-by: Frediano Ziglio 
---
 vdservice/vdservice.cpp | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/vdservice/vdservice.cpp b/vdservice/vdservice.cpp
index 1eff380..7f3a5a3 100644
--- a/vdservice/vdservice.cpp
+++ b/vdservice/vdservice.cpp
@@ -306,19 +306,20 @@ VOID WINAPI VDService::main(DWORD argc, TCHAR* argv[])
 {
 VDService* s = new VDService;
 SERVICE_STATUS* status;
-TCHAR log_path[MAX_PATH];
 TCHAR full_path[MAX_PATH];
 TCHAR temp_path[MAX_PATH];
 TCHAR* slash;
 
 ASSERT(s);
-if (GetModuleFileName(NULL, full_path, MAX_PATH) && (slash = 
wcsrchr(full_path, TCHAR('\\'))) &&
-GetTempPath(MAX_PATH, temp_path)) {
-*slash = TCHAR('\0');
-swprintf_s(s->_agent_path, MAX_PATH, VD_AGENT_PATH, full_path);
+if (GetTempPath(MAX_PATH, temp_path)) {
+TCHAR log_path[MAX_PATH];
 swprintf_s(log_path, MAX_PATH, VD_SERVICE_LOG_PATH, temp_path);
 s->_log = VDLog::get(log_path);
 }
+if (GetModuleFileName(NULL, full_path, MAX_PATH) && (slash = 
wcsrchr(full_path, TCHAR('\\' {
+*slash = TCHAR('\0');
+swprintf_s(s->_agent_path, MAX_PATH, VD_AGENT_PATH, full_path);
+}
 vd_printf("***Service started***");
 log_version();
 if (!SetPriorityClass(GetCurrentProcess(), ABOVE_NORMAL_PRIORITY_CLASS)) {
-- 
2.13.3

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

Re: [Spice-devel] [vdagent-win PATCH v4 3/5] Write code to decode PNG format

[Frediano Ziglio]

> 
> > On 18 Jul 2017, at 17:54, Christophe Fergeau  wrote:
> > 
> > On Mon, Jul 17, 2017 at 09:54:04AM +0100, Frediano Ziglio wrote:
> >> Signed-off-by: Frediano Ziglio 
> >> ---
> >> Makefile.am  |   6 +-
> >> configure.ac |   3 +
> >> vdagent/image.cpp|   8 +-
> >> vdagent/imagepng.cpp | 244
> >> +++
> >> vdagent/imagepng.h   |  25 ++
> >> 5 files changed, 277 insertions(+), 9 deletions(-)
> >> create mode 100644 vdagent/imagepng.cpp
> >> create mode 100644 vdagent/imagepng.h
> >> 
> >> diff --git a/Makefile.am b/Makefile.am
> >> index b35dd57..175d8f7 100644
> >> --- a/Makefile.am
> >> +++ b/Makefile.am
> >> @@ -23,8 +23,8 @@ LIBS = -lversion
> >> 
> >> bin_PROGRAMS = vdagent vdservice
> >> 
> >> -vdagent_LDADD = -lwtsapi32 -lgdi32 vdagent_rc.$(OBJEXT)
> >> -vdagent_CXXFLAGS = $(AM_CXXFLAGS)
> >> +vdagent_LDADD = $(LIBPNG_LIBS) $(ZLIB_LIBS) -lwtsapi32 -lgdi32
> >> vdagent_rc.$(OBJEXT)
> >> +vdagent_CXXFLAGS = $(AM_CXXFLAGS) $(LIBPNG_CFLAGS)
> >> vdagent_LDFLAGS = $(AM_LDFLAGS) -Wl,--subsystem,windows
> >> vdagent_SOURCES =  \
> >>common/vdcommon.cpp \
> >> @@ -44,6 +44,8 @@ vdagent_SOURCES =\
> >>vdagent/as_user.h   \
> >>vdagent/image.cpp   \
> >>vdagent/image.h \
> >> +  vdagent/imagepng.cpp\
> >> +  vdagent/imagepng.h  \
> >>$(NULL)
> >> 
> >> vdagent_rc.$(OBJEXT): vdagent/vdagent.rc
> >> diff --git a/configure.ac b/configure.ac
> >> index 4eac4b4..bb33075 100644
> >> --- a/configure.ac
> >> +++ b/configure.ac
> >> @@ -101,6 +101,9 @@ dnl
> >> ---
> >> dnl - Check library dependencies
> >> dnl
> >> ---
> >> 
> >> +PKG_CHECK_MODULES(LIBPNG, [libpng])
> >> +PKG_CHECK_MODULES(ZLIB, [zlib])
> >> +
> >> dnl
> >> ---
> >> dnl - Makefiles, etc.
> >> dnl
> >> ---
> >> diff --git a/vdagent/image.cpp b/vdagent/image.cpp
> >> index b32da6f..c968217 100644
> >> --- a/vdagent/image.cpp
> >> +++ b/vdagent/image.cpp
> >> @@ -21,9 +21,9 @@
> >> 
> >> #include "vdcommon.h"
> >> #include "image.h"
> >> +#include "imagepng.h"
> >> 
> >> ImageCoder *create_bitmap_coder();
> >> -ImageCoder *create_png_coder();
> >> 
> >> static ImageCoder *get_coder(uint32_t vdagent_type)
> >> {
> >> @@ -172,9 +172,3 @@ ImageCoder *create_bitmap_coder()
> >> {
> >> return new BitmapCoder();
> >> }
> >> -
> >> -// TODO
> >> -ImageCoder *create_png_coder()
> >> -{
> >> -return NULL;
> >> -}
> >> diff --git a/vdagent/imagepng.cpp b/vdagent/imagepng.cpp
> >> new file mode 100644
> >> index 000..19d9efc
> >> --- /dev/null
> >> +++ b/vdagent/imagepng.cpp
> >> @@ -0,0 +1,244 @@
> >> +/*
> >> +   Copyright (C) 2017 Red Hat, Inc.
> >> +
> >> +   This program is free software; you can redistribute it and/or
> >> +   modify it under the terms of the GNU General Public License as
> >> +   published by the Free Software Foundation; either version 2 of
> >> +   the License, or (at your option) any later version.
> >> +
> >> +   This program is distributed in the hope that it will be useful,
> >> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >> +   GNU General Public License for more details.
> >> +
> >> +   You should have received a copy of the GNU General Public License
> >> +   along with this program.  If not, see .
> >> +*/
> >> +
> >> +#include "vdcommon.h"
> >> +
> >> +#include 
> >> +#include 
> >> +
> >> +#include "imagepng.h"
> >> +
> >> +class PngCoder: public ImageCoder
> >> +{
> >> +public:
> >> +PngCoder() {};
> >> +size_t get_dib_size(const uint8_t *data, size_t size);
> >> +void get_dib_data(uint8_t *dib, const uint8_t *data, size_t size);
> >> +uint8_t *from_bitmap(const BITMAPINFO& info, const void *bits, long
> >> );
> >> +private:
> >> +size_t convert_to_dib(uint8_t *out_buf, const uint8_t *data, size_t
> >> size);
> >> +};
> >> +
> >> +struct BufferIo {
> >> +uint8_t *buf;
> >> +uint32_t pos, size;
> >> +};
> >> +
> >> +static void read_from_bufio(png_structp png, png_bytep out, png_size_t
> >> size)
> >> +{
> >> +BufferIo& io(*(BufferIo*)png_get_io_ptr(png));
> 
> As long as you do a cast form a pointer, I would rather use a pointer to
> BufferIo here.
> 

Yes and no. You are working with a C library that only understand
pointers and you know the pointer is the object (always existing)
you saved.

> >> +if (io.pos + size >= io.size)
> >> +png_error(png, "read past end");
> >> +memcpy(out, io.buf+io.pos, size);
> 
> This 

[Spice-devel] [vdagent-win PATCH v8 1/5] Move image handling to a separate file

[Frediano Ziglio]

This will make easier to change code that handle images.

Signed-off-by: Frediano Ziglio 
Acked-by: Christophe Fergeau 
---
 Makefile.am |  2 ++
 vdagent/image.cpp   | 88 +
 vdagent/image.h | 48 +
 vdagent/vdagent.cpp | 57 +-
 4 files changed, 146 insertions(+), 49 deletions(-)
 create mode 100644 vdagent/image.cpp
 create mode 100644 vdagent/image.h

diff --git a/Makefile.am b/Makefile.am
index b60a718..868199e 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -42,6 +42,8 @@ vdagent_SOURCES = \
vdagent/vdagent.cpp \
vdagent/as_user.cpp \
vdagent/as_user.h   \
+   vdagent/image.cpp   \
+   vdagent/image.h \
$(NULL)
 
 vdagent_rc.$(OBJEXT): vdagent/vdagent.rc
diff --git a/vdagent/image.cpp b/vdagent/image.cpp
new file mode 100644
index 000..960058d
--- /dev/null
+++ b/vdagent/image.cpp
@@ -0,0 +1,88 @@
+/*
+   Copyright (C) 2013-2017 Red Hat, Inc.
+
+   This program is free software; you can redistribute it and/or
+   modify it under the terms of the GNU General Public License as
+   published by the Free Software Foundation; either version 2 of
+   the License, or (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see .
+*/
+
+#include 
+
+#include "vdcommon.h"
+#include "image.h"
+
+#include "ximage.h"
+
+typedef struct ImageType {
+uint32_t type;
+DWORD cximage_format;
+} ImageType;
+
+static const ImageType image_types[] = {
+{VD_AGENT_CLIPBOARD_IMAGE_PNG, CXIMAGE_FORMAT_PNG},
+{VD_AGENT_CLIPBOARD_IMAGE_BMP, CXIMAGE_FORMAT_BMP},
+};
+
+static DWORD get_cximage_format(uint32_t type)
+{
+for (unsigned int i = 0; i < SPICE_N_ELEMENTS(image_types); i++) {
+if (image_types[i].type == type) {
+return image_types[i].cximage_format;
+}
+}
+return 0;
+}
+
+HANDLE get_image_handle(const VDAgentClipboard& clipboard, uint32_t size, 
UINT&)
+{
+HANDLE clip_data;
+DWORD cximage_format = get_cximage_format(clipboard.type);
+ASSERT(cximage_format);
+CxImage image((BYTE*)clipboard.data, size, cximage_format);
+clip_data = image.CopyToHandle();
+return clip_data;
+}
+
+uint8_t* get_raw_clipboard_image(const VDAgentClipboardRequest& 
clipboard_request,
+ HANDLE clip_data, long& new_size)
+{
+new_size = 0;
+
+CxImage image;
+uint8_t *new_data = NULL;
+DWORD cximage_format = get_cximage_format(clipboard_request.type);
+HPALETTE pal = 0;
+
+ASSERT(cximage_format);
+if (IsClipboardFormatAvailable(CF_PALETTE)) {
+pal = (HPALETTE)GetClipboardData(CF_PALETTE);
+}
+if (!image.CreateFromHBITMAP((HBITMAP)clip_data, pal)) {
+vd_printf("Image create from handle failed");
+return NULL;
+}
+if (!image.Encode(new_data, new_size, cximage_format)) {
+vd_printf("Image encode to type %u failed", clipboard_request.type);
+return NULL;
+}
+vd_printf("Image encoded to %lu bytes", new_size);
+return new_data;
+}
+
+void free_raw_clipboard_image(uint8_t *data)
+{
+// this is really just a free however is better to make
+// the free from CxImage code as on Windows the free
+// can be different between libraries
+CxImage image;
+image.FreeMemory(data);
+}
diff --git a/vdagent/image.h b/vdagent/image.h
new file mode 100644
index 000..b70f53a
--- /dev/null
+++ b/vdagent/image.h
@@ -0,0 +1,48 @@
+/*
+   Copyright (C) 2013-2017 Red Hat, Inc.
+
+   This program is free software; you can redistribute it and/or
+   modify it under the terms of the GNU General Public License as
+   published by the Free Software Foundation; either version 2 of
+   the License, or (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see .
+*/
+
+#ifndef VDAGENT_IMAGE_H_
+#define VDAGENT_IMAGE_H_
+
+/**
+ * Returns image to put in the clipboard.
+ *
+ * @param clipboard  data to write in the clipboard
+ * @param size   size of data
+ * @param[in,out] format suggested clipboard format. This can be 

[Spice-devel] [vdagent-win PATCH v8 4/5] Support encoding PNG images

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 vdagent/imagepng.cpp | 110 +--
 1 file changed, 106 insertions(+), 4 deletions(-)

diff --git a/vdagent/imagepng.cpp b/vdagent/imagepng.cpp
index 9530d71..0d03ca0 100644
--- a/vdagent/imagepng.cpp
+++ b/vdagent/imagepng.cpp
@@ -19,6 +19,7 @@
 
 #include 
 #include 
+#include 
 
 #include "imagepng.h"
 
@@ -36,9 +37,13 @@ private:
 struct BufferIo {
 uint8_t *buf;
 uint32_t pos, size;
+bool allocated;
 BufferIo(uint8_t *_buf, uint32_t _size):
-buf(_buf), pos(0), size(_size)
+buf(_buf), pos(0), size(_size),
+allocated(false)
 {}
+~BufferIo() { if (allocated) free(buf); }
+uint8_t *release() { allocated = false; return buf; }
 };
 
 static void read_from_bufio(png_structp png, png_bytep out, png_size_t size)
@@ -50,6 +55,29 @@ static void read_from_bufio(png_structp png, png_bytep out, 
png_size_t size)
 io.pos += size;
 }
 
+static void write_to_bufio(png_structp png, png_bytep in, png_size_t size)
+{
+BufferIo& io(*(BufferIo*)png_get_io_ptr(png));
+if (io.pos + size > io.size) {
+uint32_t new_size = io.size ? io.size * 2 : 4096;
+while (io.pos + size >= new_size) {
+new_size *= 2;
+}
+uint8_t *p = (uint8_t*) realloc(io.buf, new_size);
+if (!p)
+png_error(png, "out of memory");
+io.allocated = true;
+io.buf = p;
+io.size = new_size;
+}
+memcpy(io.buf+io.pos, in, size);
+io.pos += size;
+}
+
+static void flush_bufio(png_structp png)
+{
+}
+
 size_t PngCoder::get_dib_size(const uint8_t *data, size_t size)
 {
 return convert_to_dib(NULL, data, size);
@@ -225,10 +253,84 @@ void PngCoder::get_dib_data(uint8_t *dib, const uint8_t 
*data, size_t size)
 convert_to_dib(dib, data, size);
 }
 
-uint8_t *PngCoder::from_bitmap(const BITMAPINFO& info, const void *bits, long 
)
+uint8_t *PngCoder::from_bitmap(const BITMAPINFO& bmp_info, const void *bits, 
long )
 {
-// TODO not implemented
-return NULL;
+// this vector is here to avoid problems as libpng use setjmp/longjmp
+std::vector palette;
+BufferIo io(NULL, 0);
+
+png_structp png = png_create_write_struct(PNG_LIBPNG_VER_STRING, NULL, 
NULL, NULL);
+if (!png)
+return 0;
+
+png_infop info = png_create_info_struct(png);
+if (!info) {
+png_destroy_write_struct(, );
+return 0;
+}
+
+if (setjmp(png_jmpbuf(png))) {
+png_destroy_write_struct(, );
+return 0;
+}
+
+png_set_write_fn(png, , write_to_bufio, flush_bufio);
+
+const BITMAPINFOHEADER& head(bmp_info.bmiHeader);
+int color_type;
+int out_bits = head.biBitCount;
+switch (out_bits) {
+case 1:
+case 4:
+case 8:
+color_type = PNG_COLOR_TYPE_PALETTE;
+break;
+case 24:
+case 32:
+png_set_bgr(png);
+color_type = PNG_COLOR_TYPE_RGB;
+break;
+default:
+png_error(png, "BMP bit count not supported");
+break;
+}
+// TODO detect gray
+png_set_IHDR(png, info, head.biWidth, head.biHeight,
+ out_bits > 8 ? 8 : out_bits, color_type,
+ PNG_INTERLACE_NONE, PNG_COMPRESSION_TYPE_DEFAULT,
+ PNG_FILTER_TYPE_DEFAULT);
+
+// palette
+if (color_type == PNG_COLOR_TYPE_PALETTE) {
+palette.resize(head.biClrUsed);
+const RGBQUAD *rgb = bmp_info.bmiColors;
+for (unsigned int color = 0; color < head.biClrUsed; ++color) {
+palette[color].red = rgb->rgbRed;
+palette[color].green = rgb->rgbGreen;
+palette[color].blue = rgb->rgbBlue;
+++rgb;
+}
+png_set_PLTE(png, info, [0], palette.size());
+}
+
+png_write_info(png, info);
+
+const unsigned int width = head.biWidth;
+const unsigned int height = head.biHeight;
+const size_t stride = compute_dib_stride(width, out_bits);
+const size_t image_size = stride * height;
+
+// now do the actual conversion!
+const uint8_t *src = (const uint8_t*)bits + image_size;
+for (unsigned int row = 0; row < height; ++row) {
+src -= stride;
+png_write_row(png, src);
+}
+png_write_end(png, NULL);
+
+png_destroy_write_struct(, );
+size = io.pos;
+return io.release();
 }
 
 ImageCoder *create_png_coder()
-- 
2.13.3

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

[Spice-devel] [vdagent-win PATCH v8 5/5] Add test for PNG files

[Frediano Ziglio]

Test various image and formats.
The idea is to decode and encode again an image and
check for differences. ImageMagick is used to create
some test image and compare results.
Wine is used to execute a test helper.

Signed-off-by: Frediano Ziglio 
---
 Makefile.am   | 20 +
 test-png  | 50 +++
 vdagent/imagetest.cpp | 82 +++
 3 files changed, 152 insertions(+)
 create mode 100755 test-png
 create mode 100644 vdagent/imagetest.cpp

diff --git a/Makefile.am b/Makefile.am
index 175d8f7..411bf0d 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -68,6 +68,26 @@ vdservice_rc.$(OBJEXT): vdservice/vdservice.rc
 
 MAINTAINERCLEANFILES += vdservice_rc.$(OBJEXT)
 
+check_PROGRAMS = imagetest
+
+imagetest_LDADD = $(LIBPNG_LIBS) $(ZLIB_LIBS) -lwtsapi32 -lgdi32
+imagetest_CXXFLAGS = $(AM_CXXFLAGS) $(LIBPNG_CFLAGS)
+imagetest_LDFLAGS = $(AM_LDFLAGS) -Wl,--subsystem,console
+imagetest_SOURCES =\
+   common/vdcommon.cpp \
+   common/vdcommon.h   \
+   common/vdlog.cpp\
+   common/vdlog.h  \
+   vdagent/imagetest.cpp   \
+   vdagent/image.cpp   \
+   vdagent/image.h \
+   vdagent/imagepng.cpp\
+   vdagent/imagepng.h  \
+   $(NULL)
+
+TESTS = test-png
+EXTRA_DIST += test-png
+
 deps.txt:
$(AM_V_GEN)rpm -qa | grep $(host_os) | sort | unix2dos > $@
 
diff --git a/test-png b/test-png
new file mode 100755
index 000..92fcff7
--- /dev/null
+++ b/test-png
@@ -0,0 +1,50 @@
+#!/bin/bash
+
+VERBOSE=${VERBOSE:-0}
+
+IN=in.png
+OUT=out.png
+OUT_BMP=out.bmp
+
+error() {
+echo "$*" >&2
+exit 1
+}
+
+verbose() {
+if [ $VERBOSE != 0 ]; then
+"$@"
+fi
+}
+
+do_test() {
+echo "Running image $IMAGE with '$*'..."
+convert $IMAGE "$@" $IN
+wine imagetest.exe $IN $OUT_BMP $OUT
+verbose ls -lh $IN
+verbose identify $IN
+verbose identify $OUT_BMP
+DIFF=$(compare -metric AE $IN $OUT - 2>&1 > /dev/null || true)
+if [ "$DIFF" != "0" ]; then
+error "Images are too different, diff $DIFF"
+fi
+}
+
+do_test_all() {
+IMAGE="$1"
+do_test
+do_test -type Palette
+do_test -type Palette -colors 14
+do_test -type Palette -colors 3
+do_test -type TrueColor
+do_test -type Grayscale
+}
+
+set -e
+# test with small image
+do_test_all rose:
+# test with larger image
+do_test_all logo:
+rm -f $IN $OUT $OUT_BMP
+
+verbose echo Finish
diff --git a/vdagent/imagetest.cpp b/vdagent/imagetest.cpp
new file mode 100644
index 000..319b188
--- /dev/null
+++ b/vdagent/imagetest.cpp
@@ -0,0 +1,82 @@
+/*
+   Copyright (C) 2017 Red Hat, Inc.
+
+   This program is free software; you can redistribute it and/or
+   modify it under the terms of the GNU General Public License as
+   published by the Free Software Foundation; either version 2 of
+   the License, or (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see .
+*/
+
+#undef NDEBUG
+#include 
+#include 
+
+#include "vdcommon.h"
+#include "image.h"
+#include "imagepng.h"
+
+int main(int argc, char **argv)
+{
+ImageCoder *coder = create_png_coder();
+
+assert(coder);
+if (argc < 2) {
+fprintf(stderr, "Usage: %s  [ []]\n", 
argv[0]);
+return 1;
+}
+
+// read all file into memory
+FILE *f = fopen(argv[1], "rb");
+assert(f);
+assert(fseek(f, 0, SEEK_END) == 0);
+long len = ftell(f);
+assert(len > 0);
+assert(fseek(f, 0, SEEK_SET) == 0);
+
+std::vector data(len);
+assert(fread([0], 1, len, f) == (unsigned long) len);
+fclose(f);
+
+size_t dib_size = coder->get_dib_size([0], len);
+assert(dib_size);
+std::vector out(dib_size);
+memset([0], 0xcc, dib_size);
+coder->get_dib_data([0], [0], len);
+
+// looks like many tools wants this header so craft it
+BITMAPFILEHEADER head;
+memset(, 0, sizeof(head));
+head.bfType = 'B'+'M'*256u;
+head.bfSize = sizeof(head) + dib_size;
+BITMAPINFOHEADER& info(*(BITMAPINFOHEADER*)[0]);
+head.bfOffBits = sizeof(head) + sizeof(BITMAPINFOHEADER) + 4 * 
info.biClrUsed;
+
+f = fopen(argc > 2 ? argv[2] : "out.bmp", "wb");
+assert(f);
+assert(fwrite(, 1, sizeof(head), f) == sizeof(head));
+assert(fwrite([0], 1, dib_size, f) == dib_size);
+fclose(f);
+
+// convert back to PNG
+long png_size = 0;
+uint8_t *png = coder->from_bitmap(*((BITMAPINFO*)[0]), 
[sizeof(BITMAPINFOHEADER) + 4 * 

[Spice-devel] [vdagent-win PATCH v8 3/5] Write code to decode PNG format

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 Makefile.am  |   6 +-
 configure.ac |   3 +
 vdagent/image.cpp|   8 +-
 vdagent/imagepng.cpp | 237 +++
 vdagent/imagepng.h   |  25 ++
 5 files changed, 270 insertions(+), 9 deletions(-)
 create mode 100644 vdagent/imagepng.cpp
 create mode 100644 vdagent/imagepng.h

diff --git a/Makefile.am b/Makefile.am
index b35dd57..175d8f7 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -23,8 +23,8 @@ LIBS = -lversion
 
 bin_PROGRAMS = vdagent vdservice
 
-vdagent_LDADD = -lwtsapi32 -lgdi32 vdagent_rc.$(OBJEXT)
-vdagent_CXXFLAGS = $(AM_CXXFLAGS)
+vdagent_LDADD = $(LIBPNG_LIBS) $(ZLIB_LIBS) -lwtsapi32 -lgdi32 
vdagent_rc.$(OBJEXT)
+vdagent_CXXFLAGS = $(AM_CXXFLAGS) $(LIBPNG_CFLAGS)
 vdagent_LDFLAGS = $(AM_LDFLAGS) -Wl,--subsystem,windows
 vdagent_SOURCES =  \
common/vdcommon.cpp \
@@ -44,6 +44,8 @@ vdagent_SOURCES = \
vdagent/as_user.h   \
vdagent/image.cpp   \
vdagent/image.h \
+   vdagent/imagepng.cpp\
+   vdagent/imagepng.h  \
$(NULL)
 
 vdagent_rc.$(OBJEXT): vdagent/vdagent.rc
diff --git a/configure.ac b/configure.ac
index 4eac4b4..bb33075 100644
--- a/configure.ac
+++ b/configure.ac
@@ -101,6 +101,9 @@ dnl 
---
 dnl - Check library dependencies
 dnl ---
 
+PKG_CHECK_MODULES(LIBPNG, [libpng])
+PKG_CHECK_MODULES(ZLIB, [zlib])
+
 dnl ---
 dnl - Makefiles, etc.
 dnl ---
diff --git a/vdagent/image.cpp b/vdagent/image.cpp
index faec18f..82cfb0e 100644
--- a/vdagent/image.cpp
+++ b/vdagent/image.cpp
@@ -21,9 +21,9 @@
 
 #include "vdcommon.h"
 #include "image.h"
+#include "imagepng.h"
 
 ImageCoder *create_bitmap_coder();
-ImageCoder *create_png_coder();
 
 static ImageCoder *get_coder(uint32_t vdagent_type)
 {
@@ -172,9 +172,3 @@ ImageCoder *create_bitmap_coder()
 {
 return new BitmapCoder();
 }
-
-// TODO
-ImageCoder *create_png_coder()
-{
-return NULL;
-}
diff --git a/vdagent/imagepng.cpp b/vdagent/imagepng.cpp
new file mode 100644
index 000..9530d71
--- /dev/null
+++ b/vdagent/imagepng.cpp
@@ -0,0 +1,237 @@
+/*
+   Copyright (C) 2017 Red Hat, Inc.
+
+   This program is free software; you can redistribute it and/or
+   modify it under the terms of the GNU General Public License as
+   published by the Free Software Foundation; either version 2 of
+   the License, or (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see .
+*/
+
+#include "vdcommon.h"
+
+#include 
+#include 
+
+#include "imagepng.h"
+
+class PngCoder: public ImageCoder
+{
+public:
+PngCoder() {};
+size_t get_dib_size(const uint8_t *data, size_t size);
+void get_dib_data(uint8_t *dib, const uint8_t *data, size_t size);
+uint8_t *from_bitmap(const BITMAPINFO& info, const void *bits, long );
+private:
+size_t convert_to_dib(uint8_t *out_buf, const uint8_t *data, size_t size);
+};
+
+struct BufferIo {
+uint8_t *buf;
+uint32_t pos, size;
+BufferIo(uint8_t *_buf, uint32_t _size):
+buf(_buf), pos(0), size(_size)
+{}
+};
+
+static void read_from_bufio(png_structp png, png_bytep out, png_size_t size)
+{
+BufferIo& io(*(BufferIo*)png_get_io_ptr(png));
+if (io.pos + size > io.size)
+png_error(png, "read past end");
+memcpy(out, io.buf+io.pos, size);
+io.pos += size;
+}
+
+size_t PngCoder::get_dib_size(const uint8_t *data, size_t size)
+{
+return convert_to_dib(NULL, data, size);
+}
+
+typedef void line_fixup_t(uint8_t *line, unsigned int width);
+
+static void line_fixup_none(uint8_t *line, unsigned int width)
+{
+}
+
+static void line_fixup_2bpp_to_4bpp(uint8_t *line, unsigned int width)
+{
+width = (width + 3) / 4u;
+while (width--) {
+uint8_t from = line[width];
+line[width*2+1] = ((from & 0x03) << 0) | ((from & 0x0c) << 2);
+line[width*2+0] = ((from & 0x30) >> 4) | ((from & 0xc0) >> 2);
+}
+}
+
+size_t PngCoder::convert_to_dib(uint8_t *out_buf, const uint8_t *data, size_t 
size)
+{
+BufferIo io((uint8_t *)data, size);
+
+png_structp png = png_create_read_struct(PNG_LIBPNG_VER_STRING, NULL, 
NULL, NULL);
+if (!png)
+return 0;
+
+png_infop info = png_create_info_struct(png);
+if 

[Spice-devel] [vdagent-win PATCH v8 2/5] Initial rewrite of image conversion code

[Frediano Ziglio]

Remove CxImage linking.
Support Windows BMP format.

Signed-off-by: Frediano Ziglio 
---
 Makefile.am |   4 +-
 configure.ac|   4 +-
 mingw-spice-vdagent.spec.in |  10 +--
 vdagent/image.cpp   | 172 +---
 vdagent/image.h |  21 ++
 5 files changed, 158 insertions(+), 53 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 868199e..b35dd57 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -23,8 +23,8 @@ LIBS = -lversion
 
 bin_PROGRAMS = vdagent vdservice
 
-vdagent_LDADD = -lwtsapi32 $(CXIMAGE_LIBS) vdagent_rc.$(OBJEXT)
-vdagent_CXXFLAGS = $(AM_CXXFLAGS) $(CXIMAGE_CFLAGS)
+vdagent_LDADD = -lwtsapi32 -lgdi32 vdagent_rc.$(OBJEXT)
+vdagent_CXXFLAGS = $(AM_CXXFLAGS)
 vdagent_LDFLAGS = $(AM_LDFLAGS) -Wl,--subsystem,windows
 vdagent_SOURCES =  \
common/vdcommon.cpp \
diff --git a/configure.ac b/configure.ac
index ff489cc..4eac4b4 100644
--- a/configure.ac
+++ b/configure.ac
@@ -42,6 +42,7 @@ AC_DEFINE_UNQUOTED([BUILD_YEAR], "$BUILD_YEAR", [Build year])
 # Check for programs
 AC_PROG_CC
 AC_PROG_CXX
+AX_CXX_COMPILE_STDCXX_11
 AM_PROG_CC_C_O
 AC_PROG_INSTALL
 AC_CHECK_TOOL(WINDRES, [windres])
@@ -100,9 +101,6 @@ dnl 
---
 dnl - Check library dependencies
 dnl ---
 
-PKG_CHECK_MODULES(CXIMAGE, [cximage])
-CXIMAGE_LIBS=`$PKG_CONFIG --static --libs cximage`
-
 dnl ---
 dnl - Makefiles, etc.
 dnl ---
diff --git a/mingw-spice-vdagent.spec.in b/mingw-spice-vdagent.spec.in
index 563341d..f874e66 100644
--- a/mingw-spice-vdagent.spec.in
+++ b/mingw-spice-vdagent.spec.in
@@ -13,16 +13,10 @@ Source0:
vdagent-win-%{version}%{?_version_suffix}.tar.xz
 
 BuildRequires:  mingw32-filesystem >= 23
 BuildRequires:  mingw64-filesystem >= 23
-BuildRequires:  mingw32-cximage-static
-BuildRequires:  mingw64-cximage-static
-BuildRequires:  mingw32-jasper-static
-BuildRequires:  mingw64-jasper-static
-BuildRequires:  mingw32-libjpeg-turbo-static
-BuildRequires:  mingw64-libjpeg-turbo-static
+BuildRequires:  mingw32-gcc-c++
+BuildRequires:  mingw64-gcc-c++
 BuildRequires:  mingw32-libpng-static
 BuildRequires:  mingw64-libpng-static
-BuildRequires:  mingw32-libtiff-static
-BuildRequires:  mingw64-libtiff-static
 BuildRequires:  mingw32-zlib-static
 BuildRequires:  mingw64-zlib-static
 BuildRequires:  mingw32-winpthreads-static
diff --git a/vdagent/image.cpp b/vdagent/image.cpp
index 960058d..faec18f 100644
--- a/vdagent/image.cpp
+++ b/vdagent/image.cpp
@@ -16,39 +16,48 @@
 */
 
 #include 
+#include 
+#include 
 
 #include "vdcommon.h"
 #include "image.h"
 
-#include "ximage.h"
+ImageCoder *create_bitmap_coder();
+ImageCoder *create_png_coder();
 
-typedef struct ImageType {
-uint32_t type;
-DWORD cximage_format;
-} ImageType;
-
-static const ImageType image_types[] = {
-{VD_AGENT_CLIPBOARD_IMAGE_PNG, CXIMAGE_FORMAT_PNG},
-{VD_AGENT_CLIPBOARD_IMAGE_BMP, CXIMAGE_FORMAT_BMP},
-};
-
-static DWORD get_cximage_format(uint32_t type)
+static ImageCoder *get_coder(uint32_t vdagent_type)
 {
-for (unsigned int i = 0; i < SPICE_N_ELEMENTS(image_types); i++) {
-if (image_types[i].type == type) {
-return image_types[i].cximage_format;
-}
+switch (vdagent_type) {
+case VD_AGENT_CLIPBOARD_IMAGE_BMP:
+return create_bitmap_coder();
+case VD_AGENT_CLIPBOARD_IMAGE_PNG:
+return create_png_coder();
 }
-return 0;
+return NULL;
 }
 
-HANDLE get_image_handle(const VDAgentClipboard& clipboard, uint32_t size, 
UINT&)
+HANDLE get_image_handle(const VDAgentClipboard& clipboard, uint32_t size, 
UINT& format)
 {
-HANDLE clip_data;
-DWORD cximage_format = get_cximage_format(clipboard.type);
-ASSERT(cximage_format);
-CxImage image((BYTE*)clipboard.data, size, cximage_format);
-clip_data = image.CopyToHandle();
+std::unique_ptr coder(get_coder(clipboard.type));
+if (!coder) {
+return NULL;
+}
+
+format = CF_DIB;
+size_t dib_size = coder->get_dib_size(clipboard.data, size);
+if (!dib_size) {
+return NULL;
+}
+HANDLE clip_data = GlobalAlloc(GMEM_MOVEABLE, dib_size);
+if (clip_data) {
+uint8_t* dst = (uint8_t*)GlobalLock(clip_data);
+if (!dst) {
+GlobalFree(clip_data);
+return NULL;
+}
+coder->get_dib_data(dst, clipboard.data, size);
+GlobalUnlock(clip_data);
+}
 return clip_data;
 }
 
@@ -57,32 +66,115 @@ uint8_t* get_raw_clipboard_image(const 
VDAgentClipboardRequest& clipboard_reques
 {
 new_size = 0;
 
-CxImage image;
-uint8_t *new_data = NULL;
-DWORD cximage_format = 

[Spice-devel] [vdagent-win PATCH v8 0/5] Rewrite image support

[Frediano Ziglio]

CxImage is used for image conversion for clipboard support.
CxImage have currently some issue:
- library is old and unsupported;
- required an old libpng library.
Currently the MingW binary we distribute due to some
issue have PNG disabled (so no clipboard image support).
Note that currently we support (before and after this patch)
only BMP and PNG. PNG is required as the default agent format.
However Windows programs wants to have BMP (specifically DIB)
in the clipboard.
This patch remove CxImage and use directly libpng (BMP is
supported directly by Windows APIs).
Tested with various formats and the compiled agent with a
Linux client.
The main concern is actually the possible problem to
support some weird/old BMP file formats however I don't
think any actual program will handle BMP2 (BMP3 was
introduced with Windows 3 probably 30 years ago).

The test patch add a test helper executable and a script
to test PNG encoding.

Changes since v7:
- use png_set_bgr to avoid manually swap pixel components;
- use "unsigned int" instead of unsigned;
- use png_error instead of a manual longjmp;
- moved some code hunks in early patches;
- minor style changes.

Changes since v6:
- minor style changes;
- merge small change to imagetest from Uri (usage error).

Changes since v5:
- remove jpeg dependencies, were used just to satisfy
  CxImage but not used in the code.

Changes since v4:
- remove tiff dependency, was just for CxImage.

Changes since v3:
- reuse code to compute DIB stride;
- update documentation comments;
- update copyright lines.

Changes since v2:
- updated copyright lines;
- update some function documentation;
- fixed 2 bit PNGs;
- removed RFC from tests;
- improved test adding different images;
- merged Uri fix for memory.

Frediano Ziglio (5):
  Move image handling to a separate file
  Initial rewrite of image conversion code
  Write code to decode PNG format
  Support encoding PNG images
  Add test for PNG files

 Makefile.am |  28 +++-
 configure.ac|   5 +-
 mingw-spice-vdagent.spec.in |  10 +-
 test-png|  50 +++
 vdagent/image.cpp   | 174 +++
 vdagent/image.h |  69 +
 vdagent/imagepng.cpp| 339 
 vdagent/imagepng.h  |  25 
 vdagent/imagetest.cpp   |  82 +++
 vdagent/vdagent.cpp |  57 ++--
 10 files changed, 778 insertions(+), 61 deletions(-)
 create mode 100755 test-png
 create mode 100644 vdagent/image.cpp
 create mode 100644 vdagent/image.h
 create mode 100644 vdagent/imagepng.cpp
 create mode 100644 vdagent/imagepng.h
 create mode 100644 vdagent/imagetest.cpp

-- 
2.13.3

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

Re: [Spice-devel] [vdagent-win PATCH v6 2/5] Initial rewrite of image conversion code

[Christophe de Dinechin]

> On 18 Jul 2017, at 17:28, Christophe Fergeau  wrote:
> 
> On Mon, Jul 17, 2017 at 11:01:22AM +0100, Frediano Ziglio wrote:
>> Remove CxImage linking.
>> Support Windows BMP format.
> 
> Too bad there is no small/maintained library which would do that for us
> :-/ From a quick glance, looks ok.
> 
> 
>> 
>> +static inline size_t compute_dib_stride(unsigned width, unsigned bit_count)
> 
> Can you use full type names, unsigned int?

No. Really, no ;-) Otherwise, for consistency, you should replace ‘int’ with 
‘signed int’, specify whether char are signed or unsigned, and use ’signed long 
int’ instead of ‘long’. And while we are at it, decide whether “int long 
unsigned typedef ulong” is evil, or whether it’s the epitome of C syntactic 
elegance.


Christophe 
> 
> Christophe
> 
>> +{
>> +return ((width * bit_count + 31u) & ~31u) / 8u;
>> +}
>> +
>> /**
>>  * Returns image to put in the clipboard.
>>  *
>> -- 
>> 2.13.3
>> 
>> ___
>> 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

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

Re: [Spice-devel] [vdagent-win PATCH v4 3/5] Write code to decode PNG format

[Christophe de Dinechin]

> On 18 Jul 2017, at 17:54, Christophe Fergeau  wrote:
> 
> On Mon, Jul 17, 2017 at 09:54:04AM +0100, Frediano Ziglio wrote:
>> Signed-off-by: Frediano Ziglio 
>> ---
>> Makefile.am  |   6 +-
>> configure.ac |   3 +
>> vdagent/image.cpp|   8 +-
>> vdagent/imagepng.cpp | 244 
>> +++
>> vdagent/imagepng.h   |  25 ++
>> 5 files changed, 277 insertions(+), 9 deletions(-)
>> create mode 100644 vdagent/imagepng.cpp
>> create mode 100644 vdagent/imagepng.h
>> 
>> diff --git a/Makefile.am b/Makefile.am
>> index b35dd57..175d8f7 100644
>> --- a/Makefile.am
>> +++ b/Makefile.am
>> @@ -23,8 +23,8 @@ LIBS = -lversion
>> 
>> bin_PROGRAMS = vdagent vdservice
>> 
>> -vdagent_LDADD = -lwtsapi32 -lgdi32 vdagent_rc.$(OBJEXT)
>> -vdagent_CXXFLAGS = $(AM_CXXFLAGS)
>> +vdagent_LDADD = $(LIBPNG_LIBS) $(ZLIB_LIBS) -lwtsapi32 -lgdi32 
>> vdagent_rc.$(OBJEXT)
>> +vdagent_CXXFLAGS = $(AM_CXXFLAGS) $(LIBPNG_CFLAGS)
>> vdagent_LDFLAGS = $(AM_LDFLAGS) -Wl,--subsystem,windows
>> vdagent_SOURCES =\
>>  common/vdcommon.cpp \
>> @@ -44,6 +44,8 @@ vdagent_SOURCES =  \
>>  vdagent/as_user.h   \
>>  vdagent/image.cpp   \
>>  vdagent/image.h \
>> +vdagent/imagepng.cpp\
>> +vdagent/imagepng.h  \
>>  $(NULL)
>> 
>> vdagent_rc.$(OBJEXT): vdagent/vdagent.rc
>> diff --git a/configure.ac b/configure.ac
>> index 4eac4b4..bb33075 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -101,6 +101,9 @@ dnl 
>> ---
>> dnl - Check library dependencies
>> dnl 
>> ---
>> 
>> +PKG_CHECK_MODULES(LIBPNG, [libpng])
>> +PKG_CHECK_MODULES(ZLIB, [zlib])
>> +
>> dnl 
>> ---
>> dnl - Makefiles, etc.
>> dnl 
>> ---
>> diff --git a/vdagent/image.cpp b/vdagent/image.cpp
>> index b32da6f..c968217 100644
>> --- a/vdagent/image.cpp
>> +++ b/vdagent/image.cpp
>> @@ -21,9 +21,9 @@
>> 
>> #include "vdcommon.h"
>> #include "image.h"
>> +#include "imagepng.h"
>> 
>> ImageCoder *create_bitmap_coder();
>> -ImageCoder *create_png_coder();
>> 
>> static ImageCoder *get_coder(uint32_t vdagent_type)
>> {
>> @@ -172,9 +172,3 @@ ImageCoder *create_bitmap_coder()
>> {
>> return new BitmapCoder();
>> }
>> -
>> -// TODO
>> -ImageCoder *create_png_coder()
>> -{
>> -return NULL;
>> -}
>> diff --git a/vdagent/imagepng.cpp b/vdagent/imagepng.cpp
>> new file mode 100644
>> index 000..19d9efc
>> --- /dev/null
>> +++ b/vdagent/imagepng.cpp
>> @@ -0,0 +1,244 @@
>> +/*
>> +   Copyright (C) 2017 Red Hat, Inc.
>> +
>> +   This program is free software; you can redistribute it and/or
>> +   modify it under the terms of the GNU General Public License as
>> +   published by the Free Software Foundation; either version 2 of
>> +   the License, or (at your option) any later version.
>> +
>> +   This program is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +   GNU General Public License for more details.
>> +
>> +   You should have received a copy of the GNU General Public License
>> +   along with this program.  If not, see .
>> +*/
>> +
>> +#include "vdcommon.h"
>> +
>> +#include 
>> +#include 
>> +
>> +#include "imagepng.h"
>> +
>> +class PngCoder: public ImageCoder
>> +{
>> +public:
>> +PngCoder() {};
>> +size_t get_dib_size(const uint8_t *data, size_t size);
>> +void get_dib_data(uint8_t *dib, const uint8_t *data, size_t size);
>> +uint8_t *from_bitmap(const BITMAPINFO& info, const void *bits, long 
>> );
>> +private:
>> +size_t convert_to_dib(uint8_t *out_buf, const uint8_t *data, size_t 
>> size);
>> +};
>> +
>> +struct BufferIo {
>> +uint8_t *buf;
>> +uint32_t pos, size;
>> +};
>> +
>> +static void read_from_bufio(png_structp png, png_bytep out, png_size_t size)
>> +{
>> +BufferIo& io(*(BufferIo*)png_get_io_ptr(png));

As long as you do a cast form a pointer, I would rather use a pointer to 
BufferIo here.

>> +if (io.pos + size >= io.size)
>> +png_error(png, "read past end");
>> +memcpy(out, io.buf+io.pos, size);

This memcpy should be in ‘else’, unless png_error interrupts the flow of 
control (I saw a png_jumpbuf below that frightens me).

>> +io.pos += size;
>> +}
>> +
>> +size_t PngCoder::get_dib_size(const uint8_t *data, size_t size)
>> +{
>> +return convert_to_dib(NULL, data, size);
>> +}
>> +
>> +typedef void line_fixup_t(uint8_t *line, unsigned width);
>> +
>> +static void line_fixup_none(uint8_t *line, unsigned