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

Wayland windowing system #12664

Merged
merged 41 commits into from Aug 17, 2017

Conversation

Projects
None yet
10 participants
@pkerling
Member

pkerling commented Aug 12, 2017

Add native Wayland windowing implementation as result of GSoC 2017 project from https://github.com/pkerling/xbmc/

Description

This has been in the works for the last three months.

I have squashed the commits so that modifications and additions to the non-Wayland Kodi code are separate commits and that the Wayland windowing implementation itself is one commit. That implementation of course also has a history and is made up of a large number of commits (available via the linked GitHub repository) that were reviewed in feature chunks by @FernetMenta, but it would probably not be useful to keep it in mainline.

I realize that this is quite a lot of code to merge so I'm open to any suggestions that make it easier to handle.

Building with Wayland is fully integrated into the depends system and can be achieved by calling tools/depends/configure with --enable-wayland, so it should be easily integratable into Jenkins.

Motivation and Context

Linux is transitioning to the Wayland protocol instead of X11 for the long term. To offer a competitive user experience on Linux and get all the shiny new features, Kodi must support Wayland natively instead of running via Xwayland X11 emulation.

How Has This Been Tested?

Build and run test on linux64 with GL and GLES

Screenshots (if appropriate):

It really looks just like on X :-)
(except for the window decorations which are better not shown off)

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
    (This PR is mostly "new feature," but contains some commits that fix or improve core Kodi. I can list them individually if it helps.)

Checklist:

  • My code follows the Code guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the CONTRIBUTING document
  • I have added tests to cover my change
  • All new and existing tests passed
@fritsch

This comment has been minimized.

Show comment
Hide comment
@fritsch

fritsch Aug 13, 2017

Member

Nice :-) I followed every issue and every issue review - really nice work. From my pov can go in every time if it does not break anything. Perhaps - only a suggestion - it would be good to add "Wayland:" to the commits actually doing wayland features. This is especially useful when going just through a git log without seeing directly which file it changes.

But it's on your mentor to decide on this minor - only a suggestion.

Member

fritsch commented Aug 13, 2017

Nice :-) I followed every issue and every issue review - really nice work. From my pov can go in every time if it does not break anything. Perhaps - only a suggestion - it would be good to add "Wayland:" to the commits actually doing wayland features. This is especially useful when going just through a git log without seeing directly which file it changes.

But it's on your mentor to decide on this minor - only a suggestion.

@pkerling

This comment has been minimized.

Show comment
Hide comment
@pkerling

pkerling Aug 13, 2017

Member

iOS is OK now, Windows fail is unrelated

Regarding actually building the Wayland stuff, should I just copy linux64 to linux64wayland so that it can be added to jenkins?

Member

pkerling commented Aug 13, 2017

iOS is OK now, Windows fail is unrelated

Regarding actually building the Wayland stuff, should I just copy linux64 to linux64wayland so that it can be added to jenkins?

@@ -1,7 +1,5 @@
#pragma once

This comment has been minimized.

@FernetMenta

FernetMenta Aug 13, 2017

Member

git moved the deleted file. I guess this is not what you wanted to achieve

@FernetMenta

FernetMenta Aug 13, 2017

Member

git moved the deleted file. I guess this is not what you wanted to achieve

This comment has been minimized.

@pkerling

pkerling Aug 13, 2017

Member

The Keymap.h removal is a separate commit in fact. Git just detects this as move when producing the diff for the whole history. There's nothing you can do about it as far as I know.

@pkerling

pkerling Aug 13, 2017

Member

The Keymap.h removal is a separate commit in fact. Git just detects this as move when producing the diff for the whole history. There's nothing you can do about it as far as I know.

@@ -375,11 +375,7 @@ void CAdvancedSettings::Initialize()
m_enableMultimediaKeys = false;
#if defined(TARGET_DARWIN_IOS)

This comment has been minimized.

@FernetMenta

FernetMenta Aug 13, 2017

Member

are you sure you want to change behavior for ios?

