[Spice-devel] streaming assertion

[Uri Lublin]

On 05/20/2014 11:19 PM, Jonathon Jongsma wrote:
> I've been investigating bug 1086820 that results in a failed assertion in spice-server (causing the guest to exit) related to mjpeg streaming. This is the debug output when we hit this issue:
>
> (/usr/bin/qemu-kvm:3165): Spice-ERROR **: mjpeg_encoder.c:627:mjpeg_encoder_adjust_params_to_bit_rate: assertion `rate_control->num_recent_enc_frames' failed
>
> Here's what I know so far:
> - mjpeg_encoder_adjust_params_to_bit_rate() is actually called quite often with num_recent_enc_frames set to 0. But usually when num_recent_enc_frames is 0, last_enc_size is also 0, so we bail out of the function early and print a debug message "missing sample size".
> - Under some circumstances, mjpeg_encoder_reset_quality() gets called with quality_id set to the same quality id that we're currently using. The code anticipates this possibility and has a test for it: if the new quality ID is the same as the old, we don't clear last_enc_size. But we do still clear num_recent_enc_frames. This is where we become susceptible to the assert mentioned above.  Now last_enc_size is non-zero, but num_recent_enc_frames is 0.
>
> It seems to me that these two values should probably be cleared together. But I'm not sure whether it is more correct to clear them or *not* clear them when new quality == old quality.
>
> I've tested with both of the following alternative patches, and they both seem to work properly and avoid the assert. I'd appreciate input from somebody with more experience with spice-server streaming code.
>
> Jonathon
>
> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1086820


Hi Jonathon,

The first patch fixes the assert problem, but changes the logic.
It probably affects e.g. the calculation of  new_avg_enc_size and new_fps.

The second patch may be missing another place where last_enc_size is set 
to 0 (look
for "Not enough space").

A third option is to return from the function if 
!rate_control->num_recent_enc_frames
similar to !rate_control->last_enc_size (see a patch below).

I'm not sure which one is better.

Thanks,
     Uri.

----

diff --git a/server/mjpeg_encoder.c b/server/mjpeg_encoder.c
index aea4964..4e628c9 100644
--- a/server/mjpeg_encoder.c
+++ b/server/mjpeg_encoder.c
@@ -624,7 +624,10 @@ static void 
mjpeg_encoder_adjust_params_to_bit_rate(MJpegEn
          return;
      }

-    spice_assert(rate_control->num_recent_enc_frames);
+    if (!rate_control->num_recent_enc_frames)
+        spice_debug("missing recent sample");
+        return;
+    }

      if (rate_control->num_recent_enc_frames < MJPEG_AVERAGE_SIZE_WINDOW &&
          rate_control->num_recent_enc_frames < rate_control->fps) {