Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Disptime #2127

Merged
merged 4 commits into from

4 participants

@elupus
Collaborator

This pull request fixes some long standing issues with usage of display time at demuxer stage without taking into account a/v queues. This made blurays/dvds and pvr display the wrong time in gui as well as seeks ending up in unexpected places.

@FernetMenta
Collaborator

I have tested with some short 2-3 minutes samples. A small step forward (configured to 10s) brings me to end of file and terminates playback.

@elupus
Collaborator
@FernetMenta
Collaborator

Normal files. I tried a short mkv and a ts.

@elupus
Collaborator
@elupus
Collaborator
@FernetMenta
Collaborator

1)
I tested with a keyboard. Hitting key "period" sometimes results in StepForward, sometimes in SkipNext. Not clear to me when what action is taken. Sorry for confusion caused by this.

2)
see comment in code.

xbmc/cores/dvdplayer/DVDPlayer.cpp
@@ -2278,6 +2286,18 @@ 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);
+ if(state.player == DVDPLAYER_VIDEO) {
@FernetMenta Collaborator

I think we should only update if m_CurrentVideo.stated or we get an old state before a seek.

@elupus Collaborator
elupus added a note

I didn't initially like that. But now that i think about it you are absolutely right. During a seek request, we can have received a queued up time structure. This would not be flushed by the seek, so we must ignore it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
xbmc/cores/dvdplayer/DVDPlayer.cpp
@@ -2278,6 +2286,18 @@ 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);
+ if(state.player == DVDPLAYER_VIDEO) {
+ m_State = state;
+ CLog::Log(LOGDEBUG, "GRR %f", state.time);
+ }
+ else if(state.player == DVDPLAYER_AUDIO && !m_CurrentVideo.started)
@FernetMenta Collaborator

same here, check for CurrentAudio.started

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@FernetMenta
Collaborator

This works for me after a first couple of tests (and looks much better than what I had before)

As discussed with @opdenkamp we should revert c1f11e6 once you have injected this.

I am working on time shift for vnsi and will do intensive test with this.

@opdenkamp
Collaborator

yeah, forgot to ping in here about that revert, but just add it to this PR?

elupus added some commits
@elupus elupus dvdplayer: pass display time through a/v players before out to applic…
…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.
e2ce6f3
@elupus elupus dvdplayer: fixed time_offset is difference between adjusted pts and time 406ef63
@elupus elupus dvdplayer: allow 200ms of automatic time update instead of 1ms
It was originally meant to allow 1 second. But has at some point
been broken since the unit has been changed from ms to dvd time
a489fb5
@elupus elupus Revert "[pvr] fixed - seek passed the time since the epg item started…
…, which won't work for seek operations"

This reverts commit c1f11e6.
1305d23
@elupus elupus merged commit a3ade38 into xbmc:master
@elupus elupus deleted the elupus:disptime branch
@FernetMenta

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 :(

Collaborator
Collaborator

Having NOPTS_VALUE in state.dts for at least 200ms. Holding e.g. the SmallStep button results in a seek to NOPTS_VALUE.

@FernetMenta

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

Collaborator
Collaborator

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.

@jpsdr

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

Collaborator

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

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

Collaborator

navigating through chapters works fine here.

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.

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

Collaborator

@jpsdr can you try this? FernetMenta@0282626

Collaborator

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?

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.

Collaborator

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

Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 31, 2013
  1. @elupus

    dvdplayer: pass display time through a/v players before out to applic…

    elupus authored
    …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.
  2. @elupus
  3. @elupus

    dvdplayer: allow 200ms of automatic time update instead of 1ms

    elupus authored
    It was originally meant to allow 1 second. But has at some point
    been broken since the unit has been changed from ms to dvd time
  4. @elupus

    Revert "[pvr] fixed - seek passed the time since the epg item started…

    elupus authored
    …, which won't work for seek operations"
    
    This reverts commit c1f11e6.
This page is out of date. Refresh to see the latest.
View
2  xbmc/cores/dvdplayer/DVDMessage.h
@@ -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
View
60 xbmc/cores/dvdplayer/DVDPlayer.cpp
@@ -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))
+ {
+ 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)
@@ -2019,7 +2027,7 @@ void CDVDPlayer::HandleMessages()
// if input streams doesn't support seektime we must convert back to clock
if(dynamic_cast<CDVDInputStream::ISeekTime*>(m_pInputStream) == NULL)
- time -= DVD_TIME_TO_MSEC(m_State.time_offset);
+ time -= DVD_TIME_TO_MSEC(m_State.time_offset - m_offset_pts);
CLog::Log(LOGDEBUG, "demuxer seek to: %d", time);
if (m_pDemuxer && m_pDemuxer->SeekTime(time, msg.GetBackward(), &start))
@@ -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 (...)
{
@@ -2534,7 +2560,7 @@ 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());
}
@@ -2542,7 +2568,7 @@ void CDVDPlayer::GetAudioInfo(CStdString& strAudioInfo)
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());
}
@@ -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()
@@ -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)
@@ -2750,12 +2776,13 @@ int64_t CDVDPlayer::GetTime()
{
CSingleLock lock(m_StateSection);
double offset = 0;
+ const double limit = DVD_MSEC_TO_TIME(200);
if(m_State.timestamp > 0)
{
offset = CDVDClock::GetAbsoluteClock() - m_State.timestamp;
offset *= m_playSpeed / DVD_PLAYSPEED_NORMAL;
- if(offset > 1000) offset = 1000;
- if(offset < -1000) offset = -1000;
+ if(offset > limit) offset = limit;
+ if(offset < -limit) offset = -limit;
}
return llrint(m_State.time + DVD_TIME_TO_MSEC(offset));
}
@@ -3732,7 +3759,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()
@@ -3798,18 +3825,18 @@ 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;
if(m_pDemuxer)
{
@@ -3817,7 +3844,10 @@ void CDVDPlayer::UpdatePlayState(double timeout)
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;
}
@@ -3877,7 +3907,7 @@ void CDVDPlayer::UpdatePlayState(double timeout)
}
if (state.time_src == ETIMESOURCE_CLOCK)
- state.time_offset = 0;
+ state.time_offset = m_offset_pts;
else
state.time_offset = DVD_MSEC_TO_TIME(state.time) - state.dts;
@@ -3926,7 +3956,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)
View
10 xbmc/cores/dvdplayer/DVDPlayer.h
@@ -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
@@ -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;
@@ -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;
@@ -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
@@ -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;
View
11 xbmc/cores/dvdplayer/DVDPlayerAudio.cpp
@@ -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");
View
11 xbmc/cores/dvdplayer/DVDPlayerVideo.cpp
@@ -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));
View
6 xbmc/pvr/addons/PVRClient.cpp
@@ -29,7 +29,6 @@
#include "settings/AdvancedSettings.h"
#include "utils/log.h"
#include "settings/GUISettings.h"
-#include "Application.h"
using namespace std;
using namespace ADDON;
@@ -878,10 +877,7 @@ bool CPVRClient::SeekTime(int time, bool backwards, double *startpts)
{
if (IsPlaying())
{
- // player time is added to time here, which is taken from the epg
- // we can either substract it again here, or add special pvr cases in players
- int iChangeTime = time - (int)g_application.m_pPlayer->GetTime();
- try { return m_pStruct->SeekTime(iChangeTime, backwards, startpts); }
+ try { return m_pStruct->SeekTime(time, backwards, startpts); }
catch (exception &e) { LogException(e, "SeekTime()"); }
}
return false;
Something went wrong with that request. Please try again.