@FernetMenta

FernetMenta Aug 13, 2017

Member

are you sure you want to change behavior for ios?

This comment has been minimized.

@pkerling

pkerling Aug 13, 2017

Member

This does not really change iOS behavior. The only place this is used is in DisplaySettings (see below) where the check now includes g_Windowing.CanDoWindowed(), which is false for iOS. The only difference is that you now cannot force Kodi to go windowed anyway by modifying advancedsettings.xml. As this makes zero sense on iOS in either case I don't think it has any negative impact.

@pkerling

pkerling Aug 13, 2017

Member

This does not really change iOS behavior. The only place this is used is in DisplaySettings (see below) where the check now includes g_Windowing.CanDoWindowed(), which is false for iOS. The only difference is that you now cannot force Kodi to go windowed anyway by modifying advancedsettings.xml. As this makes zero sense on iOS in either case I don't think it has any negative impact.

This comment has been minimized.

@FernetMenta
@FernetMenta
* that was put there
*/
template<typename OutputIt>
static OutputIt SplitTo(OutputIt d_first, const std::string& input, const std::string& delimiter, unsigned int iMaxStrings = 0)

This comment has been minimized.

@FernetMenta

FernetMenta Aug 13, 2017

Member

IMO we should not have those fat bodies in header files.

@FernetMenta

FernetMenta Aug 13, 2017

Member

IMO we should not have those fat bodies in header files.

This comment has been minimized.

@pkerling

pkerling Aug 13, 2017

Member

Having function bodies in headers is common C++ and for template functions there really is no alternative. Almost the whole STL is implemented like this.

@pkerling

pkerling Aug 13, 2017

Member

Having function bodies in headers is common C++ and for template functions there really is no alternative. Almost the whole STL is implemented like this.

This comment has been minimized.

@FernetMenta

FernetMenta Aug 13, 2017

Member

sure, but here we have a kind of ugly mixture. when I saw the method first, I did not see it was a template and could not find it in the body.
anyway, I have no strong feelings about this. If the team is ok with it, I am too

@FernetMenta

FernetMenta Aug 13, 2017

Member

sure, but here we have a kind of ugly mixture. when I saw the method first, I did not see it was a template and could not find it in the body.
anyway, I have no strong feelings about this. If the team is ok with it, I am too

@FernetMenta FernetMenta added this to the L 18.0-alpha1 milestone Aug 13, 2017

@FernetMenta

This comment has been minimized.

Show comment
Hide comment
@FernetMenta

FernetMenta Aug 13, 2017

Member

Very good! Some minors then I think this is ready to go in.

Member

FernetMenta commented Aug 13, 2017

Very good! Some minors then I think this is ready to go in.

@pkerling

This comment has been minimized.

Show comment
Hide comment
@pkerling

pkerling Aug 14, 2017

Member

OK I think that was all of the minors

Member

pkerling commented Aug 14, 2017

OK I think that was all of the minors

cd $(PLATFORM); $(ARCHIVE_TOOL) $(ARCHIVE_TOOL_FLAGS) $(TARBALLS_LOCATION)/$(ARCHIVE)
.installed-$(PLATFORM): $(PLATFORM)
cd $(PLATFORM); $(PREFIX)/bin/python setup.py install --prefix=$(PREFIX)

This comment has been minimized.

@Rechi

Rechi Aug 14, 2017

Member

does scons not have a build step?

@Rechi

Rechi Aug 14, 2017

Member

does scons not have a build step?

This comment has been minimized.

@pkerling

pkerling Aug 14, 2017

Member

It's python setuptools, so it's debatable. I think you can call setup.py build, but not sure how useful that would be. tbh I just c&p-ed from https://github.com/xbmc/xbmc/blob/master/tools/depends/native/distribute-native/Makefile

@pkerling

pkerling Aug 14, 2017

Member

It's python setuptools, so it's debatable. I think you can call setup.py build, but not sure how useful that would be. tbh I just c&p-ed from https://github.com/xbmc/xbmc/blob/master/tools/depends/native/distribute-native/Makefile

$(TARBALLS_LOCATION)/$(ARCHIVE):
cd $(TARBALLS_LOCATION); $(RETRIEVE_TOOL) $(RETRIEVE_TOOL_FLAGS) $(BASE_URL)/$(ARCHIVE)
$(PLATFORM): $(TARBALLS_LOCATION)/$(ARCHIVE) $(DEPS)

This comment has been minimized.

This comment has been minimized.

@pkerling

pkerling Aug 14, 2017

Member

added

@pkerling
$(TARBALLS_LOCATION)/$(ARCHIVE):
cd $(TARBALLS_LOCATION); $(RETRIEVE_TOOL) $(RETRIEVE_TOOL_FLAGS) $(BASE_URL)/$(ARCHIVE)
$(PLATFORM): $(TARBALLS_LOCATION)/$(ARCHIVE) $(DEPS)

This comment has been minimized.

This comment has been minimized.

@pkerling

pkerling Aug 14, 2017

Member

added

@pkerling
[AS_HELP_STRING([--enable-wayland],
[build libraries needed for Wayland. default is no])],
[enable_wayland=$enableval],
[enable_wayland=no])

This comment has been minimized.

@Rechi

Rechi Aug 14, 2017

Member

IMO there should be a check if wayland can be used for the target platform.
OSX, ios, and android can't use it

@Rechi

Rechi Aug 14, 2017

Member

IMO there should be a check if wayland can be used for the target platform.
OSX, ios, and android can't use it

This comment has been minimized.

@pkerling

pkerling Aug 14, 2017

Member

added

@pkerling
@pkerling

This comment has been minimized.

Show comment
Hide comment
@pkerling

pkerling Aug 15, 2017

Member

Just give me a ping if this is ready for squashing

Member

pkerling commented Aug 15, 2017

Just give me a ping if this is ready for squashing

@Rechi

This comment has been minimized.

Show comment
Hide comment
@Rechi

Rechi Aug 16, 2017

Member

please switch to the new style of component logging which was introduced at #12676

Member

Rechi commented Aug 16, 2017

please switch to the new style of component logging which was introduced at #12676

pkerling added some commits Jun 8, 2017

Remove unused Keymap.h
leftover from Wayland cleanup
Remove WinSystemEGL from all platforms but IMX
This prevents having WinSystemEGL break other platforms because the
if() condition was not updated.
Add log category for timing information
This is useful for debugging AV sync etc. but should usually be disabled
because it generates a lot of information.
Move OS screen saver check in CApplication
At least for Wayland, window must have already been created in order
to create the OS screen saver implementation as it depends on the
window surface. Moving the default value setting until after the
window was created should not have any negative effects.
Let windowing system determine display latency
instead of using hardcoded number of frames and multiplying with
refresh rate
Remove clock offset from CVideoReferenceClock
The current calculation is weird and the DVD clock which is the only
user of this class has its own offset correction anyway.
Fix RenderManager VSyncAdjust enable condition
Previous code was wrong because it fails on slightly imprecise values.
For example fmod(60,29.999) is 0.002, which is OK.
But fmod(60,30.001) yields is 29.999, which will get rejected as being
above 0.01. There is no reason why 30.001 should be rejected while
29.999 is accepted.
Subtract time from the start of frame painting in PrepareNextRender f…
…or renderPts

Signed-off-by: Philipp Kerling <pkerling@casix.org>
Rename AdvancedSettings::GetDisplayLatency -> GetLatencyTweak, change…
… unit

New unit is milliseconds (instead of seconds) since most of the player
code uses it.

Renaming because the time is not related to the "real" display latency
that can somehow be measured, but rather an unspecific tweak.

