Sub fixes #4176

Merged
merged 4 commits into from Feb 9, 2014

Conversation

Projects
None yet
8 participants
@arnova

This comment has been minimized.

Show comment
Hide comment
@arnova

arnova Feb 8, 2014

Member

@elupus: Note that the auto-enabling not working, is a regression from previous behaviour as this used to work fine until the previous round of subtitle-changes went in.

Member

arnova commented Feb 8, 2014

@elupus: Note that the auto-enabling not working, is a regression from previous behaviour as this used to work fine until the previous round of subtitle-changes went in.

@elupus

This comment has been minimized.

Show comment
Hide comment
@elupus

elupus Feb 8, 2014

Member

Well. I doubt it's possible to solve without breaking something else.

We are miss-using video settings for current status.

Either we:

  • update gui with what the current status of the player is - > saved for
    next play.
  • we don't update gui, so we can re-calc next time we open.

With current impl. those are our choices. :(

Member

elupus commented Feb 8, 2014

Well. I doubt it's possible to solve without breaking something else.

We are miss-using video settings for current status.

Either we:

  • update gui with what the current status of the player is - > saved for
    next play.
  • we don't update gui, so we can re-calc next time we open.

With current impl. those are our choices. :(

@ace20022

This comment has been minimized.

Show comment
Hide comment
@ace20022

ace20022 Feb 8, 2014

Member

Taken from #4116

again.. you are missing to ALWAYS call SetSubtitleVisibleInternal. Now it's only called on a successful open. Can you please tell me what it is i'm missing to explain?

The call to SetSubtitleVisibleInternal should be after the if(!valid) check with parameters depending on what we decided above.

Thanks for the friendly reply.

You didn't reply for a week, but if I don't reply within some hours you open a pr that replaces mine?

Taken from your pr description:

It does not fix auto enable of added external subtitle after first play.

My pr did.

We must avoid all modifications of video player settings if no user action has been taken to support that.

My pr avoided that, yours does not! This can be seen here:

  if(!valid)
  {
    CloseSubtitleStream(true);
    visible = false;
  }
  SetSubtitleVisibleInternal(visible);

If a user has enables subs by default and the video has no subtitle stream, you will store false in the db. That is the reason why I only called it after a successful open(). And wrote that several times. This also fixes @arnova's issue.

Member

ace20022 commented Feb 8, 2014

Taken from #4116

again.. you are missing to ALWAYS call SetSubtitleVisibleInternal. Now it's only called on a successful open. Can you please tell me what it is i'm missing to explain?

The call to SetSubtitleVisibleInternal should be after the if(!valid) check with parameters depending on what we decided above.

Thanks for the friendly reply.

You didn't reply for a week, but if I don't reply within some hours you open a pr that replaces mine?

Taken from your pr description:

It does not fix auto enable of added external subtitle after first play.

My pr did.

We must avoid all modifications of video player settings if no user action has been taken to support that.

My pr avoided that, yours does not! This can be seen here:

  if(!valid)
  {
    CloseSubtitleStream(true);
    visible = false;
  }
  SetSubtitleVisibleInternal(visible);

If a user has enables subs by default and the video has no subtitle stream, you will store false in the db. That is the reason why I only called it after a successful open(). And wrote that several times. This also fixes @arnova's issue.

@elupus

This comment has been minimized.

Show comment
Hide comment
@elupus

elupus Feb 8, 2014

Member

I really did not intend to offend you by opening a new pull request. Sorry!

I just ended up working on it since you had ignored my comments on always
calling the Set..Internal, and gave no explanation as to why.

Like I said above. It's a tradeof between two issues. Either gui represent
player status or we support the case of external subs added later auto
enabling.

You solution caused gui to show enabled with no subs at all, to solve the
added later feature. IMHO that is not better.

Again I'm really sorry if I offended you.

Member

elupus commented Feb 8, 2014

I really did not intend to offend you by opening a new pull request. Sorry!

I just ended up working on it since you had ignored my comments on always
calling the Set..Internal, and gave no explanation as to why.

Like I said above. It's a tradeof between two issues. Either gui represent
player status or we support the case of external subs added later auto
enabling.

You solution caused gui to show enabled with no subs at all, to solve the
added later feature. IMHO that is not better.

Again I'm really sorry if I offended you.

@elupus

This comment has been minimized.

Show comment
Hide comment
@elupus

elupus Feb 8, 2014

Member

Also note. that what you proposed would be inconsistent. Take the situation of one internal forced sub, prefer external subs on:

On first play this will get enabled and set visible. It will be stored in video settings.
On second play, external subs have been added, but will not be selected due to above.

If external subs was there on first play they would have been selected.

Member

elupus commented Feb 8, 2014

Also note. that what you proposed would be inconsistent. Take the situation of one internal forced sub, prefer external subs on:

On first play this will get enabled and set visible. It will be stored in video settings.
On second play, external subs have been added, but will not be selected due to above.

If external subs was there on first play they would have been selected.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Feb 8, 2014

Auto enabling external subtitles have been working in xbmc since I can remember (definitely back to xbox days). The only difference now is that users have an option to prefer them or not. Previously, before ace2002 redid subtitle preference logic, external subtitles were preferred and enabled automatically. Not auto enabling them is definitely new and hopefully unexpected behaviour (when prefer external subtitles: on). If user sets Prefer external subtitles to on then external subtitles should be always preferred (displayed) no matter when they were actually added.

ghost commented Feb 8, 2014

Auto enabling external subtitles have been working in xbmc since I can remember (definitely back to xbox days). The only difference now is that users have an option to prefer them or not. Previously, before ace2002 redid subtitle preference logic, external subtitles were preferred and enabled automatically. Not auto enabling them is definitely new and hopefully unexpected behaviour (when prefer external subtitles: on). If user sets Prefer external subtitles to on then external subtitles should be always preferred (displayed) no matter when they were actually added.

@elupus

This comment has been minimized.

Show comment
Hide comment
@elupus

elupus Feb 9, 2014

Member

External subs will auto enable with this... It's only if you play the video
without any external subs, then added later that is the problem.

Note above used to work too. But can't be fixed without reverting bunch if
gui code.

Member

elupus commented Feb 9, 2014

External subs will auto enable with this... It's only if you play the video
without any external subs, then added later that is the problem.

Note above used to work too. But can't be fixed without reverting bunch if
gui code.

@ace20022

This comment has been minimized.

Show comment
Hide comment
@ace20022

ace20022 Feb 9, 2014

Member

I just ended up working on it since you had ignored my comments on always calling the Set..Internal, and gave no explanation as to why.

Of course I did, even several times. Last time (taken from #4116 (comment)) :

The call of SetSubtitleVisibleInternal() is at the end/latest possible place. Because if we don't have valid subs, e.g., the file has no subs at all, we decided to not alter the stored db value of subs on/off.

And that based on YOUR comment #4116 (comment) :

@elupus
If a user plays a video and doesn't modify any video settings manually, nothing should be stored.

I replied:

Then the whole auto selection/disabling of subs can't work.
With the current version of this pr we don't alter the settings in case no (valid) subtitles is available. But this has the effect of activated subs while there's no sub at all.
subs

Follwed by your comment #4116 (comment) :

Well that would be due to using video settings to display current state of subtitles instead of the players state. Nothing i think we can solve for gotham.

But i don't think it's so bad that it shows as enabled even if there are none.

And here you wrote the contrary:

Like I said above. It's a tradeof between two issues. Either gui represent player status or we support the case of external subs added later auto enabling. You solution caused gui to show enabled with no subs at all, to solve the added later feature. IMHO that is not better.

You did not intend to offend me. Okay, I want to believe that, although it is quite hard give the facts I presented above.

Member

ace20022 commented Feb 9, 2014

I just ended up working on it since you had ignored my comments on always calling the Set..Internal, and gave no explanation as to why.

Of course I did, even several times. Last time (taken from #4116 (comment)) :

The call of SetSubtitleVisibleInternal() is at the end/latest possible place. Because if we don't have valid subs, e.g., the file has no subs at all, we decided to not alter the stored db value of subs on/off.

And that based on YOUR comment #4116 (comment) :

@elupus
If a user plays a video and doesn't modify any video settings manually, nothing should be stored.

I replied:

Then the whole auto selection/disabling of subs can't work.
With the current version of this pr we don't alter the settings in case no (valid) subtitles is available. But this has the effect of activated subs while there's no sub at all.
subs

Follwed by your comment #4116 (comment) :

Well that would be due to using video settings to display current state of subtitles instead of the players state. Nothing i think we can solve for gotham.

But i don't think it's so bad that it shows as enabled even if there are none.

And here you wrote the contrary:

Like I said above. It's a tradeof between two issues. Either gui represent player status or we support the case of external subs added later auto enabling. You solution caused gui to show enabled with no subs at all, to solve the added later feature. IMHO that is not better.

You did not intend to offend me. Okay, I want to believe that, although it is quite hard give the facts I presented above.

@ace20022

This comment has been minimized.

Show comment
Hide comment
@ace20022

ace20022 Feb 9, 2014

Member

External subs will auto enable with this... It's only if you play the video without any external subs, then added later that is the problem. Note above used to work too. But can't be fixed without reverting bunch if gui code.

That must be a long time ago the following is from the file dated back to 06.05.2012:

static bool PredicateSubtitlePriority(const SelectionStream& lh, const SelectionStream& rh)

static bool PredicateSubtitlePriority(const SelectionStream& lh, const SelectionStream& rh)
{
  if(!g_settings.m_currentVideoSettings.m_SubtitleOn)
  {
    PREDICATE_RETURN(lh.flags & CDemuxStream::FLAG_FORCED
                   , rh.flags & CDemuxStream::FLAG_FORCED);
  }

  PREDICATE_RETURN(lh.type_index == g_settings.m_currentVideoSettings.m_SubtitleStream
                 , rh.type_index == g_settings.m_currentVideoSettings.m_SubtitleStream);

As you can see, if subs are on it takes the stream that was saved, and not a newly added ext. subtitle.

Member

ace20022 commented Feb 9, 2014

External subs will auto enable with this... It's only if you play the video without any external subs, then added later that is the problem. Note above used to work too. But can't be fixed without reverting bunch if gui code.

That must be a long time ago the following is from the file dated back to 06.05.2012:

static bool PredicateSubtitlePriority(const SelectionStream& lh, const SelectionStream& rh)

static bool PredicateSubtitlePriority(const SelectionStream& lh, const SelectionStream& rh)
{
  if(!g_settings.m_currentVideoSettings.m_SubtitleOn)
  {
    PREDICATE_RETURN(lh.flags & CDemuxStream::FLAG_FORCED
                   , rh.flags & CDemuxStream::FLAG_FORCED);
  }

  PREDICATE_RETURN(lh.type_index == g_settings.m_currentVideoSettings.m_SubtitleStream
                 , rh.type_index == g_settings.m_currentVideoSettings.m_SubtitleStream);

As you can see, if subs are on it takes the stream that was saved, and not a newly added ext. subtitle.

@elupus

This comment has been minimized.

Show comment
Hide comment
@elupus

elupus Feb 9, 2014

Member

I meant very long ago yes. Before the predicate stuff was added.

Yes I indeed changed my mind on what is better gui or that feature.
Probably should have been clearer on that.

Member

elupus commented Feb 9, 2014

I meant very long ago yes. Before the predicate stuff was added.

Yes I indeed changed my mind on what is better gui or that feature.
Probably should have been clearer on that.

@elupus

This comment has been minimized.

Show comment
Hide comment
@elupus

elupus Feb 9, 2014

Member

So the idea to ignore parameter has an ugly flaw.. Title ripped DVD without menu's should use normal logic.

@Voyager1 do you happen to remember the reason why you added the code to skip setting visibility of subs in OpenDefaultStreams for the navigator case in eee58b9 ?

Member

elupus commented Feb 9, 2014

So the idea to ignore parameter has an ugly flaw.. Title ripped DVD without menu's should use normal logic.

@Voyager1 do you happen to remember the reason why you added the code to skip setting visibility of subs in OpenDefaultStreams for the navigator case in eee58b9 ?

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Feb 9, 2014

@ace20022: There were some timeframes when it wasn't actually working.

I believe these are pretty common user scenarios:
1] check a video first for available subtitles, download subtitles externally from web, stop and start playback (usually faster than browsing for a subtitle) and play a video again.
2] play a video and skip through parts of it, decide to watch it later. Download external subtitles later before the actual full playback.
In both situations I'd expect to have an external subtitles displayed without any other required action.
I'm having hard time to find out a reason when user who've gone through the hassle of obtaining an external subtitle and chose to prefer external subtitles in GUI would actually not want to display them by default.
If xbmc is supposed (in some kind of scenario) to not actually prefer external subtitles when they are detected at the beginning of the playback and Prefer external subtitles setting is on, you can remove this setting from GUI completely (because it doesn't work) or rename it (that it reflects it only works on first playback). The best way would be imo to fix the current behavior rather than truncate a previously working feature.

ghost commented Feb 9, 2014

@ace20022: There were some timeframes when it wasn't actually working.

I believe these are pretty common user scenarios:
1] check a video first for available subtitles, download subtitles externally from web, stop and start playback (usually faster than browsing for a subtitle) and play a video again.
2] play a video and skip through parts of it, decide to watch it later. Download external subtitles later before the actual full playback.
In both situations I'd expect to have an external subtitles displayed without any other required action.
I'm having hard time to find out a reason when user who've gone through the hassle of obtaining an external subtitle and chose to prefer external subtitles in GUI would actually not want to display them by default.
If xbmc is supposed (in some kind of scenario) to not actually prefer external subtitles when they are detected at the beginning of the playback and Prefer external subtitles setting is on, you can remove this setting from GUI completely (because it doesn't work) or rename it (that it reflects it only works on first playback). The best way would be imo to fix the current behavior rather than truncate a previously working feature.

@elupus

This comment has been minimized.

Show comment
Hide comment
@elupus

elupus Feb 9, 2014

Member

Okey.. If having invalid GUI display is better in the no subtitles at all case, i can accept that. But it will still be invalid in the case of the video having internal subtitles and you open it once, then add external.

That is just not solvable at the moment sadly.

Member

elupus commented Feb 9, 2014

Okey.. If having invalid GUI display is better in the no subtitles at all case, i can accept that. But it will still be invalid in the case of the video having internal subtitles and you open it once, then add external.

That is just not solvable at the moment sadly.

@arnova

This comment has been minimized.

Show comment
Hide comment
@arnova

arnova Feb 9, 2014

Member

@elupus: Ok so if I understand correctly we at least agree that for movies that didn't have any subtitles before (-1 for the subtitle stream in the db), external subs added later on should always auto-enable (if subtitles are enabled in the default video settings) ?

Member

arnova commented Feb 9, 2014

@elupus: Ok so if I understand correctly we at least agree that for movies that didn't have any subtitles before (-1 for the subtitle stream in the db), external subs added later on should always auto-enable (if subtitles are enabled in the default video settings) ?

@elupus

This comment has been minimized.

Show comment
Hide comment
@elupus

elupus Feb 9, 2014

Member

I've restored the workaround for that special case. I've dropped the dvdplayer "fix" since it seems that will just make things worse.

@Voyager1 if you could test the case that was the source of: eee58b9 that would be very good.
@ace20022 this should now be almost identical to your last patch. Just that we always call SetSubtitleVisibleInternal at the end, and less code by adding relevant check to sorting.

jenkins build this please

Member

elupus commented Feb 9, 2014

I've restored the workaround for that special case. I've dropped the dvdplayer "fix" since it seems that will just make things worse.

@Voyager1 if you could test the case that was the source of: eee58b9 that would be very good.
@ace20022 this should now be almost identical to your last patch. Just that we always call SetSubtitleVisibleInternal at the end, and less code by adding relevant check to sorting.

jenkins build this please

Revert "[Fix][DVD/OMXPlayer] This fixes a regression introduced in 50…
…a1d3c."

This reverts commits bb1aeb7 and fdacd42.

The fix is invalid and causes a mismatch between GUI assumed subtitle
and what the player actually is playing.
@elupus

This comment has been minimized.

Show comment
Hide comment
@elupus

elupus Feb 9, 2014

Member

Since I managed to get some junk in that last push and had lost correct author on one commit. Dear jenkins build this please

Member

elupus commented Feb 9, 2014

Since I managed to get some junk in that last push and had lost correct author on one commit. Dear jenkins build this please

@da-anda

This comment has been minimized.

Show comment
Hide comment
@da-anda

da-anda Feb 9, 2014

Member

I only use external subs rarely, but I wouldn't want an external subtitle to be auto-enabled when I watch a movie. They only should be shown when a user manually turns on subtitles for the playing video like it's the case for embedded ones (unless they are forced ofc).
My usecase of external subs is mainly when I want to watch a movie in it's original language (en) but the video only has subs for my language (de) but I prefer the subs to be in same language as audio stream.

The setting "prefer external/downloaded over embedded subs" should only prefer the external subs over the embedded one if both are available for a certain language. It's no "always enforce external subs" setting.

Member

da-anda commented Feb 9, 2014

I only use external subs rarely, but I wouldn't want an external subtitle to be auto-enabled when I watch a movie. They only should be shown when a user manually turns on subtitles for the playing video like it's the case for embedded ones (unless they are forced ofc).
My usecase of external subs is mainly when I want to watch a movie in it's original language (en) but the video only has subs for my language (de) but I prefer the subs to be in same language as audio stream.

The setting "prefer external/downloaded over embedded subs" should only prefer the external subs over the embedded one if both are available for a certain language. It's no "always enforce external subs" setting.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Feb 9, 2014

If you don't want external subtitles to be automatically displayed it's reasonable to have Prefer external subtitles off and manually turn them on while playing a video when you need them to be displayed.

Your suggestion would actually not help your case, but would make others who use external subtitles exclusively pretty much difficult to select them manually all the time (over embedded subtitles with forced or default flags).

@elupus: Sorry in advance for maybe a "dumb" question. Do I understand correctly that the only problem with auto enabling external subtitles with your case (video having internal subtitles and you open it once, then add external) is because on first video playback xbmc saves Audio, Subtitles and Video settings to settings table in database for that file and you don't want to alter that once it saved without user interaction? If it is so why these settings are automatically saved when user didn't actually make any active change himself?

ghost commented Feb 9, 2014

If you don't want external subtitles to be automatically displayed it's reasonable to have Prefer external subtitles off and manually turn them on while playing a video when you need them to be displayed.

Your suggestion would actually not help your case, but would make others who use external subtitles exclusively pretty much difficult to select them manually all the time (over embedded subtitles with forced or default flags).

@elupus: Sorry in advance for maybe a "dumb" question. Do I understand correctly that the only problem with auto enabling external subtitles with your case (video having internal subtitles and you open it once, then add external) is because on first video playback xbmc saves Audio, Subtitles and Video settings to settings table in database for that file and you don't want to alter that once it saved without user interaction? If it is so why these settings are automatically saved when user didn't actually make any active change himself?

@da-anda

This comment has been minimized.

Show comment
Hide comment
@da-anda

da-anda Feb 9, 2014

Member

@ezechiel1917 The setting is called "prefer external subtitels". So in case I download a external sub for a language I also have an embedded sub for, the external should be preferred in the predication. That's at least how I understand this setting.
What you would want is a "force external subtitles" setting right? So whenever an external sub is present it should be used over any internal ones, regardless if it matches the preferred subtitle language or alike. And this is something completely different in my eyes.
edit: this doesn't mean that we shouldn't support it, but IMO that's not what this setting is supposed to do. So we probably need a spinner for "priority of downloaded subtitles" with options like "none", "prefer", "force". Where "prefer" is what I described above and "force" what you want.

Member

da-anda commented Feb 9, 2014

@ezechiel1917 The setting is called "prefer external subtitels". So in case I download a external sub for a language I also have an embedded sub for, the external should be preferred in the predication. That's at least how I understand this setting.
What you would want is a "force external subtitles" setting right? So whenever an external sub is present it should be used over any internal ones, regardless if it matches the preferred subtitle language or alike. And this is something completely different in my eyes.
edit: this doesn't mean that we shouldn't support it, but IMO that's not what this setting is supposed to do. So we probably need a spinner for "priority of downloaded subtitles" with options like "none", "prefer", "force". Where "prefer" is what I described above and "force" what you want.

@arnova

This comment has been minimized.

Show comment
Hide comment
@arnova

arnova Feb 9, 2014

Member

Well it is kinda strange that we store SubtitleOn = 0 (false) in the db for a subtitle which isn't there (-1 in the db), this isn't how it always been. IMO it should reflect the default video settings which says it's enabled by default, even when there's no sub (-1) (yet).

Member

arnova commented Feb 9, 2014

Well it is kinda strange that we store SubtitleOn = 0 (false) in the db for a subtitle which isn't there (-1 in the db), this isn't how it always been. IMO it should reflect the default video settings which says it's enabled by default, even when there's no sub (-1) (yet).

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Feb 9, 2014

@da-anda: That makes sense. Suggested spinner could also potentially help with elupus case if user deliberately choose Forced for External subtitles priority (Then external subtitles could be always displayed even over any previously saved SubtitleStream in settings table).

ghost commented Feb 9, 2014

@da-anda: That makes sense. Suggested spinner could also potentially help with elupus case if user deliberately choose Forced for External subtitles priority (Then external subtitles could be always displayed even over any previously saved SubtitleStream in settings table).

@elupus

This comment has been minimized.

Show comment
Hide comment
@elupus

elupus Feb 9, 2014

Member

@arnova the problem is that "they way it used to be" has been broken by: f9fff03 which started using the video settings as what to display in the GUI. Now i fully see why that change was made. But it breaks the split between GUI display and stored settings.

Member

elupus commented Feb 9, 2014

@arnova the problem is that "they way it used to be" has been broken by: f9fff03 which started using the video settings as what to display in the GUI. Now i fully see why that change was made. But it breaks the split between GUI display and stored settings.

@Voyager1

This comment has been minimized.

Show comment
Hide comment
@Voyager1

Voyager1 Feb 9, 2014

Member

@Voyager1 do you happen to remember the reason why you added the code to skip setting visibility of subs in OpenDefaultStreams for the navigator case in eee58b9 ?

The reason here was simple: we enter several times in OpenDefaultStreams - once initially, once after IsBetterStream... Problem is that during first pass, valid became false because the subtitle streams are not known yet - and thus the logic would turn off the subtitles visibility too early, even if the DVD navigator would at the second pass have subtitle streams (problem is that at the second pass there's nothing to turn them back on before finding valid subtitle streams)

Member

Voyager1 commented Feb 9, 2014

@Voyager1 do you happen to remember the reason why you added the code to skip setting visibility of subs in OpenDefaultStreams for the navigator case in eee58b9 ?

The reason here was simple: we enter several times in OpenDefaultStreams - once initially, once after IsBetterStream... Problem is that during first pass, valid became false because the subtitle streams are not known yet - and thus the logic would turn off the subtitles visibility too early, even if the DVD navigator would at the second pass have subtitle streams (problem is that at the second pass there's nothing to turn them back on before finding valid subtitle streams)

@elupus

This comment has been minimized.

Show comment
Hide comment
@elupus

elupus Feb 9, 2014

Member

@Voyager1 ok. That issue is sort of handled too by the special case commit i added. So we should be okey there.

@ezechiel1917 It's not a question of want. We can't alter it. The current code respect the selected video stream from previous run. So if we store video settings at all, we will always return to previous settings.

Member

elupus commented Feb 9, 2014

@Voyager1 ok. That issue is sort of handled too by the special case commit i added. So we should be okey there.

@ezechiel1917 It's not a question of want. We can't alter it. The current code respect the selected video stream from previous run. So if we store video settings at all, we will always return to previous settings.

@Voyager1

This comment has been minimized.

Show comment
Hide comment
@Voyager1

Voyager1 Feb 9, 2014

Member

That issue is sort of handled too by the special case commit i added. So we should be okey there.

I tested it and indeed it works. I also looked at the commit and you mention it's a temporary fix. Let's just keep in mind to be careful when re-enabling it with a better fix, because the DVD use case will always be a little bit different.

Member

Voyager1 commented Feb 9, 2014

That issue is sort of handled too by the special case commit i added. So we should be okey there.

I tested it and indeed it works. I also looked at the commit and you mention it's a temporary fix. Let's just keep in mind to be careful when re-enabling it with a better fix, because the DVD use case will always be a little bit different.

@elupus

This comment has been minimized.

Show comment
Hide comment
@elupus

elupus Feb 9, 2014

Member

@popcornmix can you check rbpi build with this, since jenkins isn't too happy about the ffmpeg changed.

Member

elupus commented Feb 9, 2014

@popcornmix can you check rbpi build with this, since jenkins isn't too happy about the ffmpeg changed.

@popcornmix

This comment has been minimized.

Show comment
Hide comment
@popcornmix

popcornmix Feb 9, 2014

Member
OMXPlayer.cpp: In member function ‘void COMXPlayer::OpenDefaultStreams(bool)’:
OMXPlayer.cpp:873:81: error: conversion from ‘OMXSelectionStream’ to non-scalar type ‘OMXSelectionStreams {aka std::vector<OMXSelectionStream>}’ requested
OMXPlayer.cpp:874:36: error: ‘OMXSelectionStreams’ has no member named ‘language’
OMXPlayer.cpp:882:15: error: ‘class PredicateSubtitlePriority’ has no member named ‘relevant’
OMXPlayer.cpp: In member function ‘virtual void COMXPlayer::Process()’:
OMXPlayer.cpp:1085:34: warning: unused variable ‘nav’ [-Wunused-variable]

You've missed this from omxplayer patch:

+ PredicateSubtitleFilter filter;
...
+      filter(lang)
OMXSelectionStreams as = m_SelectionStreams.Get(STREAM_AUDIO, GetAudioStream());

should be

OMXSelectionStream as = m_SelectionStreams.Get(STREAM_AUDIO, GetAudioStream());
Member

popcornmix commented Feb 9, 2014

OMXPlayer.cpp: In member function ‘void COMXPlayer::OpenDefaultStreams(bool)’:
OMXPlayer.cpp:873:81: error: conversion from ‘OMXSelectionStream’ to non-scalar type ‘OMXSelectionStreams {aka std::vector<OMXSelectionStream>}’ requested
OMXPlayer.cpp:874:36: error: ‘OMXSelectionStreams’ has no member named ‘language’
OMXPlayer.cpp:882:15: error: ‘class PredicateSubtitlePriority’ has no member named ‘relevant’
OMXPlayer.cpp: In member function ‘virtual void COMXPlayer::Process()’:
OMXPlayer.cpp:1085:34: warning: unused variable ‘nav’ [-Wunused-variable]

You've missed this from omxplayer patch:

+ PredicateSubtitleFilter filter;
...
+      filter(lang)
OMXSelectionStreams as = m_SelectionStreams.Get(STREAM_AUDIO, GetAudioStream());

should be

OMXSelectionStream as = m_SelectionStreams.Get(STREAM_AUDIO, GetAudioStream());
@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Feb 9, 2014

@elupus I see, but why is it needed to store video setting if user didn't change anything while playing a video? Wouldn't global default settings be sufficient in such case or is the goal here that once you play a video, to play it with exactly the same settings on next playback (including the case where user only pressed start/stop)?

ghost commented Feb 9, 2014

@elupus I see, but why is it needed to store video setting if user didn't change anything while playing a video? Wouldn't global default settings be sufficient in such case or is the goal here that once you play a video, to play it with exactly the same settings on next playback (including the case where user only pressed start/stop)?

@da-anda

This comment has been minimized.

Show comment
Hide comment
@da-anda

da-anda Feb 9, 2014

Member

IMO we should differ between resume state (which would hold the last used setting) and the default setting for a video. This would also fix this bug. So we'd need to store the audio and subtitle track on a per resume point basis for ongoing playbacks and start from defaults/stored settings on new playback.

Member

da-anda commented Feb 9, 2014

IMO we should differ between resume state (which would hold the last used setting) and the default setting for a video. This would also fix this bug. So we'd need to store the audio and subtitle track on a per resume point basis for ongoing playbacks and start from defaults/stored settings on new playback.

@ace20022

This comment has been minimized.

Show comment
Hide comment
@ace20022

ace20022 Feb 9, 2014

Member

for the record, this elupus/xbmc@c6d477d is not my patch.

Member

ace20022 commented Feb 9, 2014

for the record, this elupus/xbmc@c6d477d is not my patch.

@elupus

This comment has been minimized.

Show comment
Hide comment
@elupus

elupus Feb 9, 2014

Member

@ace20022 hmm.. okey who's is it? it had your name in your branch

Member

elupus commented Feb 9, 2014

@ace20022 hmm.. okey who's is it? it had your name in your branch

@elupus

This comment has been minimized.

Show comment
Hide comment
@elupus

elupus Feb 9, 2014

Member

@popcornmix there. let's hope that was it. btw, i have factored all this out into separate files to avoid the duplication for Gotham+1. -400 lines of code duplication :)

Member

elupus commented Feb 9, 2014

@popcornmix there. let's hope that was it. btw, i have factored all this out into separate files to avoid the duplication for Gotham+1. -400 lines of code duplication :)

@popcornmix

This comment has been minimized.

Show comment
Hide comment
@popcornmix

popcornmix Feb 9, 2014

Member

@elupus That builds okay.

Member

popcornmix commented Feb 9, 2014

@elupus That builds okay.

@ace20022

This comment has been minimized.

Show comment
Hide comment
@ace20022

ace20022 Feb 9, 2014

Member

okey who's is it? it had your name in your branch

No idea, but in the commit from my branch that looks quite similar, SetSubtitleVisibleInternal(bool bVisible); is private

Member

ace20022 commented Feb 9, 2014

okey who's is it? it had your name in your branch

No idea, but in the commit from my branch that looks quite similar, SetSubtitleVisibleInternal(bool bVisible); is private

@elupus

This comment has been minimized.

Show comment
Hide comment
@elupus

elupus Feb 9, 2014

Member

Ah, I indeed squashed another commit into yours that moved it to protected.
Will restore my name.

Member

elupus commented Feb 9, 2014

Ah, I indeed squashed another commit into yours that moved it to protected.
Will restore my name.

@ace20022

This comment has been minimized.

Show comment
Hide comment
@ace20022

ace20022 Feb 9, 2014

Member

that's even better

Member

ace20022 commented Feb 9, 2014

that's even better

@MartijnKaijser MartijnKaijser added this to the Pending for inclusion milestone Feb 9, 2014

elupus added some commits Feb 9, 2014

dvd/omxplayer: add internal version of SetSubtitleVisible
This avoids overriding result of OpenDefaultStreams forced/relevant
calculation due to SetSubtitleVisible being delayed by message
queue.

Original-patch-by: ace20022 <ace20022@xbmc.org>
dvd/omxplayer: don't override video setting visibility if no subs are…
… found

Note: This should be reverted when we have separated GUI display from
user specified settings.

It temporarily solves the use case of user starting a movie without subs,
stop it, add external subtitles, start it again.

elupus added a commit that referenced this pull request Feb 9, 2014

@elupus elupus merged commit 6fb2879 into xbmc:master Feb 9, 2014

@elupus elupus deleted the elupus:sub_fixes branch Feb 9, 2014

@jmarshallnz

This comment has been minimized.

Show comment
Hide comment
@jmarshallnz

jmarshallnz Feb 9, 2014

Member

@elupus: Please wait for an RM to merge at this point unless the RM has specifically said it's OK in the PR. Thanks for flying Gotham airways. :)

Member

jmarshallnz commented Feb 9, 2014

@elupus: Please wait for an RM to merge at this point unless the RM has specifically said it's OK in the PR. Thanks for flying Gotham airways. :)

@elupus

This comment has been minimized.

Show comment
Hide comment
@elupus

elupus Feb 9, 2014

Member

@elupus slaps himself with a large trout.

Sorry i'm usually pretty good at that :(

On Sun, Feb 9, 2014 at 9:30 PM, jmarshallnz notifications@github.comwrote:

@elupus https://github.com/elupus: Please wait for an RM to merge at
this point unless the RM has specifically said it's OK in the PR. Thanks
for flying Gotham airways. :)


Reply to this email directly or view it on GitHubhttps://github.com/xbmc/xbmc/pull/4176#issuecomment-34585354
.

Member

elupus commented Feb 9, 2014

@elupus slaps himself with a large trout.

Sorry i'm usually pretty good at that :(

On Sun, Feb 9, 2014 at 9:30 PM, jmarshallnz notifications@github.comwrote:

@elupus https://github.com/elupus: Please wait for an RM to merge at
this point unless the RM has specifically said it's OK in the PR. Thanks
for flying Gotham airways. :)


Reply to this email directly or view it on GitHubhttps://github.com/xbmc/xbmc/pull/4176#issuecomment-34585354
.

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