Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PVR] Reset ischannelpreview flag when pvr stream gets closed. Fixes trac#16840. #10264

Merged
merged 3 commits into from Aug 10, 2016

Conversation

@ksooo
Copy link
Member

commented Aug 10, 2016

Regression introduced by #10229
Fixes http://trac.kodi.tv/ticket/16840

@FernetMenta for review?
@axbmcuser maybe you can runtime test?

@axbmcuser

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2016

Yes. Just did some quick tests.
It looks like the problem is partly fixed - especially the crash seems to be gone.

What remains are problems within LiveTV-playback itself, again, best reproduced with Confluence:

  • Start LiveTV playback
  • Press "I" for VideoFullScreen INFO (without player buttons!)
  • Press "UP" a few times on the arrow keys (causing channelpreviewactive to be true, which is correct at this point, other channels are previewed within the GUI)
  • While GUI shows one of those previewed channels (don't confirm the switch!) press "I" to close VideoFullScreen INFO
  • Normal LiveTV-playback continues, GUI is now closed
  • Press "I" to open VideoFullScreen INFO again (without player buttons!)
  • Problem:
    channelpreviewactive still is true (causing some items to be hidden) - although you are located on the channel which is currently playing and you are actually not in the process of doing any channelpreview

Thanks!

@ksooo ksooo force-pushed the ksooo:trac16840 branch from ba1a878 to a5e4ce4 Aug 10, 2016
@ksooo

This comment has been minimized.

Copy link
Member Author

commented Aug 10, 2016

Second commit should fix also the other problem reported.

@axbmcuser

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2016

Quick tests with the additional second commit lead to strange results for me:
Like INFO dialog closing by itself after a short while and acting strangely. Can't easily define it in few words. Did not change anything else in the build compared to compiling with only the first commit included. Will try to test some more as soon as i can.

@axbmcuser

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2016

Just some quick feedback without being able to do extensive tests regarding the mentioned weird behaviour of the LiveTV-GUI:

(Follow exact steps - maybe not as easily reproducible as before and doesn't fail every time (Still: Confluence)):

  • Start LiveTV playback
  • Press "I" for VideoFullScreen INFO (without player buttons!)
  • Press "UP" a few times on the arrow keys (causing channelpreviewactive to be true, which is correct at this point, other channels are previewed within the GUI)
  • While GUI shows one of those previewed channels (don't confirm the switch!) press "I" to close VideoFullScreen INFO
  • Normal LiveTV-playback continues, GUI is now closed
  • Press "I" to open VideoFullScreen INFO again (without player buttons!)
  • Press "UP" a fast few times on the arrow keys (causing channelpreviewactive to be true, which is correct at this point, other channels are previewed within the GUI)
    - IMPORTANT: the following step has to follow very very closely to the step before. Means: Pressing the arrow key UP the last time has to be followed instantly by pressing "I" (!):
  • While GUI shows one of those previewed channels (don't confirm the switch!) press "I" to close VideoFullScreen INFO
  • Normal LiveTV-playback continues, GUI is now closed
  • Press "I" for VideoFullScreen INFO (without player buttons!)
  • Problem:
    INFO GUI still shows old channel preview data, after a short delay (0,5 - 2 seconds) it flickers back to the correct channel which is playing. Sometimes the just opened INFO dialog (without player buttons) even closes by itself without any good reason. (after ~ 1 second most of the time - if it does).

Sorry that it's so complicated, but maybe it helps a bit.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Aug 10, 2016

I will think about it in more detail this evening.

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Aug 10, 2016

The last problem with quickly reopening the info osd after closing it in preview mode is a known problem and not related to the recent channel preview changes or this PR. You should be able to reproduce with alpha 3 or even Jarvis. I know the root cause for this. A fix is imo not trivial.

Please, let's concentrate in this PR on fixing the regression introduced by recent changes. There are old bugs with channel preview, which should not be mixed in here.

@axbmcuser

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2016

Did some testing on Jarvis, thanks for the hint. Now i can differ the new from the old bugs.
Since i barely use LiveTV and had disabled channelpreviews in the past, i never stumbled upon it.
So, it looks like the new " channelpreviewactive-bug" and crash is fixed with the two commits discussed. If i notice otherwise, i'll let you know. Thanks again.

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Aug 10, 2016

Many thanks for reporting the issue and for testing. Much appreciated.

}
m_isPvrChannelPreview = g_PVRManager.IsChannelPreview();

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Aug 10, 2016

Member

this is not ideal for 2 reasons

  1. might bring the flickering back. updating a/v streams and preview need to be in sync.
  2. it would call IsChannelPreview on every render cycle, even if the INFO is not active

How about setting IsChannelPreview to false in CGUIInfoManager::SetShowInfo if m_playerShowInfo is set to false?

This comment has been minimized.

Copy link
@ksooo

ksooo Aug 10, 2016

Author Member

How about setting IsChannelPreview to false in CGUIInfoManager::SetShowInfo if m_playerShowInfo is set to false?

Yeah that works - if CGUIInfoManager::ToggleShowInfo also resets m_isPvrChannelPreview. I've just learned that ToggleInfo is called when hitting "I". Earlier today I tried what you suggested (yeah had the same idea!), but only in SetShowInfo. That was obviously not sufficient...

…tering it for current channel.
@ksooo ksooo force-pushed the ksooo:trac16840 branch from a5e4ce4 to fa21028 Aug 10, 2016
@ksooo

This comment has been minimized.

Copy link
Member Author

commented Aug 10, 2016

@FernetMenta commit 2 updated as you suggested.
@axbmcuser mind retesting?

m_isPvrChannelPreview = false;
}

bool CGUIInfoManager::ToggleShowInfo()

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Aug 10, 2016

Member

good idea to move the body here. much clearer now.

…ChannelInfoTag methods only if necessary.
@axbmcuser

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2016

Just compiled with commit 1 and 2(updated). Now there is commit 3. Recompiling including commit 3 now. Will retest afterwards - or are there more changes to wait for?

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Aug 10, 2016

No, that should be it.

@axbmcuser

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2016

All 3 commits included, i did not notice any new problems being introduced while making non-extensive tests. From user point of view these variants now seem to work equally:

  • commit 1 + 2 included
  • commit 1 + 2(updated after FernetMenta suggestions) included
  • comit 1 + 2(updated) + 3 included

Old bugs ignored this time, of course.

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Aug 10, 2016

Cool thanks.

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Aug 10, 2016

jenkins build this please

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Aug 10, 2016

travis error due to job timeout; not related to this PR.

@ksooo ksooo merged commit 74cb913 into xbmc:master Aug 10, 2016
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
jenkins.kodi.tv You did a great job. Have a cookie.
Details
@ksooo ksooo deleted the ksooo:trac16840 branch Aug 10, 2016
@axbmcuser

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2016

Update side note: Just noticed that there may be some side effects left regarding the ischannelpreview-issues.

Having mouse support always disabled, i noticed that having channelpreview in System -> TV -> Playback enabled apprently causes MusicOSD and "Music Fullscreen Info" to act strangely. Since i never use music, i just checked MusicVisualisation.xml and saw that ischannelpreview is also used in there.

(Again, to reproduce i use Confluence)

Quick test 1 (not much time today):

  • ENABLE channelpreview (aka "Confirm channel switches by pressing OK") in System -> TV -> Playback
  • Start playing music, go to fullscreen after playback has started
  • Press "I" for Fullscreen Info on playing Music
  • Hit ENTER (MusicOSD should open - but does not)

Quick test 2:

  • ENABLE channelpreview in System -> TV -> Playback
  • Start playing music, go to fullscreen after playback has started
  • Do not open Fullscreen Info. Just plain fullscreen music playback (without visualization in my case)
  • Hit ENTER (MusicOSD opens as it should) (in oppsite to test case 1 where it seems to be blocked)

Quick test 3:

  • DISABLE channelpreview in System -> TV -> Playback
  • retest quick test case 1
  • Different behaviour, MusicOSD opens as it should

Maybe someone has the time and/or wants to look into it and create a new PR if confirmed. I'm happy to participate in testing later on, if this find confirms to be valid.

EDIT:
Additional mini test case 4:

  • ENABLE channelpreview in System -> TV -> Playback
  • Start playing music, go to fullscreen after playback has started
  • Press "I" for Fullscreen Info on playing Music
  • Notice that the MP3 codec indicator is missing - most likely related to the issues (since !Player.ChannelPreviewActive is used in MusicVisualisation.xml as a visible-condition)
  • Press "I" to hide Fullscreen Info on playing Music
  • Notice that the MP3 codec indicator suddenly is shown for a short amount of time while fading out the Info
return m_playerShowInfo &&
(m_isPvrChannelPreview || !streamValid);
bool bReturn(false);
if (m_playerShowInfo)

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Aug 12, 2016

Member

@ksooo I think we need to check for m_currentFile->HasPVRChannelInfoTag() at this level too

This comment has been minimized.

Copy link
@ksooo

ksooo Aug 12, 2016

Author Member

Absolutely. This change will fix all of the issues reported above. Will open a PR in a minute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.