Signed-off-by: Philipp Kerling <pkerling@casix.org>
Add WIN_SYSTEM_WAILAND enum value
Preparation for adding Wayland windowing implementation
Add generic version of StringUtils::Split
When the target container is not an std::vector (e.g. an std::set), the
result of Split() must be copied because always returns a std::vector.
Therefore, add a generic version StringUtils::SplitTo that splits a
string into an output iterator irrespective of the underlying container.

The existing Split() functions are kept for compatibility and
reimplemented using SplitTo().

pkerling added some commits Aug 12, 2017

Cleanup Geometry
* Use in-class default member initializers
* Mark default constructor noexcept
* Use member initializers in constructors instead of assignment
* Use brace-initialization
* Do not make references returned from arithmetic operators const
* Consistently add (in)equality operators as free functions
* Use using instead of typedef
* Simplify conditions
* Remove superfluous "inline"
* Add multiplication and division operators to CPoint
@pkerling

This comment has been minimized.

Show comment
Hide comment
@pkerling

pkerling Aug 16, 2017

Member

please switch to the new style of component logging which was introduced at #12676

Done. Had to rebase anyway, squashed while at it.

Member

pkerling commented Aug 16, 2017

please switch to the new style of component logging which was introduced at #12676

Done. Had to rebase anyway, squashed while at it.

@fritsch

This comment has been minimized.

Show comment
Hide comment
@fritsch

fritsch Aug 16, 2017

Member

@FernetMenta when do you push the button? Need to buy champagne before that I think :-)

Member

fritsch commented Aug 16, 2017

@FernetMenta when do you push the button? Need to buy champagne before that I think :-)

@FernetMenta

This comment has been minimized.

Show comment
Hide comment
@FernetMenta

FernetMenta Aug 17, 2017

Member

if no objections, I will merge this this evening.

Member

FernetMenta commented Aug 17, 2017

if no objections, I will merge this this evening.

elseif(WAYLAND_RENDER_SYSTEM STREQUAL "gles")
list(APPEND PLATFORM_REQUIRED_DEPS OpenGLES)
else()
message(SEND_ERROR "You need to decide whether you want to use GL- or GLES-based rendering in combination with the Wayland windowing system. Please set WAYLAND_RENDER_SYSTEM to either \"gl\" or \"gles\". For normal desktop systems, you will usually want to use \"gl\".")

This comment has been minimized.

@wsnipex

wsnipex Aug 17, 2017

Member

Can we set sane defaults here? I.e gl on x86 and gles on arm

@wsnipex

wsnipex Aug 17, 2017

Member

Can we set sane defaults here? I.e gl on x86 and gles on arm

This comment has been minimized.

@pkerling

pkerling Aug 17, 2017

Member

@FernetMenta was against that and he convinced me. I also think now that it's better to not have a hardly predictable default that depends on CPU architecture and/or installed libraries.

@pkerling

pkerling Aug 17, 2017

Member

@FernetMenta was against that and he convinced me. I also think now that it's better to not have a hardly predictable default that depends on CPU architecture and/or installed libraries.

This comment has been minimized.

@wsnipex

wsnipex Aug 17, 2017

Member

I am pretty sure this will cause confusion. Having gl as default on x86 is hardly unpredictable

@wsnipex

wsnipex Aug 17, 2017

Member

I am pretty sure this will cause confusion. Having gl as default on x86 is hardly unpredictable

This comment has been minimized.

@FernetMenta

FernetMenta Aug 17, 2017

Member

it is, most wayland application most likely use GLES. this has absolutely nothing to do with processor arch.

EDIT: and no, GLES is not a good default option for Kodi for the time being.

@FernetMenta

FernetMenta Aug 17, 2017

Member

it is, most wayland application most likely use GLES. this has absolutely nothing to do with processor arch.

EDIT: and no, GLES is not a good default option for Kodi for the time being.

This comment has been minimized.

@pkerling

pkerling Aug 17, 2017

Member

I am pretty sure this will cause confusion. Having gl as default on x86 is hardly unpredictable

Hm, not sure why it would be confusing to have to choose. The message also explicitly states to try gl on normal systems (we can of course reword the message if it isn't clear enough). And if you're compiling for embedded you should know whether you want GLES or not.

EDIT: Personally I'm not opposed to defaulting to full GL, but then independently of anything such as processor architecture.

@pkerling

pkerling Aug 17, 2017

Member

I am pretty sure this will cause confusion. Having gl as default on x86 is hardly unpredictable

Hm, not sure why it would be confusing to have to choose. The message also explicitly states to try gl on normal systems (we can of course reword the message if it isn't clear enough). And if you're compiling for embedded you should know whether you want GLES or not.

EDIT: Personally I'm not opposed to defaulting to full GL, but then independently of anything such as processor architecture.

This comment has been minimized.

@wsnipex

wsnipex Aug 17, 2017

Member

I'd add them to optional, so both get searched for, but still retain the check here if one was enabled

@wsnipex

wsnipex Aug 17, 2017

Member

I'd add them to optional, so both get searched for, but still retain the check here if one was enabled

This comment has been minimized.

@pkerling

pkerling Aug 17, 2017

Member

I can do that of course, but I think it's confusing because they are not in fact optional. You'd also have to disallow setting either of them to AUTO (since configuring must fail if it's not found). The current system isn't quite cut out for more complex dependencies like "you need either X or Y" :-/

@pkerling

pkerling Aug 17, 2017

Member

I can do that of course, but I think it's confusing because they are not in fact optional. You'd also have to disallow setting either of them to AUTO (since configuring must fail if it's not found). The current system isn't quite cut out for more complex dependencies like "you need either X or Y" :-/

This comment has been minimized.

@wsnipex

wsnipex Aug 17, 2017

Member

True, those are valid concerns. Another option would be ENABLE_WAYLAND=OPENGL/OPENGLES

@wsnipex

wsnipex Aug 17, 2017

Member

True, those are valid concerns. Another option would be ENABLE_WAYLAND=OPENGL/OPENGLES

This comment has been minimized.

@pkerling

pkerling Aug 17, 2017

Member

I'm not sure I understand. You want to rename the variable from WAYLAND_RENDER_SYSTEM to ENABLE_WAYLAND?

@pkerling

pkerling Aug 17, 2017

Member

I'm not sure I understand. You want to rename the variable from WAYLAND_RENDER_SYSTEM to ENABLE_WAYLAND?

This comment has been minimized.

@wsnipex

wsnipex Aug 17, 2017

Member

I'm sorry, hard to keep an overview on a phone during holidays. I forgot that Wayland is enabled with core_platform name and not via enable_Wayland. So this doesn't make sense. Let s just keep what you have for now and see how it goes. We can always optimize later.

@wsnipex

wsnipex Aug 17, 2017

Member

I'm sorry, hard to keep an overview on a phone during holidays. I forgot that Wayland is enabled with core_platform name and not via enable_Wayland. So this doesn't make sense. Let s just keep what you have for now and see how it goes. We can always optimize later.

@FernetMenta

This comment has been minimized.

Show comment
Hide comment
@FernetMenta

FernetMenta Aug 17, 2017

Member

congratualtions and many thanks to @pkerling

Member

FernetMenta commented Aug 17, 2017

congratualtions and many thanks to @pkerling

@FernetMenta FernetMenta merged commit 3716e49 into xbmc:master Aug 17, 2017

1 check passed

default You're awesome. Have a cookie
Details
@MartijnKaijser

This comment has been minimized.

Show comment
Hide comment
@MartijnKaijser

MartijnKaijser Aug 17, 2017

Member

Congrats and well done. Also for the other semi related fixes.

Member

MartijnKaijser commented Aug 17, 2017

Congrats and well done. Also for the other semi related fixes.

@fritsch

This comment has been minimized.

Show comment
Hide comment
@fritsch

fritsch Aug 17, 2017

Member

No need to stop helping and improving kodi for us, now that is merged :-) Thanks very much for this milestone work that pushes kodi to the next level into a great standard linux future.

Your work is highly appreciated.

Member

fritsch commented Aug 17, 2017

No need to stop helping and improving kodi for us, now that is merged :-) Thanks very much for this milestone work that pushes kodi to the next level into a great standard linux future.

Your work is highly appreciated.

@paulpach

This comment has been minimized.

Show comment
Hide comment
@paulpach

paulpach Aug 17, 2017

This was exciting to watch. This was the only reason why I don't have fedora in my HTPC, A million thanks to @pkerling

paulpach commented Aug 17, 2017

This was exciting to watch. This was the only reason why I don't have fedora in my HTPC, A million thanks to @pkerling

@da-anda

This comment has been minimized.

Show comment
Hide comment
@da-anda

da-anda Aug 17, 2017

Member

congrats and thanks a lot pkerling

Member

da-anda commented Aug 17, 2017

congrats and thanks a lot pkerling

@pkerling pkerling deleted the pkerling:wayland-for-master branch Aug 18, 2017

@pkerling

This comment has been minimized.

Show comment
Hide comment
@pkerling

pkerling Aug 18, 2017

Member

And there it goes 🎉 Thanks a lot to @FernetMenta for reviewing all of this stuff and giving valuable advice at every stage :-)

Member

pkerling commented Aug 18, 2017

And there it goes 🎉 Thanks a lot to @FernetMenta for reviewing all of this stuff and giving valuable advice at every stage :-)

@raneon

This comment has been minimized.

Show comment
Hide comment
@raneon

raneon Aug 18, 2017

Thank you so much pkerling!!! Highly appreciated :-)

raneon commented Aug 18, 2017

Thank you so much pkerling!!! Highly appreciated :-)

@Memphiz

This comment has been minimized.

Show comment
Hide comment
@Memphiz

Memphiz Aug 20, 2017

Member

Can you elaborate on this just for me please?

Member

Memphiz commented on 6a01797 Aug 20, 2017

Can you elaborate on this just for me please?

This comment has been minimized.

Show comment
Hide comment
@pkerling

pkerling Aug 23, 2017

Member

Sure. XBMCApplication.m includes XBMC_events.h, which now includes guilib/Resolution.h. That in turn includes C++ headers such as string. Therefore, XBMCApplication.m must be compiled in C++-compatible mode, or the C++ stuff will not work. Changing file extension to .mm enables C++ compatibility for the Objective-C compiler. All other files already used the .mm extension.

Member

pkerling replied Aug 23, 2017

Sure. XBMCApplication.m includes XBMC_events.h, which now includes guilib/Resolution.h. That in turn includes C++ headers such as string. Therefore, XBMCApplication.m must be compiled in C++-compatible mode, or the C++ stuff will not work. Changing file extension to .mm enables C++ compatibility for the Objective-C compiler. All other files already used the .mm extension.

This comment has been minimized.

Show comment
Hide comment
@Memphiz

Memphiz Aug 23, 2017

Member

Absolutely - thx for the clarification

Member

Memphiz replied Aug 23, 2017

Absolutely - thx for the clarification

@FernetMenta

This comment has been minimized.

Show comment
Hide comment
@FernetMenta

FernetMenta Nov 18, 2017

Member

@afedchin @peak3d @lrusak I dropped this in #13047
please make sure that your implementations of SetFullScreen and ResizeWindow return proper values

Member

FernetMenta commented on xbmc/guilib/GraphicContext.cpp in e015168 Nov 18, 2017

@afedchin @peak3d @lrusak I dropped this in #13047
please make sure that your implementations of SetFullScreen and ResizeWindow return proper values

@FernetMenta

This comment has been minimized.

Show comment
Hide comment
@FernetMenta

FernetMenta May 16, 2018

Member

@pkerling not sure what you are doing here. This method is supposed to set parameter current

Member

FernetMenta commented on xbmc/settings/DisplaySettings.cpp in 49fb1d6 May 16, 2018

@pkerling not sure what you are doing here. This method is supposed to set parameter current

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