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) {
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel