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

Mir windowing system for Kodi #10898

Merged
merged 1 commit into from Dec 2, 2016

Conversation

@BrandonSchaefer

BrandonSchaefer commented Nov 8, 2016

Mir windowing system for Kodi.

Description

Implements a mir windowing system. Supports both OpenGL/ES for rendering. Keyboard/Pointer events but I need to test out touch events. Supports both cmake/autotools. I also need to test out arm/64.

cmake -DENABLE_MIR=ON or for autotools --enable-mir

Motivation and Context

The motivation for this change is to support a mir windowing system. Allow a Kodi media center to run on mir.

How Has This Been Tested?

Tested on ubuntu 16.10 using mir 0.24 and 0.25. There is some mir 0.25 code that is commented out but can be re-enabled once 0.25 has been released.

To test you'll need a mir server + kodi built with mir enabled

On ubuntu 16.10:

Getting a mir server running:
apt-get install miral-examples (gives you miral-shell the mir server)
Running on X11 you can simply run: ./miral-shell
You'll see a window pop up saying Mir-on-X which is the mir server running on the X11 platform.

Getting kodi compiled/working.
apt-get install libmirclient-dev
From this branch you can either use autotools (--enable-mir) or cmake (-DENABLE_MIR=ON)
Once configured make annnd wait.

Once you have kodi.bin and the miral-shell (server) running:
./kodi.bin

This will start kodi.bin on the mir server.

From there tested video/pictures/add-ons/skins/music etc. I could have missed somethings to manually test that I was unsure about.

This code change is very low impact, as its turned off by default and the code it self is stuffed into a directory.

I also think I need to write a README.mir

Screenshots (if appropriate):

https://www.youtube.com/watch?v=26A6qK7vNGo

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

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
@FernetMenta

This comment has been minimized.

Member

FernetMenta commented Nov 8, 2016

Good work. Looks really nice after a fist glance.

@@ -29,6 +29,12 @@
#elif defined(TARGET_WINDOWS) && defined(HAS_DX)
#include "windows/WinSystemWin32DX.h"
#elif defined(TARGET_LINUX) && defined(HAVE_MIR) && defined(HAS_GL)
#include "mir/WinSystemMirGLContext.h"

This comment has been minimized.

@hudokkow

hudokkow Nov 8, 2016

Member

Not a code review but please use only one space around operators here and below.
Yes, I know... existing code doesn't follow this request. 😉

This comment has been minimized.

@hudokkow

hudokkow Nov 8, 2016

Member

heheh, that was fast! Thx!

This comment has been minimized.

@BrandonSchaefer

BrandonSchaefer Nov 8, 2016

No problem was an easy fix!

@BrandonSchaefer

This comment has been minimized.

BrandonSchaefer commented Nov 8, 2016

Also note I do plan on maintaining this port. I Plan to maintain with the latest Mir release (currently 0.24.1 but 0.25 is coming soon). That is until I need to support multiple Mir versions.

@fetzerch

Nice to see this feature wise :)
I can only comment on the cmake part and found a few minors.

#
# and the following imported targets::
#
# Mir::Mir - The Mir library

This comment has been minimized.

@fetzerch

fetzerch Nov 9, 2016

Member

You don't define the imported target. Since we currently don't use them you can just drop those lines here.

This comment has been minimized.

@BrandonSchaefer
@@ -118,6 +118,12 @@ if(CORE_SYSTEM_NAME STREQUAL android)
list(APPEND required_deps Zip)
endif()
# Missing mir support in these libraries
if (ENABLE_MIR)

This comment has been minimized.

@fetzerch

fetzerch Nov 9, 2016

Member

Please move this to project/cmake/scripts/linux/ArchSetup.cmake

This comment has been minimized.

@BrandonSchaefer

BrandonSchaefer Nov 9, 2016

O didnt even know about that cmake file, excellent!

core_optional_dep(X ENABLE_X11)
core_optional_dep(LibDRM ENABLE_X11)
core_optional_dep(XRandR ENABLE_X11)
if (ENABLE_MIR)

This comment has been minimized.

@fetzerch

fetzerch Nov 9, 2016

Member

Good reminder that I need to clean up this spaghetti code. Ok for now, but make it if( without the space everywhere please.

This comment has been minimized.

@BrandonSchaefer
# Missing mir support in these libraries
if (ENABLE_MIR)
set(ENABLE_VAAPI OFF CACHE BOOL "Disabling VAAPI since no mir support" FORCE)
set(ENABLE_VDPAU OFF CACHE BOOL "Disabling VDPAU since no mir support" FORCE)

This comment has been minimized.

@wsnipex

wsnipex Nov 9, 2016

Member

please also add a message telling the user that HW acceleration is disabled because of mir

This comment has been minimized.

@BrandonSchaefer

BrandonSchaefer Nov 9, 2016

Hmm its still using opengl/es meaning its accelerated. Whats not implemented is hardware encoding/decoding for mir in these libraries.

This comment has been minimized.

@BrandonSchaefer

BrandonSchaefer Nov 9, 2016

Different acceleration you were talking about. Fixed!

PATHS ${PC_MIR_INCLUDE_DIRS})
find_library(MIR_LIBRARY NAMES mirclient
PATHS ${PC_MIR_LIBRARIES} ${PC_LIBRARY_DIRS})

This comment has been minimized.

@wsnipex

wsnipex Nov 9, 2016

Member

PC_MIR_LIBRARY_DIRS?

This comment has been minimized.

@BrandonSchaefer
core_optional_dep(LibDRM ENABLE_X11)
core_optional_dep(XRandR ENABLE_X11)
if (ENABLE_MIR)
core_optional_dep(Mir ENABLE_MIR)

This comment has been minimized.

@wsnipex

wsnipex Nov 9, 2016

Member

core_require_dep please. The dependency is required once the user enables mir.

And please add an option on line 58 for ENABLE_MIR

This comment has been minimized.

@BrandonSchaefer

BrandonSchaefer Nov 9, 2016

For the option on line 58, how would we define a system is mir and not x11? As any system thats linux will most likely be for an x11 system (that possibly supports mir)

This comment has been minimized.

@BrandonSchaefer

BrandonSchaefer Nov 9, 2016

Im now assuming you ment an option for OFF :)

@wsnipex

This comment has been minimized.

Member

wsnipex commented Nov 10, 2016

@FernetMenta is this something we could consider for v17, because it's very contained and doesn't touch existing code?

@fritsch

This comment has been minimized.

Member

fritsch commented Nov 10, 2016

What I wonder is: Can we get VAAPI working? We don't use any X11 dependencies in the code. What is currently missing in that regard?

@fritsch

This comment has been minimized.

Member

fritsch commented Nov 10, 2016

For my part v17: I does not harm, we have a maintainer with it. It is not build by default via our ppa - so users using it will most likely be only self compilers.

If ubuntu considers it for their next release, the hw decoding, especially with vaapi should be considered investigating.

@@ -41,6 +42,7 @@ class CWinEventsMir : public IWinEvents
private:
std::queue<XBMC_Event> events;
std::mutex mutex;

This comment has been minimized.

@fritsch

fritsch Nov 10, 2016

Member

for members we use m_ so in your case:

m_events;
m_mutex;

This comment has been minimized.

@BrandonSchaefer

BrandonSchaefer Nov 10, 2016

Opps missed that! Fixed

@@ -329,10 +331,12 @@ bool CWinEventsMir::MessagePump()
size_t CWinEventsMir::GetQueueSize()
{
std::lock_guard<decltype(mutex)> event_lock(mutex);

This comment has been minimized.

@fritsch

fritsch Nov 10, 2016

Member

that's a nice c++11 feature which could replace most of our own locking patterns, that implemented this "by hand", when there was no std::mutex and no std::lock_guard available.

This comment has been minimized.

@BrandonSchaefer

BrandonSchaefer Nov 10, 2016

c++11 (and 14) have added quite a bit of quality of life :)

@MartijnKaijser

This comment has been minimized.

Member

MartijnKaijser commented Nov 10, 2016

no objection

@BrandonSchaefer

This comment has been minimized.

BrandonSchaefer commented Nov 10, 2016

For VAAPI support (I dug into a little bit) it needs a mir backend pretty much. Something Im interested in as well. No promises of when though!

https://cgit.freedesktop.org/libva/tree/va/va_backend.h

@fritsch

This comment has been minimized.

Member

fritsch commented Nov 10, 2016

I thought that would somehow work via EGL / DRM via: https://cgit.freedesktop.org/libva/tree/va/egl/va_backend_egl.h

@BrandonSchaefer

This comment has been minimized.

BrandonSchaefer commented Nov 10, 2016

That... should work, ill have to test that out!

@BrandonSchaefer

This comment has been minimized.

BrandonSchaefer commented Nov 10, 2016

Im seeing some issues with the egl windowing system battling against mir when using opengles. I would prefer not to add an EGL Mir type soo im going to mess with that a bit.

@fritsch

This comment has been minimized.

Member

fritsch commented Nov 10, 2016

If you have comments on how we can improve that without you having to introduce pseudo types that duplicate given definitions, please do not hessitate to point them out.

@FernetMenta

This comment has been minimized.

Member

FernetMenta commented Nov 10, 2016

Mir won't work with vaapi unless someone implements something like vaGetDisplayMIR

@fritsch

This comment has been minimized.

Member

fritsch commented Nov 10, 2016

@BrandonSchaefer

This comment has been minimized.

BrandonSchaefer commented Nov 10, 2016

I made some changes in 21b89b2

Soo I dont need to create a EGL Mir type. Pretty much ignore the windowing egl system if mir is enabled!

@BrandonSchaefer

This comment has been minimized.

BrandonSchaefer commented Nov 11, 2016

Also looking at cores/VideoPlayer/DVDCodecs/Video/VAAPI.h seems to depend on X11.

@FernetMenta

This comment has been minimized.

Member

FernetMenta commented Nov 11, 2016

I know about the DRM backend but I doubt that it would work when the final goal is to display the surface.

@BrandonSchaefer

This comment has been minimized.

BrandonSchaefer commented Nov 12, 2016

Soo messing around with the VAAPI, im pretty sure its working using DRM/EGL render nodes. (Was running into issues with our kms backend since it opens up the same dri device which work buuut render nodes seems to work)
http://paste.ubuntu.com/23464991/

A very hacked proof of concept:
http://paste.ubuntu.com/23465033/

From there its just manually checking the /dev/dri/renderD* for the correct number depending on the machine. (as long as you've a kernel >= 3.15)

Which brings me to wondering if we can replace the VAAPI DVDCodec bits with drm only? Or does the x11 specific bring better support for x11? I dont really understand everything thats going on there :).

@FernetMenta

This comment has been minimized.

Member

FernetMenta commented Nov 12, 2016

Which brings me to wondering if we can replace the VAAPI DVDCodec bits with drm only?

needs to be tested with wayland too. there is nothing that keeps us sticking to x11 because x11 will die anyway.

did you get a picture with drm?

@BrandonSchaefer

This comment has been minimized.

BrandonSchaefer commented Nov 12, 2016

Yeah testing for sure. I did get picture and tested with print statements in the vaapi renderer + break point on vaRenderPicture which were all going off (which makes me think its working!). Let me screen shot though!

@BrandonSchaefer

This comment has been minimized.

@fritsch

This comment has been minimized.

Member

fritsch commented Nov 12, 2016

Great! Just great!

@fritsch

This comment has been minimized.

Member

fritsch commented Nov 12, 2016

For the record, still works fine with X11 Display (what was to be expected). Btw. if we are at it - there are some additional options for "transfering" * the surfaces via DRM insteand of using EGL, but still being able to render them afterwards.

drm-vaapi

  • sharing in zero copy approach

Code used, directly basing on your cleaned up "hack": http://sprunge.us/cDgS - node, that is opened needs to be closed, which is also missing here.

@fritsch

This comment has been minimized.

Member

fritsch commented Nov 12, 2016

fritsch@372a3ff should do :-)

@BrandonSchaefer

This comment has been minimized.

BrandonSchaefer commented Nov 12, 2016

O sweet, yeah i double checked with X11 as well which was working but didnt know about that player process info. That helps secure the fact its vaapi. Also note it doesnt work for *.divx (which im not very familiar with codecs in general I just assume vaapi doesnt support that codec.)

Just needs cleaning up on an RAII class to hold the FD as well as selecting the render node (since I assume mir/wayland will hold onto the device so you cannot drmOpen on it)

@fritsch

This comment has been minimized.

Member

fritsch commented Nov 12, 2016

For DIVX: Player Settings (Expert Hierarchy) -> Enable Mpeg4

@BrandonSchaefer BrandonSchaefer referenced this pull request Nov 12, 2016

Merged

Use DRM as a fallback if not on X11 for VAAPI #10922

4 of 9 tasks complete
@BrandonSchaefer

This comment has been minimized.

BrandonSchaefer commented Nov 13, 2016

Comparison on this pull request (#10922) with mir using VAAPI vs X11 and vs mir with no hardware accel (ie. this branch currently)

Makefile.in Outdated
@@ -135,8 +135,10 @@ endif
ifeq (@USE_OPENGLES@,1)
DIRECTORY_ARCHIVES += xbmc/rendering/gles/rendering_gles.a
ifneq (@USE_MIR@,1)

This comment has been minimized.

@FernetMenta

FernetMenta Nov 30, 2016

Member

please drop changes for autotools. we are about to drop it

This comment has been minimized.

@BrandonSchaefer
@@ -0,0 +1,39 @@
#pragma once
#ifndef WINDOW_SYSTEM_MIR_GLES_CONTEXT_H_

This comment has been minimized.

@FernetMenta

FernetMenta Nov 30, 2016

Member

what are those outdated macros good for?

This comment has been minimized.

@BrandonSchaefer

BrandonSchaefer Dec 1, 2016

#pragma is not part of the standard

This comment has been minimized.

@BrandonSchaefer

BrandonSchaefer Dec 1, 2016

Im happy to remove them though

This comment has been minimized.

@FernetMenta

FernetMenta Dec 1, 2016

Member

we require it in many places. a compiler that does not support pragma won't ever be able to build kodi

This comment has been minimized.

@BrandonSchaefer

BrandonSchaefer Dec 1, 2016

And perfectly reasonable, removing!

@@ -0,0 +1,40 @@
#pragma once

This comment has been minimized.

@garbear

garbear Dec 1, 2016

Member

Add a copyright header to this and WinSystemMirGLESContext.h please

This comment has been minimized.

@BrandonSchaefer

BrandonSchaefer Dec 1, 2016

Opps, missed two files. Fixing

@FernetMenta FernetMenta merged commit 374da77 into xbmc:master Dec 2, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@BrandonSchaefer BrandonSchaefer referenced this pull request Dec 2, 2016

Merged

Enable VAAPI support for Mir using DRM #11040

4 of 9 tasks complete

@MartijnKaijser MartijnKaijser modified the milestone: L 18.0-alpha1 Dec 3, 2016

@FernetMenta

This comment has been minimized.

Member

FernetMenta commented on xbmc/windowing/CMakeLists.txt in 8cf9815 Jan 7, 2017

this inverted logic is kind of ugly. I don't think we want to extend this with NOT Mir Wayland, blaba
every platform only ever include one WinEventsXY. hence the logic should be: if platform bla, include WinEventsBla

This comment has been minimized.

BrandonSchaefer replied Jan 8, 2017

I agree 100%. I can try to get a new patch up soon!

This comment has been minimized.

Member

FernetMenta replied Jan 9, 2017

I had a discussion with @fetzerch. He is working on something better already.

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