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@xxxxxxxxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel