Fix state not saved after #5842 if playing from "Recently added".

[anaconda]

See:

• http://forum.kodi.tv/showthread.php?tid=210479
• Fix off-by-1 errors in saving video settings when in a playlists. #5842 (comment)
• http://trac.kodi.tv/ticket/15603
• http://trac.kodi.tv/ticket/15660

Playing something from "Recently added" creates a play list with a single item. Oops.

Needs backport to Helix

[MartijnKaijser]

@Montellese or @mkortstiege could you confirm that this is correct?

[mkortstiege]

Looks valid but i really wonder why recently added items are wrapped in a playlist instead of "just" played. Is there a reason for this behavior?

[Voyager1]

Depending on how you start playback within the GUI, there are even more cases where an item gets wrapped into a playlist. I confirm that this change is correct.

[Montellese]

@mkortstiege: Everything should be put into a playlist for playback. There are parts in our code that work based on the assumption that if something is playing there also must be a playlist that contains this item. I've seen quite a few issues with cases where this is not true (playing from skin widgets was one of them).

@anaconda: Could it happen that CPlaylistPlayer::IsSingleItemNonRepeatPlaylist() is also true in cases where we don't want this special handling?

[anaconda]

(Rebased.) Apart from this PR, it's not used outside of PlaylistPlayer.cpp currently. It explicitly checks for size() <= 1, so no, it can't be true in other cases.
The bug originally fixed by #5842 is not affected by this PR.

[Montellese]

I'm not sure if we are talking about the same thing. It doesn't really matter where it is used but the question is under what circumstances does it return true. It doesn't really only return true when playing something from recently added but probably also when simply playing a single movie from the library.

[anaconda]

I'm not sure if we are talking about the same thing. It doesn't really matter where it is used

Yeah, that was just me thinking loudly...

but the question is under what circumstances does it return true. It doesn't really only return true when playing something from recently added but probably also when simply playing a single movie from the library.

That'd be fine too.
This is fixing reports that playing from recently added is not saving file state, but technically it fixes playlists with a single item. I wasn't sure if that also happens somewhere else, hence why I didn't explicitly mention it.

However, current logic is fine when playing an item from the library (GetCurrentPlaylist() == PLAYLIST_NONE is true in that case). (5842 has a brief summary of how saving state currently works.)
This additional check was added in 5842 to prevent overwriting state with the wrong details while in a playlist with multiple items.
So here we only care to know if we're playing a single item (either because wer're not in a playlist or the current playlist has only a single item). In the latter case - this PR - size() <= 1 is what we want.
(I have reworded the comment above the if to make it more clear, and a typo somewhere else)

tl;dr I believe IsSingleItemNonRepeatPlaylist() is exactly what's needed here.

[Montellese]

OK. I didn't have time to look at the code in detail. I just wanted to be sure this was thought through for most/all possible cases.

[piejanssens]

IMO This is a major issue that affects a huge number of users. Isn't this considered for 14.0?

[MartijnKaijser]

no it's not considered as there is no agreement on proper fix.

[anaconda]

Updated to only ignore video playlists as I've noticed an issue with music playlists with this PR (why does music work differently...). As I already store the playlist I've switched to size() instead of IsSingleItemNonRepeatPlaylist() - previous comment, in general, still applies.

[piejanssens]

Saw this issue popping up in the following threads too:
http://forum.xda-developers.com/fire-tv/general/kodi-resume-playback-starts-form-t2972613
http://forum.kodi.tv/showthread.php?tid=210480

People that don't know how to compile with this fix are running the nightly's to cope with this issue atm. Moving towards 14.1 and no sign of progress in fixing this seems strange.

If I'm not mistaken this PR fixes the regression for current state of confluence recently added homescreen items or third party skin widget items, while not breaking the way-to-go method of creating a playlist for any video played even if it's a single item.

So why don't you merge this already and ping ronie to create playlists for items played from the home screen?

[MartijnKaijser]

@piejanssens it was not (yet) merged because it could cause other regressions. This needs to be pretty damn sure before it gets merged to 14.1

[piejanssens]

Who can vouch for this? @Montellese maybe?
Otherwise put it in for 14.1 RC and go for another round.

[anaconda]

it was not (yet) merged because it could cause other regressions. This needs to be pretty damn sure before it gets merged to 14.1

What about giving this a try (this is for master) and only backport to Helix if no issues are reported. However, this has been in Milhouse builds for 3 weeks now.

[peterchs]

Another few mentions of this bug highlighting scale of users being affected by this in support threads.
http://forum.kodi.tv/showthread.php?tid=214157
http://forum.kodi.tv/showthread.php?tid=213265

I'm not sure the scale of this issue has been properly realised. Starting videos from home shelf is commonplace, and this bug effectively breaks resume points for those users. Resume points are a key feature.

This fix missed 14.0 and it looks like its going to miss 14.1 too? :(

[Tolriq]

Did not see this PR.

But @Montellese I got hundred of reports that resume point does no more work on Kodi.
Seems starting media from JSON api have the same behavior and no resume point will be set on stop and not a problem with stop as reported in JSON thread.

This is a major break as really a lot of people does use JSON to start medias.

If I can test or validate things, please tell but this and Webserver crashses on Kodi have made lot's of users unhappy :(

[piejanssens]

Can anyone provide a windows build of the latest Helix branch with this commit patched onto it?
I tried for several hours but gave up. The wiki' win build instructions are not very good.

[NedScott]

@piejanssens if I did this right, one will be available on http://mirrors.xbmc.org/test-builds/win32/ as "Helix-fix-5842-testing" within an hour.

[piejanssens]

It's there. Thanks @NedScott.

[NedScott]

There are builds for all platforms if other people want to test with Win, OS X, Android, or iOS. Just look for "Helix-fix-5842-testing" in the respective OS folder for http://mirrors.xbmc.org/test-builds/

[piejanssens]

I'll test Win, OS X and iOS.

STATUS - watched status for widgets
STATUS - watched status for recently added
STATUS - watched status for movies or tv show via main library view
STATUS - resume for widgets
STATUS - resume for recently added
STATUS - resume for movies or tv show via main library view
STATUS - resume song playback
STATUS - resume album playback
STATUS - resume playlist playback

Anything missing or irrelevant for testing?

[NedScott]

Per Tolriq, also test playing back something using a smartphone/tablet remote, which should use the JSON-RPC api. In other words, use the a remote (such as Yatse) and launch the video file from the remote, rather than navigating to it on Kodi's GUI.

Since this is having a big impact on users right now, I've thrown up the Helix+Recentlyadded-fix builds in the v14.1 testing thread: http://forum.kodi.tv/showthread.php?tid=213108

If desired, I can also start a thread with v15/master+Recentlyadded-fix and ask users to test that as well.

[Tolriq]

Return from holidays for me and tons of things to catch up but will also make some testing next week.

[Tolriq]

After some testing (Only JSON) I can confirm that the build Helix-fix-5842-testing does fix the JSON related problems about resume point not being set when using Player.Open.

Would love this fix in 14.1 to avoid tons of mails per week :)

[MartijnKaijser]

Seems enough people have tested this. Time for merge and backports? @Montellese @mkortstiege

[Montellese]

Finally had the time to look at this. I don't think that this is the best fix but I don't have any better ideas either right now so I'm ok with this. Maybe add a remark to the comment that this needs to be re-visited.

[anaconda]

Finally had the time to look at this. I don't think that this is the best fix but I don't have any better ideas either right now so I'm ok with this. Maybe add a remark to the comment that this needs to be re-visited.
Thanks. TODO added.

[Montellese]

Thanks.

[gentoo-root]

This patch leads to regression. Assume you have a folder with two videos, default zoom is 1.0 for both.

1. Start playing the second video, set zoom=0.5, stop playing.
2. Start playing the first video, its zoom is 1.0 by default.
3. Press [X] in the corner, so that you get into menu, but video is played in backgroupd.
4. Start playing the second video (without stopping first one). Its zoom=0.5.
5. Stop playing the second video and start playing the first one. Its zoom will be 0.5, but should be 1.0.

When one video is played, and I start the next one without stopping the first one, settings of first video get overwritten by settings of second video.

[anaconda]

Did you git bisect to determine this?
This sounds similar to the issue #5842 has addressed for playlists.
I think it's caused by https://github.com/xbmc/xbmc/blob/master/xbmc/Application.cpp#L3845
Are you able to test 13.2?

Saving file settings is indeed quite messed up, see the comment this PR adds to Application.cpp.

[gentoo-root]

At first, https://github.com/xbmc/xbmc/blob/master/xbmc/Application.cpp#L3845 is not a culprit. This line does correct saving of settings for previous video. But then https://github.com/xbmc/xbmc/blob/master/xbmc/Application.cpp#L4399 does incorrect saving of settings because this time https://github.com/xbmc/xbmc/blob/master/xbmc/utils/SaveFileStateJob.cpp#L129 leads to applying next video settings to previous video (progressTrackingFile is still previous video). So settings for previous video are rewritten by settings of next video. This is what issue #5842 about.

I put my comment here, because this PR leads to regression.

If the line https://github.com/xbmc/xbmc/blob/master/xbmc/Application.cpp#L4398 looks like if (!m_progressTrackingItem->IsPVRChannel() && !(playlist == PLAYLIST_VIDEO && g_playlistPlayer.GetPlaylist(playlist).size() > 1)) then issue #5842 is fixed for playlists of two or more videos, but is not fixed for the case when I manually start next video without stopping previous one.

If that line looks like before: if (!m_progressTrackingItem->IsPVRChannel() && g_playlistPlayer.GetCurrentPlaylist() == PLAYLIST_NONE) then issue #5842 is fixed both for playlists of many videos and for manually started videos, but is not fixed for playing from "Recently added", as @anaconda says.

[anaconda]

Thanks. I'll try to look into this - unfortunately I'm pretty busy right now (and I will be for a while).
Best way to fix playlists, "Recently added" and the issue you're experiencing would be to move the logic to the players (addressing the comments added by this PR).