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

Fix waiting for window close animation blocking JSON requests after SetFullScreen #15593

Merged
merged 1 commit into from Mar 21, 2019

Conversation

Projects
None yet
5 participants
@DaveTBlake
Copy link
Member

commented Feb 23, 2019

To see the issue:

  • Run Kodi in a window (not full screen), and play something, music or video it does not matter.
  • During playback minimise Kodi onto task bar so still active but no longer displayed.
  • Send JSON request to show Kodi full screen
{"id":1,"jsonrpc":"2.0","method":"GUI.SetFullscreen","params":{"fullscreen":true}}

But this does not get a reply, and afterwards other JSON requests either timeout or no longer work at all. For example of a request that times out

 {"id":1,"jsonrpc":"2.0","method":"Player.GetActivePlayers"}

The cause is CGUIWindow::Close_Internal is waiting for close animation to finish before closing previous windows but since Kodi is minimised there's no rendering the so the close animation never finishes. Obviously there is no point in waiting for close animation when there is no GUI rendering, and so the close can be instant.

Tested on Win64 and should be correct for other platforms where Kodi can be minimised

@fritsch

This comment has been minimized.

Copy link
Member

commented Feb 23, 2019

This is wrong. Same reason as yesterday. Some note for the future: Whenever you need to access g_application from somewhere else that can easily live without that, to "fix" something, chances are 99.9% that you don't fix anything but workaround a single use-cause by forcing behaviour into an implementation and degrading the reusability of this object,

So, please stop adding globals into common code. If you see the GuiWindow there is two places where g_application is used, which is needed (still) to not run into serious trouble - design. There is no reason to explicitely depend on that again, Especially for you relevant is the Close-Function, why do you think it is distinguished between "running from thread1" vs. "not running from thread1"?

In your case even worse, you try to workaround something that you think is generic ... but in fact it's a platform problem.

On Ubuntu, start kodi, play some music, navigate to main menu. Now minimize kodi.

Now set music to fullscreen:

curl -g --data-binary '{"id":1,"jsonrpc":"2.0","method":"GUI.SetFullscreen","params":{"fullscreen":true}}' --header 'content-type: application/json;' http://localhost:8080/jsonrpc

Now set the volume to 50%

curl -g --data-binary '{ "jsonrpc": "2.0", "method": "Application.SetVolume", "id": 1, "params": { "volume": 50} }' --header 'content-type: application/json;' http://localhost:8080/jsonrpc

Music dims, means it's working properly.

Now restore kodi, see that music is in fullscreen window and all other json commands are still working.

So second point: No, don't add stuff in generic code to workaround platform specific stuff, please.

@Memphiz

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

I just tried this on Mac OS to come up with a better solution but the issue just doesn't happen here. When minimized we set appPort->SetRenderGUI(false).

I then forced a close of a window via remote app but it simply seems to close - meaning there is no endless waiting in close_internal like what you seem to see.

There must be something special in what windows does when rendergui is false.

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Mar 1, 2019

@fritsch while it is clear that something is wrong somewhere in the existing code and probably design (we are getting unwanted behaviour an obvious sign of "wrongness"), since g_application is already used by CGUIWindow it did not cross my mind for a moment that there was anything controversial about using it in the fix.

If we are to stop using globals all together in the new architecture then
a) what is the functional decomposition of this architecture?
b) how are functionally separate areas/levels going to communicate?
Without documentation of some kind all anyone has to go on is the existing code. Seeing g_application being used encouraged its use in the fix.

Anyway you have made your opposition clear and the preference for some other solution, but not how that could be achieved (even if you say "easily live without" g_application).

Next is the issue of genericity and platform dependence. I don't think this can be assumed to be only a Windows issue from the testing that you have done. Here is why

  • Not all JSON commands exhibit the problem, and neither of you report attempts with the example I gave. Some hang, some fail, but some still work!
  • It may depend upon what is on the screen when the commands are sent (not everything has close animation).
  • It may depend upon if Kodi window is visible (and thus rendered) or hidden behind other windows.
  • What happens on the other platforms e.g. Android

