bugfix: fixed initial playback of stacked iso images #1356

Merged
merged 1 commit into from Sep 8, 2012

Conversation

Projects
None yet
4 participants
Member

fetzerch commented Sep 2, 2012

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?

Member

Voyager1 commented Sep 2, 2012

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.
Member

jmarshallnz commented Sep 3, 2012

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 was assigned Sep 3, 2012

Member

Voyager1 commented Sep 3, 2012

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

@fetzerch fetzerch and 1 other commented on an outdated diff Sep 3, 2012

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 Sep 3, 2012

Member

@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

Voyager1 Sep 3, 2012

Member

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.

Member

Voyager1 commented Sep 3, 2012

looks good to me, thanks fetzerch!!

@MartijnKaijser MartijnKaijser added a commit that referenced this pull request Sep 8, 2012

@MartijnKaijser MartijnKaijser Merge pull request #1356 from fetzerch/bugfix-isostack
bugfix: fixed initial playback of stacked iso images
fabf961

@MartijnKaijser MartijnKaijser merged commit fabf961 into xbmc:master Sep 8, 2012

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