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 basic resolution changing #98

Merged
merged 2 commits into from Jun 15, 2018

Conversation

@GTechAlpha
Copy link

GTechAlpha commented Jun 12, 2018

Description

Add basic resolution changing.

Motivation and Context

RetroPlayer does not render after resolution change.

How Has This Been Tested?

PCSX, GenesisPlus, Nestopia. Example: FFVII on PCSX changes resolution every time the in-game menu is accessed.

Screenshots (if appropriate):

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)
@GTechAlpha GTechAlpha changed the title [RetroPlayer] add basic resolution changing [RetroPlayer] Add basic resolution changing Jun 12, 2018
@garbear garbear force-pushed the garbear:retroplayer-18alpha2 branch 2 times, most recently from 6a15d41 to 2908c0d Jun 12, 2018
@@ -177,7 +183,10 @@ void CRPRenderManager::FrameMove()

if (m_state == RENDER_STATE::CONFIGURING)
{
UpdateResolution();

This comment has been minimized.

Copy link
@garbear

garbear Jun 12, 2018

Owner

Currently this function is dead code. Its purpose is borrowed from VideoPlayer for adjusting the screen's refresh rate to match the video. Is this a setting that makes sense for games?

@@ -140,20 +150,16 @@ void CRPRenderManager::AddFrame(const uint8_t* data, size_t size, unsigned int w
{
std::vector<uint8_t> cachedFrame = std::move(m_cachedFrame);

if (!m_bHasCachedFrame)

This comment has been minimized.

Copy link
@garbear

garbear Jun 12, 2018

Owner

removing this check skips an important operation that elides a memory copy. is there a reason for this change? if unrelated to resolution changing it should go in a separate commit.

This comment has been minimized.

Copy link
@GTechAlpha

GTechAlpha Jun 13, 2018

Author

If IIRC, there was an issue with the frame caching after resolution change. Since pausing is asynchronous, the cached frame could be updated after pause? The memcopy will only occur if the emulator sends a frame during pause so there should be no harm.

This comment has been minimized.

Copy link
@garbear

garbear Jun 13, 2018

Owner

I forgot that this code is only hit on pause. Change is OK.

CSingleLock lock(m_bufferMutex);

if (!HasRenderBuffer(bufferPool) && m_bHasCachedFrame)
{
std::vector<uint8_t> cachedFrame = std::move(m_cachedFrame);
if (!cachedFrame.empty())

This comment has been minimized.

Copy link
@garbear

garbear Jun 12, 2018

Owner

removing this check allows for garbage data to be passed to CopyFrame().

This comment has been minimized.

Copy link
@GTechAlpha

GTechAlpha Jun 13, 2018

Author

This is a duplicate check. If m_bHasCachedFrame is true, then there is a frame.

This comment has been minimized.

Copy link
@garbear

garbear Jun 13, 2018

Owner

If the check was duplicate, then we could drop m_bHasCachedFrame. When writing this I discovered an edge case that caused me to store the cached frame's state externally in the bool. I made the mistake of not documenting why, but I don't think it's 100% safe to drop the second check.

This comment has been minimized.

Copy link
@garbear

garbear Jun 13, 2018

Owner

I remember! Instead of explaining here I'll add documentation to the source.

This comment has been minimized.

Copy link
@GTechAlpha

GTechAlpha Jun 13, 2018

Author

Re-added

@@ -175,6 +177,7 @@ namespace RETRO
bool m_bHasCachedFrame = false;
std::set<std::string> m_failedShaderPresets;
bool m_bTriggerUpdateResolution = false;
std::atomic<bool> m_bFlush;

This comment has been minimized.

Copy link
@garbear

garbear Jun 12, 2018

Owner

If my c++11 is correct, this can be initialized in the header like std::atomic<bool> m_bFlush{};

@@ -73,16 +73,20 @@ bool CBaseRenderBufferPool::Configure(AVPixelFormat format, unsigned int width,

IRenderBuffer *CBaseRenderBufferPool::GetBuffer(size_t size)
{
if (!m_bConfigured)
if (!m_bConfigured) {
CLog::Log(LOGDEBUG, "RetroPlayer[RENDER]: buffer pool is not configured");

This comment has been minimized.

Copy link
@garbear

garbear Jun 12, 2018

Owner

Please place unrelated changes in a separate commit


// If frame size is unknown, set it now
if (m_frameSize == 0)
m_frameSize = size;

// Changing sizes is not implemented
if (m_frameSize != size)
if (m_frameSize != size) {

This comment has been minimized.

Copy link
@garbear

garbear Jun 12, 2018

Owner

Coding style is { on new line. This has the added benefit of lowering the number of deleted lines in a pull request (an important measure)

@garbear

This comment has been minimized.

Copy link
Owner

garbear commented Jun 12, 2018

In general this is a good approach. I only found one flaw, which I sent you in https://github.com/GTechAlpha/xbmc/pull/1. Once this gets cleaned up it's good to go.

@garbear garbear force-pushed the garbear:retroplayer-18alpha2 branch from 2908c0d to 2cc5ded Jun 12, 2018
CLog::Log(LOGDEBUG, "RetroPlayer[RENDER]: Configuring renderer(s)");

for (auto &renderer : m_renderers)
renderer->Configure(m_format, m_width, m_height);

This comment has been minimized.

Copy link
@garbear

garbear Jun 13, 2018

Owner

Renderers are created on demand, and configured on creation, so this shouldn't be necessary. Is m_renderers even nonempty at this point?

This comment has been minimized.

Copy link
@GTechAlpha

GTechAlpha Jun 13, 2018

Author

This is critical. It is for re-configuring existing renderers on resolution change. I will add a comment to clarify.

This comment has been minimized.

Copy link
@garbear

garbear Jun 13, 2018

Owner

I see this now. Thanks.

@garbear

This comment has been minimized.

Copy link
Owner

garbear commented Jun 13, 2018

@garbear

This comment has been minimized.

Copy link
Owner

garbear commented Jun 13, 2018

I see one more change that's needed:

std::map<AVPixelFormat, SwsContext*> m_scalers;

m_scalers needs to be indexed by format, width and height instead of just format.

@GTechAlpha

This comment has been minimized.

Copy link
Author

GTechAlpha commented Jun 13, 2018

This should be good to go, AFAIK.

@garbear

This comment has been minimized.

Copy link
Owner

garbear commented Jun 13, 2018

What about this? we could split configuration and reconfiguration: rp-vide

Switch to Fullscreen msg should only be sent once. I imagine your amlogic patch doesn't need to be called multiple times either. "re"configuration of the renderers, by definition, doesn't need to happen in the first configuration. So having separate configuration and reconfiguration states makes sense.

@garbear

This comment has been minimized.

Copy link
Owner

garbear commented Jun 14, 2018

Also this: rp-buff

Buffers need to be protected by the buffer mutex.

@GTechAlpha

This comment has been minimized.

Copy link
Author

GTechAlpha commented Jun 14, 2018

The buffer is protected by m_bFlush plus this being on the render thread.

@garbear

This comment has been minimized.

Copy link
Owner

garbear commented Jun 14, 2018

The game thread can access the buffers.

Also I looked into the m_scalers comment above and no changes are needed for this.

@garbear

This comment has been minimized.

Copy link
Owner

garbear commented Jun 14, 2018

I see that m_bFlush protects the buffers. Still, should we be defensive?

@GTechAlpha

This comment has been minimized.

Copy link
Author

GTechAlpha commented Jun 14, 2018

It is inefficient to lock when unnecessary though right? Maybe a comment?

@garbear

This comment has been minimized.

Copy link
Owner

garbear commented Jun 14, 2018

Comment is fine. When I see buffer access without a mutex I get worried, so explaining why would be good.

Also, thoughts on #98 (comment)?

@GTechAlpha

This comment has been minimized.

Copy link
Author

GTechAlpha commented Jun 14, 2018

What about this? we could split configuration and reconfiguration: rp-vide

Switch to Fullscreen msg should only be sent once. I imagine your amlogic patch doesn't need to be called multiple times either. "re"configuration of the renderers, by definition, doesn't need to happen in the first configuration. So having separate configuration and reconfiguration states makes sense.

Yes, the Amlogic code only needs to be called once. I think would be a good addition.

@garbear

This comment has been minimized.

Copy link
Owner

garbear commented Jun 14, 2018

K, Configure() needs a way to know which state to set

@GTechAlpha

This comment has been minimized.

Copy link
Author

GTechAlpha commented Jun 14, 2018

I'll add when I get a chance.

@GTechAlpha

This comment has been minimized.

Copy link
Author

GTechAlpha commented Jun 14, 2018

Anything else on this one?

@garbear

This comment has been minimized.

Copy link
Owner

garbear commented Jun 14, 2018

Almost :) In the buffer lock commit, m_bHasCachedFrame and m_cachedFrame should be reset within the lock. Also I'll send you a PR with a reconfig tweak that I think improves the code.

@garbear

This comment has been minimized.

@garbear

This comment has been minimized.

Copy link
Owner

garbear commented Jun 14, 2018

squash into two commits and i'll merge!

@GTechAlpha

This comment has been minimized.

Copy link
Author

GTechAlpha commented Jun 14, 2018

👍

@garbear garbear merged commit f904ad8 into garbear:retroplayer-18alpha2 Jun 15, 2018
@GTechAlpha GTechAlpha deleted the GTechAlpha:gb_pr_video_resolution_18a2 branch Jun 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.