Load channel settings properly when <cacheindvdplayer> is disabled #4333

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
7 participants
@Jalle19
Member

Jalle19 commented Mar 5, 2014

There is currently a bug in XBMC which causes channel settings to not be loaded at all when pvr.cacheindvdplayer is disabled. The setting is used to dramatically speed up the time it takes to switch channels on backends with an internal demuxer. The problem is that the call to load the settings is done in a block that is never reached when the setting is disabled.

Because standard files and PVR channels work quite differently I've opted to contain loading/saving of channel settings in PVRManager. Currently they're loaded in Open/CloseLiveStream() and PerformChannelSwitch(). They were previously loaded in DVDPlayer and sometimes stored in Application in addition to whenever the current audio/video settings were changed.

I've also changed the Save/Load methods to take the channel in question as a parameter, this way it is easier to put the call in the proper location. It also alleviates the need to defer to PVRClients to handle the logic.

@opdenkamp @FernetMenta would be nice if you could chip in
@margro can you check for regressions with some backend other than tvheadend/vdr?

@Jalle19

This comment has been minimized.

Show comment
Hide comment
@Jalle19

Jalle19 Mar 11, 2014

Member

@FernetMenta I removed the last commit (the one you commented on, naturally Github lost it...). Did you find the time to look at the rest of it? It would be nice to see this make it into Gotham, without this pvr.cacheindvdplayer is all but useless.

Member

Jalle19 commented Mar 11, 2014

@FernetMenta I removed the last commit (the one you commented on, naturally Github lost it...). Did you find the time to look at the rest of it? It would be nice to see this make it into Gotham, without this pvr.cacheindvdplayer is all but useless.

@xhaggi xhaggi added the PVR label Mar 12, 2014

@FernetMenta

This comment has been minimized.

Show comment
Hide comment
@FernetMenta

FernetMenta Mar 12, 2014

Member

I remember lots of trouble with those functions until we got it work for almost all scenarios. I assess the risk that something may break as relatively high. IMO we should rather prevent pvr.cacheindvdplayer from being disabled because it forces the system into an unstable state in which it is not supposed to run. When an audio stream is started AE fills the various buffers which in sum hold up to approximately 0.8 seconds of audio, If you reduce pvr caching from the default values those buffers never fill. It may work somehow but this is not the stable state.

Apart from this I agree that load/save of channel settings needs rework. G+1

Member

FernetMenta commented Mar 12, 2014

I remember lots of trouble with those functions until we got it work for almost all scenarios. I assess the risk that something may break as relatively high. IMO we should rather prevent pvr.cacheindvdplayer from being disabled because it forces the system into an unstable state in which it is not supposed to run. When an audio stream is started AE fills the various buffers which in sum hold up to approximately 0.8 seconds of audio, If you reduce pvr caching from the default values those buffers never fill. It may work somehow but this is not the stable state.

Apart from this I agree that load/save of channel settings needs rework. G+1

@Jalle19

This comment has been minimized.

Show comment
Hide comment
@Jalle19

Jalle19 Mar 12, 2014

Member

The only trouble has been that loading the settings has been done in the most non-obvious place available (HandlePlaySpeed). With these changes it is much more predictable.

I'd rather not see the advanced setting removed, it speeds up zapping considerably.

Member

Jalle19 commented Mar 12, 2014

The only trouble has been that loading the settings has been done in the most non-obvious place available (HandlePlaySpeed). With these changes it is much more predictable.

I'd rather not see the advanced setting removed, it speeds up zapping considerably.

@FernetMenta

This comment has been minimized.

Show comment
Hide comment
@FernetMenta

FernetMenta Mar 12, 2014

Member

We have told you multiple times that your attempts to speed up channel switching sacrifice behavior we have implemented for good reasons. Fast channel changing is worth nothing if audio stalls some time later. Your changes my work somehow on you system but for sure won't work on every system. Removing the caching is no option.

Member

FernetMenta commented Mar 12, 2014

We have told you multiple times that your attempts to speed up channel switching sacrifice behavior we have implemented for good reasons. Fast channel changing is worth nothing if audio stalls some time later. Your changes my work somehow on you system but for sure won't work on every system. Removing the caching is no option.

@Jalle19

This comment has been minimized.

Show comment
Hide comment
@Jalle19

Jalle19 Mar 12, 2014

Member

These changes don't remove the caching option (I closed that PR yesterday precisely because like you say it may have unintended consequences). I'm trying to make an optional (advanced) setting which disables it actually work, that's all.

Member

Jalle19 commented Mar 12, 2014

These changes don't remove the caching option (I closed that PR yesterday precisely because like you say it may have unintended consequences). I'm trying to make an optional (advanced) setting which disables it actually work, that's all.

@Jalle19

This comment has been minimized.

Show comment
Hide comment
@Jalle19

Jalle19 Mar 12, 2014

Member

Anyway, if these changes don't cut it, do you have any suggestions on alternative ways of doing it?

Member

Jalle19 commented Mar 12, 2014

Anyway, if these changes don't cut it, do you have any suggestions on alternative ways of doing it?

@FernetMenta

This comment has been minimized.

Show comment
Hide comment
@FernetMenta

FernetMenta Mar 12, 2014

Member

I did not say the changes in this pr are wrong. They are reasonable and should be discussed G+1 because the risk that something breaks is high.

Member

FernetMenta commented Mar 12, 2014

I did not say the changes in this pr are wrong. They are reasonable and should be discussed G+1 because the risk that something breaks is high.

@Jalle19

This comment has been minimized.

Show comment
Hide comment
@Jalle19

Jalle19 Mar 12, 2014

Member

Okay, I can live with that. If there were a less intrusive way to make this work correctly I'd go for that, but it seems it's not possible.

Member

Jalle19 commented Mar 12, 2014

Okay, I can live with that. If there were a less intrusive way to make this work correctly I'd go for that, but it seems it's not possible.

@t-nelson t-nelson added the Helix label Apr 5, 2014

@Jalle19

This comment has been minimized.

Show comment
Hide comment
@Jalle19

Jalle19 May 7, 2014

Member

@opdenkamp any comment on this?

Member

Jalle19 commented May 7, 2014

@opdenkamp any comment on this?

@piotrasd

This comment has been minimized.

Show comment
Hide comment
@piotrasd

piotrasd May 12, 2014

could you rebase ? dont work with actual git master

could you rebase ? dont work with actual git master

@opdenkamp

This comment has been minimized.

Show comment
Hide comment
@opdenkamp

opdenkamp May 12, 2014

Member

been away all week end and haven't had time to look at this yet

@piotrasd please rebase yourself if you like to test this and don't post about it on github. hundreds of people get an email about this.

Member

opdenkamp commented May 12, 2014

been away all week end and haven't had time to look at this yet

@piotrasd please rebase yourself if you like to test this and don't post about it on github. hundreds of people get an email about this.

@Jalle19

This comment has been minimized.

Show comment
Hide comment
@Jalle19

Jalle19 May 15, 2014

Member

@piotrasd it's rebased now, I'm in the process of build testing it but at a glance it seems fine.

Member

Jalle19 commented May 15, 2014

@piotrasd it's rebased now, I'm in the process of build testing it but at a glance it seems fine.

@Jalle19

This comment has been minimized.

Show comment
Hide comment
@Jalle19

Jalle19 May 15, 2014

Member

Rebased again (didn't have the latest local copy), don't know why the merge button is still gray though.

Member

Jalle19 commented May 15, 2014

Rebased again (didn't have the latest local copy), don't know why the merge button is still gray though.

@t-nelson

This comment has been minimized.

Show comment
Hide comment
@t-nelson

t-nelson May 15, 2014

Contributor

Looks like you forgot to push or rebased against the wrong tree.

Contributor

t-nelson commented May 15, 2014

Looks like you forgot to push or rebased against the wrong tree.

@opdenkamp

This comment has been minimized.

Show comment
Hide comment
@opdenkamp

opdenkamp May 15, 2014

Member

@FernetMenta or me needs to review this properly and +1 it before this is allowed to go in

Member

opdenkamp commented May 15, 2014

@FernetMenta or me needs to review this properly and +1 it before this is allowed to go in

@Jalle19

This comment has been minimized.

Show comment
Hide comment
@Jalle19

Jalle19 May 15, 2014

Member

@t-nelson should be fixed now, I rebased the wrong local branch and pushed that. No wonder the light didn't turn green.

@opdenkamp of course

Member

Jalle19 commented May 15, 2014

@t-nelson should be fixed now, I rebased the wrong local branch and pushed that. No wonder the light didn't turn green.

@opdenkamp of course

@piotrasd

This comment has been minimized.

Show comment
Hide comment
@piotrasd

piotrasd May 15, 2014

Yeep, works great !!! thanks.

Yeep, works great !!! thanks.

@@ -5761,10 +5758,6 @@ void CApplication::SaveCurrentFileSettings()
dbs.Close();
}
}
- else if (m_itemCurrentFile->IsPVRChannel())

This comment has been minimized.

@FernetMenta

FernetMenta May 15, 2014

Member

this will fail if you have live tv playing and open some other file. then it will load the video settings of the file before you saved the channels settings.
those errors can be avoided if you move load/save channel settings to the same locations where load/save video settings is done.

@FernetMenta

FernetMenta May 15, 2014

Member

this will fail if you have live tv playing and open some other file. then it will load the video settings of the file before you saved the channels settings.
those errors can be avoided if you move load/save channel settings to the same locations where load/save video settings is done.

This comment has been minimized.

@Jalle19

Jalle19 May 15, 2014

Member

The settings are now saved in CloseLiveStream(), shouldn't that cover this scenario?

@Jalle19

Jalle19 May 15, 2014

Member

The settings are now saved in CloseLiveStream(), shouldn't that cover this scenario?

This comment has been minimized.

@FernetMenta

FernetMenta May 16, 2014

Member

A new file can be opened while the old one still plays. This way the old one can keep playing if open of the new file fails. There is a reason why video settings are saved here.

@FernetMenta

FernetMenta May 16, 2014

Member

A new file can be opened while the old one still plays. This way the old one can keep playing if open of the new file fails. There is a reason why video settings are saved here.

This comment has been minimized.

@Jalle19

Jalle19 May 16, 2014

Member

If you watch a channel and open a file that can't be played, settings won't be saved and the old channel will continue playing. When you finally switch to something that works, CloseLiveStream() will be called and the settings will be saved. I don't see the problem, or am I still missing something?

@Jalle19

Jalle19 May 16, 2014

Member

If you watch a channel and open a file that can't be played, settings won't be saved and the old channel will continue playing. When you finally switch to something that works, CloseLiveStream() will be called and the settings will be saved. I don't see the problem, or am I still missing something?

This comment has been minimized.

@FernetMenta

FernetMenta May 16, 2014

Member

If you open a file while pvr is playing, it loads the settings of the file before the channels settings are saved. Then you save the settings of the file to the channel.

@FernetMenta

FernetMenta May 16, 2014

Member

If you open a file while pvr is playing, it loads the settings of the file before the channels settings are saved. Then you save the settings of the file to the channel.

This comment has been minimized.

@Jalle19

Jalle19 May 16, 2014

Member

Okay, now I get it. How would you like to see this done? Modify SaveFileStateJob to handle PVR channels too and then add the needed extra calls where necessary (like PerformChannelSwitch)?

@Jalle19

Jalle19 May 16, 2014

Member

Okay, now I get it. How would you like to see this done? Modify SaveFileStateJob to handle PVR channels too and then add the needed extra calls where necessary (like PerformChannelSwitch)?

This comment has been minimized.

@FernetMenta

FernetMenta May 16, 2014

Member

I would put load/save of video settings and channel settings into the same place. Ideally there won't be any extra treatment for pvr but I don't know if this feasible without much refactoring. If there is an extra load required for PerformChannelSwitch should be ok for this iteration.

@FernetMenta

FernetMenta May 16, 2014

Member

I would put load/save of video settings and channel settings into the same place. Ideally there won't be any extra treatment for pvr but I don't know if this feasible without much refactoring. If there is an extra load required for PerformChannelSwitch should be ok for this iteration.

@MartijnKaijser MartijnKaijser added this to the Pending for inclusion milestone May 21, 2014

@Jalle19

This comment has been minimized.

Show comment
Hide comment
@Jalle19

Jalle19 May 24, 2014

Member

Replaced by #4783

Member

Jalle19 commented May 24, 2014

Replaced by #4783

@Jalle19 Jalle19 closed this May 24, 2014

@MartijnKaijser MartijnKaijser modified the milestones: Abandoned, obsolete or superseeded, Pending for inclusion May 24, 2014

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