Skip to content

Commit

Permalink
dvdplayer: pass display time through a/v players before out to applic…
Browse files Browse the repository at this point in the history 
…ation

This gives proper display of time to outside world, taking queues into
account and allow seeks to end up where the user expected it to in the
case of input streams which provide time at demux.
  • Loading branch information
@elupus
elupus committed Jan 31, 2013
1 parent ea274a3 commit e2ce6f3
Show file tree
Hide file tree
Showing 5 changed files with 73 additions and 12 deletions.
2 changes: 2 additions & 0 deletions xbmc/cores/dvdplayer/DVDMessage.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@ class CDVDMsg : public IDVDResourceCounted<CDVDMsg>
PLAYER_CHANNEL_SELECT, // switches to the provided channel
PLAYER_STARTED, // sent whenever a sub player has finished it's first frame after open

PLAYER_DISPLAYTIME, // display time struct from av players

// demuxer related messages

DEMUXER_PACKET, // data packet
Expand Down
51 changes: 40 additions & 11 deletions xbmc/cores/dvdplayer/DVDPlayer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1708,6 +1708,14 @@ void CDVDPlayer::UpdateTimestamps(CCurrentStream& current, DemuxPacket* pPacket)
current.dur = 0.1 * (current.dur * 9 + (dts - current.dts));

current.dts = dts;

/* send a playback state structure periodically */
if(current.dts_state == DVD_NOPTS_VALUE
|| abs(current.dts - current.dts_state) > DVD_MSEC_TO_TIME(200))

This comment has been minimized.

Sign in to view

Copy link
@FernetMenta

FernetMenta Feb 5, 2013

Contributor

I am observing races here. After a flushBuffer CurrentVideo/audio.dts is NOPTS_VALUE -> InputState is updated -> dts and time_offset sent to players are invalid.

We have a update timeout in UpdatePlayState and here :(

This comment has been minimized.

Sign in to view

Copy link
@elupus

elupus via email Feb 5, 2013

Author Contributor

This comment has been minimized.

Sign in to view

Copy link
@FernetMenta

FernetMenta Feb 6, 2013

Contributor

Having NOPTS_VALUE in state.dts for at least 200ms. Holding e.g. the SmallStep button results in a seek to NOPTS_VALUE.
{
current.dts_state = current.dts;
SendPlayerMessage(new CDVDMsgType<SPlayerState>(CDVDMsg::PLAYER_DISPLAYTIME, m_StateInput), current.player);
}
}

