Skip to content

Commit

Permalink
added: overload of CDVDClock::GetClock that also returns the absolute…
Browse files Browse the repository at this point in the history
… dvdclock time, this fixes a problem in CDVDPlayerVideo::OutputPicture() where two different clock samples were used to convert a pts into a presenttime, if the clock moves forward in between calls, so does the presenttime
  • Loading branch information
bobo1on1 committed Jul 8, 2011
1 parent b366660 commit 41da564
Show file tree
Hide file tree
Showing 3 changed files with 17 additions and 3 deletions.
13 changes: 13 additions & 0 deletions xbmc/cores/dvdplayer/DVDClock.cpp
Expand Up @@ -96,6 +96,19 @@ double CDVDClock::GetClock(bool interpolated /*= true*/)
return SystemToPlaying(g_VideoReferenceClock.GetTime(interpolated));
}

double CDVDClock::GetClock(double& absolute, bool interpolated /*= true*/)
{
int64_t current = g_VideoReferenceClock.GetTime(interpolated);
{
CSingleLock lock(m_systemsection);
CheckSystemClock();
absolute = SystemToAbsolute(current);
}

CSharedLock lock(m_critSection);
return SystemToPlaying(current);
}

void CDVDClock::SetSpeed(int iSpeed)
{
// this will sometimes be a little bit of due to rounding errors, ie clock might jump abit when changing speed
Expand Down
1 change: 1 addition & 0 deletions xbmc/cores/dvdplayer/DVDClock.h
Expand Up @@ -43,6 +43,7 @@ class CDVDClock
~CDVDClock();

double GetClock(bool interpolated = true);
double GetClock(double& absolute, bool interpolated = true);

void Discontinuity(double currentPts = 0LL);

Expand Down
6 changes: 3 additions & 3 deletions xbmc/cores/dvdplayer/DVDPlayerVideo.cpp
Expand Up @@ -1041,10 +1041,10 @@ int CDVDPlayerVideo::OutputPicture(DVDVideoPicture* pPicture, double pts)
pts += m_iVideoDelay;

// calculate the time we need to delay this picture before displaying
double iSleepTime, iClockSleep, iFrameSleep, iCurrentClock, iFrameDuration;
double iSleepTime, iClockSleep, iFrameSleep, iPlayingClock, iCurrentClock, iFrameDuration;

iCurrentClock = m_pClock->GetAbsoluteClock(false); // snapshot current clock
iClockSleep = pts - m_pClock->GetClock(false); //sleep calculated by pts to clock comparison
iPlayingClock = m_pClock->GetClock(iCurrentClock, false); // snapshot current clock
iClockSleep = pts - iPlayingClock; //sleep calculated by pts to clock comparison

This comment has been minimized.

Copy link
@TheSwissKnife

TheSwissKnife Jul 8, 2011

So is this temporary? Are you planning to make into an offset and let render manager convert to clock sleep/wait? Would be handy to know so I don't waste time merging my code with a transitionary version.

This comment has been minimized.

Copy link
@bobo1on1

bobo1on1 Jul 8, 2011

Member

Nope, this is basically it, I wanted to make a function to convert a pts directly into a presenttime, but dvdplayer also needs to know the absolute dvdclock time for autosync, so there's not much of a point to doing that.
Doing "pts + iCurrentClock - iPlayingClock" basically just adds an offset to pts, since iCurrentClock and iPlayingClock are both the value of g_VideoReferenceClock with an offset (but different for each one), so you add the value of g_VideoReferenceClock and then you subtract it again, so you only add the differences between offsets.

This comment has been minimized.

Copy link
@TheSwissKnife

TheSwissKnife Jul 9, 2011

Well I confess I have no idea what autosync purpose is...always looked like dead code to me (though I never removed it). Anyway this takes us back full circle to our previous discussion - the fact that using interpolated value therefore must be done, else you end up with similar issue as having separate clock calls for absolute and relative.

This comment has been minimized.

Copy link
@elupus

elupus Jul 9, 2011

Contributor

I think you need to explain why by examples, since i can't see why it's a problem.

This comment has been minimized.

Copy link
@bobo1on1

bobo1on1 Jul 9, 2011

Member

Because there is no problem, basic algebra proves it.

This comment has been minimized.

Copy link
@TheSwissKnife

TheSwissKnife Jul 9, 2011

I think I see what you mean now. Since making sure the two values come from the same reference clock value the value being passed as presenttime becomes independent of the clock time completely and thus will be no different whether interpolated or not. Thus neither is actually incorrect (as they are identical) and in fact mentioning the clock in the calculation is a source for confusion - better to request just offset for clarity. The offset only changes at discontinuity or clock re-sync. Am I right?

What is this autosync (m_autosync is always 1 from what I can see)? What is purpose of m_FlipTimeStamp and iFrameSleep?

This comment has been minimized.

Copy link
@bobo1on1

bobo1on1 Jul 9, 2011

Member

Correct.
Autosync is a timestamp smoother, but doesn't work 100% right because it causes an offset when the frameduration is reported incorrectly.

This comment has been minimized.

Copy link
@TheSwissKnife

TheSwissKnife Jul 9, 2011

So get rid of autosync? It seems to me all of the clock stuff in OutputPicture is either wrong, confusing or not required. The current pts calculation should come from render manager as only it has a reasonable chance of getting it right, the clock values for presenttime calculation are not required and should be replaced with pure offsets as you said, and iFrameSleep and m_iFlipTimestamp do not seem to be useful at all. The lateness testing is innaccurate and prone to bursts of dropping. I am about to implement a lateness/dropping routine to replace that part. All in all I can't much purpose for the large bulk of the clock related stuff in there at present (only possibly the limit stuff).

This comment has been minimized.

Copy link
@TheSwissKnife

TheSwissKnife Jul 22, 2011

I am back looking at this again and though it makes sense for playspeed normal (where my method gives the exact same result as yours) it is not for other playspeeds where interpolated method is more accurate right?

This comment has been minimized.

Copy link
@bobo1on1

bobo1on1 Jul 22, 2011

Member

Correct.

This comment has been minimized.

Copy link
@TheSwissKnife

TheSwissKnife Jul 22, 2011

So is there still a reason to use non-interpolated?

iFrameSleep = m_FlipTimeStamp - iCurrentClock; // sleep calculated by duration of frame
iFrameDuration = pPicture->iDuration;

Expand Down

0 comments on commit 41da564

Please sign in to comment.