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

[Frediano Ziglio]

> 
> 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) ?

>          encoder->video_bit_rate = bit_rate;
>          set_pipeline_changes(encoder, SPICE_GST_VIDEO_PIPELINE_BITRATE);
>      }

Frediano