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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/spice-devel
