[Spice-devel] [spice v10 16/27] server: Respect the GStreamer encoder's valid bit rate range

[Francois Gouget]

On Fri, 4 Mar 2016, Christophe Fergeau wrote:

> On Tue, Mar 01, 2016 at 04:55:01PM +0100, Francois Gouget wrote:
> > Otherwise it may get wrapped to a much lower value than intended.
> 
> I don't understand this comment. If we go lower or higher than the range
> of the bitrate property, I'd expect that it's going to be clamped to
> either the minimum or the maximum, but a runtime warning will be issued.

That's a reasonable expectation!
But it's not what happens. For instance with x264enc:

set_gstenc_bitrate: current bitrate=4096
set_gstenc_bitrate: bitrate=2172292 1 - 2048000
set_gstenc_bitrate: setting the GStreamer bitrate to 2172292
(Xorg:7957): GLib-GObject-WARNING **: value "2172292" of type 'guint' is invalid or out of range for property 'bitrate' of type 'guint'
set_gstenc_bitrate: new bitrate=4096

I expect this is a general GObject behavior: when given an out of range 
value g_object_set() returns an error instead of clamping the value. So 
if we did not do the clamping ourselves the bitrate would remain 
unchanged.

This is a major issue because the encoders typically have pretty low 
default bitrates, 2Mbps for x264enc. Also the maximum bitrate is pretty 
low for x264enc: 2 million now but until recently, in GStreamer 1.x, it 
was around 100000, i.e. 100Mbps. This can easily be lower than Spice's 
initial bandwidth estimate when running on lo, in which case x264enc 
would end up using 2Mbps instead, resulting in poor quality.


[...]
> > +        case G_TYPE_INT64: {
> > +            GParamSpecInt64 *range = G_PARAM_SPEC_INT64(param);
> > +            gst_bit_rate = MAX(range->minimum, MIN(range->maximum, gst_bit_rate));
> > +            break;
> > +        }
> > +        default:
> > +            spice_debug("the %s property has an unsupported type %zu",
> > +                        g_param_spec_get_name(param), param->value_type);
> > +        }
> > +        spice_debug("setting the GStreamer %s to %"PRIu64,
> > +                    g_param_spec_get_name(param), gst_bit_rate);
> > +        g_object_set(G_OBJECT(encoder->gstenc),
> > +                     g_param_spec_get_name(param), gst_bit_rate,
> > +                     NULL);
> 
> Hmm too bad we have to go through such complications :(
> 
> I'm not sure this is 100% correct though, you are passing a 64 bit 
> gst_bit_rate to g_object_set, I think a 32 bit integer is usually 
> expected for G_TYPE_INT/G_TYPE_UINT. This will probably be fine on 
> little-endian arches, but I'm not sure it's going to be ok on 
> big-endian.

You're right, the set operation needs to be tailored for the type.


> One way around it could be to use g_object_set_property() rather than
> g_object_set().

g_object_set_property() takes a GValue which does not seem like it's 
going to make things simpler.


> Or it seems all these bitrate properties are int anyway, no int64 
> ones,

ffenc_mjpeg takes an uint64_t.

It's tempting to try to hardcode things with a switch on MJPEG/VP8/H264 
and an #ifdef for 0.10. But as mentioned above the x264enc limits have 
changed between GStreamer 1.x versions so a query needs to be done to 
get the range for it anyway. Then it's simpler to just go the generic 
way.



-- 
Francois Gouget <fgouget at codeweavers.com>