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

RetroPlayer: Add renderer for Windows, OpenGL, OpenGLES and MMAL #12715

Merged
merged 7 commits into from Sep 28, 2017

Conversation

@garbear
Copy link
Member

commented Aug 25, 2017

This takes a significant step forward by separating the RetroPlayer and VideoPlayer renderers. The new RP renderer is pretty bare, and just contains enough code to render RGB images delivered from game add-ons.

Motivation and Context

Based on garbear#84

How Has This Been Tested?

Tested on Windows, Linux and OSX. The OpenGLES renderer hasn't been tested yet, but it uses mostly the same code as the OpenGL renderer.

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)

@garbear garbear added this to the L 18.0-alpha1 milestone Aug 25, 2017

@garbear garbear force-pushed the garbear:rp-renderer branch 2 times, most recently from ddc9dac to b6a6b3b Aug 25, 2017

@garbear

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2017

Rebased after the merge of #12665. Still TODO are a few remaining steps to fully separate RP and VP, including the separation of CProcessInfo.

@garbear garbear force-pushed the garbear:rp-renderer branch from d3e972a to 1a5e94d Aug 26, 2017

m_dataCache->SetRenderClockSync(false);
m_dataCache->SetStateSeeking(false);
m_dataCache->SetSpeed(1.0f, 1.0f);
m_dataCache->SetGuiRender(true); //! @todo

This comment has been minimized.

Copy link
@garbear

garbear Aug 26, 2017

Author Member

@FernetMenta What is gui render? This isn't documented.

This comment has been minimized.

Copy link
@VelocityRa

VelocityRa Sep 8, 2017

Member

Presumably it's for when rendering the GUI pass in VP/RM (Render is called with the gui parameter set to true).
Likewise, SetVideoRender below is probably for when rendering the output frame. But I don't know if you just need to set both to true once, or update them multiple times every frame while rendering.
As with anything from CDataCacheCore, updating it is optional.

m_dataCache->SetStateSeeking(false);
m_dataCache->SetSpeed(1.0f, 1.0f);
m_dataCache->SetGuiRender(true); //! @todo
m_dataCache->SetVideoRender(true); //! @todo

This comment has been minimized.

Copy link
@garbear

garbear Aug 26, 2017

Author Member

@FernetMenta What is video render? This isn't documented.

@garbear

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2017

jenkins build this please (to check GLES code)

@popcornmix

This comment has been minimized.

Copy link
Member

commented Aug 27, 2017

Just a warning that rendering through GLES will be significantly less efficient.

Prior to this PR on Pi we will directly render in the game's native bitmap format (e.g. RGB565, RGB888, RGBA32) to a separate layer without any copying required.
Now, then texture upload is effectively a memcpy to GPU memory, followed by a format conversion to a tiled texture format. There may also be an additional format conversion before texture upload to get the image into GL_BGRA format.

A few years ago all ARM platforms rendered video through GL. But performance sucked and now all ARM platforms render video directly on a separate video plane, mostly using zero copy.

I understand that for emulating 3D consoles you need GL rendering (although whether performance will be adequate for this class of emulation is questionable). Also you may want it for shader based scanline effects. But I worry this scheme will rule out a class of emulation that previously worked acceptably but will no longer with the GL renderer.

It is possible we will want to support platform specific (e.g. MMAL) renderers for RP in the same way as we do for VP.

@garbear

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2017

Agreed, a MMAL renderer for RetroPlayer would be desirable. Do you have time to implement this? It's pretty much just a matter of copying the VP renderer and removing all the video stuff. If not, I can handle this eventually.

Using MMAL for rendering would preclude the use of shaders, right?

@popcornmix

This comment has been minimized.

Copy link
Member

commented Aug 27, 2017

I can help with MMAL renderer support when I get some time.
Ideally MMAL would be an optional renderer alongside GL.
If game uses GL or custom shaders then GL path is used otherwise use MMAL renderer.

I don't see a good way to use shaders alongside MMAL (you would lose the zero copy path so may as well render with GL as your pixels are already there).

@fritsch

This comment has been minimized.

Copy link
Member

commented Aug 27, 2017

