Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

DVD various fixes related to libdvdnav issues and skip to menu #4296

Merged
merged 2 commits into from Mar 16, 2014

Conversation

Projects
None yet
6 participants
Member

Voyager1 commented Feb 28, 2014

  • first commit is to make the "skip to DVD menu" setting an expert setting with a default value of true:
    See: http://forum.xbmc.org/showthread.php?tid=187379
  • first commit is to align the skip to DVD menu functionality with VLC code. Essentially they also try to fallback to the root menu if title menu wasn't found.
    See: https://github.com/videolan/vlc/blob/master/modules/access/dvdnav.c#L324
  • second commit is to attempt a fix for http://trac.xbmc.org/ticket/14957 - not so much the fact that that DVD doesn't play, but rather that it blocks/freezes XBMC. The root cause is a badly authored dvd and libdvdnav can't properly handle it, but down the road it causes an endless cycle between NAVRESULT_NOP type actions inside DVDInputStreamNavigator.
    It constantly cycles between DVDNAV_SPU_STREAM_CHANGE, DVDNAV_AUDIO_STREAM_CHANGE
    DVDNAV_NAV_PACKET, DVDNAV_CELL_CHANGE, DVDNAV_SPU_CLUT_CHANGE, DVDNAV_SPU_STREAM_CHANGE ...
    and never returns. The brute-force solution is to put a limit to the amount we can cycle through the "while(true)" loop and allow this Read() method to return back to ffmpeg and yield control back to xbmc so that it can process keystrokes (e.g. to press STOP).
    I chose an arbitrary of 1000 NOPs before the brute force return, but I'm open to alternative suggestions.

@elupus, I'd appreciate your input on these.

Member

ace20022 commented Mar 1, 2014

What happens if the loop exits because of 1000 nops? Does the dvd play then, or does the user have to close the player? If the latter is true you maybe could stop playback via ?return -1? and pop up a toast like we do if a blury menu fails to play.

Member

Voyager1 commented Mar 1, 2014

The only thing that happens is that the player briefly gets control, avoiding that the user has to forcefully close (kill) xbmc. This is deep inside the Read method, and there's no timeout mechanism. Technically, it's playing, but stuck in an endless loop.

Member

ace20022 commented Mar 1, 2014

So the dvd gets played back normally?

Member

Voyager1 commented Mar 1, 2014

@ace20022 yes and no. The issue that this tries to resolve is a dvd that does not play well, except if 'skip to menu' is turned on. When turned off it cycles forever and xbmc has to be killed. At least with this 'fix' we can normally stop playback instead of killing. See the trac ticket reference in the description.
Edit: in terms of what it plays: a black screen with a yellow square, instead of the expected menu.

Member

ace20022 commented Mar 1, 2014

Ok, i.e, the user has no other choice than to stop? If so we could automatically stop like we do with blurays.

Member

Voyager1 commented Mar 1, 2014

yes. if you can point me to the way we auto-stop blurays that would help. I tried with return -1 (for error) but that didn't make any difference.

Member

ace20022 commented Mar 1, 2014

Two different approaches:

First one similar to 44cca8b:

 .../DVDInputStreams/DVDInputStreamNavigator.cpp          |   14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/xbmc/cores/dvdplayer/DVDInputStreams/DVDInputStreamNavigator.cpp b/xbmc/cores/dvdplayer/DVDInputStreams/DVDInputStreamNavigator.cpp
index de4f3d3..9274034 100644
--- a/xbmc/cores/dvdplayer/DVDInputStreams/DVDInputStreamNavigator.cpp
+++ b/xbmc/cores/dvdplayer/DVDInputStreams/DVDInputStreamNavigator.cpp
@@ -37,6 +37,7 @@
 #define HOLDMODE_HELD 1 /* set internally when we wish to flush demuxer */
 #define HOLDMODE_SKIP 2 /* set by inputstream user, when they wish to skip the held mode */
 #define HOLDMODE_DATA 3 /* set after hold mode has been exited, and action that inited it has been executed */
