[Spice-devel] [PATCH v2 spice-server] gstreamer-encoder: fix compiler warning with Fedora 30

[Kevin Pouget]

On Wed, Jul 3, 2019 at 2:47 PM Frediano Ziglio <fziglio at redhat.com> wrote:
>
> >
> > Fedora 30 / gcc 9.1.1 20190503 (Red Hat 9.1.1-1) fails to build
> > because of this error/warning:
> >
> > > gstreamer-encoder.c: In function 'set_video_bit_rate':
> > > gstreamer-encoder.c:518:17: error: taking the absolute value of
> > > unsigned type 'uint64_t' {aka 'long unsigned int'} has no effect
> > > [-Werror=absolute-value]
> > >  518 |     } else  if (abs(bit_rate - encoder->video_bit_rate) >
> > >  encoder->video_bit_rate * SPICE_GST_VIDEO_BITRATE_MARGIN) {
> > >      |                 ^~~
> > > gstreamer-encoder.c:518:17: error: absolute value function 'abs'
> > > given an argument of type 'uint64_t' {aka 'long unsigned int'}
> >
> > This patches solves these two warnings:
> >
> > 1) cast the substraction to a signed type (int64_t instead of
> > uint64_t) to preserve the operation meaning;
> >
> > 2) use a custom version of abs() to avoid data truncation and/or
> > platform-dependent type lengths (abs/labs/llabs)
> > ---
> >  server/gstreamer-encoder.c | 2 +-
> >  server/utils.h             | 5 +++++
> >  2 files changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
> > index 6416b688..54aa1963 100644
> > --- a/server/gstreamer-encoder.c
> > +++ b/server/gstreamer-encoder.c
> > @@ -515,7 +515,7 @@ static void set_video_bit_rate(SpiceGstEncoder *encoder,
> > uint64_t bit_rate)
> >          encoder->video_bit_rate = bit_rate;
> >          set_gstenc_bitrate(encoder);
> >
> > -    } else  if (abs(bit_rate - encoder->video_bit_rate) >
> > encoder->video_bit_rate * SPICE_GST_VIDEO_BITRATE_MARGIN) {
> > +    } else  if (i64abs((int64_t)(bit_rate - encoder->video_bit_rate)) >
> > encoder->video_bit_rate * SPICE_GST_VIDEO_BITRATE_MARGIN) {
>
> Is it fine for you if I split the line at 100 characters (after " >") ?

yes, sure
I was confused with the line length limitation, but I saw in the
coding style that it's 100,
I understand that lines longer than that get cut patch by patch!

> >          encoder->video_bit_rate = bit_rate;
> >          set_pipeline_changes(encoder, SPICE_GST_VIDEO_PIPELINE_BITRATE);
> >      }
> > diff --git a/server/utils.h b/server/utils.h
> > index 54bc9d49..a54d5433 100644
> > --- a/server/utils.h
> > +++ b/server/utils.h
> > @@ -75,4 +75,9 @@ int red_channel_name_to_type(const char *name);
> >
> >  void red_dump_openssl_errors(void);
> >
> > +static inline int64_t i64abs(int64_t value)
> > +{
> > +    return (value >= 0) ? value : -value;
> > +}
> > +
> >  #endif /* UTILS_H_ */
>
> Otherwise patch is fine for me.

thanks