[Spice-devel,1/12] server: Convert a couple of rate control checks into asserts in the mjpeg video encoder. (take 3)

Submitted by Francois Gouget on June 10, 2015, 3:33 p.m.

Details

Message ID alpine.DEB.2.20.1506101053330.23792@amboise
State New
Headers show

Not browsing as part of any series.

Commit Message

Francois Gouget June 10, 2015, 3:33 p.m.
The checks would lead the reader to think these functions can be called when bit rate control is off when in fact they are only called when it is active.

Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
---

This patch stands on its own and I think it makes sense even if the 
remainder of the series is not applied. It seemed ok in round 1 so 
again no change this time around. Let me know if it needs further 
refinements for inclusion.

 server/mjpeg_encoder.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

Patch hide | download patch | download mbox

diff --git a/server/mjpeg_encoder.c b/server/mjpeg_encoder.c
index 12447da..95d841f 100644
--- a/server/mjpeg_encoder.c
+++ b/server/mjpeg_encoder.c
@@ -607,9 +607,7 @@  static void mjpeg_encoder_adjust_params_to_bit_rate(MJpegEncoder *encoder)
     uint32_t latency = 0;
     uint32_t src_fps;
 
-    if (!encoder->rate_control_is_active) {
-        return;
-    }
+    spice_assert(encoder->rate_control_is_active);
 
     rate_control = &encoder->rate_control;
     quality_eval = &rate_control->quality_eval_data;
@@ -694,9 +692,8 @@  static void mjpeg_encoder_adjust_fps(MJpegEncoder *encoder, uint64_t now)
     MJpegEncoderRateControl *rate_control = &encoder->rate_control;
     uint64_t adjusted_fps_time_passed;
 
-    if (!encoder->rate_control_is_active) {
-        return;
-    }
+    spice_assert(encoder->rate_control_is_active);
+
     adjusted_fps_time_passed = (now - rate_control->adjusted_fps_start_time) / 1000 / 1000;
 
     if (!rate_control->during_quality_eval &&

Comments

On Wed, 10 Jun 2015, Francois Gouget wrote:

> The checks would lead the reader to think these functions can be called when bit rate control is off when in fact they are only called when it is active.
> 
> Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
> ---
> 
> This patch stands on its own and I think it makes sense even if the 
> remainder of the series is not applied. It seemed ok in round 1 so 
> again no change this time around. Let me know if it needs further 
> refinements for inclusion.

This patch was acked a while ago but has not been committed yet.
Does it need changes? Should I drop it from the series?
(Same thing for the next one, 2/12)

http://lists.freedesktop.org/archives/spice-devel/2015-May/019843.html



>  server/mjpeg_encoder.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/server/mjpeg_encoder.c b/server/mjpeg_encoder.c
> index 12447da..95d841f 100644
> --- a/server/mjpeg_encoder.c
> +++ b/server/mjpeg_encoder.c
> @@ -607,9 +607,7 @@ static void mjpeg_encoder_adjust_params_to_bit_rate(MJpegEncoder *encoder)
>      uint32_t latency = 0;
>      uint32_t src_fps;
>  
> -    if (!encoder->rate_control_is_active) {
> -        return;
> -    }
> +    spice_assert(encoder->rate_control_is_active);
>  
>      rate_control = &encoder->rate_control;
>      quality_eval = &rate_control->quality_eval_data;
> @@ -694,9 +692,8 @@ static void mjpeg_encoder_adjust_fps(MJpegEncoder *encoder, uint64_t now)
>      MJpegEncoderRateControl *rate_control = &encoder->rate_control;
>      uint64_t adjusted_fps_time_passed;
>  
> -    if (!encoder->rate_control_is_active) {
> -        return;
> -    }
> +    spice_assert(encoder->rate_control_is_active);
> +
>      adjusted_fps_time_passed = (now - rate_control->adjusted_fps_start_time) / 1000 / 1000;
>  
>      if (!rate_control->during_quality_eval &&
> -- 
> 2.1.4
Hey,

On Mon, Jun 22, 2015 at 06:17:43PM +0200, Francois Gouget wrote:
> On Wed, 10 Jun 2015, Francois Gouget wrote:
> 
> > The checks would lead the reader to think these functions can be called when bit rate control is off when in fact they are only called when it is active.
> > 
> > Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
> > ---
> > 
> > This patch stands on its own and I think it makes sense even if the 
> > remainder of the series is not applied. It seemed ok in round 1 so 
> > again no change this time around. Let me know if it needs further 
> > refinements for inclusion.
> 
> This patch was acked a while ago but has not been committed yet.
> Does it need changes? Should I drop it from the series?
> (Same thing for the next one, 2/12)
> 
> http://lists.freedesktop.org/archives/spice-devel/2015-May/019843.html

Sorry for the delay, I've pushed it now.

Christophe