Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

improve dvdplayer synchronization objects #1084

Merged
merged 6 commits into from

2 participants

@elupus
Collaborator

This series improves dvdplayer's synchronization objects, to avoid the deadlocks (with timeout) if one player is waiting for init while another is waiting for sync object before notifying init.

I'm pretty sure this resolves #12984

elupus added some commits
@elupus elupus [dvdplayer] remove old disabled looping still frame handling code
Disabled code that should be written differently if we
every need it again.
d6a631b
@elupus elupus [dvdplayer] remove old delayed processing of player sync
It should not be needed with the new handling of continuity
and the change most likley fixes issues with EDL seeking
causing long delays.
095624e
@elupus elupus [dvdplayer] rewrite syncronization message to be more efficient
This new sync message will now wakeup directly on object release
to check situation. It should reduce any scheduling delay that
previous implementation had.
9303d3c
@elupus elupus [dvdplayer] push back sync objects in message queue to process prio m…
…essage

Note, this could be moved into message queue and hidden from players
but i'm not sure it's that beneficial.
a88ec7a
@elupus elupus [dvdplayer] restore proper starttime behaviour
It should represent first dts of stream and is used to add start delays
to individual streams that start later than their sibling streams.

This got broken by cd36abf then further code started depending on the broken behaviour.
8996c05
@dteirney
Collaborator

I've tested this patch and it appears to resolve the issue with the seeking timeout and the problem with "go slow mode" after the seek. I also tested it with a number of my EDL testing files and I have observed some slightly strange behavior for some of my samples that have lots of small cuts that are close together. I've added a log file to http://trac.xbmc.org/ticket/12984

@elupus
Collaborator
@elupus
@elupus elupus merged commit 23ee0be into xbmc:master
@tru tru referenced this pull request from a commit in RasPlex/plex-home-theatre
@tru tru Tweaks to image request sizes
Fixes #1084
Also added unit tests to the MediaUrlParser.
1ad997a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 17, 2012
  1. @elupus

    [dvdplayer] remove old disabled looping still frame handling code

    elupus authored
    Disabled code that should be written differently if we
    every need it again.
  2. @elupus

    [dvdplayer] remove old delayed processing of player sync

    elupus authored
    It should not be needed with the new handling of continuity
    and the change most likley fixes issues with EDL seeking
    causing long delays.
  3. @elupus

    [dvdplayer] rewrite syncronization message to be more efficient

    elupus authored
    This new sync message will now wakeup directly on object release
    to check situation. It should reduce any scheduling delay that
    previous implementation had.
  4. @elupus

    [dvdplayer] push back sync objects in message queue to process prio m…

    elupus authored
    …essage
    
    Note, this could be moved into message queue and hidden from players
    but i'm not sure it's that beneficial.
  5. @elupus

    [dvdplayer] restore proper starttime behaviour

    elupus authored
    It should represent first dts of stream and is used to add start delays
    to individual streams that start later than their sibling streams.
    
    This got broken by cd36abf then further code started depending on the broken behaviour.
Commits on Jul 2, 2012
  1. @elupus
This page is out of date. Refresh to see the latest.
View
81 xbmc/cores/dvdplayer/DVDMessage.cpp
@@ -24,35 +24,86 @@
#include "DVDDemuxers/DVDDemuxUtils.h"
#include "DVDStreamInfo.h"
#include "utils/TimeUtils.h"
+#include "utils/log.h"
+#include "threads/CriticalSection.h"
+#include "threads/Condition.h"
+#include "threads/SystemClock.h"
+#include "utils/MathUtils.h"
+class CDVDMsgGeneralSynchronizePriv
+{
+public:
+ CDVDMsgGeneralSynchronizePriv(unsigned int timeout, unsigned int sources)
+ : timeout(timeout)
+ , sources(sources ? sources : SYNCSOURCE_ALL)
+ , reached(0)
+ {}
+ unsigned int sources;
+ unsigned int reached;
+ CCriticalSection section;
+ XbmcThreads::ConditionVariable condition;
+ XbmcThreads::EndTime timeout;
+};
/**
* CDVDMsgGeneralSynchronize --- GENERAL_SYNCRONIZR
*/
-CDVDMsgGeneralSynchronize::CDVDMsgGeneralSynchronize(DWORD timeout, DWORD sources) : CDVDMsg(GENERAL_SYNCHRONIZE)
+CDVDMsgGeneralSynchronize::CDVDMsgGeneralSynchronize(unsigned int timeout, unsigned int sources) : CDVDMsg(GENERAL_SYNCHRONIZE)
+ , m_p(new CDVDMsgGeneralSynchronizePriv(timeout, sources))
{
- if( sources )
- m_sources = sources;
- else
- m_sources = SYNCSOURCE_ALL;
+}
- m_objects = 0;
- m_timeout = timeout;
+CDVDMsgGeneralSynchronize::~CDVDMsgGeneralSynchronize()
+{
+ delete m_p;
}
-void CDVDMsgGeneralSynchronize::Wait(volatile bool *abort, DWORD source)
+bool CDVDMsgGeneralSynchronize::Wait(unsigned long milliseconds, unsigned int source)
{
+ if(source == 0)
+ source = SYNCSOURCE_OWNER;
+
/* if we are not requested to wait on this object just return, reference count will be decremented */
- if (source && !(m_sources & source)) return;
+ if (!(m_p->sources & source))
+ return true;
+
+ CSingleLock lock(m_p->section);
- AtomicIncrement(&m_objects);
+ XbmcThreads::EndTime timeout(milliseconds);
- XbmcThreads::EndTime timeout(m_timeout);
+ m_p->reached |= source & m_p->sources;
- if (abort)
- while( m_objects < GetNrOfReferences() && !timeout.IsTimePast() && !(*abort)) Sleep(1);
- else
- while( m_objects < GetNrOfReferences() && !timeout.IsTimePast() ) Sleep(1);
+ while( (long)MathUtils::bitcount(m_p->reached) < GetNrOfReferences() )
+ {
+ milliseconds = std::min(m_p->timeout.MillisLeft(), timeout.MillisLeft());
+ if(m_p->condition.wait(lock, milliseconds))
+ continue;
+ if(m_p->timeout.IsTimePast())
+ return true; /* global timeout, we are done */
+ if(timeout.IsTimePast())
+ return false; /* request timeout, should be retried */
+ }
+ return true;
+}
+
+void CDVDMsgGeneralSynchronize::Wait(volatile bool *abort, unsigned int source)
+{
+ while(!Wait(100, source))
+ {
+ if(abort && *abort)
+ return;
+ }
+}
+
+long CDVDMsgGeneralSynchronize::Release()
+{
+ CSingleLock lock(m_p->section);
+ long count = --m_refs;
+ m_p->condition.notifyAll();
+ lock.Leave();
+ if (count == 0)
+ delete this;
+ return count;
}
/**
View
15 xbmc/cores/dvdplayer/DVDMessage.h
@@ -147,20 +147,23 @@ class CDVDMsgGeneralResync : public CDVDMsg
#define SYNCSOURCE_AUDIO 0x00000001
#define SYNCSOURCE_VIDEO 0x00000002
#define SYNCSOURCE_SUB 0x00000004
-#define SYNCSOURCE_ALL (SYNCSOURCE_AUDIO | SYNCSOURCE_VIDEO | SYNCSOURCE_SUB)
+#define SYNCSOURCE_OWNER 0x80000000 /* only allowed for the constructor of the object */
+#define SYNCSOURCE_ALL (SYNCSOURCE_AUDIO | SYNCSOURCE_VIDEO | SYNCSOURCE_SUB | SYNCSOURCE_OWNER)
+class CDVDMsgGeneralSynchronizePriv;
class CDVDMsgGeneralSynchronize : public CDVDMsg
{
public:
- CDVDMsgGeneralSynchronize(DWORD timeout, DWORD sources);
+ CDVDMsgGeneralSynchronize(unsigned int timeout, unsigned int sources);
+ ~CDVDMsgGeneralSynchronize();
+ virtual long Release();
// waits until all threads waiting, released the object
// if abort is set somehow
- void Wait(volatile bool *abort, DWORD source);
+ bool Wait(unsigned long ms , unsigned int source);
+ void Wait(volatile bool *abort, unsigned int source);
private:
- DWORD m_sources;
- long m_objects;
- unsigned int m_timeout;
+ class CDVDMsgGeneralSynchronizePriv* m_p;
};
template <typename T>
View
102 xbmc/cores/dvdplayer/DVDPlayer.cpp
@@ -1403,8 +1403,6 @@ void CDVDPlayer::HandlePlaySpeed()
|| (!m_dvdPlayerVideo.AcceptsData() && !m_CurrentAudio.started))
{
caching = CACHESTATE_DONE;
- SAFE_RELEASE(m_CurrentAudio.startsync);
- SAFE_RELEASE(m_CurrentVideo.startsync);
}
}
}
@@ -1494,9 +1492,17 @@ bool CDVDPlayer::CheckStartCaching(CCurrentStream& current)
bool CDVDPlayer::CheckPlayerInit(CCurrentStream& current, unsigned int source)
{
- if(current.startpts != DVD_NOPTS_VALUE
- && current.dts != DVD_NOPTS_VALUE)
+ if(current.inited)
+ return false;
+
+ if(current.startpts != DVD_NOPTS_VALUE)
{
+ if(current.dts == DVD_NOPTS_VALUE)
+ {
+ CLog::Log(LOGDEBUG, "%s - dropping packet type:%d dts:%f to get to start point at %f", __FUNCTION__, source, current.dts, current.startpts);
+ return true;
+ }
+
if((current.startpts - current.dts) > DVD_SEC_TO_TIME(20))
{
CLog::Log(LOGDEBUG, "%s - too far to decode before finishing seek", __FUNCTION__);
@@ -1510,26 +1516,15 @@ bool CDVDPlayer::CheckPlayerInit(CCurrentStream& current, unsigned int source)
m_CurrentTeletext.startpts = current.dts;
}
- if(current.startpts <= current.dts)
- current.startpts = DVD_NOPTS_VALUE;
- }
-
- // await start sync to be finished
- if(current.startpts != DVD_NOPTS_VALUE)
- {
- CLog::Log(LOGDEBUG, "%s - dropping packet type:%d dts:%f to get to start point at %f", __FUNCTION__, source, current.dts, current.startpts);
- return true;
- }
-
- // send of the sync message if any
- if(current.startsync)
- {
- SendPlayerMessage(current.startsync, source);
- current.startsync = NULL;
+ if(current.dts < current.startpts)
+ {
+ CLog::Log(LOGDEBUG, "%s - dropping packet type:%d dts:%f to get to start point at %f", __FUNCTION__, source, current.dts, current.startpts);
+ return true;
+ }
}
//If this is the first packet after a discontinuity, send it as a resync
- if (current.inited == false && current.dts != DVD_NOPTS_VALUE)
+ if (current.dts != DVD_NOPTS_VALUE)
{
current.inited = true;
current.startpts = current.dts;
@@ -1559,7 +1554,7 @@ bool CDVDPlayer::CheckPlayerInit(CCurrentStream& current, unsigned int source)
starttime = m_CurrentVideo.startpts;
starttime = current.startpts - starttime;
- if(starttime > 0)
+ if(starttime > 0 && setclock)
{
if(starttime > DVD_SEC_TO_TIME(2))
CLog::Log(LOGWARNING, "CDVDPlayer::CheckPlayerInit(%d) - Ignoring too large delay of %f", source, starttime);
@@ -1612,43 +1607,6 @@ void CDVDPlayer::CheckContinuity(CCurrentStream& current, DemuxPacket* pPacket)
if( pPacket->dts == DVD_NOPTS_VALUE || current.dts == DVD_NOPTS_VALUE)
return;
-#if 0
- // these checks seem to cause more harm, than good
- // looping stillframes are not common in normal files
- // and a better fix for this behaviour would be to
- // correct the timestamps with some offset
-
- if (current.type == STREAM_VIDEO
- && m_CurrentAudio.dts != DVD_NOPTS_VALUE
- && m_CurrentVideo.dts != DVD_NOPTS_VALUE)
- {
- /* check for looping stillframes on non dvd's, dvd's will be detected by long duration check later */
- if( m_pInputStream && !m_pInputStream->IsStreamType(DVDSTREAM_TYPE_DVD))
- {
- /* special case for looping stillframes THX test discs*/
- /* only affect playback when not from dvd */
- if( (m_CurrentAudio.dts > m_CurrentVideo.dts + DVD_MSEC_TO_TIME(200))
- && (m_CurrentVideo.dts == pPacket->dts) )
- {
- CLog::Log(LOGDEBUG, "CDVDPlayer::CheckContinuity - Detected looping stillframe");
- SynchronizePlayers(SYNCSOURCE_VIDEO);
- return;
- }
- }
-
- /* if we haven't received video for a while, but we have been */
- /* getting audio much more recently, make sure video wait's */
- /* this occurs especially on thx test disc */
- if( (pPacket->dts > m_CurrentVideo.dts + DVD_MSEC_TO_TIME(200))
- && (pPacket->dts < m_CurrentAudio.dts + DVD_MSEC_TO_TIME(50)) )
- {
- CLog::Log(LOGDEBUG, "CDVDPlayer::CheckContinuity - Potential long duration frame");
- SynchronizePlayers(SYNCSOURCE_VIDEO);
- return;
- }
- }
-#endif
-
double mindts = DVD_NOPTS_VALUE, maxdts = DVD_NOPTS_VALUE;
UpdateLimits(mindts, maxdts, m_CurrentAudio.dts);
UpdateLimits(mindts, maxdts, m_CurrentVideo.dts);
@@ -1699,7 +1657,7 @@ bool CDVDPlayer::CheckSceneSkip(CCurrentStream& current)
if(current.dts == DVD_NOPTS_VALUE)
return false;
- if(current.startpts != DVD_NOPTS_VALUE)
+ if(current.inited == false)
return false;
CEdl::Cut cut;
@@ -1722,8 +1680,8 @@ void CDVDPlayer::CheckAutoSceneSkip()
* If there is a startpts defined for either the audio or video stream then dvdplayer is still
* still decoding frames to get to the previously requested seek point.
*/
- if(m_CurrentAudio.startpts != DVD_NOPTS_VALUE
- || m_CurrentVideo.startpts != DVD_NOPTS_VALUE)
+ if(m_CurrentAudio.inited == false
+ || m_CurrentVideo.inited == false)
return;
if(m_CurrentAudio.dts == DVD_NOPTS_VALUE
@@ -1785,7 +1743,7 @@ void CDVDPlayer::CheckAutoSceneSkip()
}
-void CDVDPlayer::SynchronizeDemuxer(DWORD timeout)
+void CDVDPlayer::SynchronizeDemuxer(unsigned int timeout)
{
if(IsCurrentThread())
return;
@@ -1798,32 +1756,22 @@ void CDVDPlayer::SynchronizeDemuxer(DWORD timeout)
message->Release();
}
-void CDVDPlayer::SynchronizePlayers(DWORD sources)
+void CDVDPlayer::SynchronizePlayers(unsigned int sources)
{
- /* if we are awaiting a start sync, we can't sync here or we could deadlock */
- if(m_CurrentAudio.startsync
- || m_CurrentVideo.startsync
- || m_CurrentSubtitle.startsync
- || m_CurrentTeletext.startsync)
- {
- CLog::Log(LOGDEBUG, "%s - can't sync since we are already awaiting a sync", __FUNCTION__);
- return;
- }
-
/* we need a big timeout as audio queue is about 8seconds for 2ch ac3 */
const int timeout = 10*1000; // in milliseconds
CDVDMsgGeneralSynchronize* message = new CDVDMsgGeneralSynchronize(timeout, sources);
if (m_CurrentAudio.id >= 0)
- m_CurrentAudio.startsync = message->Acquire();
+ m_dvdPlayerAudio.SendMessage(message->Acquire());
if (m_CurrentVideo.id >= 0)
- m_CurrentVideo.startsync = message->Acquire();
+ m_dvdPlayerVideo.SendMessage(message->Acquire());
/* TODO - we have to rewrite the sync class, to not require
all other players waiting for subtitle, should only
be the oposite way
if (m_CurrentSubtitle.id >= 0)
- m_CurrentSubtitle.startsync = message->Acquire();
+ m_dvdPlayerSubtitle.SendMessage(message->Acquire());
*/
message->Release();
}
View
9 xbmc/cores/dvdplayer/DVDPlayer.h
@@ -69,13 +69,11 @@ class CCurrentStream
const int player;
// stuff to handle starting after seek
double startpts;
- CDVDMsg* startsync;
CCurrentStream(StreamType t, int i)
: type(t)
, player(i)
{
- startsync = NULL;
Clear();
}
@@ -89,9 +87,6 @@ class CCurrentStream
stream = NULL;
inited = false;
started = false;
- if(startsync)
- startsync->Release();
- startsync = NULL;
startpts = DVD_NOPTS_VALUE;
}
@@ -306,8 +301,8 @@ class CDVDPlayer : public IPlayer, public CThread, public IDVDPlayer
void HandlePlaySpeed();
bool IsInMenu() const;
- void SynchronizePlayers(DWORD sources);
- void SynchronizeDemuxer(DWORD timeout);
+ void SynchronizePlayers(unsigned int sources);
+ void SynchronizeDemuxer(unsigned int timeout);
void CheckAutoSceneSkip();
void CheckContinuity(CCurrentStream& current, DemuxPacket* pPacket);
bool CheckSceneSkip(CCurrentStream& current);
View
6 xbmc/cores/dvdplayer/DVDPlayerAudio.cpp
@@ -419,8 +419,10 @@ int CDVDPlayerAudio::DecodeFrame(DVDAudioFrame &audioframe, bool bDropPacket)
}
else if (pMsg->IsType(CDVDMsg::GENERAL_SYNCHRONIZE))
{
- ((CDVDMsgGeneralSynchronize*)pMsg)->Wait( &m_bStop, SYNCSOURCE_AUDIO );
- CLog::Log(LOGDEBUG, "CDVDPlayerAudio - CDVDMsg::GENERAL_SYNCHRONIZE");
+ if(((CDVDMsgGeneralSynchronize*)pMsg)->Wait( 100, SYNCSOURCE_AUDIO ))
+ CLog::Log(LOGDEBUG, "CDVDPlayerAudio - CDVDMsg::GENERAL_SYNCHRONIZE");
+ else
+ m_messageQueue.Put(pMsg->Acquire(), 1); /* push back as prio message, to process other prio messages */
}
else if (pMsg->IsType(CDVDMsg::GENERAL_RESYNC))
{ //player asked us to set internal clock
View
16 xbmc/cores/dvdplayer/DVDPlayerVideo.cpp
@@ -362,13 +362,19 @@ void CDVDPlayerVideo::Process()
if (pMsg->IsType(CDVDMsg::GENERAL_SYNCHRONIZE))
{
- ((CDVDMsgGeneralSynchronize*)pMsg)->Wait( &m_bStop, SYNCSOURCE_VIDEO );
- CLog::Log(LOGDEBUG, "CDVDPlayerVideo - CDVDMsg::GENERAL_SYNCHRONIZE");
+ if(((CDVDMsgGeneralSynchronize*)pMsg)->Wait(100, SYNCSOURCE_VIDEO))
+ {
+ CLog::Log(LOGDEBUG, "CDVDPlayerVideo - CDVDMsg::GENERAL_SYNCHRONIZE");
+
+ /* we may be very much off correct pts here, but next picture may be a still*/
+ /* make sure it isn't dropped */
+ m_iNrOfPicturesNotToSkip = 5;
+ }
+ else
+ m_messageQueue.Put(pMsg->Acquire(), 1); /* push back as prio message, to process other prio messages */
+
pMsg->Release();
- /* we may be very much off correct pts here, but next picture may be a still*/
- /* make sure it isn't dropped */
- m_iNrOfPicturesNotToSkip = 5;
continue;
}
else if (pMsg->IsType(CDVDMsg::GENERAL_RESYNC))
View
8 xbmc/utils/MathUtils.h
@@ -208,6 +208,14 @@ namespace MathUtils
return (a < 0) ? -a : a;
}
+ inline unsigned bitcount(unsigned v)
+ {
+ unsigned c = 0;
+ for (c = 0; v; c++)
+ v &= v - 1; // clear the least significant bit set
+ return c;
+ }
+
inline void hack()
{
// stupid hack to keep compiler from dropping these
Something went wrong with that request. Please try again.