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

ActiveAE: Enable display lost callbacks for EGL #8995

Merged
merged 2 commits into from Jun 12, 2016

Conversation

@popcornmix
Copy link
Member

popcornmix commented Jan 30, 2016

This adds back in support for "Delay after change of refresh rate"
which was lost in the VideoPlayer rework.

@popcornmix

This comment has been minimized.

Copy link
Member Author

popcornmix commented Jan 30, 2016

@@ -291,7 +291,7 @@ CActiveAE::~CActiveAE()

void CActiveAE::Dispose()
{
#if defined(HAS_GLX) || defined(TARGET_DARWIN)
#if defined(HAS_GLX) || defined(HAS_EGL) || defined(TARGET_DARWIN)

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Jan 30, 2016

Member

the entire ifdeffery can be dropped. Windowing has to support it

@FernetMenta

This comment has been minimized.

Copy link
Member

FernetMenta commented Jan 30, 2016

+1

@popcornmix

This comment has been minimized.

Copy link
Member Author

popcornmix commented Jan 30, 2016

Removed ifdefs.
jenkins build this please

@FernetMenta

This comment has been minimized.

Copy link
Member

FernetMenta commented Jan 30, 2016

Removed ifdefs.

forgot to push?

@popcornmix popcornmix force-pushed the popcornmix:delayrefresh branch from de117b8 to 64881ae Jan 30, 2016
@popcornmix

This comment has been minimized.

Copy link
Member Author

popcornmix commented Jan 30, 2016

Yes. Pushed now.

@popcornmix

This comment has been minimized.

Copy link
Member Author

popcornmix commented Jan 30, 2016

jenkins build this please

@popcornmix

This comment has been minimized.

Copy link
Member Author

popcornmix commented Jan 30, 2016

Seems this causes a hang in some circumstances so don't merge yet.

@popcornmix

This comment has been minimized.

Copy link
Member Author

popcornmix commented Jan 31, 2016

Added a fix for the hang discovered.
jenkins build this please

@@ -1294,6 +1294,7 @@ void CVideoPlayer::Process()
CSingleLock lock(m_StateSection);
if (m_displayLost)
{
CSingleExit unlock(m_StateSection);

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Jan 31, 2016

Member

CSingleExit makes no sense here. Why did you ignore my advice to drop the lock completely?

This comment has been minimized.

Copy link
@popcornmix

popcornmix Jan 31, 2016

Author Member

Not sure why it makes no sense. Seems a valid fix that is done similarly in many places in the code.
But I can switch to using atomics for m_displayLost if that is desired.

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Jan 31, 2016

Member

You would use CSingleExit if you want to exit a lock and grab it again. This is not the case here. It would grab the lock again and on the next line the outer lock gets destructed anyway.

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Jan 31, 2016

Member

look at the entire block:


    // check display lost
    {
      CSingleLock lock(m_StateSection);
      if (m_displayLost)
      {
        CSingleExit unlock(m_StateSection);
        Sleep(50);
        continue;
      }
    }
@popcornmix

This comment has been minimized.

Copy link
Member Author

popcornmix commented Feb 7, 2016

Updated the PR to pause/resume video playback when display is lost. This works a lot better for me, especially when "Delay after change of refresh rate" is used.
@FernetMenta ?

@FernetMenta

This comment has been minimized.

Copy link
Member

FernetMenta commented Feb 7, 2016

I already mentioned that the entire WinSystemEGL will disappear. It would be great if you could start factoring out Pi from there. EGL is not responsible for creating or destroying windows, it is just the glue code to the GL context.

@popcornmix popcornmix force-pushed the popcornmix:delayrefresh branch from fd7283c to 0908f5e Jun 11, 2016
@popcornmix

This comment has been minimized.

Copy link
Member Author

popcornmix commented Jun 11, 2016

@FernetMenta
This is updated to include the fix for stalling when video is rendered to a separate layer.

@FernetMenta

This comment has been minimized.

Copy link
Member

FernetMenta commented Jun 11, 2016

jenkins build this please

@FernetMenta

This comment has been minimized.

Copy link
Member

FernetMenta commented Jun 12, 2016

@popcornmix build errors on Linux and Windows

popcornmix added 2 commits Jan 30, 2016
This adds back in support for "Delay after change of refresh rate"
which was lost in the VideoPlayer rework.
This allows delay after refresh rate change to continue when video is playing with no gui overlay
@popcornmix popcornmix force-pushed the popcornmix:delayrefresh branch from 0908f5e to 9104b4b Jun 12, 2016
@popcornmix

This comment has been minimized.

Copy link
Member Author

popcornmix commented Jun 12, 2016

jenkins build this please

@popcornmix

This comment has been minimized.

Copy link
Member Author

popcornmix commented Jun 12, 2016

@FernetMenta jenkins is happy, okay to merge?

@FernetMenta FernetMenta merged commit 6ff3a12 into xbmc:master Jun 12, 2016
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
default Build finished.
Details
@popcornmix popcornmix deleted the popcornmix:delayrefresh branch Jun 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.