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 <[email protected]> > --- > 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
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/spice-devel
