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

[Christophe Fergeau]

On Sat, Mar 05, 2016 at 10:44:16AM +0100, Francois Gouget wrote:
> 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.

Ah ok, I'd make the comment a bit more detailed then, something like
"Out-of-range values are going to be ignored, which could cause us to
use a much lower default value than intended when we try to use a too high
bitrate value"

> > 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.

It would likely makes things even more complicated, this is just one
potential way of handling the int32 vs int64 case.

> 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.

Fine with me.

Christophe
-------------- 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/20160307/066f414c/attachment.sig>