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.
--
Francois Gouget <fgouget@xxxxxxxxxxxxxxx>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel