[Spice-devel] [spice-server v1 2/3] clang: Fix possible wrong computation

[Victor Toso]

Hi,

On Thu, Dec 21, 2017 at 09:48:10AM -0500, Frediano Ziglio wrote:
> > 
> > From: Victor Toso <me at victortoso.com>
> > 
> > Clang's warning about absolute value below is not entirely correct as
> > the difference between the two bitrate values could be negative but
> > using abs() could be wrong.
> > 
> > Wrapping between MAX() - MIN() is the simplest option.
> > 
> > > gstreamer-encoder.c:513:17: error: taking the absolute value of unsigned
> > > type 'unsigned long' has no effect
> > >     } else  if (abs(bit_rate - encoder->video_bit_rate) >
> > >     encoder->video_bit_rate * SPICE_GST_VIDEO_BITRATE_MARGIN) {
> > >                 ^
> > > gstreamer-encoder.c:513:17: note: remove the call to 'abs' since unsigned
> > > values cannot be negative
> > >     } else  if (abs(bit_rate - encoder->video_bit_rate) >
> > >     encoder->video_bit_rate * SPICE_GST_VIDEO_BITRATE_MARGIN) {
> > >                 ^~~
> >                                         1
> > Signed-off-by: Victor Toso <victortoso at redhat.com>
> > ---
> >  server/gstreamer-encoder.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
> > index 04f0c02f..869acf11 100644
> > --- a/server/gstreamer-encoder.c
> > +++ b/server/gstreamer-encoder.c
> > @@ -505,12 +505,17 @@ static void set_gstenc_bitrate(SpiceGstEncoder
> > *encoder);
> >  
> >  static void set_video_bit_rate(SpiceGstEncoder *encoder, uint64_t bit_rate)
> >  {
> > +    uint64_t diff_rate;
> > +
> >      if (encoder->video_bit_rate != bit_rate &&
> >          encoder->gstenc_bitrate_is_dynamic) {
> >          encoder->video_bit_rate = bit_rate;
> >          set_gstenc_bitrate(encoder);
> > +        return;
> > +    }
> >  
> > -    } else  if (abs(bit_rate - encoder->video_bit_rate) >
> > encoder->video_bit_rate * SPICE_GST_VIDEO_BITRATE_MARGIN) {
> > +    diff_rate = MAX(bit_rate, encoder->video_bit_rate) - MIN(bit_rate,
> > encoder->video_bit_rate);
> > +    if (diff_rate > encoder->video_bit_rate *
> > SPICE_GST_VIDEO_BITRATE_MARGIN) {
> 
> Why not abs((int64_t) bit_rate - (int64_t) encoder->video_bit_rate) ?

Hmm, should be okay too. I'll send a v2.

Cheers,
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20171221/45ee7bf2/attachment.sig>