Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

bugfix: fixed initial playback of stacked iso images #1356

Merged
merged 1 commit into from

4 participants

@fetzerch

Currently, stacked iso movies don't play when pressing the play button.
There's no user feedback. (see log: http://pastebin.com/raw.php?i=XSqPvH87)
The files play fine, when choosing the part instead of just pressing play.

This PR initializes the FileItem's m_lStartPartNumber with 1 so that the playback starts on the first iso when the user didn't specify the part to play.

@Voyager-xbmc: I think this issue was introduced by #1099, could you review my fix?

@Voyager1
Collaborator

I never saw this case during the development, because my xbmc menu is configured to show all choices (Settings > Videos > File Lists > Default Select Action = Choose). But now I understand why some stacks did not play when selected from the "recently added" items... So thanks for the good catch :-)

I reviewed your fix, and I'm OK with it as it sets the right default value. A cleaner solution would be to cover for the default case in the logic (CApplication::PlayStack - case 1 for stacked ISOs).

Edit: code example, line 3811, right after the if (startoffset == STARTOFFSET_RESUME) block:

if (selectedFile == 0) selectedFile++; // need at least a default part value of 1, otherwise nothing is played.
@jmarshallnz
Owner

Both should go in I think unless there is some use in having the start part being 0?

@fetzerch mind making that change as well?

@jmarshallnz jmarshallnz was assigned
@Voyager1
Collaborator

You're right, and there's no reason for a value of 0.

xbmc/Application.cpp
@@ -3808,6 +3808,10 @@ bool CApplication::PlayStack(const CFileItem& item, bool bRestart)
CLog::Log(LOGERROR, "%s - Cannot open VideoDatabase", __FUNCTION__);
}
+ // need at least a default part value of 1, otherwise nothing is played
+ if (selectedFile == 0)
+ selectedFile = 1;
+
// set startoffset in movieitem, track stack item for updating purposes, and finally play disc part
if (selectedFile > 0 && selectedFile <= (int)movieList.Size())
@fetzerch
fetzerch added a note

@Voyager-xbmc @jmarshallnz : you're checking the boundaries of selectedFiles here anyway. So wouldn't it be better to get rid of the if here and make sure that selectedFile is always between 1 and movieList.Size()?


if (selectedFile <= 0) selectedFile = 1;
if (selectedFile > (int)movieList.Size()) selectedFile = (int)movieList.Size();

@Voyager1 Collaborator
Voyager1 added a note

If you're coming from the GUI, two situations can happen:

  • you've used the resume button or the play part button, so the selectedFile should always be between 1 and movieList.Size()
  • you've used the default play function, and the selectedFile is still 0 (or the default from the constructor, so now 1). It's only this second case that needed fixing.

If coming from other call origins (e.g. remote procedure call / HTTP ??) your proposal does make a lot of sense. I would just maybe add a debug log line in case selectedFile > size, so we have a trace.

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

looks good to me, thanks fetzerch!!

@MartijnKaijser MartijnKaijser merged commit fabf961 into xbmc:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 3, 2012
  1. @fetzerch
This page is out of date. Refresh to see the latest.
Showing with 17 additions and 8 deletions.
  1. +15 −6 xbmc/Application.cpp
  2. +2 −2 xbmc/FileItem.cpp
View
21 xbmc/Application.cpp
@@ -3808,14 +3808,23 @@ bool CApplication::PlayStack(const CFileItem& item, bool bRestart)
CLog::Log(LOGERROR, "%s - Cannot open VideoDatabase", __FUNCTION__);
}
- // set startoffset in movieitem, track stack item for updating purposes, and finally play disc part
- if (selectedFile > 0 && selectedFile <= (int)movieList.Size())
+ // make sure that the selected part is within the boundaries
+ if (selectedFile <= 0)
+ {
+ CLog::Log(LOGWARNING, "%s - Selected part %d out of range, playing part 1", __FUNCTION__, selectedFile);
+ selectedFile = 1;
+ }
+ else if (selectedFile > movieList.Size())
{
- movieList[selectedFile - 1]->m_lStartOffset = startoffset > 0 ? STARTOFFSET_RESUME : 0;
- movieList[selectedFile - 1]->SetProperty("stackFileItemToUpdate", true);
- *m_stackFileItemToUpdate = item;
- return PlayFile(*(movieList[selectedFile - 1]));
+ CLog::Log(LOGWARNING, "%s - Selected part %d out of range, playing part %d", __FUNCTION__, selectedFile, movieList.Size());
+ selectedFile = movieList.Size();
}
+
+ // set startoffset in movieitem, track stack item for updating purposes, and finally play disc part
+ movieList[selectedFile - 1]->m_lStartOffset = startoffset > 0 ? STARTOFFSET_RESUME : 0;
+ movieList[selectedFile - 1]->SetProperty("stackFileItemToUpdate", true);
+ *m_stackFileItemToUpdate = item;
+ return PlayFile(*(movieList[selectedFile - 1]));
}
// case 2: all other stacks
else
View
4 xbmc/FileItem.cpp
@@ -69,7 +69,7 @@ CFileItem::CFileItem(const CSong& song)
m_strPath = song.strFileName;
GetMusicInfoTag()->SetSong(song);
m_lStartOffset = song.iStartOffset;
- m_lStartPartNumber = 0;
+ m_lStartPartNumber = 1;
SetProperty("item_start", song.iStartOffset);
m_lEndOffset = song.iEndOffset;
m_strThumbnailImage = song.strThumb;
@@ -325,7 +325,7 @@ void CFileItem::Reset()
m_dateTime.Reset();
m_iDriveType = CMediaSource::SOURCE_TYPE_UNKNOWN;
m_lStartOffset = 0;
- m_lStartPartNumber = 0;
+ m_lStartPartNumber = 1;
m_lEndOffset = 0;
m_iprogramCount = 0;
m_idepth = 1;
Something went wrong with that request. Please try again.