New RetroPlayer renderer based on VideoPlayer

VelocityRa commented 7 years ago

Description

This PR is a new renderer for the RetroPlayer core and is heavily based on VideoPlayer's renderer.

The vast majority of of dependencies to VP have been eliminated, with only some hardware renderers (MMAL and AML) relying on VideoPlayer files.

~I did my best to factor out common code between VP and RP. Common files were placed in the cores/ directory.~ Edit: Undid that based on Fernet's message below.

Almost all of of the work is done, with the exception of a few files as mentioned above. Doing it for these too would involve moving most of VideoPlayer/DVDCodecs to the cores/ directory, which I'm not sure is ideal.

Motivation and Context

RetroPlayer has always heavily depended on VideoPlayer for rendering and this was a problem.

To quote @fernetmenta:

Note that RetroPlayer in its current state uses private APIs of VideoPlayer which is not ok. RP is not supposed to use VP's rendering system.

How Has This Been Tested?

The changes have only been tested on Windows. I'm submitting this PR to test them on other platforms too.

Screenshots (if appropriate):

Visually identical to the previous renderer, so no screenshot needed.

Types of change

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

Checklist:

[x] 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
[x] I have read the CONTRIBUTING document
[ ] I have added tests to cover my change
[ ] All new and existing tests passed

FernetMenta commented 7 years ago

don't move things from VP, only copy. VP does not want to share its clock implementation with any other component. same for overlays and every other file in VP tree.

btw: I doubt that RP needs RenderCapture or VideoReferenceClock. the latter is for smooth video aks sync playback to display.

VelocityRa commented 7 years ago

@FernetMenta Gotcha, I'll just do what I did before moving those, which is include things from VP and only copy if I know they're useful for RP too.

I'll remove VideoReferenceClock, RenderCapture and the other unnecessary things we talked about.

da-anda commented 7 years ago

all the Boblight/Hyperion/homebrew-ambilight users might want to have rendercapture also for gaming, so if it's not too much of an issue to maintain, I'd probably keep it. At least to my knowledge these add-ons use rendercapture

VelocityRa commented 7 years ago

@da-anda Hi, I removed RenderCapture because @FernetMenta said that it's old code and should be re-written from scratch. RP has the advantage that nodody uses this feature yet.

So I/someone else will add support for RenderCapture when it's re-written.

peak3d commented 7 years ago

Can pls. anyone explain why RetroPlayer can NOT rely on VideoPlayer Render? For my understanding RetroPlayer is nothing else like a decoder wich produces images wich has to be displayed.

With the introduction of VideoBuffers it should be easy to use all Renderer's from VP core just by filling the RetroPlayer produced frames into such a buffer.

VelocityRa commented 7 years ago

@peak3d Due to its difference in requirements, RetroPlayer's renderer can be quite simpler than VideoPlayer.

To quote a few of the rest of the points by FernetMenta:

VP will advance to work without a gui, as infrastructure for transcoding, requirements will diverge from RP’s more and more
everything below the VideoPlayer folder is private to VP
RetroPlayer uses VideoPlayers private APIs and as a result it will break very often
RP needs its own settings and APIs

da-anda commented 7 years ago

speaking of transcoding and VP: a) what about gamestream? It's basically just a video being displayed, thus we'd need the video processing pipelines within retroplayer b) and what about streaming of games from Kodi to the outer world? Like twitch or just another Kodi instance of a friend? For this the AV output of retroplayer would have to be transcoded and I'm not sure if a reimplementation of RenderCapture would suffice for this.

These are ofc no demands as of now, but things to keep in mind when designing the APIs.

peak3d commented 7 years ago

@VelocityRa

thanks for your thoughts / explantations!

Its a question of concept, how much VP API you really use.

We are currently not talking about the full VP pipeline, we're talking about RenderManager + Renderer. Sure, you have to introduce a new VPRP wich feeds the RM, but there is IMO no need to copy code from VP to RP.

For maintainers of kodi this is the worst case - just had it with popcornmix MMAL implementation. Changes / Improvements in Render - Pipeline has to be done on 2 places. Me as a maintainer of AML and Android its important to have things only once.

My big please is that you (+@fernetmenta) evaluate if it would be possible to open the internal VP / RM API in a way it will fulfill your needs. And don't do code duplication.

B.t.w: AddonVideoCodec benefits from DXVA Renderer directly only by using VideoBuffers to render. The first implemenation had also platform dependend code in it, but we decided to introduce VideoBuffers and we're happy with this.

@da-anda GameStream: Isn't it a feature wich could also be nice for normal Video Playback?

da-anda commented 7 years ago

I don't want to get too off topic here, but Gamestream (= playing a game actively from a remote server) requires the client to send input signals back to the server. Thus we need proper input support and especially also the "key capture" mode of Retroplayer, so that keymaps of Kodi are ignored while playing and pressing random keys. Thus I don't think it would work well for just VideoPlayer.

A pure "spectator" mode (like twitch, ...) would ofc also work just fine with VP, but that's a different thing.

VelocityRa commented 7 years ago

We are currently not talking about the full VP pipeline, we're talking about RenderManager + Renderer. Sure, you have to introduce a new VPRP wich feeds the RM, but there is IMO no need to copy code from VP to RP.

I'm new to all of this, I just did it the way people recommended that I do it. Doing it any other way would have probably made things much more difficult. Factoring out common code requires decent understanding of both VP and RP's (present and future) requirements.

My big please is that you (+@FernetMenta) evaluate if it would be possible to open the internal VP / RM API in a way it will fulfill your needs. And don't do code duplication.

Code duplication in RM and the renderers could perhaps be eliminated with a BaseRenderManager and a shared cores/VideoRenderers directory. This really is a lot of work though, the renderers rely heavily on VideoPlayer. My initial proposal (I'm a GSoC student) was about adding shader support to RetroPlayer, I feel like this is out of scope for me right now.

peak3d commented 7 years ago

@VelocityRa the things I'm saying are just thoughts in hope that they make your development easier. Finally its @FernetMenta who decides how to open / move things to give you an API wich fits.

Just to be said: I have a huge respect about what you have figured out in the short time! Thanx for this!

FernetMenta commented 7 years ago

There has never been any VP APIs, there is only an IPlayer API used by the application. That does not mean that there won't be a decoding or rendering API in the future. For designing an API one has to know the requirements of different clients. Seperating RP and stripping off the things not needed is a first step in evaluating the requirements for RP At this time code duplication is the way to go. Much better than having RP broken with every change in VP.

garbear / xbmc