[Spice-devel,v6] server: Provide a framerate estimate based on the initial frames

Submitted by Francois Gouget on Dec. 2, 2015, 2:36 p.m.

Details

Message ID alpine.DEB.2.20.1512021533250.31357@amboise
State New
Headers show
Series "server: Provide a framerate estimate based on the initial frames" ( rev: 5 ) in Spice

Not browsing as part of any series.

Commit Message

Francois Gouget Dec. 2, 2015, 2:36 p.m.
This way the video encoder can actually count on a real estimate when
it is initializing.
Note that the server only creates a video stream if at least 20 bitmap
'blits' of the same size and type arrive, each within a maximum time
interval from the previous one. So it was only keeping track of the
frame to frame interval. Thus to get an average frame rate over all the
20 frames it's necessary to also keep track of the first_frame_time.

Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
---
 server/display-channel.c |  2 +-
 server/display-channel.h |  1 +
 server/stream.c          | 21 ++++++++++++++++++---
 server/stream.h          |  1 +
 4 files changed, 21 insertions(+), 4 deletions(-)

No change from v5 except for a more detailed commit log and updates to 
keep up with the Spice refactoring.

Patch hide | download patch | download mbox

diff --git a/server/display-channel.c b/server/display-channel.c
index 580025f..bcf16c7 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -1112,7 +1112,7 @@  Drawable *display_channel_drawable_try_new(DisplayChannel *display,
 
     bzero(drawable, sizeof(Drawable));
     drawable->refs = 1;
-    drawable->creation_time = red_get_monotonic_time();
+    drawable->creation_time = drawable->first_frame_time =red_get_monotonic_time();
     ring_item_init(&drawable->list_link);
     ring_item_init(&drawable->surface_list_link);
     ring_item_init(&drawable->tree_item.base.siblings_link);
diff --git a/server/display-channel.h b/server/display-channel.h
index 69d287a..7a63369 100644
--- a/server/display-channel.h
+++ b/server/display-channel.h
@@ -66,6 +66,7 @@  struct Drawable {
     Ring glz_ring;
 
     red_time_t creation_time;
+    red_time_t first_frame_time;
     int frames_count;
     int gradual_frames_count;
     int last_gradual_frame;
diff --git a/server/stream.c b/server/stream.c
index 3bd91b6..6eb54c3 100644
--- a/server/stream.c
+++ b/server/stream.c
@@ -425,7 +425,16 @@  static void display_channel_create_stream(DisplayChannel *display, Drawable *dra
     SpiceBitmap *bitmap = &drawable->red_drawable->u.copy.src_bitmap->u.bitmap;
     stream->top_down = !!(bitmap->flags & SPICE_BITMAP_FLAGS_TOP_DOWN);
     drawable->stream = stream;
-    stream->input_fps = MAX_FPS;
+    /* Provide an fps estimate the video encoder can use when initializing
+     * based on the frames that lead to the creation of the stream. Round to
+     * the nearest integer, for instance 24 for 23.976.
+     */
+    uint64_t duration = drawable->creation_time - drawable->first_frame_time;
+    if (duration > (uint64_t)drawable->frames_count * 1000 * 1000 * 1000 / MAX_FPS) {
+        stream->input_fps = ((uint64_t)drawable->frames_count * 1000 * 1000 * 1000 + duration / 2) / duration;
+    } else {
+        stream->input_fps = MAX_FPS;
+    }
     stream->num_input_frames = 0;
     stream->input_fps_start_time = drawable->creation_time;
     display->streams_size_total += stream->width * stream->height;
@@ -433,21 +442,24 @@  static void display_channel_create_stream(DisplayChannel *display, Drawable *dra
     FOREACH_DCC(display, dcc_ring_item, next, dcc) {
         dcc_create_stream(dcc, stream);
     }
-    spice_debug("stream %d %dx%d (%d, %d) (%d, %d)",
+    spice_debug("stream %d %dx%d (%d, %d) (%d, %d) %u fps",
                 (int)(stream - display->streams_buf), stream->width,
                 stream->height, stream->dest_area.left, stream->dest_area.top,
-                stream->dest_area.right, stream->dest_area.bottom);
+                stream->dest_area.right, stream->dest_area.bottom,
+                stream->input_fps);
     return;
 }
 
 // returns whether a stream was created
 static int stream_add_frame(DisplayChannel *display,
                             Drawable *frame_drawable,
+                            red_time_t first_frame_time,
                             int frames_count,
                             int gradual_frames_count,
                             int last_gradual_frame)
 {
     update_copy_graduality(display, frame_drawable);
+    frame_drawable->first_frame_time = first_frame_time;
     frame_drawable->frames_count = frames_count + 1;
     frame_drawable->gradual_frames_count  = gradual_frames_count;
 
@@ -514,6 +526,7 @@  void stream_trace_update(DisplayChannel *display, Drawable *drawable)
                                        &trace->dest_area, trace->time, NULL, FALSE) !=
                                        STREAM_FRAME_NONE) {
             if (stream_add_frame(display, drawable,
+                                 trace->first_frame_time,
                                  trace->frames_count,
                                  trace->gradual_frames_count,
                                  trace->last_gradual_frame)) {
@@ -559,6 +572,7 @@  void stream_maintenance(DisplayChannel *display,
                                  FALSE);
         if (is_next_frame != STREAM_FRAME_NONE) {
             stream_add_frame(display, candidate,
+                             prev->first_frame_time,
                              prev->frames_count,
                              prev->gradual_frames_count,
                              prev->last_gradual_frame);
@@ -910,6 +924,7 @@  void stream_trace_add_drawable(DisplayChannel *display, Drawable *item)
 
     trace = &display->items_trace[display->next_item_trace++ & ITEMS_TRACE_MASK];
     trace->time = item->creation_time;
+    trace->first_frame_time = item->first_frame_time;
     trace->frames_count = item->frames_count;
     trace->gradual_frames_count = item->gradual_frames_count;
     trace->last_gradual_frame = item->last_gradual_frame;
diff --git a/server/stream.h b/server/stream.h
index c436c2e..752aff0 100644
--- a/server/stream.h
+++ b/server/stream.h
@@ -114,6 +114,7 @@  void                  stream_clip_item_unref                        (DisplayChan
 
 typedef struct ItemTrace {
     red_time_t time;
+    red_time_t first_frame_time;
     int frames_count;
     int gradual_frames_count;
     int last_gradual_frame;

Comments

Could you please review this patch?
It's the same as v5 but rebased on the current source and with a more 
verbose commit log. Iḿ still hoping for inclusion some time this year...

On Wed, 2 Dec 2015, Francois Gouget wrote:

> This way the video encoder can actually count on a real estimate when
> it is initializing.
> Note that the server only creates a video stream if at least 20 bitmap
> 'blits' of the same size and type arrive, each within a maximum time
> interval from the previous one. So it was only keeping track of the
> frame to frame interval. Thus to get an average frame rate over all the
> 20 frames it's necessary to also keep track of the first_frame_time.
> 
> Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
> ---
>  server/display-channel.c |  2 +-
>  server/display-channel.h |  1 +
>  server/stream.c          | 21 ++++++++++++++++++---
>  server/stream.h          |  1 +
>  4 files changed, 21 insertions(+), 4 deletions(-)
> 
> No change from v5 except for a more detailed commit log and updates to 
> keep up with the Spice refactoring.
> 
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 580025f..bcf16c7 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -1112,7 +1112,7 @@ Drawable *display_channel_drawable_try_new(DisplayChannel *display,
>  
>      bzero(drawable, sizeof(Drawable));
>      drawable->refs = 1;
> -    drawable->creation_time = red_get_monotonic_time();
> +    drawable->creation_time = drawable->first_frame_time =red_get_monotonic_time();
>      ring_item_init(&drawable->list_link);
>      ring_item_init(&drawable->surface_list_link);
>      ring_item_init(&drawable->tree_item.base.siblings_link);
> diff --git a/server/display-channel.h b/server/display-channel.h
> index 69d287a..7a63369 100644
> --- a/server/display-channel.h
> +++ b/server/display-channel.h
> @@ -66,6 +66,7 @@ struct Drawable {
>      Ring glz_ring;
>  
>      red_time_t creation_time;
> +    red_time_t first_frame_time;
>      int frames_count;
>      int gradual_frames_count;
>      int last_gradual_frame;
> diff --git a/server/stream.c b/server/stream.c
> index 3bd91b6..6eb54c3 100644
> --- a/server/stream.c
> +++ b/server/stream.c
> @@ -425,7 +425,16 @@ static void display_channel_create_stream(DisplayChannel *display, Drawable *dra
>      SpiceBitmap *bitmap = &drawable->red_drawable->u.copy.src_bitmap->u.bitmap;
>      stream->top_down = !!(bitmap->flags & SPICE_BITMAP_FLAGS_TOP_DOWN);
>      drawable->stream = stream;
> -    stream->input_fps = MAX_FPS;
> +    /* Provide an fps estimate the video encoder can use when initializing
> +     * based on the frames that lead to the creation of the stream. Round to
> +     * the nearest integer, for instance 24 for 23.976.
> +     */
> +    uint64_t duration = drawable->creation_time - drawable->first_frame_time;
> +    if (duration > (uint64_t)drawable->frames_count * 1000 * 1000 * 1000 / MAX_FPS) {
> +        stream->input_fps = ((uint64_t)drawable->frames_count * 1000 * 1000 * 1000 + duration / 2) / duration;
> +    } else {
> +        stream->input_fps = MAX_FPS;
> +    }
>      stream->num_input_frames = 0;
>      stream->input_fps_start_time = drawable->creation_time;
>      display->streams_size_total += stream->width * stream->height;
> @@ -433,21 +442,24 @@ static void display_channel_create_stream(DisplayChannel *display, Drawable *dra
>      FOREACH_DCC(display, dcc_ring_item, next, dcc) {
>          dcc_create_stream(dcc, stream);
>      }
> -    spice_debug("stream %d %dx%d (%d, %d) (%d, %d)",
> +    spice_debug("stream %d %dx%d (%d, %d) (%d, %d) %u fps",
>                  (int)(stream - display->streams_buf), stream->width,
>                  stream->height, stream->dest_area.left, stream->dest_area.top,
> -                stream->dest_area.right, stream->dest_area.bottom);
> +                stream->dest_area.right, stream->dest_area.bottom,
> +                stream->input_fps);
>      return;
>  }
>  
>  // returns whether a stream was created
>  static int stream_add_frame(DisplayChannel *display,
>                              Drawable *frame_drawable,
> +                            red_time_t first_frame_time,
>                              int frames_count,
>                              int gradual_frames_count,
>                              int last_gradual_frame)
>  {
>      update_copy_graduality(display, frame_drawable);
> +    frame_drawable->first_frame_time = first_frame_time;
>      frame_drawable->frames_count = frames_count + 1;
>      frame_drawable->gradual_frames_count  = gradual_frames_count;
>  
> @@ -514,6 +526,7 @@ void stream_trace_update(DisplayChannel *display, Drawable *drawable)
>                                         &trace->dest_area, trace->time, NULL, FALSE) !=
>                                         STREAM_FRAME_NONE) {
>              if (stream_add_frame(display, drawable,
> +                                 trace->first_frame_time,
>                                   trace->frames_count,
>                                   trace->gradual_frames_count,
>                                   trace->last_gradual_frame)) {
> @@ -559,6 +572,7 @@ void stream_maintenance(DisplayChannel *display,
>                                   FALSE);
>          if (is_next_frame != STREAM_FRAME_NONE) {
>              stream_add_frame(display, candidate,
> +                             prev->first_frame_time,
>                               prev->frames_count,
>                               prev->gradual_frames_count,
>                               prev->last_gradual_frame);
> @@ -910,6 +924,7 @@ void stream_trace_add_drawable(DisplayChannel *display, Drawable *item)
>  
>      trace = &display->items_trace[display->next_item_trace++ & ITEMS_TRACE_MASK];
>      trace->time = item->creation_time;
> +    trace->first_frame_time = item->first_frame_time;
>      trace->frames_count = item->frames_count;
>      trace->gradual_frames_count = item->gradual_frames_count;
>      trace->last_gradual_frame = item->last_gradual_frame;
> diff --git a/server/stream.h b/server/stream.h
> index c436c2e..752aff0 100644
> --- a/server/stream.h
> +++ b/server/stream.h
> @@ -114,6 +114,7 @@ void                  stream_clip_item_unref                        (DisplayChan
>  
>  typedef struct ItemTrace {
>      red_time_t time;
> +    red_time_t first_frame_time;
>      int frames_count;
>      int gradual_frames_count;
>      int last_gradual_frame;
> -- 
> 2.6.2
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>
Hi,

On Thu, Dec 10, 2015 at 10:28:01AM +0100, Francois Gouget wrote:
>
> Could you please review this patch?
> It's the same as v5 but rebased on the current source and with a more
> verbose commit log. Iḿ still hoping for inclusion some time this year...
>

Sorry, I forgot to cc Frediano in v5.

Patch does look good to me, just small question below and a extra space
that can be included when one does push it.

Acked-by: Victor Toso <victortoso@redhat.com>

> On Wed, 2 Dec 2015, Francois Gouget wrote:
> 
> > This way the video encoder can actually count on a real estimate when
> > it is initializing.
> > Note that the server only creates a video stream if at least 20 bitmap
> > 'blits' of the same size and type arrive, each within a maximum time
> > interval from the previous one. So it was only keeping track of the
> > frame to frame interval. Thus to get an average frame rate over all the
> > 20 frames it's necessary to also keep track of the first_frame_time.
> > 
> > Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
> > ---
> >  server/display-channel.c |  2 +-
> >  server/display-channel.h |  1 +
> >  server/stream.c          | 21 ++++++++++++++++++---
> >  server/stream.h          |  1 +
> >  4 files changed, 21 insertions(+), 4 deletions(-)
> > 
> > No change from v5 except for a more detailed commit log and updates to 
> > keep up with the Spice refactoring.
> > 
> > diff --git a/server/display-channel.c b/server/display-channel.c
> > index 580025f..bcf16c7 100644
> > --- a/server/display-channel.c
> > +++ b/server/display-channel.c
> > @@ -1112,7 +1112,7 @@ Drawable *display_channel_drawable_try_new(DisplayChannel *display,
> >  
> >      bzero(drawable, sizeof(Drawable));
> >      drawable->refs = 1;
> > -    drawable->creation_time = red_get_monotonic_time();
> > +    drawable->creation_time = drawable->first_frame_time =red_get_monotonic_time();
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
need extra space!

> >      ring_item_init(&drawable->list_link);
> >      ring_item_init(&drawable->surface_list_link);
> >      ring_item_init(&drawable->tree_item.base.siblings_link);
> > diff --git a/server/display-channel.h b/server/display-channel.h
> > index 69d287a..7a63369 100644
> > --- a/server/display-channel.h
> > +++ b/server/display-channel.h
> > @@ -66,6 +66,7 @@ struct Drawable {
> >      Ring glz_ring;
> >
> >      red_time_t creation_time;
> > +    red_time_t first_frame_time;
> >      int frames_count;
> >      int gradual_frames_count;
> >      int last_gradual_frame;
> > diff --git a/server/stream.c b/server/stream.c
> > index 3bd91b6..6eb54c3 100644
> > --- a/server/stream.c
> > +++ b/server/stream.c
> > @@ -425,7 +425,16 @@ static void display_channel_create_stream(DisplayChannel *display, Drawable *dra
> >      SpiceBitmap *bitmap = &drawable->red_drawable->u.copy.src_bitmap->u.bitmap;
> >      stream->top_down = !!(bitmap->flags & SPICE_BITMAP_FLAGS_TOP_DOWN);
> >      drawable->stream = stream;
> > -    stream->input_fps = MAX_FPS;
> > +    /* Provide an fps estimate the video encoder can use when initializing
> > +     * based on the frames that lead to the creation of the stream. Round to
> > +     * the nearest integer, for instance 24 for 23.976.
> > +     */
> > +    uint64_t duration = drawable->creation_time - drawable->first_frame_time;
> > +    if (duration > (uint64_t)drawable->frames_count * 1000 * 1000 * 1000 / MAX_FPS) {
> > +        stream->input_fps = ((uint64_t)drawable->frames_count * 1000 * 1000 * 1000 + duration / 2) / duration;

Why + (duration/2) ? This would be the same as [ x/2 + 1/2 ] so I'm
guessing it is for the rounding, right?

> > +    } else {
> > +        stream->input_fps = MAX_FPS;
> > +    }
> >      stream->num_input_frames = 0;
> >      stream->input_fps_start_time = drawable->creation_time;
> >      display->streams_size_total += stream->width * stream->height;
> > @@ -433,21 +442,24 @@ static void display_channel_create_stream(DisplayChannel *display, Drawable *dra
> >      FOREACH_DCC(display, dcc_ring_item, next, dcc) {
> >          dcc_create_stream(dcc, stream);
> >      }
> > -    spice_debug("stream %d %dx%d (%d, %d) (%d, %d)",
> > +    spice_debug("stream %d %dx%d (%d, %d) (%d, %d) %u fps",
> >                  (int)(stream - display->streams_buf), stream->width,
> >                  stream->height, stream->dest_area.left, stream->dest_area.top,
> > -                stream->dest_area.right, stream->dest_area.bottom);
> > +                stream->dest_area.right, stream->dest_area.bottom,
> > +                stream->input_fps);
> >      return;
> >  }
> >
> >  // returns whether a stream was created
> >  static int stream_add_frame(DisplayChannel *display,
> >                              Drawable *frame_drawable,
> > +                            red_time_t first_frame_time,
> >                              int frames_count,
> >                              int gradual_frames_count,
> >                              int last_gradual_frame)
> >  {
> >      update_copy_graduality(display, frame_drawable);
> > +    frame_drawable->first_frame_time = first_frame_time;
> >      frame_drawable->frames_count = frames_count + 1;
> >      frame_drawable->gradual_frames_count  = gradual_frames_count;
> >
> > @@ -514,6 +526,7 @@ void stream_trace_update(DisplayChannel *display, Drawable *drawable)
> >                                         &trace->dest_area, trace->time, NULL, FALSE) !=
> >                                         STREAM_FRAME_NONE) {
> >              if (stream_add_frame(display, drawable,
> > +                                 trace->first_frame_time,
> >                                   trace->frames_count,
> >                                   trace->gradual_frames_count,
> >                                   trace->last_gradual_frame)) {
> > @@ -559,6 +572,7 @@ void stream_maintenance(DisplayChannel *display,
> >                                   FALSE);
> >          if (is_next_frame != STREAM_FRAME_NONE) {
> >              stream_add_frame(display, candidate,
> > +                             prev->first_frame_time,
> >                               prev->frames_count,
> >                               prev->gradual_frames_count,
> >                               prev->last_gradual_frame);
> > @@ -910,6 +924,7 @@ void stream_trace_add_drawable(DisplayChannel *display, Drawable *item)
> >
> >      trace = &display->items_trace[display->next_item_trace++ & ITEMS_TRACE_MASK];
> >      trace->time = item->creation_time;
> > +    trace->first_frame_time = item->first_frame_time;
> >      trace->frames_count = item->frames_count;
> >      trace->gradual_frames_count = item->gradual_frames_count;
> >      trace->last_gradual_frame = item->last_gradual_frame;
> > diff --git a/server/stream.h b/server/stream.h
> > index c436c2e..752aff0 100644
> > --- a/server/stream.h
> > +++ b/server/stream.h
> > @@ -114,6 +114,7 @@ void                  stream_clip_item_unref                        (DisplayChan
> >
> >  typedef struct ItemTrace {
> >      red_time_t time;
> > +    red_time_t first_frame_time;
> >      int frames_count;
> >      int gradual_frames_count;
> >      int last_gradual_frame;
> > -- 
> > 2.6.2
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> >
>
>
> --
> Francois Gouget <fgouget@codeweavers.com>
On Thu, 10 Dec 2015, Victor Toso wrote:
[...]
> > > +    drawable->creation_time = drawable->first_frame_time =red_get_monotonic_time();
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
> need extra space!

Yes. I fixed it locally but I'm hesitant to resubmit just for that.
In the worst case it will be fixed with the rename to 
spice_get_monotonic_ns().


[...]
> > > +    /* Provide an fps estimate the video encoder can use when initializing
> > > +     * based on the frames that lead to the creation of the stream. Round to
> > > +     * the nearest integer, for instance 24 for 23.976.
> > > +     */
> > > +    uint64_t duration = drawable->creation_time - drawable->first_frame_time;
> > > +    if (duration > (uint64_t)drawable->frames_count * 1000 * 1000 * 1000 / MAX_FPS) {
> > > +        stream->input_fps = ((uint64_t)drawable->frames_count * 1000 * 1000 * 1000 + duration / 2) / duration;
> 
> Why + (duration/2) ? This would be the same as [ x/2 + 1/2 ] so I'm
> guessing it is for the rounding, right?

Yes, it's to round to the nearest integer.
The general principle is that if your division truncates, then 
round_to_nearest( ((double)x) / n ) = (x + n/2) / n

I'll also note that this is a case where we want the most accurate 
estimate of the fps so round to nearest makes sense.
Hi,

On Thu, Dec 10, 2015 at 12:16:19PM +0100, Francois Gouget wrote:
> On Thu, 10 Dec 2015, Victor Toso wrote:
> [...]
> > > > +    drawable->creation_time = drawable->first_frame_time =red_get_monotonic_time();
> > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
> > need extra space!
>
> Yes. I fixed it locally but I'm hesitant to resubmit just for that.
> In the worst case it will be fixed with the rename to
> spice_get_monotonic_ns().

Yes, no need to resend.

>
> [...]
> > > > +    /* Provide an fps estimate the video encoder can use when initializing
> > > > +     * based on the frames that lead to the creation of the stream. Round to
> > > > +     * the nearest integer, for instance 24 for 23.976.
> > > > +     */
> > > > +    uint64_t duration = drawable->creation_time - drawable->first_frame_time;
> > > > +    if (duration > (uint64_t)drawable->frames_count * 1000 * 1000 * 1000 / MAX_FPS) {
> > > > +        stream->input_fps = ((uint64_t)drawable->frames_count * 1000 * 1000 * 1000 + duration / 2) / duration;
> >
> > Why + (duration/2) ? This would be the same as [ x/2 + 1/2 ] so I'm
> > guessing it is for the rounding, right?
>
> Yes, it's to round to the nearest integer.
> The general principle is that if your division truncates, then
> round_to_nearest( ((double)x) / n ) = (x + n/2) / n
>
> I'll also note that this is a case where we want the most accurate
> estimate of the fps so round to nearest makes sense.

Sure

>
> --
> Francois Gouget <fgouget@codeweavers.com>


If no one complains I'll be pushing this later on.
Thanks for your work,
  toso