Most ARMs (including IMX) offer some pseudo vendor optimized GL extensions to simulate a zero copy or at least a DMA copy to the 3D GPU. Still that would also be less performant as "passthrough" rendering, but perhaps some custom shader / GL code could be supported that way?

eglCreateImage might be a candidate when doing it in a standard way?

m_targetPixFormat = AV_PIX_FMT_BGRA;
m_targetDxFormat = DXGI_FORMAT_B8G8R8X8_UNORM;
if (g_Windowing.IsFormatSupport(DXGI_FORMAT_B8G8R8A8_UNORM, D3D11_USAGE_DYNAMIC))
m_targetDxFormat = DXGI_FORMAT_B8G8R8A8_UNORM;

This comment has been minimized.

Copy link
@afedchin

afedchin Aug 29, 2017

Member

three lines above may be replaced with
m_targetDxFormat = g_Windowing.GetBackBuffer()->GetFormat();

This comment has been minimized.

Copy link
@garbear

garbear Aug 29, 2017

Author Member

Done

@@ -69,7 +69,7 @@ bool CRPWinRenderer::Configure(AVPixelFormat format, unsigned int width, unsigne
m_renderOrientation = orientation;

m_targetPixFormat = AV_PIX_FMT_BGRA;
m_targetDxFormat = DXGI_FORMAT_B8G8R8X8_UNORM;
m_targetDxFormat = g_Windowing.GetBackBuffer()->GetFormat();
if (g_Windowing.IsFormatSupport(DXGI_FORMAT_B8G8R8A8_UNORM, D3D11_USAGE_DYNAMIC))

This comment has been minimized.

Copy link
@afedchin

afedchin Aug 29, 2017

Member

this (and line below) may be removed then

This comment has been minimized.

Copy link
@garbear

garbear Aug 31, 2017

Author Member

Thanks

@garbear garbear force-pushed the garbear:rp-renderer branch from 663f477 to 8e5b287 Aug 31, 2017

@garbear

This comment has been minimized.

Copy link
Member Author

commented Sep 3, 2017

Updated with fixes from the last week

@garbear garbear force-pushed the garbear:rp-renderer branch 3 times, most recently from 1b90589 to 606dfde Sep 3, 2017

@garbear

This comment has been minimized.

Copy link
Member Author

commented Sep 7, 2017

Rebased after #12761, included recent changes from my shaders work and squashed. Just one bit of code in the GLES renderer needed before merging.

/*
glTexEnvf(GL_TEXTURE_ENV, GL_TEXTURE_ENV_MODE, GL_MODULATE);
glBegin(GL_QUADS);

This comment has been minimized.

Copy link
@garbear

garbear Sep 7, 2017

Author Member

How do I render an RGB texture with GLES?

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Sep 7, 2017

Member

this is not implemented for GLES

This comment has been minimized.

Copy link
@garbear

garbear Sep 7, 2017

Author Member

Then what do I do? Convert to YUV and use Kodi's GLES YUV code?

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Sep 7, 2017

Member

how have you done it in the past?

This comment has been minimized.

Copy link
@garbear

garbear Sep 7, 2017

Author Member

In the past I used VP's RenderManager. I converted to YUV and handed it off.

Now, with the new renderer, I'm trying to avoid YUV and render the RGB image directly.

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Sep 7, 2017

Member

If the source is RGB, it should be rendered RGB. If renderer does not support this format there are two options: a) implement workaround in renderer, b) disabled RP on this platform
IMO full platform coverage is not a requirement for a feature. Missing parts can be implemented by platform maintainers at a later time.

This comment has been minimized.

Copy link
@VelocityRa

VelocityRa Sep 8, 2017

Member

Looks like OpenGL ES supports at least some the formats we want (RGB555, RGB565) Source in the GLES spec

Edit: Note that these are just the ones implementers are required to implement. Most support many more than what is listed there, so we should be fine.

@garbear If I recall correctly libretro cores also need BGRA8 support right? So, we can probably use OES_rgb8_rgba8 and swizzle our way into BGRA8 in the shader.

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Sep 8, 2017

Member

rendering RBG is trivial, it's just not implemented by Kodi's GLES renderer

This comment has been minimized.

Copy link
@VelocityRa

VelocityRa Sep 8, 2017

Member

"RGB" isn't very helpful, unless you mean RGB8 since that's the most common.
It's pretty peculiar that the spec doesn't include RGB8... but who cares, everyone most likely implements it.
Anyway, the current renderer simply outputs in BGRA8. However, it relies on sws_scale to do the conversion from the possible input formats taken from libretro cores.

My point above was about using those formats as the format of the backbuffer, so that we can achieve "zero copy" rendering and avoid sws_scale/software scaling altogether.
I looked into that for the D3D renderer, but these formats are only supported on DXGI 1.2+ devices unfortunately (no D3D 9), so we didn't go for it.

~CGUIRenderFullScreen() override = default;

void Render() override;
void RenderEx() override;

This comment has been minimized.

Copy link
@VelocityRa

VelocityRa Sep 8, 2017

Member

What's this? How is it different from Render? Going by APIs like Win32, MethodEx would stand for "MethodExtended", meaning that it would provide you with more arguments to tweak, which isn't the case here.
This comment goes for every other definition of RenderEx.

This comment has been minimized.

Copy link
@garbear

garbear Sep 9, 2017

Author Member

Render() is called on the GUI pass. RenderEx() is called on the video rendering pass. Eventually RetroPlayer will support h264 decoding for game streaming, so we'll need use of the video pass.

@garbear

This comment has been minimized.

Copy link
Member Author

commented Sep 9, 2017

@popcornmix I've started working on MMAL support. The buffers in RetroPlayer are very basic and can't handle things like buffer pools. I'll let you know when the new buffer architecture is complete so we can get MMAL rendering to work.

One of the features that flew into this PR before the last rebase was the ability to have multiple renderers at the same time. In the GSoC code, renderers are indexed by shader preset and are swapped around as needed. I don't see any reason we can't switch between MMAL and GLES depending on which features they support.

@garbear

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2017

@popcornmix I've added a MMAL renderer based on the VideoPlayer code. It needs some more work, but should serve as a good base for MMAL support.

Now that we have a buffer and buffer pool abstraction, I'll port the existing renderers to it so that we can achieve zero-copy support.

@garbear garbear changed the title RetroPlayer: Add renderer for Windows, OpenGL and OpenGLES RetroPlayer: Add renderer for Windows, OpenGL, OpenGLES and MMAL Sep 13, 2017

CGUIInfoLabel m_viewModeInfo;

CGUIRenderSettings m_renderSettings;
// Rendering properties
bool m_bHasShaderPreset = false;

This comment has been minimized.

Copy link
@Rechi

Rechi Sep 14, 2017

Member

unused private field

This comment has been minimized.

Copy link
@garbear

garbear Sep 14, 2017

Author Member

Good catch, this snuck its way in from garbear#86

@garbear garbear referenced this pull request Sep 15, 2017

Merged

Add Volume to Game OSD settings #12765

1 of 4 tasks complete
@Rechi

This comment has been minimized.

Copy link
Member

commented Sep 20, 2017

@garbear this needs a rebase

@garbear

This comment has been minimized.

Copy link
Member Author

commented Sep 24, 2017

@MilhouseVH try applying this PR to newclock5 now

@garbear garbear closed this Sep 24, 2017

@garbear garbear reopened this Sep 24, 2017

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2017

Thanks, this now builds for both x86.

For RPi (with newclock5), I had to revert fd269e5 and 940050c before applying this PR on top of newclock5, and this now fails to build: http://sprunge.us/NGjJ

@garbear garbear force-pushed the garbear:rp-renderer branch from f1681f8 to 43f33a8 Sep 24, 2017

@garbear

This comment has been minimized.

Copy link
Member Author

commented Sep 24, 2017

I applied this PR to newclock5. There was only 1 line that needed to be changed to fix the build breakage you just got. I've pushed the rebased PR to: https://github.com/garbear/xbmc/commits/rp-renderer-pi. Shall I open a PR against newclock5, or can you just cherry-pick the commits?

@garbear garbear force-pushed the garbear:rp-renderer branch from 43f33a8 to f1681f8 Sep 24, 2017

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2017

I'm not sure you need to open a PR against newclock5 as newclock5 already contains the changes you have in this PR (1, 2), hence the two conflicts when applying this PR on top of newclock5.

I'm locally reverting the two commits from this PR that I mentioned previously in order to be able to apply this PR on top of newclock5, and that works for me. Assuming this PR goes in first then @popcornmix can just rebase the newclock5 branch as normal and drop his two commits.

I can cherry-pick garbear@af7aa57 for now. With it, this PR now builds for RPi (on top of newclock5), and x86 also builds so looking good. Not yet run-time tested, but will include in tonight's release if no obvious problems with video playback.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Sep 25, 2017

@garbear legacy OpenGL aka fixed function pipeline does not use shaders. That means that we have to migrate GUI rendering and picture rendering to profile 3.2. VP does already make use of shaders.

@VelocityRa

This comment has been minimized.

Copy link
Member

commented Sep 25, 2017

@FernetMenta

VP does already make use of shaders.

Yeah it does, but AFAIK making use of shaders doesn't mean that it's not fixed function. The OpenGL renderer (CLinuxRendererGL) is still fixed function right? [1], [2], etc. GLES isn't of course since it's not even available there.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Sep 25, 2017

@VelocityRa you are right, there are some parts still legacy.
I am working on migration: https://github.com/FernetMenta/kodi-agile/commit/5a7d4d1a0a5164667c90a5581a7d40aa1faea54e
Then I will bump (OpenGL) shaders to >= 1.5

@garbear

This comment has been minimized.

Copy link
Member Author

commented Sep 27, 2017

I fixed the GLES renderer on the RPi. MMAL isn't ready yet, so it's been disabled. This PR is ready to go in now. @FernetMenta can you sign off on the two guilib changes?

@garbear garbear force-pushed the garbear:rp-renderer branch 2 times, most recently from 7385696 to 4c2bcda Sep 27, 2017

@@ -91,7 +91,7 @@ class CBaseTexture

virtual void CreateTextureObject() = 0;
virtual void DestroyTextureObject() = 0;
virtual void LoadToGPU() = 0;
virtual void LoadToGPU(bool bFreeSysMem = true) = 0;

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Sep 27, 2017

Member

I don't like this default parameter hacks. From a design POV those have zero value. You only do this to avoid touching more code. Whenever I hit one of those hacks, I remove the default.

Why don't you add a new method for your purpose and factor out the common code into a separate protected method. Don't bother the rest of the app with a paramter is does not use.

This comment has been minimized.

Copy link
@garbear

garbear Sep 27, 2017

Author Member

Updated. The new commit adds a new method instead of using a default parameter in LoadToGPU(). How's that look?

garbear added some commits Aug 22, 2017

RetroPlayer: Add renderer for MMAL
This renderer is based on a combination of the old RetroPlayer code and VP's
renderer.

Incomplete and untested, but a good starting point.
Split render system types into new header
This splits several enums and a typedef  into a new header file,
RenderSystemTypes.h. This avoids a dependence on the base rendering system
class if external code only uses the type.
Remove PauseUnpause() function
This better encapsulates RetroPlayer quirks.
Pause game asynchronously
This allows for a single frame to reach the RenderManager after pausing.

@garbear garbear force-pushed the garbear:rp-renderer branch from 9fb157e to e00cb97 Sep 28, 2017

@garbear

This comment has been minimized.

Copy link
Member Author

commented Sep 28, 2017

@FernetMenta sign off on the updated guilib commit?

@garbear

This comment has been minimized.

Copy link
Member Author

commented Sep 28, 2017

jenkins build this please

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Sep 28, 2017

@garbear much better. thanks

@garbear

This comment has been minimized.

Copy link
Member Author

commented Sep 28, 2017

win32 error unrelated

@garbear garbear merged commit f0ec8aa into xbmc:master Sep 28, 2017

1 check failed

default Sorry, building this PR failed. Please check the logs for the errors.
Details

@garbear garbear deleted the garbear:rp-renderer branch Sep 28, 2017

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.