GNOME Bugzilla – Bug 640665
basesink: drops too many buffers when there's no duration
Last modified: 2011-04-15 17:14:44 UTC
Coming from bug #640610. "videotestsrc is-live=1 ! element-that-drops-framerate-and-duration ! xvimagesink" That magical element could be a network using RTP. Since the sink doesn't know the framerate or duration, the "stop" time is always NONE. So the only thing that prevents frames from being dropped is the "max-lateness", which is often not enough if there is enough processing hapenning. The issue at hand is that the sink decides to drop "too many buffers".
Created attachment 179394 [details] [review] basesink: calculate rstop when get_times() can't Proposed fix. Suppose the buffers don't have a duration (coming from rtp), and suppose there's no framerate (again, coming from rtp), or other means to calculate the stop time. Then the start time is assumed as the stop time, leaving a very small margin where a buffer can come in, and droping then the rest of the time, even if there was plenty of margin. Instead, we can calculate when the previous buffer arrived, and the time difference would be a safe bet when the next buffer should have arrived.
Created attachment 179406 [details] [review] basesink: Don't drop without a stop time For the record, I don't think estimating the stop time is the right way to go. The sink should probably just abstain from dropping frames if there is no stop time. Not having a stop time mean a frame will be valid until the next one comes. Upstream elements could decide to drop further packets (because they know how late the current one was compared to it's start time). That said, due to bug #640610, in a live pipelines, all frames arrive late compared to their start time.
I'm thinking that -1 as the duration on a buffer means that the buffer has infinite duration and should thus be displayed at all times. On the other hand, we don't want to display a frame that should have been displayed a long time ago because it might end up badly synced with the audio. For 'low' framerates, the eye does not care about late frames much but for high framerates the effects can be very disturbing. So I'm thinking that there should be a limit, even for frames with a duration. Probably this limit should depend on the average number of frames per second or something. Calculating the average time between incoming buffer timestamps is probably something interesting we can use in this metric too.
(In reply to comment #3) > So I'm thinking that there should be a limit, even for frames with a duration. > Probably this limit should depend on the average number of frames per second or > something. How would you calculate the average fps?
Created attachment 179922 [details] [review] basesink: calculate rstop when get_times() can't New proposal using already present avg_pt.
Ping.
(In reply to comment #6) > Ping. - As said in Comment #2, estimating the stop time is the right thing to do. You want to have the original buffer properties preserved as much as possible so that you can make better decisions later. - The avg_pt is not a good indication for the interframe spacing because it includes the synchronization jitter. I've made some patches to improve things. commit f8828eace691a20f37a7dcad7766ab3e6e13331c Author: Wim Taymans <wim.taymans@collabora.co.uk> Date: Mon Feb 14 14:00:38 2011 +0100 basesink: improve duration calculation Keep track of the average distance between incomming timestamps and use that to estimate the frame duration when buffers have no duration set on them. commit 79665e8247d9e2beae3f6a282a834bff65cfb5e3 Author: Wim Taymans <wim.taymans@collabora.co.uk> Date: Mon Feb 14 13:49:10 2011 +0100 basesink: improve rate calculation When there is no duration on input buffers, assume the rate is 1.0 instead of (the undefined) 0.0. commit 9661a713badfa854f54645e18695a0bd8cda1dfb Author: Wim Taymans <wim.taymans@collabora.co.uk> Date: Mon Feb 14 13:47:02 2011 +0100 basesink: improve average duration calculation Improve the calculation of the duration. When we have no input duration set on the input buffers stop is set to start and then we end up using a 0 duration in the average calculation. commit dc0120fe28001471eaf155e8dfed9677ebf424b7 Author: Wim Taymans <wim.taymans@collabora.co.uk> Date: Mon Feb 14 12:21:39 2011 +0100 basesink: rename variable Rename an internal variable to better reflact what its value means.
I think this fixes this bug.
(In reply to comment #7) > - The avg_pt is not a good indication for the interframe spacing because it > includes the synchronization jitter. That is what you and Edward told me to do. I suggested the solution you did. > commit f8828eace691a20f37a7dcad7766ab3e6e13331c > Author: Wim Taymans <wim.taymans@collabora.co.uk> > Date: Mon Feb 14 14:00:38 2011 +0100 > > basesink: improve duration calculation > > Keep track of the average distance between incomming timestamps and > use that to estimate the frame duration when buffers have no duration set > on > them. Yeah, this fixes the problem. (In reply to comment #8) > I think this fixes this bug. Yeah, but I investigated the problem, I proposed multiple solutions, I tested them, I discussed with multiple people, and now I'm also testing the solution. I even asked you if I should do the patch exactly the way you ended up doing it, and of course if I would have done it this way if you just had said so. Yet, in the end I'm not even mentioned in the commit message. Way to incentivize contributors.