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

New RetroPlayer renderer based on VideoPlayer #84

Closed
wants to merge 22 commits into
from

Conversation

Projects
None yet
5 participants
@VelocityRa

VelocityRa commented Jul 2, 2017

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

  • 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.

Show comment
Hide comment
@FernetMenta

FernetMenta Jul 3, 2017

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.

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

This comment has been minimized.

Show comment
Hide comment
@VelocityRa

VelocityRa Jul 3, 2017

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

@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

This comment has been minimized.

Show comment
Hide comment
@da-anda

da-anda Jul 4, 2017

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

da-anda commented Jul 4, 2017

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

This comment has been minimized.

Show comment
Hide comment
@VelocityRa

VelocityRa Jul 4, 2017

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

@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

This comment has been minimized.

Show comment
Hide comment
@peak3d

peak3d Jul 5, 2017

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.

peak3d commented Jul 5, 2017

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

This comment has been minimized.

Show comment
Hide comment
@VelocityRa

VelocityRa Jul 5, 2017

@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

@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

This comment has been minimized.

Show comment
Hide comment
@da-anda

da-anda Jul 5, 2017

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.

da-anda commented Jul 5, 2017

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

This comment has been minimized.

Show comment
Hide comment
@peak3d

peak3d Jul 5, 2017

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

peak3d commented Jul 5, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@da-anda

da-anda Jul 5, 2017

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.

da-anda commented Jul 5, 2017

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

This comment has been minimized.

Show comment
Hide comment
@VelocityRa

VelocityRa Jul 5, 2017

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.

VelocityRa commented Jul 5, 2017

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

This comment has been minimized.

Show comment
Hide comment
@peak3d

peak3d Jul 5, 2017

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

peak3d commented Jul 5, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@FernetMenta

FernetMenta Jul 5, 2017

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.

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.

Show outdated Hide outdated xbmc/cores/RetroPlayer/RetroPlayer.h
@@ -137,9 +136,9 @@ namespace RETRO
// implementation of IRenderMsg
void VideoParamsChange() override { }
void GetDebugInfo(std::string &audio, std::string &video, std::string &general) override { }
void UpdateClockSync(bool enabled) override;
void UpdateClockSync(bool enabled) override { }

This comment has been minimized.

@garbear

garbear Jul 5, 2017

Owner

This is a non-cosmetic change

@garbear

garbear Jul 5, 2017

Owner

This is a non-cosmetic change

garbear and others added some commits Oct 6, 2015

[PR 12434] Implement basic gameplay persistence (autosave)
This implements a rudimentary autosave/autoload feature. When the game is
closed, the state and its metadata are saved to the hard drive. When the
game is played again, the state is automatically loaded.

This is called "gameplay persistence" because the game resumes where it was
last left off, with no explicit saving/loading action by the user.

Additionally, to guard against power loss, the game is autosaved every 10
seconds.

Savestates and metadata are stored alongside the game by replacing the file
extension.

If a different game client is used, Kodi will warn the user that it will
overwrite their existing save. This is because savestates are incompatible
between emulators, and storing more than a single savestate isn't
implemented yet.
[temp] Add repository for IARL
Thanks to Zach Morris for providing this awesome add-on.
Fix broken virtual keyboard when using a controller
By setting "a" to the Shift action, the correct Select actions were being
blocked, making the virtual keyboard unusable.
RetroPlayer: Disable audio and video codecs (used for GameStream)
Temporary, to simplify porting/development of the new renderer.
RetroPlayer: Introduce hardware renderers
DXVA(-HD), VAAPI and VDPAU renderers/APIs aren't ported over because
they're used for accelerating video decoding, which is irrelevant for
RetroPlayer
RetroPlayer: Introduce RPRenderUtils
* Right now has some FFmpeg utility functions ported from
  VideoPlayer/DVDCodecs/DVDCodecUtils.

* Also introduce RPRenderFlags.
RetroPlayer: Introduce VideoShaders
All these file are identical to their VideoPlayer counterparts, so moving
them to a higher directory that VideoPlayer/ and using them from there
might be preffered in the future.
RetroPlayer: Introduce RPRenderManager
Finally introduce our RenderManager, which is the interface RetroPlayer
directly interacts with.
RetroPlayer: Add a CMake file for the renderers
Add a CMakeLists.txt file for the VideoRenderers directory
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment