[Spice-devel] [spice v10 08/27] server: Add VP8 support to the GStreamer video encoder

[Christophe Fergeau]

On Thu, Mar 03, 2016 at 05:22:58PM +0100, Christophe Fergeau wrote:
> On Thu, Mar 03, 2016 at 09:50:26AM -0500, Frediano Ziglio wrote:
> > > diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
> > > index aae47c7..3dfbdfb 100644
> > > --- a/server/gstreamer-encoder.c
> > > +++ b/server/gstreamer-encoder.c
> > > @@ -188,14 +188,70 @@ static void set_appsrc_caps(SpiceGstEncoder *encoder)
> > >      g_object_set(G_OBJECT(encoder->appsrc), "caps", encoder->src_caps,
> > >      NULL);
> > >  }
> > >  
> > > +static int physical_core_count = 0;
> > > +static int get_physical_core_count(void)
> > > +{
> > > +    if (!physical_core_count) {
> > > +#ifdef HAVE_G_GET_NUMPROCESSORS
> > > +        physical_core_count = g_get_num_processors();
> > > +#endif
> > 
> > you could use sysconf if g_get_num_processors is not detected.
> > 
> > > +        if (system("egrep -l '^flags\\b.*: .*\\bht\\b' /proc/cpuinfo
> > > >/dev/null 2>&1") == 0) {
> > > +            /* Hyperthreading is enabled so divide by two to get the number
> > > +             * of physical cores.
> > > +             */
> > > +            physical_core_count = physical_core_count / 2;
> > > +        }
> > 
> > Or as you are using /proc/cpuinfo detect from it.
> > 
> > I'm not sure we should consider compatibility with other processors (like ARM).
> 
> In general, I don't think we want to silently use as many cores as
> possible for cpu intensive work. This will break down as soon as 2 VMs
> trigger the streaming code at the same time, they both will be trying to
> use all the available physical cores. Even with a single VM, it could
> use 4 vcpus and do CPU intensive tasks on some of them, thus the vp8
> encoder would fight for these resources.
> I understand in some controlled setups you may want to give more threads
> to the encoder, but I think doing it automatically is going to cause us
> some issues. I'm not sure what is the best way of making this
> configurable is though :( Can you split this patch for now so that the
> rest of the vp8 support can go in?

Apart from this, looks good to me

Acked-by: Christophe Fergeau <cfergeau at redhat.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20160303/7a5532b3/attachment.sig>