Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

pvr: dvdplayer - correct determination if video is playing #2423

Merged
merged 1 commit into from

5 participants

@FernetMenta
Collaborator

follow-up from #1377

1597 does not fix the entire issue because it does not change anything for VIDEOPLAYER_COVER

@elupus
Collaborator
@opdenkamp
Collaborator

closed #1377

@FernetMenta
Collaborator

elupus, I have updated the pr. I have set the bool to false in the contructor and close because pvr does not open a file on channel change.

@elupus
Collaborator
@FernetMenta FernetMenta dvdplayer: make HasVideo return true if a video stream was opened sin…
…ce playback has started - fixes missing video info on channel change
bd49c32
@FernetMenta FernetMenta merged commit 0b44ba6 into xbmc:master
@FernetMenta FernetMenta deleted the FernetMenta:videoinfo branch
@FernetMenta
Collaborator

@elupus should we set the same strategy for HasAudio? Currently audio visualization pops up for a short while when starting a video. I think this is the reason for that.

Collaborator

IMO should certainly be the same for HasAudio, just for consistency. I do wonder whether this is a good solution, we might as well check for eg. m_CurrentVideo.id > 0 then, right?

Owner

@FernetMenta IMO the reason it pops up is because we use m_bStop to control IsPlaying(). Looks to me like it'd be worth keeping track of a bool for when playback has actually started.

Collaborator

m_CurrentVideo.id > 0

We just dropped that because it didn't work for pvr. When changing channels there are no open streams.

EDIT: oh, you mean OR this condition?

Collaborator

Nope, didn't mean OR. Are you sure there isn't any stuff that depends on the fact that HasVideo() means an actual video stream is available?

Collaborator

I have been running a similar change for 5 months now and wsnipex built binaries for quite a couple of testers. I haven't seen any complaints about this.

Collaborator

That's not the point ;-) The point is that this stuff is very sensitive (I've discovered this myself quite a few times before) and more specifically easily introduces race problems. So even if it doesn't seem to break anything right now you really want to be sure it won't in the future either. And again I must stress that I REALLY wonder whether this is the right approach. Another solution could be passing a bool argument to HasVideo/HasAudio telling it to be strict or loose when checking...

Collaborator

I would rather simplify things than over-complicate it. Can you construct a case where this approach does harm? Anyway we need to wait for a comment by elupus.

Collaborator
Collaborator

elupus, anything you want to to do here for now?

Collaborator
Collaborator

We could set the bool to false in CloseVideo/AudioStream unless a channel switch is active. When the channel change is completed we receive a SPECIALID_STREAMCHANGE and call OpenDefaultStreams again. The bool would be set to false for obsolete streams.
What it the best method to detect a channel change in progress?

Collaborator

Not in Close*Stream(). That would be racy, dvd's will close and reopen streams all the time. We should really just update it on STREAMCHANGE for those that support that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 12, 2013
  1. @FernetMenta

    dvdplayer: make HasVideo return true if a video stream was opened sin…

    FernetMenta authored
    …ce playback has started - fixes missing video info on channel change
This page is out of date. Refresh to see the latest.
View
8 xbmc/cores/dvdplayer/DVDPlayer.cpp
@@ -412,6 +412,7 @@ CDVDPlayer::CDVDPlayer(IPlayerCallback& callback)
m_offset_pts = 0.0;
m_playSpeed = DVD_PLAYSPEED_NORMAL;
m_caching = CACHESTATE_DONE;
+ m_HasVideo = false;
memset(&m_SpeedState, 0, sizeof(m_SpeedState));
@@ -511,6 +512,8 @@ bool CDVDPlayer::CloseFile()
m_Edl.Clear();
m_EdlAutoSkipMarkers.Clear();
+ m_HasVideo = false;
+
CLog::Log(LOGNOTICE, "DVDPlayer: finished waiting");
#if defined(HAS_VIDEO_PLAYBACK)
g_renderManager.UnInit();
@@ -2410,9 +2413,7 @@ bool CDVDPlayer::IsPaused() const
bool CDVDPlayer::HasVideo() const
{
- if (m_pInputStream && m_pInputStream->IsStreamType(DVDSTREAM_TYPE_DVD)) return true;
-
- return m_SelectionStreams.Count(STREAM_VIDEO) > 0 ? true : false;
+ return m_HasVideo;
}
bool CDVDPlayer::HasAudio() const
@@ -2924,6 +2925,7 @@ bool CDVDPlayer::OpenVideoStream(int iStream, int source, bool reset)
m_CurrentVideo.hint = hint;
m_CurrentVideo.stream = (void*)pStream;
m_CurrentVideo.started = false;
+ m_HasVideo = true;
/* we are potentially going to be waiting on this */
m_dvdPlayerVideo.SendMessage(new CDVDMsg(CDVDMsg::PLAYER_STARTED), 1);
View
2  xbmc/cores/dvdplayer/DVDPlayer.h
@@ -489,4 +489,6 @@ class CDVDPlayer : public IPlayer, public CThread, public IDVDPlayer
} m_EdlAutoSkipMarkers;
CPlayerOptions m_PlayerOptions;
+
+ bool m_HasVideo;
};
Something went wrong with that request. Please try again.