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
Index: spice-0.12.4/server/mjpeg_encoder.c
===================================================================
--- spice-0.12.4.orig/server/mjpeg_encoder.c
+++ spice-0.12.4/server/mjpeg_encoder.c
@@ -393,6 +393,8 @@ static inline void mjpeg_encoder_reset_q
spice_warning("JMJ-%s[%p] %x old qid:%d, new qid: %d\n", __FUNCTION__, encoder, pthread_self(), encoder->rate_control.quality_id, quality_id);
if (rate_control->quality_id != quality_id) {
rate_control->last_enc_size = 0;
+ rate_control->sum_recent_enc_size = 0;
+ rate_control->num_recent_enc_frames = 0;
}
if (rate_control->quality_eval_data.reason == MJPEG_QUALITY_EVAL_REASON_RATE_CHANGE) {
@@ -416,8 +418,6 @@ static inline void mjpeg_encoder_reset_q
rate_control->adjusted_fps_num_frames = 0;
rate_control->base_enc_size = frame_enc_size;
- rate_control->sum_recent_enc_size = 0;
- rate_control->num_recent_enc_frames = 0;
spice_warning("TEUF-%s[%p] %x: %d\n", __FUNCTION__, encoder,
pthread_self(), encoder->rate_control.num_recent_enc_frames);
spice_warning("TEUF2-%s[%p] %x %d\n", __FUNCTION__, encoder, pthread_self(), encoder->rate_control.last_enc_size);
Index: spice-0.12.4/server/mjpeg_encoder.c
===================================================================
--- spice-0.12.4.orig/server/mjpeg_encoder.c
+++ spice-0.12.4/server/mjpeg_encoder.c
@@ -391,9 +391,6 @@ static inline void mjpeg_encoder_reset_q
rate_control->during_quality_eval = FALSE;
spice_warning("JMJ-%s[%p] %x old qid:%d, new qid: %d\n", __FUNCTION__, encoder, pthread_self(), encoder->rate_control.quality_id, quality_id);
- if (rate_control->quality_id != quality_id) {
- rate_control->last_enc_size = 0;
- }
if (rate_control->quality_eval_data.reason == MJPEG_QUALITY_EVAL_REASON_RATE_CHANGE) {
memset(&rate_control->server_state, 0, sizeof(MJpegEncoderServerState));
@@ -415,6 +412,7 @@ static inline void mjpeg_encoder_reset_q
rate_control->adjusted_fps_start_time = 0;
rate_control->adjusted_fps_num_frames = 0;
rate_control->base_enc_size = frame_enc_size;
+ rate_control->last_enc_size = 0;
rate_control->sum_recent_enc_size = 0;
rate_control->num_recent_enc_frames = 0;
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel