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] Fix epg tag <-> timer tag association handling. #10024

Merged
merged 4 commits into from Jun 24, 2016

Conversation

@ksooo
Copy link
Member

commented Jun 23, 2016

Under certain circumstances, epg tags did not have the right timer tag associated. Visible effect was for example in EPG grid window, context menu for events to record showing "Record" and "Add timer" instead of "Delete Timer" or vica versa.

Second commit is just non-funtional cleanup. Ctors now use initializer list instead of assignment in ctor body, where possible.

@xhaggi mind taking a look.

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Jun 23, 2016

I noticed that sometimes epg grid items are not repainted correctly. For instance, after selecting "Delete timer" from event's context menu the little clock icon indicating that the event shall be recorded sometimes does not disapear. One needs to move the event in question out of the view area of the grid to get it painted correctly. This seems to be a caching problem at gui level, not a pvr core issue, as the correct timer tags should always be assigned to / removed from the respective epg tags immediately with this PR. 3rd commit should fix this.

@ksooo ksooo force-pushed the ksooo:pvr-fix-epgtag-timer-assoc branch from e8a1bd1 to 376baa8 Jun 24, 2016
@ksooo

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2016

jenkins build this please

@ksooo ksooo merged commit d9fd774 into xbmc:master Jun 24, 2016
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins.kodi.tv You did a great job. Have a cookie.
Details
@ksooo ksooo deleted the ksooo:pvr-fix-epgtag-timer-assoc branch Jun 24, 2016
@bas-t

This comment has been minimized.

Copy link

commented Jun 25, 2016

"3rd commit should fix this."
But it does not. I've been building this in Ubuntu Xenial and Debian Stretch. No improvement whatsoever.
However, the fix presented by @dnairb in http://trac.kodi.tv/ticket/16621 does fix it for me.

Since this dnairb guy does not respond to my question regarding a PR, I ask you: should I make a PR from his patch?

@bas-t

This comment has been minimized.

Copy link

commented Jun 25, 2016

Hmm, that link to the ticket does not work.
Should be:
http://trac.kodi.tv/ticket/16621

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Jun 25, 2016

"3rd commit should fix this." But it does not.

For me, it does. I spent three nights debugging this, most of the time went into understanding how things are supposed to work. I'm convinced that I now know this part of kodi code very well and that I found and fixed at least one bug that causes exactly the problem described in the trac issue. Without this PR, with just the skin change #10031 applied, things cannot (!) reliably work. If the skin change fixes something for you - fine, but this is by coincidence, then, not because this fixes the root cause.

@bas-t

This comment has been minimized.

Copy link

commented Jun 25, 2016

Ok, I get that.
This might just be a workaround, but one that works!

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Jun 25, 2016

OMG! I should have had some more sleep last nights. ;-) My comment re understanding things applies to commit 1, not really to commit 3. Commit 3 is more educated guess and trial than real understanding of the root cause. Nevertheless, this is something that must be fixed in kodi code, not in any skin.

@bas-t

This comment has been minimized.

Copy link

commented Jun 25, 2016

Ok, I guess you're right about having to fix this in kodi code.
I hope that you sleep very well the next few nights and that the approach of this dnairb guy leads you to the real solution.

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Jun 25, 2016

@bas-t could you do me a favor and test whether https://github.com/ksooo/xbmc/tree/pvr-fix-epg-grid-timer-icons fixes things for you? It's just a test.

@bas-t

This comment has been minimized.

Copy link

commented Jun 25, 2016

Will do.

@bas-t

This comment has been minimized.

Copy link

commented Jun 25, 2016

I'm sorry, no improvement whatsoever.

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Jun 25, 2016

Thanks anyway. This is so strange. Just did a check on LibreELEC latest master -it is working just fine, even without the test patch.

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Jun 25, 2016

Short question: do you see the little notification dialog "timer added" after scheduling a new recording? This is evidence for the confirmation from the pvr backend.

@bas-t

This comment has been minimized.

Copy link

commented Jun 25, 2016

No, I don't.
But I did disable notifications of timer updates.
I think of them as utterly annoying.

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Jun 25, 2016

Could you please tempoarily enable the notifications and answer my question, then?

@bas-t

This comment has been minimized.

Copy link

commented Jun 25, 2016

Yes, but that'll be tomorrow, or the day after that, I'm going to get some sleep now, as you should.

@bas-t

This comment has been minimized.

Copy link

commented Jun 26, 2016

@ksooo same problem with the nightly.

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2016

Sorry, then I'm out of ideas.

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2016

Is your xenial installation somehow special compared to the standard installation and configuration (I'm using)? What graphic card are you using?

@bas-t

This comment has been minimized.

Copy link

commented Jun 26, 2016

No special things in my machine, I'm using a simple nvidia hdmi card with the nvidia-340 driver

@bas-t

This comment has been minimized.

Copy link

commented Jun 26, 2016

And MATE desktop

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2016

What happens if you use unity?

@bas-t

This comment has been minimized.

Copy link

commented Jun 26, 2016

Still no timer icons auto-update.
To make sure I did a fresh install and use the nightly kodi build.

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2016

@bas-t: I'm clueless. There must be something very special about your system. One last chance: Can you enable debug in system settings and send me a kodi.log, please?

@Glenn-1990: could you do me a favor and test whether you still experience any timer icon problems withlatest kodi master?

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2016

nvidia-340

you mean you are using driver version 340.x.y? This is f****g old, if you ask me. Go and get yourself a more recent graphics card, then. Not 100% sure but there are chances that the driver is causing the problems. ;-)

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2016

what card do you exactly have?

could you try LibreELEC from USB stick on the machine in question and report back. Latest LE should have my PRs for timer icon fixes already => https://libreelec.tv/download-temp/alpha-preview/

@bas-t

This comment has been minimized.

Copy link

commented Jun 26, 2016

I've got a NVIDIA Geforce 210 (Asus with hdmi) in this workstation, but my usual frontend has NVIDIA ION2 (uses the same nvidia driver). Both suffer from the timer icon issue in exactly the same way.

I've tried LibreELEC from USB stick, sadly with the same result.

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2016

Most probably not a kodi problem. Go and get yourself a more recent graphics card. Thanks for testing, but I guess there is nothing I can do for you to fix this problem.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Jun 26, 2016

It is more than unlikely that the gfx card is root cause of this issue.

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2016

@FernetMenta I'm out of ideas and really not an expert on this, but @fritsch stated that he can imagine that this issue is gfx card driver related. If you can assist here, you are more than welcome.

@fritsch

This comment has been minimized.

Copy link
Member

commented Jun 26, 2016

In fact we had numerous issues on compositing managers and also with various nvidia drivers. The 100% cpu load, when not forcing gl_yield via usleep ... though I did not expect that there also will be an issue with LE. I'd like to see an LE screenshot and a complete debuglog if possible.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Jun 26, 2016

I don't see a single logical relationship between a missing timer icon and a gfx card. Building a hypothesis around this is like voodoo. Depending on the refresh rate, most likely 50Hz or 60Hz, we draw the UI 50 or 60 times a second. Do you think the gfx driver eats the same texture on every cycle? I have never heard of anything similar. Much more likely is that the timer icon is not rendered.

@fritsch

This comment has been minimized.

Copy link
Member

commented Jun 26, 2016

I don't see a single logical relationship between a missing timer icon and a gfx card

One logical relationship: Without the GPU there would not be any output on any screen.

Another possibility: http://loadion.com/ii/218292240_8f853b189b.jpg <- see on the right, the desktop is missing :-)

I still like to see a debuglog that shows adding the timer and so on.

@stefansaraev

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2016

@fritsch as I said today, no way it can be a driver bug. or we are doomed.

prior to this PR, the bug was easy to reproduce here:

  1. go to tv -> guide
  2. click "record" on a random item. you get a notification that timer is added, but no change in the grid, icon not shown
  3. go "clear data" in settings -> tv -> epg
  4. go back to epg grid, you'll see how the timer icon appears after ~ 1 second

this PR fixes the bug for me.

@bas-t please tripple check if this PR is included in the build you are runing

@fritsch

This comment has been minimized.

Copy link
Member

commented Jun 26, 2016

As this github repo is used as (single) user support forum - a full debuglog that shows the complete sequence is mandatory - not optional. For now - we don't even know what he is running and as long as we don't - speculation continues.

@stefansaraev

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2016

latest LE alpha does NOT contain this fix (does it? EDIT: hell, no, it was released one week before this PR was submitted). all I ask for is: tripple check if you compile from kodi git-master (with this PR in), and run exactly what you compiled, no custom skins in ~/.kodi are allowed!

@bas-t

This comment has been minimized.

Copy link

commented Jun 26, 2016

@stefansaraev I'm absolutely sure that this PR is included in what I run and no custom skins are used.
In how to reproduce nr 3 and 4 can be replaced by anything that moves the not showing timer-icon out of sight, like pressing right a couple of times, then pressing left a couple of times. And voila: the timer-icon shows.

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Jun 26, 2016

Please move this discussion to forum.

@stefansaraev

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2016

ok. I can reproduce.

if using context menu -> record. timer is shown immediately. and disappears immediately if using context menu -> delete timer
if using select (ok) button on an entry to open the dialog that contains find similar/switch/record, after hitting "record" the icon is not rendered. but kodi already knows the timer is there as it allows me to remove it

EDIT: this is likely to be different issue, not related to the fix in this PR, as before this PR, kodi did not even know there was a timer while staying in epg grid, neither context menu, nor the dialog buttons were updated before.

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2016

I have a fix for the not (immediately) appearing icon when recording is scheduled from the info dialog opened from inside guide window. Will come up with a PR.

@bast-t: please tell me exactly how (ctx menu, info dialog button, ...) you add a timer that afterwards has no icon in the guide window. And please tell me what timer type the problematic timers have (One time, One time (guide-based), ... There seem to be some problems left with non-guide-based timers.

@bas-t

This comment has been minimized.

Copy link

commented Jun 27, 2016

I've set the default action to perform on select (ok) button on an entry to 'record'.
So I mostly use that. I guess that'll be One time (guide-based), not sure.
Sometimes I use the info dialog button, with the same result.
I can confirm that the context menu behaves different, but not reliable. Sometimes it shows the icon, sometimes it does not and sometimes it only shows the icon when I move focus to another item.

All of this is fixed for me by PR #10031

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2016

All of this is fixed for me by PR #10031

we need to find the root cause which definitely is not a buggy skin xml file

@bas-t

This comment has been minimized.

Copy link

commented Jun 27, 2016

Of course you do need to find the root cause, but this buggy skin xml file might just point you in the right direction.

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2016

@bas-t could you please try this patch: https://github.com/ksooo/xbmc/commits/pvr-fix-timer-icon-issues

btw: this time it is far more than educated guess or voodoo. If I'm right this was actually a very stupid bug that was in the code for ages. Things worked only by luck. And yeah it has nothing to do with your gfx card or gfx card driver.

@bas-t

This comment has been minimized.

Copy link

commented Jun 27, 2016

@ksooo That did the trick indeed!
I'd say it's fixed. When you merge this, you may want to mark the trac ticket as fixed and close it.
Thanks.

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