[Fix] Store subtitle settings only on user action #4981

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
7 participants
Member

ace20022 commented Jul 3, 2014

This is a follow-up on PR #4921.
Video settings are now stored only on user actions.

Thanks @Voyager1 for your help!

Member

ace20022 commented Jul 3, 2014

@elupus could you please have a look?

@elupus elupus commented on the diff Jul 4, 2014

xbmc/cores/dvdplayer/DVDPlayer.cpp
@@ -3295,6 +3289,7 @@ int CDVDPlayer::OnDVDNavResult(void* pData, int iMessage)
bool visible = !(iStream & 0x80);
SetSubtitleVisibleInternal(visible);
+ CMediaSettings::Get().GetCurrentVideoSettings().m_SubtitleOn = visible;
@elupus

elupus Jul 4, 2014

Member

This likely should only be done when not in a menu for dvd's. Otherwise user settings is going to end up being toggled on/off by entering dvd menu.

@ace20022

ace20022 Jul 4, 2014

Member

Exactly, and it this case (outside menu) g_application.m_pPlayer is responsible for this.

@Voyager1

Voyager1 Jul 4, 2014

Member

not exactly. It's a catch-22 situation, because the Navigator will change the subs visibility "from behind", and you can't detect that at the g_application.m_pPlayer level. As I explained it creates a difference between the SETTING_SUBTITLE_ENABLE (which is correctly set to false after turning off the sub via the dvd menu), and the videoSettings.m_SubtitleOn value. As soon as entering CGUIDialogAudioSubtitleSettings::FrameMove() it will try to force it back to the value of videoSettings.m_SubtitleOn.

The solution can be to make sure that videoSettings.m_SubtitleOn is also correct as I suggested in this commit.

Unfortunately, you need to use OnDVDNavResult for this.
Good news, no need to do this all the time. I have tested @elupus ' suggestion, i.e. only do this when outside of menu's to avoid incorrect setting changes.

        if (!pStream->IsInMenu())
          CMediaSettings::Get().GetCurrentVideoSettings().m_SubtitleOn = visible;

This works fine too.

@Voyager1

Voyager1 Jul 4, 2014

Member

or else... CGUIDialogAudioSubtitleSettings::FrameMove() should not force the value to be taken from videoSettings.m_SubtitleOn, but rather from g_application.m_pPlayer->GetSubtitleVisible(). The risk is only that visibility as set through the DVD menu never gets saved.

@Voyager1

Voyager1 Jul 4, 2014

Member

update: indeed changing the FrameMove() code to
m_settingsManager->SetBool(SETTING_SUBTITLE_ENABLE, g_application.m_pPlayer->GetSubtitleVisible()); solves the issue as well, but as I suspected, a change of setting through the dvd menu does not get saved, and as a result of that, stopping and resuming a movie is inconsistent in subtitle visibility.

@ace20022

ace20022 Jul 5, 2014

Member

update: indeed changing the FrameMove() code to
m_settingsManager->SetBool(SETTING_SUBTITLE_ENABLE, g_application.m_pPlayer->GetSubtitleVisible()); solves the issue as well, but as I suspected, a change of setting through the dvd menu does not get saved, and as a result of that, stopping and resuming a movie is inconsistent in subtitle visibility.

Thanks for the hint, this issue applies for all files. What if we set the user setting when stopping a video instead of storing it in g_application? @elupus?

@ace20022

ace20022 Jul 5, 2014

Member

But that would save the auto selection settings (without user action), hmm...

@Voyager1

Voyager1 Jul 5, 2014

Member

you're right... It seems that for DVDs, once the setting is saved, it always needs to be saved so that we have consistency between the saved setting and the state of the navigator which is also saved. Otherwise, if we have not saved it, auto-selection and the state of the dvd navigator should bring back the same selection upon resume.
Alternatively, we could NOT use the subtitle setting upon resume of a DVD, let the navigator decide. That would also resolve the inconsistency...

@Montellese What do you mean by that TODO comment?

See the discussion in http://trac.xbmc.org/ticket/15210 and #4734. The original code (before my settings dialog refactor) didn't handle this and I thought it could easily be handled but then someone reported that it actually made things worse so in the end we decided to remove it again but I left a comment so that we know that something is missing.

Owner

ace20022 replied Jul 6, 2014

Thanks! I somehow missed that pr completely. If I understood it right, this change here falls into the same category, right?

Yup.

Member

ace20022 commented Jul 6, 2014

So this pr is a total flop...

Member

Voyager1 commented Jul 6, 2014

In that case, leave the original code in FrameMove and apply the video setting in OnDvdNavResult with a condition !IsInMenu... Looks like its the best interim workaround.

Member

ace20022 commented Jul 6, 2014

The problem persists. Assume you have set subs on (default) and your file does not have your preferred subtitle language, thus subs are off. When you open the settings dialog subs turn from off to on...

Member

Voyager1 commented Jul 7, 2014

The problem persists. Assume you have set subs on (default) and your file does not have your preferred subtitle language, thus subs are off. When you open the settings dialog subs turn from off to on...

this can also be fixed here: https://github.com/ace20022/xbmc/blob/subs_1_pr/xbmc/cores/dvdplayer/DVDPlayer.cpp#L802
when you open the subtitle stream using the Predicate filter, depending on the "valid" flag or "visible" flag you could update the value of CMediaSettings::Get().GetCurrentVideoSettings().m_SubtitleOn accordingly.
I know, it's not ideal, but at least you have full consistency until a better solution for the issue with the CGUIDialogAudioSubtitleSettings::FrameMove gets resolved properly.

Member

ace20022 commented Jul 8, 2014

I don't think that this would help, since there are there are multiple threads involved anyway.

Member

Voyager1 commented Jul 8, 2014

I don't think that this would help, since there are there are multiple threads involved anyway.

Incorrect. The GUIDialogAudioSubtitleSettings is always re-initialized when opening it in InitializeSettings(), with the visibility status from the player here: https://github.com/ace20022/xbmc/blob/9e648d42e8374ca89559d56807144f527e6f9e46/xbmc/video/dialogs/GUIDialogAudioSubtitleSettings.cpp#L357)
This value will be set correctly as soon as you get past https://github.com/ace20022/xbmc/blob/subs_1_pr/xbmc/cores/dvdplayer/DVDPlayer.cpp#L803.
So if you also set the videosetting there, as I suggested, there will be no discrepancy between the setting and the player's subtitle visibility status.

Member

ace20022 commented Jul 8, 2014

I could be wrong, but SetSubtitleVisible (https://github.com/ace20022/xbmc/blob/subs_1_pr/xbmc/cores/dvdplayer/DVDPlayer.cpp#L2734) is also done via a message. If you open the GUIDialogAudioSubtitleSettings and that message wasn't consumed already, m_subtitleVisible will be set to a wrong value.

Member

Voyager1 commented Jul 8, 2014

This is not an issue when done through the dialog, and in DVDPlayer::OpenDefaultStreams also not an issue (since calling SetSubtitleVisibleInternal). The only way we can get the problem you describe is to toggle subs another way e.g. remote, and then REALLY quickly open the dialog, before the message is consumed.

Member

Voyager1 commented Jul 8, 2014

@ace20022 - good news (I hope / think)... I'm onto something here.
The first thing is, forget all the sync'ing between player subtitle visibility and video setting.
Just make sure that upon opening (InitializeSettings) of the GUIDialogAudioSubtitleSettings, that the current status of the player subtitle visibility gets copied to the setting, to make sure it's sync'ed at that point: https://github.com/Voyager1/xbmc/commit/5cd0f1e7dcd98fb37f00667133c8204c9f8a3137
Secondly: make sure that OpenDefaultStreams does NOT set visibility on or off if "resuming" a DVD with a stored navigator state: https://github.com/Voyager1/xbmc/commit/23591a30553dc96fc70ca10f53dec0383caaf28a
This allows the stored visibility from the navigator state to be taken instead.
I tested it and it resolved all issues with visibility, all container types (DVD, mkv).
The only 'downside' is that the video setting does not always get saved (except if you perform a user action or go to the dialog), but that's not an issue. Anyway, the preferred setting is only used as an input to the subtitle predicate, as well as when DVDs are played from the start (ignoring the resume point).

Member

Voyager1 commented Jul 8, 2014

@ace20022 - forgot to mention, with those 2 commits we don't need ace20022/xbmc@2efed6f anymore.

Member

ace20022 commented Jul 9, 2014

Sorry for being a pain in the neck, but the purpose of this pr is to not store the changed settings if they were changed automatically, but only if a user did that. So is this https://github.com/Voyager1/xbmc/commit/5cd0f1e7dcd98fb37f00667133c8204c9f8a3137 really needed (no time to test)?

Member

Voyager1 commented Jul 9, 2014

Sorry for being a pain in the neck, but the purpose of this pr is to not store the changed settings if they were changed automatically, but only if a user did that. So is this Voyager1@5cd0f1e really needed (no time to test)?

I agree with you, and to a certain extent (for the most part) the goal of the PR is met.

The remaining problem is the Dialog. We use the video-setting as well as the actual value for one and the same toggle control. When we open, we show the "actual" value, and then we compare it to the setting and potentially update the actual value based on the setting (triggers OnSettingChanged). Makes no sense. Unless we decouple these we will never be able to solve the issue. The commit you reference is essentially making sure that they are aligned at the time you enter the dialog. If you never use the dialog, the objective of your PR is fully met.

Member

Voyager1 commented Jul 9, 2014

that said, we could compare both values in InitializeSettings() and "remember" it when there's difference. Then display the "actual" value in the toggle. In FrameMove you would see there's a difference, but since you "remember" it was there since the IniitializeSettings(), you wouldn't trigger a value changed. Only upon user action you could "forget" the difference and go back to normal... Thoughts?

Member

FernetMenta commented Jul 9, 2014

why don't you cache subsVisible the same way as audio- and subsstream?

Member

Voyager1 commented Jul 9, 2014

why don't you cache subsVisible the same way as audio- and subsstream?

Can be done, but I don't think it has anything to do with solving the issue we're discussing here (discrepancy between the videosetting and the actual value, which could be changed behind the back)

Member

FernetMenta commented Jul 9, 2014

we are in this mess because settings are used like globals. for audio I have removed all calls to audio settings outside AE. if any other component wants to know something about audio, it has to ask AE: I would do the same with this case here. have a single source of truth and remove calls to settings from all other places.

Member

Voyager1 commented Jul 9, 2014

also true, but mostly solved by this PR. The remaining issue is here that the SettingsDialog is mixing up the setting for the current video with the current state of the player.

Member

FernetMenta commented Jul 9, 2014

also true, but mostly solved by this PR

i don't think so. searching for CMediaSettings::Get().GetCurrentVideoSettings().m_SubtitleOn shows it is accessed by DVDPlayer, PLayerController, ../legace/Player, and GUIInfoManager.

The remaining issue is here that the SettingsDialog is mixing up the setting for the current video with the current state of the player

this goes wrong all over the place because settings are abused for storing state. we have to clearly distinguish between settings ans state. and we won't achieve this by "mostly here, and mostly there". don't leave the little things unfixed.

Member

Voyager1 commented Jul 9, 2014

i don't think so. searching for CMediaSettings::Get().GetCurrentVideoSettings().m_SubtitleOn shows it is accessed by DVDPlayer, PLayerController, ../legace/Player, and GUIInfoManager.

Come on... this is incorrect. With the code of this PR in, there's only one place left (ApplicationPlayer) that accesses the setting in "write" mode. (search for CMediaSettings::Get().GetCurrentVideoSettings().m_SubtitleOn = ). The other areas are legitimately reading the setting (like DVDPlayer upon initialization of the stream).

this goes wrong all over the place because settings are abused for storing state

again this is what this is trying to solve. Please contribute something constructive.

we have to clearly distinguish between settings ans state

and yes this is what we were trying to solve - especially with this Dialog that has a clear issue in this regard.

Let's get to the bottom of this.

Member

ace20022 commented Jul 9, 2014

we are in this mess because settings are used like globals. for audio I have removed all calls to audio settings outside AE.

What does AE mean in this context?

if any other component wants to know something about audio, it has to ask AE: I would do the same with this case here. have a single source of truth and remove calls to settings from all other places.

You left quite a few places untouched that read/write audio/subs settings in the pr that you quote.
This PR is meant to handle subs, a PR for audio would follow.

Member

FernetMenta commented Jul 10, 2014

AE == audio engine. This was just an example. What I was trying to say here is that I would solve this properly, even if this means to bring up the hammer.

Please contribute something constructive.

this is what I was trying to do here. I consider reading settings in various places as wrong and error prone. this is just like having globals.

Given the fact that this issue is hard to fix and complicated shows that there is something conceptually wrong. We have a general issues how we deal with settings. video resolutions is also an example of a big mess.

Member

Voyager1 commented Jul 10, 2014

I consider reading settings in various places as wrong and error prone. this is just like having globals.

I cannot disagree with that. But please take a look at the actual code: m_subtitleOn is used in the following places, taking into account this PR's code:

ApplicationPlayer (read/write): legit
DVDPlayer (read only): legit, because we need to know the setting when starting playback
GUIDialogAudioSubtitleSettings: legit, because that's the main function of this dialog!
MediaSettings: legit (= settings management)
PVRClients: legit (I think)
PVRDatabase, VideoDatabase, VideoSettings: all legit
.. that's it!

Given the fact that this issue is hard to fix and complicated

It's not hard and complicated. As a matter of fact the solution for the dialog is simple and straightforward, it just took a bit of time to understand the "mess".

I just pushed another update to this branch - the top 2 commits fix the setting vs. state inconsistency issues for DVD playback as well as in the Dialog (as a matter of fact there's no need at all to update the setting in FrameMove because it shouldn't really change while the Dialog is up)

Check it out here:
https://github.com/Voyager1/xbmc/commits/subs-pr-fix
https://github.com/Voyager1/xbmc/compare/subs-pr-fix

Member

ace20022 commented Jul 10, 2014

What I was trying to say here is that I would solve this properly, even if this means to bring up the hammer.

Are trying to say that I'm not willing to do it properly? Your comments imply that you know the solution, I would be really happy to see it.

Member

Voyager1 commented Jul 10, 2014

@ace20022 - forget it, this is not constructive. Any feedback on the latest proposal (see https://github.com/Voyager1/xbmc/commits/subs-pr-fix), also dropping ace20022/xbmc@2efed6f?

Member

jmarshallnz commented Jul 10, 2014

Communication FTW :p

The changes proposed in subs-pr-fix look fine to me - certainly moving to the "only one class controls the settings" model, with state no longer being held in settings from what I can tell. I've made a couple of very minor comments regarding comments :)

Member

FernetMenta commented Jul 10, 2014

@ace20022 @Voyager1 please don't get me wrong, I did not want to criticize your work here. I spent a lot of time on the borked video resolutions and promised myself that next time I touch it I will rewrite it. I was thinking this might be a similar case here but I could be wrong. If you feel comfortable with this solution I am fine.

Member

Voyager1 commented Jul 11, 2014

@ace20022 - subs-pr-fix branch updated per @jmarshallnz comments.

Member

ace20022 commented Jul 11, 2014

@Voyager1 thanks! I have one more solution in my mind that we could discuss, but I'm overloaded with work till the end of July. I'll try to find some time, nevertheless...

Member

jmarshallnz commented Jul 11, 2014

Perhaps @Voyager1 could put his branch up as a PR (for build testing) so that we can get this in in the meantime then?

Member

Voyager1 commented Jul 11, 2014

ok - PR stood up, but jenkins is not collaborating :-(

ace20022 closed this Jul 11, 2014

@ace20022 ace20022 added a commit that referenced this pull request Aug 3, 2014

@ace20022 ace20022 Merge pull request #5003 from Voyager1/subs-pr-fix
Store subtitle settings only on user action - replacing #4981
2f96b34

ace20022 deleted the ace20022:subs_1_pr branch Aug 10, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment