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

fix stereoscopic playback #13395

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
4 participants
@afl1
Copy link
Contributor

commented Jan 19, 2018

After #12799 Stereoscopic manager is unable set stereoscopic mode. Stereoscopic manager is informed by GUI_MSG_PLAYBACK_STARTED message at opening stream. Attempt to set 3D mode fails as the Amlogic decoder is open after demuxing first video frame.

Description

At open amlogic decoder DVDVideoCodecAmlogic send GUI_MSG_PLAYBACK_STARTED to ask Stereoscpic manager for action in right moment.

Motivation and Context

It's most likely broken 3D mode for other devices. This PR fixed 3D mode only for Amlogic HW decoding.

How Has This Been Tested?

It was tested in Amlogic S905/S912 community builds.

@da-anda

This comment has been minimized.

Copy link
Member

commented Jan 20, 2018

I'm sorry, but this is not the correct fix. I'm aware of the issue but didn't have the time yet to implement the proper fix (haven't done anything C++ in ages and also don't have a dev env atm). The correct way IMO would be to rename the "onPlaybackStarted" method of StereoscopicsManager to "onStreamChange", call it from https://github.com/xbmc/xbmc/blob/master/xbmc/Application.cpp#L3426 and unsubscribe from "GUI_MSG_PLAYBACK_STARTED" signal. This should ensure that the according logic is triggered when all required info from the decoders is available. Could you give this a try please?

@afl1 afl1 force-pushed the afl1:3Dmanager branch from 090990e to 7cb7992 Jan 20, 2018

@afl1

This comment has been minimized.

Copy link
Contributor Author

commented Jan 20, 2018

I rebuild fix under @da-anda recommendations. It looks like it fixed stereoscopic playback commonly for all platforms.

@da-anda
Copy link
Member

left a comment

apart from the minor, looks good from my end. Thanks.

@@ -395,7 +395,6 @@ bool CStereoscopicsManager::OnMessage(CGUIMessage &message)
switch (message.GetMessage())
{
case GUI_MSG_PLAYBACK_STARTED:
OnPlaybackStarted();

This comment has been minimized.

Copy link
@da-anda

da-anda Jan 21, 2018

Member

mind removing that "case" altogether? Doesn't make much sense to keep it

@afl1 afl1 force-pushed the afl1:3Dmanager branch from 7cb7992 to 7d21da4 Jan 21, 2018

@afl1

This comment has been minimized.

Copy link
Contributor Author

commented Jan 21, 2018

I replaced case with if.

@da-anda da-anda changed the title DVDVideoCodecAmlogic: fix stereoscopic playback fix stereoscopic playback Jan 21, 2018

@da-anda

This comment has been minimized.

Copy link
Member

commented Jan 21, 2018

thanks

@FernetMenta do you see any possible deadlock/threading issues with current approach, or should the StereoscopicsManager be triggered via AppMessenger (or whatever fits for this purpose)? IIRC StereoscopicsManager needs to be triggered from GUI thread, but I could be wrong. It's interacting with:

  • GUI, as it can show a select dialog to pick the desired playback mode
  • calls CServiceBroker::GetRenderSystem().SupportsStereo()
  • calls g_graphicsContext.GetStereoMode(); and g_graphicsContext.SetStereoMode();
  • calls g_application.GetAppPlayer().GetVideoStreamInfo(CURRENT_STREAM, videoInfo); to detect current stereo mode of playing video

thanks.

break;
case GUI_MSG_PLAYBACK_STOPPED:
case GUI_MSG_PLAYLISTPLAYER_STOPPED:
if ((message.GetMessage() == GUI_MSG_PLAYBACK_STOPPED) || (message.GetMessage() == GUI_MSG_PLAYLISTPLAYER_STOPPED))

This comment has been minimized.

Copy link
@Rechi

Rechi Jan 21, 2018

Member

no need for the extra brackets around the two equality checks

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Jan 21, 2018

@da-anda I see no reason why this should run on the gui thread. btw: calling g_application.GetAppPlayer().GetVideoStreamInfo is wrong. see my pr that introduced the new callback.

@da-anda

This comment has been minimized.

Copy link
Member

commented Jan 21, 2018

@FernetMenta ok, thanks. I only recall threading issues when I implemented builtin actions for 3D stuff. That's why I'm more than cautios now in this regard. And since I'm not a C++ dev I better ask :)

if g_application.GetAppPlayer().GetVideoStreamInfo() is still working, I'd delay the proper fix for this to a followup PR, unless @afl1 is willing to also address this

@afl1 afl1 force-pushed the afl1:3Dmanager branch from 7d21da4 to ec0edb7 Jan 21, 2018

@da-anda

This comment has been minimized.

Copy link
Member

commented Jan 21, 2018

@afl1 in case you'd like to address the GetVideoStream issue Fernet mentioned, I think you can replace it with CServiceBroker::GetDataCacheCore().GetVideoStereoMode(). Might need some additional adjustments (like incluing the CServiceBroker header). Since I don't have a dev env atm, I can only give pointers.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Jan 21, 2018

if g_application.GetAppPlayer().GetVideoStreamInfo() is still working,

it may work by chance and is not reliable. GetVideoStreamInfo provides info for user dialog stream selection. this is not updated at any time later

@afl1 afl1 closed this Jan 22, 2018

@afl1 afl1 deleted the afl1:3Dmanager branch Jan 22, 2018

@da-anda

This comment has been minimized.

Copy link
Member

commented Jan 23, 2018

@afl1 may I ask why you closed the PR? Is it because of me asking if you'd like to also address the change Fernet mentioned? If it wasn't clear, you didn't have to do it. I just asked if you also wanted to give it a go. If you said no, I would have merged your PR as is.

Are you ok with reopening and merging this PR as is?

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.