+#define HOLDMODE_ERROR 4 /* Unrecoverable error encountered */

 CDVDInputStreamNavigator::CDVDInputStreamNavigator(IDVDPlayer* player) : CDVDInputStream(DVDSTREAM_TYPE_DVD)
 {
@@ -224,6 +225,8 @@ void CDVDInputStreamNavigator::Close()

 int CDVDInputStreamNavigator::Read(uint8_t* buf, int buf_size)
 {
+  if (m_holdmode == HOLDMODE_ERROR)
+    return -1;
   if (!m_dvdnav || m_bEOF) return 0;
   if (buf_size < DVD_VIDEO_BLOCKSIZE)
   {
@@ -232,12 +235,19 @@ int CDVDInputStreamNavigator::Read(uint8_t* buf, int buf_size)
   }

   int iBytesRead;
-
+  int NOPcount = 0;
   while(true) {
     int navresult = ProcessBlock(buf, &iBytesRead);
     if (navresult == NAVRESULT_HOLD)       return 0; // return 0 bytes read;
     else if (navresult == NAVRESULT_ERROR) return -1;
     else if (navresult == NAVRESULT_DATA)  return iBytesRead;
+    else if (navresult == NAVRESULT_NOP && NOPcount == 1000)
+    {
+      m_holdmode = HOLDMODE_ERROR;
+      m_pDVDPlayer->OnDVDNavResult(NULL, -2);
+      return -1; // allow dvdplayer to get control back if "endless" cycling thru NOP events
+    }
+    else if (navresult == NAVRESULT_NOP) NOPcount++;
   }

   return iBytesRead;
@@ -790,7 +800,7 @@ CDVDInputStream::ENextStream CDVDInputStreamNavigator::NextStream()
   if(m_holdmode == HOLDMODE_HELD)
     m_holdmode = HOLDMODE_SKIP;

-  if(m_bEOF)
+  if(m_bEOF || m_holdmode == HOLDMODE_ERROR)
     return NEXTSTREAM_NONE;
   else if(m_lastevent == DVDNAV_VTS_CHANGE)
     return NEXTSTREAM_OPEN;

And

 xbmc/cores/dvdplayer/DVDPlayer.cpp |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/xbmc/cores/dvdplayer/DVDPlayer.cpp b/xbmc/cores/dvdplayer/DVDPlayer.cpp
index b056c8b..6bd4f58 100644
--- a/xbmc/cores/dvdplayer/DVDPlayer.cpp
+++ b/xbmc/cores/dvdplayer/DVDPlayer.cpp
@@ -3562,6 +3562,14 @@ int CDVDPlayer::OnDVDNavResult(void* pData, int iMessage)
         m_dvd.state = DVDSTATE_NORMAL;
       }
       break;
+    case -2:
+    {
+      m_dvd.state = DVDSTATE_NORMAL;
+      CLog::Log(LOGDEBUG, "CDVDPlayer::OnDVDNavResult - libdvdnav read error, stopping playback");
+      CGUIDialogKaiToast::QueueNotification(g_localizeStrings.Get(25008), g_localizeStrings.Get(25009));
+      break;
+    }
     default:
     {}
       break;

Second one, because I saw CApplicationMessenger::Get().MediaStop(false); by accident which is used by pvr i guess.

@@ -232,12 +232,18 @@ int CDVDInputStreamNavigator::Read(uint8_t* buf, int buf_size)
   }

   int iBytesRead;
-
+  int NOPcount = 0;
   while(true) {
     int navresult = ProcessBlock(buf, &iBytesRead);
     if (navresult == NAVRESULT_HOLD)       return 0; // return 0 bytes read;
     else if (navresult == NAVRESULT_ERROR) return -1;
     else if (navresult == NAVRESULT_DATA)  return iBytesRead;
+    else if (navresult == NAVRESULT_NOP && NOPcount == 1000)
+    {
+      m_pDVDPlayer->OnDVDNavResult(NULL, -2);
+      return -1; // allow dvdplayer to get control back if "endless" cycling thru NOP events
+    }
+    else if (navresult == NAVRESULT_NOP) NOPcount++;
   }

   return iBytesRead;

And

@@ -3562,6 +3562,14 @@ int CDVDPlayer::OnDVDNavResult(void* pData, int iMessage)
         m_dvd.state = DVDSTATE_NORMAL;
       }
       break;
+    case -2:
+    {
+      m_dvd.state = DVDSTATE_NORMAL;
+      CLog::Log(LOGDEBUG, "CDVDPlayer::OnDVDNavResult - libdvdnav read error, stopping playback");
+      CGUIDialogKaiToast::QueueNotification(g_localizeStrings.Get(25008), g_localizeStrings.Get(25009));
+      CApplicationMessenger::Get().MediaStop(false);
+      break;
+    }
     default:
     {}
       break;

@elupus ?

Member

Voyager1 commented Mar 1, 2014

@ace20022 - I think our updates just crossed. Can you take a look at my updated commit? I leveraged the DVDNAV_STOP constant (instead of -2), which was already there for that purpose, and apparently there's no need to explicitly call MediaStop(). Playback stops nicely (because I set m_bEOF to true in the navigator!) and a toast popup shows up (referring to please check the log).

Member

ace20022 commented Mar 1, 2014

Of course you're right with DVDNAV_STOP ;) When I wrote the bluray code, elupus told me to use m_holdmode because eof is a different thing and I agree on that.

Member

Voyager1 commented Mar 1, 2014

But eof is already used for a similar case in DVDInputstreamNavigator, so I suspect it's correct.

@MartijnKaijser MartijnKaijser added this to the Pending for inclusion milestone Mar 2, 2014

@elupus elupus and 1 other commented on an outdated diff Mar 2, 2014

system/settings/settings.xml
@@ -988,8 +988,8 @@
<control type="spinner" format="string" />
</setting>
<setting id="dvds.automenu" type="boolean" label="21882" help="36196">
- <level>1</level>
- <default>false</default>
+ <level>3</level>
+ <default>true</default>
@elupus

elupus Mar 2, 2014

Member

Absolutely not. We should by default behave as a normal DVD player. It can be hidden as advanced, that i don't mind. But in no way can it be default on.

@elupus

elupus Mar 2, 2014

Member

Okey.. let me retract the somewhat quick to judge reaction. Imho it's a bad idea to do this by default, but i'll yield to public opinion on this. I've seen dvd's that don't work with this turned on, but admitedly that was back in the xbox days.

@Voyager1

Voyager1 Mar 2, 2014

Member

@elupus : I have posted a forum thread here (http://forum.xbmc.org/showthread.php?tid=187379) to ask devs their opinion, and this was the preference (till now). The underlying reason being that more DVDs play well with it turned on than with it turned off (see the two trac tickets that led to this choice). Even more so, VLC has this on "by default" (also see forum post).

@Voyager1

Voyager1 Mar 2, 2014

Member

@elupus : I'm withdrawing this proposal... you were right about it, there are several DVDs in my collection, typically with multi-lingual menus, where the "attempt skip to menu" doesn't do much good. VLC equally sucks in this case. Without anything better I suggest to leave this as it was.

@elupus elupus and 1 other commented on an outdated diff Mar 2, 2014

...dvdplayer/DVDInputStreams/DVDInputStreamNavigator.cpp
while(true) {
int navresult = ProcessBlock(buf, &iBytesRead);
if (navresult == NAVRESULT_HOLD) return 0; // return 0 bytes read;
else if (navresult == NAVRESULT_ERROR) return -1;
else if (navresult == NAVRESULT_DATA) return iBytesRead;
+ else if (navresult == NAVRESULT_NOP && NOPcount == 1000)
@elupus

elupus Mar 2, 2014

Member

Set the count much higher than 1000. It's a failsafe, even a rbpi will loop through 1000 very quickly.

Also, please only check if(NAVRESULT_NOP), then do the NOPcount check inside that if clause.

@Voyager1

Voyager1 Mar 2, 2014

Member

1000 is indeed quite quick, but why much higher, do you think there are legit DVD navigation structures that let you loop a 1000x through NOP type operations before reading any video data? Anyway, another suggestion: would 2000 or 5000 be better for you?

Member

Voyager1 commented Mar 2, 2014

@elupus - addressed your comment about the if() clause - hopefully that's in line with what you meant.

Member

Voyager1 commented Mar 3, 2014

jenkins build this please

Member

Voyager1 commented Mar 11, 2014

@elupus - could you please review and follow up, thanks. Also I feel this is Gotham material, if you're OK I'll merge and resubmit to Gotham branch.

@jmarshallnz jmarshallnz added the Gotham label Mar 14, 2014

Member

elupus commented Mar 16, 2014

Okay by me

Voyager1 added a commit that referenced this pull request Mar 16, 2014

Merge pull request #4296 from Voyager1/dvd-various-fixes
DVD various fixes related to libdvdnav issues and skip to menu

@Voyager1 Voyager1 merged commit 12296dc into xbmc:master Mar 16, 2014

1 check passed

default Merged build #310 succeeded in 54 min
Details
Member

jmarshallnz commented Mar 16, 2014

Please remember that only RMs are merging at this point in the release cycle. Once Gotham is out things will be free again.

Member

Voyager1 commented Mar 16, 2014

my mistake, I thought this was only for the Gotham branch...

@Voyager1 Voyager1 modified the milestones: Gotham13.0-beta3, Pending for inclusion Mar 21, 2014

Voyager1 added a commit that referenced this pull request Mar 21, 2014

Merge pull request #4296 from Voyager1/dvd-various-fixes
DVD various fixes related to libdvdnav issues and skip to menu

@t-nelson t-nelson removed the Gotham label Mar 21, 2014

@Voyager1 Voyager1 deleted the Voyager1:dvd-various-fixes branch May 1, 2014

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