[spice-server,v2] clang: fix warning and possible wrong computation

Submitted by Victor Toso on Dec. 21, 2017, 3:04 p.m.

Details

Message ID 20171221150455.10462-1-victortoso@redhat.com
State New
Headers show
Series "misc clang fixes" ( rev: 2 ) in Spice

Not browsing as part of any series.

Commit Message

Victor Toso Dec. 21, 2017, 3:04 p.m.
From: Victor Toso <me@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.

> 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) {
>            ^~~

Signed-off-by: Victor Toso <victortoso@redhat.com>
---
 server/gstreamer-encoder.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
index 04f0c02f..febce911 100644
--- a/server/gstreamer-encoder.c
+++ b/server/gstreamer-encoder.c
@@ -505,12 +505,18 @@  static void set_gstenc_bitrate(SpiceGstEncoder *encoder);
 
 static void set_video_bit_rate(SpiceGstEncoder *encoder, uint64_t bit_rate)
 {
+    uint64_t diff, margin;
+
     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 = labs((int64_t) bit_rate - (int64_t) encoder->video_bit_rate);
+    margin = encoder->video_bit_rate * SPICE_GST_VIDEO_BITRATE_MARGIN;
+    if (diff > margin) {
         encoder->video_bit_rate = bit_rate;
         set_pipeline_changes(encoder, SPICE_GST_VIDEO_PIPELINE_BITRATE);
     }

Comments

On Thu, Dec 21, 2017 at 04:04:55PM +0100, Victor Toso wrote:
> From: Victor Toso <me@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.
> 
> > 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) {
> >            ^~~
> 
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>  server/gstreamer-encoder.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
> index 04f0c02f..febce911 100644
> --- a/server/gstreamer-encoder.c
> +++ b/server/gstreamer-encoder.c
> @@ -505,12 +505,18 @@ static void set_gstenc_bitrate(SpiceGstEncoder *encoder);
>  
>  static void set_video_bit_rate(SpiceGstEncoder *encoder, uint64_t bit_rate)
>  {
> +    uint64_t diff, margin;
> +

Actually, margin should be a gdouble.

>      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 = labs((int64_t) bit_rate - (int64_t) encoder->video_bit_rate);
> +    margin = encoder->video_bit_rate * SPICE_GST_VIDEO_BITRATE_MARGIN;
> +    if (diff > margin) {
>          encoder->video_bit_rate = bit_rate;
>          set_pipeline_changes(encoder, SPICE_GST_VIDEO_PIPELINE_BITRATE);
>      }
> -- 
> 2.15.1
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel