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

Submitted by Francois Gouget on Nov. 13, 2015, 3:13 p.m.

Details

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

Not browsing as part of any series.

Commit Message

Francois Gouget Nov. 13, 2015, 3:13 p.m.
This way the video encoder can actually count on a real estimate when
it is initializing.

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

I sent this patch in June:
http://lists.freedesktop.org/archives/spice-devel/2015-June/thread.html

Then again a week ago:
http://lists.freedesktop.org/archives/spice-devel/2015-November/thread.html

But never got any feedback on it. What's up???

Patch hide | download patch | download mbox

diff --git a/server/display-channel.h b/server/display-channel.h
index c3dcc29..24f9da5 100644
--- a/server/display-channel.h
+++ b/server/display-channel.h
@@ -152,6 +152,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/red_worker.c b/server/red_worker.c
index 9673288..b141a1e 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -984,6 +984,7 @@  static void display_stream_trace_add_drawable(DisplayChannel *display, Drawable
 
     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;
@@ -1772,7 +1773,12 @@  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;
+    stream->input_fps = ((uint64_t)drawable->frames_count * 1000 * 1000 * 1000 + duration / 2) / duration;
     stream->num_input_frames = 0;
     stream->input_fps_start_time = drawable->creation_time;
     display->streams_size_total += stream->width * stream->height;
@@ -1780,10 +1786,10 @@  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;
 }
 
@@ -2009,11 +2015,13 @@  static int is_stream_start(Drawable *drawable)
 // returns whether a stream was created
 static int display_channel_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;
 
@@ -2074,6 +2082,7 @@  static void display_channel_stream_maintenance(DisplayChannel *display,
                                  FALSE);
         if (is_next_frame != STREAM_FRAME_NONE) {
             display_channel_stream_add_frame(display, candidate,
+                                             prev->first_frame_time,
                                              prev->frames_count,
                                              prev->gradual_frames_count,
                                              prev->last_gradual_frame);
@@ -2235,6 +2244,7 @@  static void red_use_stream_trace(DisplayChannel *display, Drawable *drawable)
                                        &trace->dest_area, trace->time, NULL, FALSE) !=
                                        STREAM_FRAME_NONE) {
             if (display_channel_stream_add_frame(display, drawable,
+                                                 trace->first_frame_time,
                                                  trace->frames_count,
                                                  trace->gradual_frames_count,
                                                  trace->last_gradual_frame)) {
@@ -2664,7 +2674,7 @@  static Drawable *get_drawable(RedWorker *worker, uint8_t effect, RedDrawable *re
 
     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/stream.h b/server/stream.h
index f77fa96..d9f1a66 100644
--- a/server/stream.h
+++ b/server/stream.h
@@ -108,6 +108,7 @@  StreamClipItem *stream_clip_item_new(DisplayChannelClient* dcc,
 
 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

> 
> This way the video encoder can actually count on a real estimate when
> it is initializing.
> 
> Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
> ---
>  server/display-channel.h |  1 +
>  server/red_worker.c      | 18 ++++++++++++++----
>  server/stream.h          |  1 +
>  3 files changed, 16 insertions(+), 4 deletions(-)
> 
> I sent this patch in June:
> http://lists.freedesktop.org/archives/spice-devel/2015-June/thread.html
> 
> Then again a week ago:
> http://lists.freedesktop.org/archives/spice-devel/2015-November/thread.html
> 
> But never got any feedback on it. What's up???
> 

Sorry for the delay.

> 
> diff --git a/server/display-channel.h b/server/display-channel.h
> index c3dcc29..24f9da5 100644
> --- a/server/display-channel.h
> +++ b/server/display-channel.h
> @@ -152,6 +152,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;

This here looks weird. ItemTrace should trace/record the frames and have a first and count.
I think however is more of a design problem than something related to your patch.
There are too many fields related to stream in Drawable.

> diff --git a/server/red_worker.c b/server/red_worker.c
> index 9673288..b141a1e 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -984,6 +984,7 @@ static void
> display_stream_trace_add_drawable(DisplayChannel *display, Drawable
>  
>      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;
> @@ -1772,7 +1773,12 @@ 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;
> +    stream->input_fps = ((uint64_t)drawable->frames_count * 1000 * 1000 *
> 1000 + duration / 2) / duration;

Just to be 100% sure I would check for duration == 0 and limit input_fps to
MAX_FPS.

>      stream->num_input_frames = 0;
>      stream->input_fps_start_time = drawable->creation_time;
>      display->streams_size_total += stream->width * stream->height;
> @@ -1780,10 +1786,10 @@ 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;
>  }
>  
> @@ -2009,11 +2015,13 @@ static int is_stream_start(Drawable *drawable)
>  // returns whether a stream was created
>  static int display_channel_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;
>  

is not clear why the time keep updating with new frames.

> @@ -2074,6 +2082,7 @@ static void
> display_channel_stream_maintenance(DisplayChannel *display,
>                                   FALSE);
>          if (is_next_frame != STREAM_FRAME_NONE) {
>              display_channel_stream_add_frame(display, candidate,
> +                                             prev->first_frame_time,
>                                               prev->frames_count,
>                                               prev->gradual_frames_count,
>                                               prev->last_gradual_frame);
> @@ -2235,6 +2244,7 @@ static void red_use_stream_trace(DisplayChannel
> *display, Drawable *drawable)
>                                         &trace->dest_area, trace->time, NULL,
>                                         FALSE) !=
>                                         STREAM_FRAME_NONE) {
>              if (display_channel_stream_add_frame(display, drawable,
> +                                                 trace->first_frame_time,
>                                                   trace->frames_count,
>                                                   trace->gradual_frames_count,
>                                                   trace->last_gradual_frame))
>                                                   {
> @@ -2664,7 +2674,7 @@ static Drawable *get_drawable(RedWorker *worker,
> uint8_t effect, RedDrawable *re
>  
>      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/stream.h b/server/stream.h
> index f77fa96..d9f1a66 100644
> --- a/server/stream.h
> +++ b/server/stream.h
> @@ -108,6 +108,7 @@ StreamClipItem
> *stream_clip_item_new(DisplayChannelClient* dcc,
>  
>  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
> 

Surely my knowledge on this feature is not enough.

Which kind of test did you do?

Frediano
On Fri, 13 Nov 2015, Frediano Ziglio wrote:
[...]
> > diff --git a/server/display-channel.h b/server/display-channel.h
> > index c3dcc29..24f9da5 100644
> > --- a/server/display-channel.h
> > +++ b/server/display-channel.h
> > @@ -152,6 +152,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;
> 
> This here looks weird. ItemTrace should trace/record the frames and have a first and count.
> I think however is more of a design problem than something related to your patch.
> There are too many fields related to stream in Drawable.
[...]
> is not clear why the time keep updating with new frames.

The video stream detection code is pretty complex and undocumented (and 
buggy too). But my understanding is that a video stream is created only 
if at least twenty image 'blits' of the same size and type arrive, each 
within a maximum time interval from the previous one.

So the code was only keeping track of the current frame's creation time, 
and that of the previous one to check for the 'maximum time interval' 
condition. But basing a framerate estimate on just one frame interval 
would not be very precise. So to get a proper estimate I need to also 
keep track of the timestamp for the first frame in the series, so I can 
compute an average over the first 20 frames (which represents 0.67s at 
30fps so long enough to smooth over scheduling delays). Hence the need 
for the first_frame_time field.

Also the code does not keep the twenty frames in memory. My rough 
understanding and recollection of it is that as soon as it's done with a 
frame it frees it and only keeps the essential data into an ItemTrace 
structure. So to preserve the time of the first frame it's necessary to 
copy the first_frame_time field from frame to frame, and back and forth 
between ItemTrace and Drawable structures.


[...]
> > +    /* 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;
> > +    stream->input_fps = ((uint64_t)drawable->frames_count * 1000 * 1000 *
> > 1000 + duration / 2) / duration;
> 
> Just to be 100% sure I would check for duration == 0 and limit input_fps to
> MAX_FPS.

Sure. Will do.


> Which kind of test did you do?

Playing video with mplayer, totem and Flash (YouTube), with and without 
SpiceDeferredFPS and EnableSurfaces. The fps estimates were always 
within 1fps of the actual framerate.