static void UpdateLimits(double& minimum, double& maximum, double dts)
Expand Down Expand Up @@ -2278,6 +2286,24 @@ void CDVDPlayer::HandleMessages()
m_CurrentVideo.started = true;
CLog::Log(LOGDEBUG, "CDVDPlayer::HandleMessages - player started %d", player);
}
else if (pMsg->IsType(CDVDMsg::PLAYER_DISPLAYTIME))
{
CDVDPlayer::SPlayerState& state = ((CDVDMsgType<CDVDPlayer::SPlayerState>*)pMsg)->m_value;

CSingleLock lock(m_StateSection);
/* prioritize data from video player, but only accept data *
* after it has been started to avoid race conditions after seeks */
if(m_CurrentVideo.started)
{
if(state.player == DVDPLAYER_VIDEO)
m_State = state;
}
else if(m_CurrentAudio.started)
{
if(state.player == DVDPLAYER_AUDIO)
m_State = state;
}
}
}
catch (...)
{
Expand Down Expand Up @@ -2534,15 +2560,15 @@ bool CDVDPlayer::SeekScene(bool bPlus)
void CDVDPlayer::GetAudioInfo(CStdString& strAudioInfo)
{
{ CSingleLock lock(m_StateSection);
strAudioInfo.Format("D(%s)", m_State.demux_audio.c_str());
strAudioInfo.Format("D(%s)", m_StateInput.demux_audio.c_str());
}
strAudioInfo.AppendFormat(" P(%s)", m_dvdPlayerAudio.GetPlayerInfo().c_str());
}

void CDVDPlayer::GetVideoInfo(CStdString& strVideoInfo)
{
{ CSingleLock lock(m_StateSection);
strVideoInfo.Format("D(%s)", m_State.demux_video.c_str());
strVideoInfo.Format("D(%s)", m_StateInput.demux_video.c_str());
}
strVideoInfo.AppendFormat(" P(%s)", m_dvdPlayerVideo.GetPlayerInfo().c_str());
}
Expand All @@ -2565,7 +2591,7 @@ void CDVDPlayer::GetGeneralInfo(CStdString& strGeneralInfo)

CStdString strBuf;
CSingleLock lock(m_StateSection);
if(m_State.cache_bytes >= 0)
if(m_StateInput.cache_bytes >= 0)
{
strBuf.AppendFormat(" cache:%s %2.0f%%"
, StringUtils::SizeToString(m_State.cache_bytes).c_str()
Expand Down Expand Up @@ -2609,7 +2635,7 @@ float CDVDPlayer::GetPercentage()
float CDVDPlayer::GetCachePercentage()
{
CSingleLock lock(m_StateSection);
return m_State.cache_offset * 100; // NOTE: Percentage returned is relative
return m_StateInput.cache_offset * 100; // NOTE: Percentage returned is relative
}

void CDVDPlayer::SetAVDelay(float fValue)
Expand Down Expand Up @@ -3732,7 +3758,7 @@ int CDVDPlayer::AddSubtitle(const CStdString& strSubPath)
int CDVDPlayer::GetCacheLevel() const
{
CSingleLock lock(m_StateSection);
return (int)(m_State.cache_level * 100);
return (int)(m_StateInput.cache_level * 100);
}

double CDVDPlayer::GetQueueTime()
Expand Down Expand Up @@ -3798,26 +3824,29 @@ int CDVDPlayer::AddSubtitleFile(const std::string& filename, const std::string&

void CDVDPlayer::UpdatePlayState(double timeout)
{
if(m_State.timestamp != 0
&& m_State.timestamp + DVD_MSEC_TO_TIME(timeout) > CDVDClock::GetAbsoluteClock())
if(m_StateInput.timestamp != 0
&& m_StateInput.timestamp + DVD_MSEC_TO_TIME(timeout) > CDVDClock::GetAbsoluteClock())
return;

SPlayerState state(m_State);
SPlayerState state(m_StateInput);

if (m_CurrentVideo.dts != DVD_NOPTS_VALUE)
state.dts = m_CurrentVideo.dts;
else if(m_CurrentAudio.dts != DVD_NOPTS_VALUE)
state.dts = m_CurrentAudio.dts;
else
state.dts = m_clock.GetClock();
state.dts = DVD_NOPTS_VALUE;

This comment has been minimized.

Sign in to view

Copy link
@FernetMenta

FernetMenta Feb 6, 2013

Contributor

I am wondering if we should set this to m_CurrentVideo/audio.startpts. Then we have correct state.time right after a seek.

This comment has been minimized.

Sign in to view

Copy link
@elupus

elupus via email Feb 6, 2013

Author Contributor

This comment has been minimized.

Sign in to view

Copy link
@FernetMenta

FernetMenta Feb 6, 2013

Contributor

I did some tests and this seems to work. I know, startpts is for accurate seeks, maybe we need an additional parameter: http://www.xbmclogs.com/show.php?id=33207

Note line 30 of the diff, I think we need to add m_offset_pts now.

if(m_pDemuxer)
{
state.chapter = m_pDemuxer->GetChapter();
state.chapter_count = m_pDemuxer->GetChapterCount();
m_pDemuxer->GetChapterName(state.chapter_name);

state.time = DVD_TIME_TO_MSEC(m_clock.GetClock() + m_offset_pts);
if(state.dts == DVD_NOPTS_VALUE)
state.time = 0;
else
state.time = DVD_TIME_TO_MSEC(state.dts + m_offset_pts);
state.time_total = m_pDemuxer->GetStreamLength();
state.time_src = ETIMESOURCE_CLOCK;
}
Expand Down Expand Up @@ -3926,7 +3955,7 @@ void CDVDPlayer::UpdatePlayState(double timeout)
state.timestamp = CDVDClock::GetAbsoluteClock();

CSingleLock lock(m_StateSection);
m_State = state;
m_StateInput = state;
}

void CDVDPlayer::UpdateApplication(double timeout)
Expand Down
10 changes: 9 additions & 1 deletion xbmc/cores/dvdplayer/DVDPlayer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ class CCurrentStream
int source;
double dts; // last dts from demuxer, used to find disncontinuities
double dur; // last frame expected duration
double dts_state; // when did we last send a playback state update
CDVDStreamInfo hint; // stream hints, used to notice stream changes
void* stream; // pointer or integer, identifying stream playing. if it changes stream changed
int changes; // remembered counter from stream to track codec changes
Expand All @@ -87,6 +88,7 @@ class CCurrentStream
id = -1;
source = STREAM_SOURCE_NONE;
dts = DVD_NOPTS_VALUE;
dts_state = DVD_NOPTS_VALUE;
dur = DVD_NOPTS_VALUE;
hint.Clear();
stream = NULL;
Expand Down Expand Up @@ -407,11 +409,15 @@ class CDVDPlayer : public IPlayer, public CThread, public IDVDPlayer
ETIMESOURCE_MENU,
};

friend class CDVDPlayerVideo;
friend class CDVDPlayerAudio;

struct SPlayerState
{
SPlayerState() { Clear(); }
void Clear()
{
player = 0;
timestamp = 0;
time = 0;
time_total = 0;
Expand All @@ -434,6 +440,8 @@ class CDVDPlayer : public IPlayer, public CThread, public IDVDPlayer
cache_offset = 0.0;
}

int player; // source of this data

double timestamp; // last time of update
double time_offset; // difference between time and pts

Expand Down Expand Up @@ -461,7 +469,7 @@ class CDVDPlayer : public IPlayer, public CThread, public IDVDPlayer
double cache_level; // current estimated required cache level
double cache_delay; // time until cache is expected to reach estimated level
double cache_offset; // percentage of file ahead of current position
} m_State;
} m_State, m_StateInput;
CCriticalSection m_StateSection;

CEvent m_ready;
Expand Down
11 changes: 11 additions & 0 deletions xbmc/cores/dvdplayer/DVDPlayerAudio.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,17 @@ int CDVDPlayerAudio::DecodeFrame(DVDAudioFrame &audioframe, bool bDropPacket)
if(m_started)
m_messageParent.Put(new CDVDMsgInt(CDVDMsg::PLAYER_STARTED, DVDPLAYER_AUDIO));
}
else if (pMsg->IsType(CDVDMsg::PLAYER_DISPLAYTIME))
{
CDVDPlayer::SPlayerState& state = ((CDVDMsgType<CDVDPlayer::SPlayerState>*)pMsg)->m_value;

if(state.time_src == CDVDPlayer::ETIMESOURCE_CLOCK)
state.time = DVD_TIME_TO_MSEC(m_pClock->GetClock(state.timestamp) + state.time_offset);
else
state.timestamp = CDVDClock::GetAbsoluteClock();
state.player = DVDPLAYER_AUDIO;
m_messageParent.Put(pMsg->Acquire());
}
else if (pMsg->IsType(CDVDMsg::GENERAL_EOF))
{
CLog::Log(LOGDEBUG, "CDVDPlayerAudio - CDVDMsg::GENERAL_EOF");
Expand Down
11 changes: 11 additions & 0 deletions xbmc/cores/dvdplayer/DVDPlayerVideo.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,17 @@ void CDVDPlayerVideo::Process()
if(m_started)
m_messageParent.Put(new CDVDMsgInt(CDVDMsg::PLAYER_STARTED, DVDPLAYER_VIDEO));
}
else if (pMsg->IsType(CDVDMsg::PLAYER_DISPLAYTIME))
{
CDVDPlayer::SPlayerState& state = ((CDVDMsgType<CDVDPlayer::SPlayerState>*)pMsg)->m_value;

if(state.time_src == CDVDPlayer::ETIMESOURCE_CLOCK)
state.time = DVD_TIME_TO_MSEC(m_pClock->GetClock(state.timestamp) + state.time_offset);
else
state.timestamp = CDVDClock::GetAbsoluteClock();
state.player = DVDPLAYER_VIDEO;
m_messageParent.Put(pMsg->Acquire());
}
else if (pMsg->IsType(CDVDMsg::GENERAL_STREAMCHANGE))
{
CDVDMsgVideoCodecChange* msg(static_cast<CDVDMsgVideoCodecChange*>(pMsg));
Expand Down

11 comments on commit e2ce6f3

@jpsdr
Copy link
Contributor

@jpsdr jpsdr commented on e2ce6f3 Feb 19, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this issue be linked to this ?
http://trac.xbmc.org/ticket/14105

@FernetMenta
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we are working on #2189 which might fix this as well.

@jpsdr
Copy link
Contributor

@jpsdr jpsdr commented on e2ce6f3 Feb 23, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested a build i've made today (23/02), which include these commits, but issue is not solved.

@FernetMenta
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

navigating through chapters works fine here.

@jpsdr
Copy link
Contributor

@jpsdr jpsdr commented on e2ce6f3 Feb 24, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just in case it would have been caused by my build, i've just tested the nightly XBMCSetup-20130222-4b9a54c-master.exe version. Behavior is still exactly the same i've described in ticket http://trac.xbmc.org/ticket/14105. When doing next chapter, and then immediatly after doing next chapter again (or even if you wait 1s or 2s), i'm looping on the same chapter.

@jpsdr
Copy link
Contributor

@jpsdr jpsdr commented on e2ce6f3 Feb 26, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Log file in debug from build made the 23/02, with the issue :
http://pastebin.com/YZWrejZA
Log file in debug from build made the 26/01, without the issue :
http://pastebin.com/dfrt2xKt

@FernetMenta
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jpsdr can you try this? FernetMenta@0282626

@FernetMenta
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one of those anime samples, right? I guess something must be irregular with dts, seeking is always off by a couple of secs.

@elupus if an input stream has no or irregular dts, we may have problems with this:

  /* send a playback state structure periodically */
  if(current.dts_state == DVD_NOPTS_VALUE
  || abs(current.dts - current.dts_state) > DVD_MSEC_TO_TIME(200))
  {

Should we count packets instead and send the structure every n-th packet?

@jpsdr
Copy link
Contributor

@jpsdr jpsdr commented on e2ce6f3 Feb 26, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is not specific to one file, all the files i have tested have it, are they new or several years old files. They are all mkv files with chapters. I've just tested on a mkv file of a movie (not anime) with chapters, issue is the same. It seems to happen to all mkv files with chapters, the fact they are anime or not seems irrelevant.

@elupus
Copy link
Contributor Author

@elupus elupus commented on e2ce6f3 Feb 26, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FernetMenta we could count packets too i suppose. But i wonder if that is what happens here.I sort of doubt it.

@FernetMenta
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created #2314. We sent too many old states to a/v players.

Please sign in to comment.