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] [confluence] [WIP] timeshift GUI #9045

Closed
wants to merge 9 commits into from

Conversation

Glenn-1990
Copy link
Contributor

Confluence is missing a good timeshift indicator, also the timeshift gui methods seems a bit limited or unusefull.

My idea is to add markers to the process bar, these markers will indicate the start en stop buffer position. Also the timeshift delay will be shown when not at the live point.

PLAYER_PROGRESS_CACHE will now also point to the timeshift buffer end, in this way existing skins will also show the buffer, I'm not sure if this is correct though..
All other timeshift (older) methods seems a bit useless and can be deleted to reduce code complexity?

Updates from markers and delay time seems a bit bumpy from time to time, but I think that's a common issue with PVRGUIInfo, still investigating.

schermafdruk van 2016-02-04 11 44 50

@ksooo @xhaggi @ronie @Jalle19

@ksooo ksooo added Type: Improvement non-breaking change which improves existing functionality Component: PVR v17 Krypton labels Feb 4, 2016
@@ -308,6 +309,12 @@ void CPVRGUIInfo::UpdateTimeshift(void)
tmp.SetFromUTCDateTime(iTimeshiftPlayTime);
std::string strTimeshiftPlayTime = tmp.GetAsLocalizedTime("", true);

time_t iTimeshiftDelay = iTimeshiftEndTime-iTimeshiftPlayTime;

This comment was marked as spam.

@ksooo
Copy link
Member

ksooo commented Feb 4, 2016

Regarding functionality: How will it look like if timeshift buffer start is before the start of the currently playing event or timeshift buffer end is after the end of the currently playing event or both or if there is no current epg event at all?

BTW, I really appreciate that you picked up this topic. It is so long overdue, if you ask me. Thanks.

@Glenn-1990
Copy link
Contributor Author

@ksooo the marker will be placed on the show running at that time, so the end marker can be on the next epgtag while the start marker is still on the previous epgtag. Let's say you are watching show b, but the timeshift start position is somewhere on show a. Now, if you skip back in time, over the start position of show b, show a will be displayed and also the start marker (the stopmarker is invisible as it's on show b).
I'm not sure what will be displayed when there is no epg event in the future, I'll have to test this.

What do you think of removing methods?
"PVR_TIMESHIFT_START_TIME"
"PVR_TIMESHIFT_END_TIME"
"PVR_TIMESHIFT_PLAY_TIME"
"PVR_TIMESHIFT_PROGRESS"
These are unused and only adding complexity.

@ksooo
Copy link
Member

ksooo commented Feb 4, 2016

the marker will be placed on the show running at that time, so the end marker can be on the next epgtag while the start marker is still on the previous epgtag.

Sorry, not sure I understand that. Simple question: What is start and end of the progressbar representing? Start/end of the currently running show or start/end of the timeshift buffer?

I'm not sure what will be displayed when there is no epg event in the future,

its not just about future. There are channels that do not have epg data at all, but ofc timeshifting should work for those channels as well. => #9025

@Glenn-1990
Copy link
Contributor Author

@ksooo
with epg:
progressbar start = begin of the show
progress bar end = end of the show
markers are placed relative to the current show

without epg:
progressbar start = begin of buffer
progress bar end = end of buffer
markers are not visible

@ksooo
Copy link
Member

ksooo commented Feb 4, 2016

Okay, thanks.

msgctxt "#31041"
msgid "Timeshifting: "
msgstr ""

#empty string with id 31041

This comment was marked as spam.

@Glenn-1990
Copy link
Contributor Author

@ksooo the current GUI methods are a bit messy and are also causing other issues and glitches, mainly in the video OSD (jumping progressbar, flickering video and audio details,..). So I'll need to address those first before adding new functionality to wrong code. I'll add some commits later this evening.

@ksooo
Copy link
Member

ksooo commented Feb 7, 2016

@ksooo the current GUI methods are a bit messy and are also causing other issues and glitches, mainly in the video OSD (jumping progressbar, flickering video and audio details,..).

Improvements in this area are really very appreciated. Thanks for taking a look.

@FernetMenta
Copy link
Contributor

As a user, how would I know that I can back to the last show or even the show before that one?

@FernetMenta
Copy link
Contributor

he current GUI methods are a bit messy and are also causing other issues and glitches, mainly in the video OSD (jumping progressbar, flickering video and audio details,..)

please elaborate. I have been using those for quite some while and did not notice those issues.

@ksooo
Copy link
Member

ksooo commented Feb 7, 2016

jumping progressbar, flickering video and audio details

@FernetMenta I was referring esp. to these problems, not to the current timeshift guiinfo labels which are working fine for me (For my personal kodi build, I'm using confluence timeshift UI based on your initial approach ;-)

@Glenn-1990
Copy link
Contributor Author

@ksooo Added some more commits.

@FernetMenta
I removed the old timeshift methods as they weren't used currently and they are similar than the start and stop time of the playing tag. Also the old timeshift methods ignores the time format setting.
https://github.com/xbmc/xbmc/blob/master/xbmc/GUIInfoManager.cpp#L3783
If this is a no go, I'll revert this commit, but maybe you can test my approach to get the whole idea.

EDIT: You can just skip back to the previous show by rewinding, you can go back in time until the progress bar hits the timeshift start marker. The same thing applies when forwarding towards the live point.

PVRGUIInfo will now take care of the gui data for the current active channel, with or without timeshift/epg data. This Somehow fools the player as we do not want to get the 'real' player data in the osd, but we want to get the data relative to the current epg event and/or timeshift buffer.

@FernetMenta
Copy link
Contributor

I already mentioned this to @ksooo. Further relating playing time with epg info is dead end. With this changes info comes from 3 different sources: PVRManager (addon), EPG, g_application (asks player). As a result you need rather ugly hacks like this Glenn-1990@68f20c8

PVRGUIInfo will now take care of the gui data for the current active channel, with or without timeshift/epg data. This Somehow fools the player as we do not want to get the 'real' player data in the osd, but we want to get the data relative to the current epg event and/or timeshift buffer.

This assumption is wrong. We definitely don't want to do this.

@FernetMenta
Copy link
Contributor

@Glenn-1990 in order to avoid possible disappointment if your changes get rejected I suggest that you elaborate on what you are trying to achieve in advance. This PR creates quite a lot circular dependencies that won't get merged.

@Glenn-1990
Copy link
Contributor Author

@FernetMenta Yes we are using data from 3 different places, but this was already the case, so you suggest to pass everything to the player and let guiinfo fetch it from here, also timeshift info?

@Glenn-1990
Copy link
Contributor Author

I'll push my small fixes in a separate PR to get started :p

@FernetMenta
Copy link
Contributor

Yes we are using data from 3 different places, but this was already the case

I know and I had a similar discussion with @ksooo just a couple of days ago. We need some changes in this area before we start to add new behaviour which makes redesign harder.

so you suggest to pass everything to the player and let guiinfo fetch it from here, also timeshift info

player accesses PVR through InputStreamPVRManager. I think we can simplify a lot if we convey all information through this path. Then we can keep the state of the various parameters consistent.
The EPG container is good for EPG grid and what's playing next. But info about the current playing item needs to com from the addon. End times can change and epg container may not be aware of this.

It also simplifies work for the skinners. They don't need to create different items for normal FileItems and PVR.

@Hedda
Copy link
Contributor

Hedda commented Feb 8, 2016

@Glenn-1990 FYI; there is a long dicussion thread in the forum with ideas for timeshift buffer indicator, see:

http://forum.kodi.tv/showthread.php?tid=226287

@Glenn-1990
Copy link
Contributor Author

@FernetMenta, Yes passing everything through InputStreamPVRManager seems the best idea. Only why do you want to get start and stop times directly from the addon? In this way, the info on the osd might be different than the (outdated) info from the epg grid and that might confuse people. At least with tvheadend and pvr api 5.0 the epg grid should be up to date at every moment.

@FernetMenta
Copy link
Contributor

Pulling information from different sources is error prone. In case of start/end time it never worked reliable. Further the introduced constraints make no sense. The resulting workarounds even make less sense. Why should a tv show without an epg record in the epg container have to start/end times displayed? You and ksoo worked around this issue by saying ok "either epg data or timeshift". Lets wait some time longer and the next case/conditions will be introduced.
Fact is that a tv show does have start/end times even no epg is available. epg start/end times get corrected by some algo because it does not allow overlapping. Another thing that is difficult to get in line with actual data.

At least with tvheadend and pvr api 5.0 the epg grid should be up to date at every moment.

if this is the case your first sentence is wrong

In this way, the info on the osd might be different than the (outdated) info from the epg grid and that might confuse people

@Hedda Hedda mentioned this pull request Feb 9, 2016
@razzeee
Copy link
Member

razzeee commented Apr 24, 2016

I'll close this for now, as confluence is not the default skin anymore. So feel free to redo it for estuary, if wanted.

@razzeee razzeee closed this Apr 24, 2016
@razzeee razzeee removed Type: Improvement non-breaking change which improves existing functionality v17 Krypton Component: PVR labels Apr 24, 2016
@NedScott
Copy link
Contributor

Does @HitcherUK have a repo for v17 Confluence?

@MartijnKaijser
Copy link
Member

@NedScott https://github.com/xbmc/skin.confluence

@Hedda
Copy link
Contributor

Hedda commented Apr 26, 2016

@razzeee Doesn't the patch in this PR go deeper than Confluence skin mods?

Maybe reo-pen and replace the Confluence specific changes with Estuary?

@razzeee
Copy link
Member

razzeee commented Apr 26, 2016

We can always reopen if somebody actually has the work done.

@Glenn-1990
Copy link
Contributor Author

As @FernetMenta pointed out, PVRGUIInfo/GUIInfo should be reworked first to get it inline with normal video playback, but I've no space time for that at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants