[Spice-devel] streaming assertion

[Jonathon Jongsma]

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 at redhat.com>
> To: "Jonathon Jongsma" <jjongsma at redhat.com>, spice-devel at freedesktop.org
> Sent: Wednesday, May 21, 2014 12:00:03 PM
> Subject: Re: [Spice-devel] 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) {
> 
> 
> 
>