Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

[dvdplayer] Fix synchronize timeout with non-flushed seeking. #944

Closed
wants to merge 1 commit into from

2 participants

@dteirney
Collaborator

Non-flushed seeking (like used for automatic EDL and commercial break skipping) no longer uses the internal SynchronizePlayers function, which times out waiting for sync in favour of a typical GeneralSynchronize message to both the video and audio players.

@elupus, I've looked at the SynchronizePlayers function for a while now and I can't figure out how it's supposed to work. The only other place SynchronizePlayers is used is when an audio stream is opened and there is already a video stream opened. I'm not even sure if the method would ever work prior to the timeout being triggered.

I've tested the changes with short and long commercial breaks and EDL cuts and this change makes all of those test cases work smoothly again. Looking for a review to see if this is the best way to resolve the problem. Non-flushed seeking is only used by the automatic EDL and CommBreak seeks based on the references I found in dvdplayer.

@elupus elupus was assigned
@dteirney
Collaborator

@elupus, do you also know why the VIDEO_NOSKIP message would need to be sent to the video player? I've left the TODO in there to remind me to ask. I'll take that out and amend the commit with either a change to remove that message or to comment why it is needed.

David Teirney [dvdplayer] Fix synchronize timeout with non-flushed seeking. Ticket …
…#12984

Non-flushed seeking (like used for automatic EDL and commercial break skipping) no longer uses the internal SynchronizePlayers function, which times out waiting for sync in favour of a typical GeneralSynchronize message to both the video and audio players.
6d6cd56
@elupus
Collaborator

You sort of break the whole point of the non flushed seek by waiting on the sync to complete :).

The reason it's written the way it is, is that at the call to synchronize players, there doesn't need to be any audio player consuming packets in the audio queue. Thus, the sync would not have any effect. If the following frame contain a audio frame, the audio thread get created and and we fail to block that new audio stream until video catches up.

This could probably be improved, but it's rather tricky.

Now, i don't really know why you get the delay. If it's long, have you tried breaking in debugger and checking backtraces for the threads?

@dteirney
Collaborator

So as far as you know non-flushed seeking is indeed working? I wasn't sure given the only code path that I could see using it was the commercial skipping. The delay is always identical to the timeout used within SynchronizePlayers so somehow the lock isn't getting released.

I've had limited success getting my debugging environment working but I'll have another look at the weekend. I guess the next step is to find where the threads are sitting while that lock is waiting to timeout.

The way that the messages end up getting back to the place where the lock is released is pretty complicated so I'm not sure I'll be able to do more than identify where each thread is hanging.

@elupus
Collaborator
@dteirney
Collaborator

Backtrace included in the trac ticket at http://trac.xbmc.org/ticket/12984#comment:5

Not sure how useful it is though :(

@dteirney
Collaborator

@elupus is there anything else I can help with for this? The problem unfortunately makes playback of any content that has commercial skips very frustrating.

@elupus
Collaborator
@elupus
Collaborator

Should be resolved by: #1084

@elupus elupus closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 8, 2012
  1. [dvdplayer] Fix synchronize timeout with non-flushed seeking. Ticket …

    David Teirney authored
    …#12984
    
    Non-flushed seeking (like used for automatic EDL and commercial break skipping) no longer uses the internal SynchronizePlayers function, which times out waiting for sync in favour of a typical GeneralSynchronize message to both the video and audio players.
This page is out of date. Refresh to see the latest.
Showing with 7 additions and 1 deletion.
  1. +7 −1 xbmc/cores/dvdplayer/DVDPlayer.cpp
View
8 xbmc/cores/dvdplayer/DVDPlayer.cpp
@@ -3040,10 +3040,16 @@ void CDVDPlayer::FlushBuffers(bool queued, double pts, bool accurate)
{
m_dvdPlayerAudio.SendMessage(new CDVDMsg(CDVDMsg::GENERAL_RESET));
m_dvdPlayerVideo.SendMessage(new CDVDMsg(CDVDMsg::GENERAL_RESET));
+ // TODO: Why the VIDEO_NO_SKIP message?
m_dvdPlayerVideo.SendMessage(new CDVDMsg(CDVDMsg::VIDEO_NOSKIP));
m_dvdPlayerSubtitle.SendMessage(new CDVDMsg(CDVDMsg::GENERAL_RESET));
m_dvdPlayerTeletext.SendMessage(new CDVDMsg(CDVDMsg::GENERAL_RESET));
- SynchronizePlayers(SYNCSOURCE_ALL);
+
+ CDVDMsgGeneralSynchronize* msg = new CDVDMsgGeneralSynchronize(100, SYNCSOURCE_ALL);
+ m_dvdPlayerAudio.SendMessage(msg->Acquire(), 1);
+ m_dvdPlayerVideo.SendMessage(msg->Acquire(), 1);
+ msg->Wait(&m_bStop, 0);
+ msg->Release();
}
else
{
Something went wrong with that request. Please try again.