Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

PVR: Show full screen video information even if stream isn't working #1377

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants
Contributor

mikrohard commented Sep 6, 2012

I'm moving this pull request from opdekamps repo here to continue the discussion on what the best solution for this problem is.

Old pull request for description and discussion history: opdenkamp#623

This may not be the cleanest way to do it but it fixes the described problem without breaking pvr radio visualizations. Any other suggestions?

Member

jmarshallnz commented Sep 6, 2012

Wouldn't it be better is IsPlayingVideo returned true, or there was some better flag to check at the app level?

@ghost ghost assigned alanwww1 Sep 15, 2012

@ghost ghost assigned opdenkamp Oct 8, 2012

Member

opdenkamp commented Oct 9, 2012

ping @mikrohard @elupus @FernetMenta
any updates on this? see the discussion on opdenkamp#623

Contributor

mikrohard commented Oct 9, 2012

I didn't research this any further. The code in current pull request doesn't break visualizations and fixes the described problem.

I agree that it would be cleaner to fix this in applications IsPlayingVideo or players HasVideo. But we need a non-locking way to check for radio if we don't want to break pvr radio visualizations.

Member

FernetMenta commented Oct 9, 2012

Maybe we are thinking much too complicated here. We are struggling to determine whether a video is playing or not, dealing with selected streams.
How about asking renderer: CXBMCRenderManager::IsConfigured() ?
As long as renderer is configured, we bring to display what renderer has to offer.

bool CDVDPlayer::HasVideo() const
{
  return g_renderManager.IsConfigured()
}

That would work during a channel switch and visualization for audio should work as well.

Member

opdenkamp commented Oct 9, 2012

sounds good to me

Member

opdenkamp commented Oct 27, 2012

i've tested changing CApplication::IsPlayingVideo() to check g_renderManager.IsConfigured() and it worked fine.

ping @FernetMenta @cptspiff what are your ideas about this? would this be safe to change for frodo? other option would be

--- a/xbmc/Application.cpp
+++ b/xbmc/Application.cpp
@@ -4333,6 +4333,8 @@ bool CApplication::IsPlayingVideo() const
 {
   if (!m_pPlayer)
     return false;
+  if (PVR::CPVRManager::Get().IsPlayingTV())
+    return true;
   if (!m_pPlayer->IsPlaying())
     return false;
   if (m_pPlayer->HasVideo())
Member

FernetMenta commented Oct 27, 2012

I have been testing this for a while: FernetMenta/xbmc@3d95c5c

From a logical point of view it makes most sense to fix CDVDPlayer::HasVideo(). It is dvdplayer which calls renderManager.init/uninit. Hence between those two calls it clearly has video.

Member

elupus commented Oct 27, 2012

Check my upnp pull. I'm changing that check to is playing instead. No major
problems with that. Should solve this too right?

Member

elupus commented Oct 27, 2012

Okey.. i admit there are some issues with it. confluence seem to display both music title and video title in some places.

Member

opdenkamp commented Oct 27, 2012

@FernetMenta but then this would need to be fixed for other players too. would be nice if we could fix the check that applies to all players

@elupus does IsPlaying() return true when nothing is actually playing, e.g. when switching channels?

Member

FernetMenta commented Oct 27, 2012

I think this has to be fixed in several places. Even if a change in application may fix this particular problem, player::HasVideo is not correct. It might be called by a skin, right?. Yes, I would fix this method in all players.

Member

elupus commented Oct 27, 2012

@opdenkamp IsPlaying() does return true while switching players if the switch happens inside dvdplayer and not via a new call to OpenFile

Member

elupus commented Oct 27, 2012

I've updated my pull at #1597 to fix the display issues I had with my solution. Check the first commits of that pull request.

Member

opdenkamp commented Oct 27, 2012

that looks like it could fix it indeed after @elupus PR. i'll test it as soons as i got time.

Member

elupus commented Oct 27, 2012

I've noticed a bug with music tags that need to be resolved. But after
that, all those thing can go in separate of the upnp stuff.

Member

elupus commented Nov 4, 2012

Soo.. my upnp pull is not going to go in for Frodo. Maybe some parts can go in thou.

Member

elupus commented Feb 10, 2013

So what is the status of this now. The upnp stuff has been merged so it's time to take a look to see what on this pull is required.

Member

opdenkamp commented Feb 11, 2013

haven't had time to do anything with this yet.

Member

opdenkamp commented Mar 10, 2013

ping @FernetMenta FernetMenta/xbmc@3d95c5c the right solution for this? if so, could you push it to master and close this PR

@opdenkamp opdenkamp closed this Mar 12, 2013

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