I don't have access to Unbuntu, and I don't know if it disables rendering when minimized, maybe it is immune. As for Mac I asked a friend to try and they repeated the issue. Not the same as seeing it myself or another team member, (and I don't have the details etc.) but certainly worthy of further pursuit.

@Memphiz thanks for trying to come up with a better solution, perhaps try some more to at least repeat the problem.

I think waiting for a close animation to finish when rendering isn't actually happening is an obvious mistake. How can that be avoided within the preferred new architecture? I don't know.

@Memphiz

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

The solution against globals is dependency injection and I tried exactly with your example.

I played a movie - then I hit “back” to have the main menu in front of it. Then I minimised Kodi. Then I issued the Json which sets full screen playback. Hence I should get the close animation of the main menu.

There is no more info from your side which Dialog exactly you closed - just that you played something. So before I try “some more” it would be good to have a somewhat more exact reproduction description if possible.

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2019

After more testing I have discovered that waiting for close animation when Kodi has been minimised and return to full screen requsted by JSON also blocks flow of playback to the next item.

@Memphiz a more detailed reproduction description, exactly as I did it, that shows playback delay. It requires that you have at least one album scanned into the music library.

  • Run Kodi in a window (not full screen)
  • Click on Music on the side menu
  • Click on Albums node on the music menu
  • Press <p> on a selected album or "Play" on the context menu. This adds all the songs from the album to the current playlist and playback of the first song starts.
  • While the library screen is still on display (before the display times out to OSD) Library Screen click to minimise Kodi down onto the task bar so Kodi window no longer visible.
  • Send JSON request to show Kodi full screen
{"id":1,"jsonrpc":"2.0","method":"GUI.SetFullscreen","params":{"fullscreen":true}}

I use Postmaster app to send this, but CURL should behave the same.

  • Wait to see JSON request response. In Postmaster this only returns if I click on the Kodi icon on the task bar and switch to Kodi, don't do that just wait. It does not get a reply.
  • Continue to wait until the playing song completes. Playback of the next song on the album (and in the current playlist) does not start.
  • Some subsequent JSON requests, even made before playback of the first song completes, also hang or timeout. Specifically I tried Player.GetActivePlayers from a different Postmater tab and that hangs, but Application.SetVolume as tried by @fritsch does work. I have also tried making JSON requests from a different app, they also hang.

Finally click on the Kodi icon on the task bar and switch to Kodi manually, playback of the new song starts and the JSON requests complete.

I this is not the only sequence that demonstrates the issue, but what is on screen when it is minimised does have an effect. In particular the OSD does not show the issue. Anyway I hope that the detail helps others to reproduce.

I really don't see how this can be a Windows platform only issue. Moreover having

while (window->IsAnimating(ANIM_TYPE_WINDOW_CLOSE))
ProcessRenderLoop(true);
on the main thread does not seem a robust design to me.

@Memphiz

This comment has been minimized.

Copy link
Member

commented Mar 2, 2019

Thx for the detailed description - I will see wen I get to it.

That method you marked is called “CloseWindowSync”. That suggests it is a blocking method. In those situations the GUI thread would stall if nobody would process the message loop. I am confident that similar code can be found in the windows os modal dialogs itself.

@Memphiz

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

Could reproduce it now. Will try to come up with a fix.

@Memphiz

This comment has been minimized.

Copy link
Member

commented Mar 9, 2019

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Mar 9, 2019

Thanks @Memphiz for confirming that this issue also happens on OSX as I expected. It is worth seeing if it happens on any other platforms?

Thanks also for the alternative fix. Not sure the easiest way to bring into this PR, or do you want to raise as new? Here would be useful because of the details about reproducing the issue, but credit is all yours.

@Memphiz

This comment has been minimized.

Copy link
Member

commented Mar 9, 2019

That issue is on all platforms because it is caused by platform independent code. You can take my changes. I don‘t care about the credit and have no time to do a PR atm.
You can simply reset your branch to master and cherry pick my commits if you feel like the author should be retained.

Assuming your remote for the Kodi repo is called „origin“
git fetch origin
git checkout winminunblockJSON
git reset —hard origin/master
git remote add memphiz git://github.com/Memphiz/xbmc.git -f
git cherry-pick 7e8d0bb
git cherry-pick 874998f
git push -f

In any other case you can do whatever fits your git knowledge to get the changes into this PR ;)

@DaveTBlake DaveTBlake force-pushed the DaveTBlake:winminunblockJSON branch from 389856d to 56aec11 Mar 9, 2019

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Mar 9, 2019

Thanks @Memphiz both for the alternative fix and Git advice.

@fritsch could you confirm this approach to a solution is acceptable to you.

@fritsch

This comment has been minimized.

Copy link
Member

commented Mar 9, 2019

Fine with me, optimum of what we can do currently. Thanks much!

Show resolved Hide resolved xbmc/Application.h Outdated
GUIWindowManager - wait for the closing animation to finish when clos…
…ing a window sync only when GUI is rendered

Declare GetRenderGUI in IWIndowManagerCallback and adjust method in CApplication subclass to override, so callback can determine if render GUI is enabled

@DaveTBlake DaveTBlake force-pushed the DaveTBlake:winminunblockJSON branch from 56aec11 to 25029f8 Mar 10, 2019

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Mar 10, 2019

Changed to use GetRenderGUI, moving the declaration from application into the callback class. Is that correct @Memphiz or @afedchin ?

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Mar 10, 2019

Just a thought @Memphiz but what happens to any queued close animations, could they cause a memory leak?

@Memphiz

This comment has been minimized.

Copy link
Member

commented Mar 10, 2019

You could debug it - but i think the animation is carried out once Kodi is not minimized anymore because the application thread takes over the process loop then. Else the window wouldn‘t close/deinit at all.
All this PR does is that the close is async in case the GUI is not rendered. Close is defered until render GUI is true again.

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Mar 17, 2019

jenkins fail unrelated to this PR. This fix has been lurking waiting to merge, any objections?
@Memphiz takes the credit for an acceptable fix.

@Memphiz

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

you wanna ping @MartijnKaijser about it :)

@MartijnKaijser MartijnKaijser merged commit 26972c7 into xbmc:master Mar 21, 2019

1 check passed

default You're awesome. Have a cookie
Details

@DaveTBlake DaveTBlake deleted the DaveTBlake:winminunblockJSON branch Mar 27, 2019

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