[Spice-devel] streaming assertion

Submitted by Uri Lublin on May 21, 2014, 5 p.m.

Details

Message ID 537CDB93.4070704@redhat.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Uri Lublin May 21, 2014, 5 p.m.
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.

----

          rate_control->num_recent_enc_frames < rate_control->fps) {

Patch hide | download patch | download mbox

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 &&

Comments

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@redhat.com>
> To: "Jonathon Jongsma" <jjongsma@redhat.com>, spice-devel@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) {
> 
> 
> 
>