After spending some more time trying to understand the subtleties of this code, I think that perhaps returning early (as in your third option) is probably simplest approach. Jonathon ----- Original Message ----- > From: "Uri Lublin" <uril@xxxxxxxxxx> > To: "Jonathon Jongsma" <jjongsma@xxxxxxxxxx>, spice-devel@xxxxxxxxxxxxxxx > Sent: Wednesday, May 21, 2014 12:00:03 PM > Subject: Re: streaming assertion > > 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