[Spice-devel] [spice v10 13/27] server: Shape the bit rate of the GStreamer codecs output

[Christophe Fergeau]

On Tue, Mar 01, 2016 at 04:54:03PM +0100, Francois Gouget wrote:
> The GStreamer codecs don't follow the specified bit rate very closely:
> they can decide to exceed it for ten seconds or more if they consider
> the scene deserves it. Such long bursts are enough to cause network
> congestion, resulting in many lost frames which cause significant
> display corruption.
> So the GStreamer video encoder now uses a short 300ms virtual buffer
> to shape the compressed video output and ensure we don't exceed the
> target bit rate for any significant length of time.
> It could instead rely on the network feedback (when available) to lower
> the bit rate. However frequent GStreamer bit rate changes lower the
> overall compression level and also result in a lower average bit rate,
> both of which result in lower video quality.
> The GStreamer video encoder also keeps track of the encoded frame size
> so it can gather statistics and call update_client_playback_delay()
> with accurate information and also annotate the client report debug
> traces with the corresponding bit rate information.
> 
> Signed-off-by: Francois Gouget <fgouget at codeweavers.com>
> ---
>  server/gstreamer-encoder.c | 291 +++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 284 insertions(+), 7 deletions(-)
> 
> diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
> index f3fef41..3bf66fa 100644
> --- a/server/gstreamer-encoder.c
> +++ b/server/gstreamer-encoder.c
> @@ -26,6 +26,7 @@
>  
>  #include "red-common.h"
>  #include "video-encoder.h"
> +#include "utils.h"
>  
>  
>  #define SPICE_GST_DEFAULT_FPS 30
> @@ -45,6 +46,11 @@ typedef struct SpiceGstVideoBuffer {
>      GstMapInfo map;
>  } SpiceGstVideoBuffer;
>  
> +typedef struct {
> +    uint32_t mm_time;
> +    uint32_t size;
> +} SpiceGstFrameInformation;
> +
>  typedef struct SpiceGstEncoder {
>      VideoEncoder base;
>  
> @@ -81,6 +87,45 @@ typedef struct SpiceGstEncoder {
>      /* The frame counter for GStreamer buffers */
>      uint32_t frame;
>  
> +
> +    /* ---------- Encoded frame statistics ---------- */
> +
> +    /* Should be >= than FRAME_STATISTICS_COUNT. This is also used to annotate
> +     * the client report debug traces with bit rate information.
> +     */
> +#   define SPICE_GST_HISTORY_SIZE 60
> +
> +    /* A circular buffer containing the past encoded frames information. */
> +    SpiceGstFrameInformation history[SPICE_GST_HISTORY_SIZE];
> +
> +    /* The indices of the oldest and newest frames in the history buffer. */
> +    uint32_t history_first;
> +    uint32_t history_last;
> +
> +    /* How many frames to take into account when computing the effective
> +     * bit rate, average frame size, etc. This should be large enough so the
> +     * I and P frames average out, and short enough for it to reflect the
> +     * current situation.
> +     */
> +#   define SPICE_GST_FRAME_STATISTICS_COUNT 21
> +
> +    /* The index of the oldest frame taken into account for the statistics. */
> +    uint32_t stat_first;
> +
> +    /* Used to compute the average frame size. */
> +    uint64_t stat_sum;
> +
> +    /* Tracks the maximum frame size. */
> +    uint32_t stat_maximum;
> +
> +
> +    /* ---------- Encoder bit rate control ----------
> +     *
> +     * GStreamer encoders don't follow the specified bit rate very
> +     * closely. These fields are used to ensure we don't exceed the desired
> +     * stream bit rate, regardless of the GStreamer encoder's output.
> +     */
> +
>      /* The bit rate target for the outgoing network stream. (bits per second) */
>      uint64_t bit_rate;
>  
> @@ -89,6 +134,27 @@ typedef struct SpiceGstEncoder {
>  
>      /* The default bit rate */
>  #   define SPICE_GST_DEFAULT_BITRATE (8 * 1024 * 1024)
> +
> +    /* The bit rate control is performed using a virtual buffer to allow short
> +     * term variations: bursts are allowed until the virtual buffer is full.
> +     * Then frames are dropped to limit the bit rate. VBUFFER_SIZE defines the
> +     * size of the virtual buffer in milliseconds worth of data.
> +     */
> +#   define SPICE_GST_VBUFFER_SIZE 300
> +
> +    int32_t vbuffer_size;
> +    int32_t vbuffer_free;
> +
> +    /* When dropping frames, this is set to the minimum mm_time of the next
> +     * frame to encode. Otherwise set to zero.
> +     */
> +    uint32_t next_frame;

I would name it next_frame_mm_time otherwise I get a weird mismatch in
my head when I see operations like
next_frame + get_mm_time(..); (why are we adding frame data with a time
?)

> +
> +    /* Defines the minimum allowed fps. */
> +#   define SPICE_GST_MAX_PERIOD (NSEC_PER_SEC / 3)
> +
> +    /* How big of a margin to take to cover for latency jitter. */
> +#   define SPICE_GST_LATENCY_MARGIN 0.1
>  } SpiceGstEncoder;
>  
>  
> @@ -125,6 +191,18 @@ static uint32_t get_source_fps(SpiceGstEncoder *encoder)
>          encoder->cbs.get_source_fps(encoder->cbs.opaque) : SPICE_GST_DEFAULT_FPS;
>  }
>  
> +static uint32_t get_network_latency(SpiceGstEncoder *encoder)
> +{
> +    /* Assume that the network latency is symmetric */
> +    return encoder->cbs.get_roundtrip_ms ?
> +        encoder->cbs.get_roundtrip_ms(encoder->cbs.opaque) / 2 : 0;
> +}
> +
> +static inline int rate_control_is_active(SpiceGstEncoder* encoder)
> +{
> +    return encoder->cbs.get_roundtrip_ms != NULL;
> +}
> +
>  static inline int is_pipeline_configured(SpiceGstEncoder *encoder)
>  {
>      return encoder->src_caps != NULL;
> @@ -146,6 +224,182 @@ static void free_pipeline(SpiceGstEncoder *encoder)
>      }
>  }
>  
> +
> +/* ---------- Encoded frame statistics ---------- */
> +
> +static inline uint32_t get_last_frame_mm_time(SpiceGstEncoder *encoder)
> +{
> +    return encoder->history[encoder->history_last].mm_time;
> +}
> +
> +/* Returns the current bit rate based on the last
> + * SPICE_GST_FRAME_STATISTICS_COUNT frames.
> + */
> +static uint64_t get_effective_bit_rate(SpiceGstEncoder *encoder)
> +{
> +    uint32_t elapsed = encoder->history[encoder->history_last].mm_time -
> +        encoder->history[encoder->stat_first].mm_time;
> +    if (encoder->next_frame) {
> +        elapsed += encoder->next_frame - get_last_frame_mm_time(encoder);

I don't know if this can be made a bit more straightforward, but elapsed
is initially 
uint32_t elapsed = encoder->history[encoder->history_last].mm_time - encoder->history[encoder->stat_first].mm_time;
then we substract get_last_frame_mm_time(encoder), which turns out to be encoder->history[encoder->history_last].mm_time;
as well.
Should get_last_frame_mm_time have been used in the inital computation
of elapsed?
uint32_t elapsed = get_last_frame_mm_time(encoder) - encoder->history[encoder->stat_first].mm_time;

Should this be changed to just elapsed = encoder->next_frame - encoder->history[encoder->stat_first].mm_time; ?

Looks good to me otherwise, though I tend to get lost in all these
calculations on a circular buffer. Let's say it's fine :)

Christophe

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20160304/f43c9798/attachment-0001.sig>