Skip to content
New issue

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

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix queuing of next episode using playlist #204

Closed
wants to merge 3 commits into from
Closed

Fix queuing of next episode using playlist #204

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
48f6c7b
Fix queuing of next episode using playlist
Jul 29, 2020
8788969
Refactor slightly to pass linting
Jul 29, 2020
ff2c5d2
Exclude too-many-branches linting check
Jul 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions resources/lib/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,10 @@ def queue_next_item(self, episode):
def reset_queue():
jsonrpc(method='Playlist.Remove', id=0, params=dict(playlistid=1, position=0))

@staticmethod
def dequeue_next_item():
jsonrpc(method='Playlist.Remove', id=0, params=dict(playlistid=1, position=1))

def get_next_in_playlist(self, position):
result = jsonrpc(method='Playlist.GetItems', params=dict(
playlistid=1,
Expand Down
18 changes: 13 additions & 5 deletions resources/lib/playbackmanager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,6 @@ def launch_popup(self, episode, playlist_item):
if not include_play_count or self.state.current_episode_id == episode_id:
return

if not playlist_item:
self.state.queued = self.api.queue_next_item(episode)

Comment on lines -53 to -55
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was added here to try and ensure there was enough time for the next item to actually be enqueued and not get caught up with the closing of the previous item.

An alternate method that resolves the incorrect dequeueing can be seen here: MoojMidge/service.upnext@d5c46a2

# We have a next up episode choose mode
if get_setting_int('simpleMode') == 0:
next_up_page = UpNext('script-upnext-upnext-simple.xml', addon_path(), 'default', '1080i')
Expand All @@ -63,7 +60,8 @@ def launch_popup(self, episode, playlist_item):

showing_next_up_page, showing_still_watching_page = self.show_popup_and_wait(episode,
next_up_page,
still_watching_page)
still_watching_page,
playlist_item)
should_play_default, should_play_non_default = self.extract_play_info(next_up_page,
showing_next_up_page,
showing_still_watching_page,
Expand Down Expand Up @@ -94,7 +92,7 @@ def launch_popup(self, episode, playlist_item):
# Play local media
self.api.play_kodi_item(episode)

def show_popup_and_wait(self, episode, next_up_page, still_watching_page):
def show_popup_and_wait(self, episode, next_up_page, still_watching_page, playlist_item): # pylint: disable=too-many-branches
try:
play_time = self.player.getTime()
total_time = self.player.getTotalTime()
Expand All @@ -113,6 +111,9 @@ def show_popup_and_wait(self, episode, next_up_page, still_watching_page):
showing_still_watching_page = False
if int(self.state.played_in_a_row) <= int(played_in_a_row_number):
self.log('showing next up page as played in a row is %s' % self.state.played_in_a_row, 2)
# Queue the next episode
if not playlist_item:
self.state.queued = self.api.queue_next_item(episode)
next_up_page.show()
set_property('service.upnext.dialog', 'true')
showing_next_up_page = True
Expand Down Expand Up @@ -144,6 +145,13 @@ def show_popup_and_wait(self, episode, next_up_page, still_watching_page):
elif showing_still_watching_page:
still_watching_page.update_progress_control(remaining=remaining, runtime=runtime)
sleep(100)
if next_up_page.is_cancel() and not playlist_item:
# Playback of next episode was cancelled - dequeue the next episode
self.api.dequeue_next_item()
self.state.queued = False
if still_watching_page.is_still_watching() and not playlist_item:
# User is still watching - queue the next episode
self.state.queued = self.api.queue_next_item(episode)
return showing_next_up_page, showing_still_watching_page

def extract_play_info(self, next_up_page, showing_next_up_page, showing_still_watching_page, still_watching_page):
Expand Down
8 changes: 5 additions & 3 deletions resources/lib/player.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,11 @@ def onPlayBackStopped(self): # pylint: disable=invalid-name

def onPlayBackEnded(self): # pylint: disable=invalid-name
"""Will be called when Kodi has ended playing a file"""
self.reset_queue()
self.api.reset_addon_data()
self.state = State() # Reset state
if not self.state.queued:
# Reset if playback ended without the next episode being queued
self.reset_queue()
self.api.reset_addon_data()
self.state = State() # Reset state
Comment on lines +60 to +64
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The queue needs to be reset here, but the state should not be.

See MoojMidge/service.upnext@d5c46a2 for an alternative method.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes issues with queuing of next episode via playlist:

  1. We should not reset state if an episode ends, triggering the onPlayBackEnded event, when we have the next episode queued.

This should happen the other way round. The event is triggered independently of this addon, but while the state should not be reset in this event handler, the queue does need to be reset in the onPlayBackEnded event.

Yes, that makes sense. The problem was with resetting state at that point as it prevents Still Watching? from ever being invoked.

  1. We should queue the next episode when the "Up Next" dialog is presented, but dequeue the next episode if the user cancels playback of the next episode.

Good pickup.

  1. We should not queue the next episode when the "Still Watching?" dialog is presented until and unless the user indicates they are indeed still watching.

Did this cause any problems? Need to be careful to not delay the queuing too much otherwise the original problem from #139 can re-occur.

It's an affirmative action by the user to say they are still watching. While it's true that if the user presses the button at the very last moment before playback ends then the problem from #139 could happen, it's not a major issue from a user experience perspective.

But the alternative would be to enqueue the next episode by default and then dequeue it if the Still Watching dialog times out without the user pressing the button - but the concern there is that a similar race condition could happen where the RPC to dequeue could be processed after the next episode has already started to be played back. Again, from a user experience perspective, this would be a worse scenario than the above.

Copy link
Collaborator

@MoojMidge MoojMidge Aug 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes issues with queuing of next episode via playlist:

  1. We should not reset state if an episode ends, triggering the onPlayBackEnded event, when we have the next episode queued.

This should happen the other way round. The event is triggered independently of this addon, but while the state should not be reset in this event handler, the queue does need to be reset in the onPlayBackEnded event.

Yes, that makes sense. The problem was with resetting state at that point as it prevents Still Watching? from ever being invoked.

I never used the Still Watching? feature, but upon checking my settings I saw it was actually set to 3, so in theory I should have seen the Still Watching? popup. I guess the state reset meant that this never worked on some systems.

  1. We should queue the next episode when the "Up Next" dialog is presented, but dequeue the next episode if the user cancels playback of the next episode.

Good pickup.

  1. We should not queue the next episode when the "Still Watching?" dialog is presented until and unless the user indicates they are indeed still watching.

Did this cause any problems? Need to be careful to not delay the queuing too much otherwise the original problem from #139 can re-occur.

It's an affirmative action by the user to say they are still watching. While it's true that if the user presses the button at the very last moment before playback ends then the problem from #139 could happen, it's not a major issue from a user experience perspective.

But the alternative would be to enqueue the next episode by default and then dequeue it if the Still Watching dialog times out without the user pressing the button - but the concern there is that a similar race condition could happen where the RPC to dequeue could be processed after the next episode has already started to be played back. Again, from a user experience perspective, this would be a worse scenario than the above.

I'm not sure either scenario is better or worse, one results in failure to playback, the other in unwanted playback. Personally I think it is a bigger issue that the addon doesn't perform its main intended function, but the Still Watching? function should not be broken to achieve this.

There is also a third scenario - where the autoPlayMode has been changed from the default. This could also result in unwanted playback.

I think the only way to resolve this for all scenarios is to use an onAVStarted event handler that would stop unwanted playback.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually onAVStarted may not be required.

Can you see if https://github.com/MoojMidge/service.upnext/tree/98f6d65781f18755ab47f3bd6b24bfbbc7470340 works for you?

Should also cover the fourth scenario where unwanted playback could occur - when hitting the close button on the Still Watching? popup.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not working right. After the nth episode when the "Still Watching?" popup is displayed, I click "Continue watching" and the next episode plays immediately as expected, however UpNext does not trigger for that next episode at all.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have seen that happen before but haven't been able to reliably replicate. I believe it is related to the onPlayBackStarted event handler activating (when going to next playlist item) but before there is actually any stream to play.

Should be able to be fixed, but to help narrow down the problem can you clarify:

  1. What type of content is being played? Local? Streaming via internet? From local or remote media server?
  2. Is there any delay before playback starts once the previous file completes?
  3. Observed behaviour is that clicking "Continue Watching" results in the the next episode playing, but after that UpNext fails to activate at all. Does this happen consistently whenever "Continue Watching" is clicked?
  4. What happens when "Watch Now" is clicked from a regular UpNext notification, rather than waiting for playback to finish?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • What type of content is being played? Local? Streaming via internet? From local or remote media server?

From a Synology NAS via SMB over gigabit Ethernet and using a shared MariaDb database rather than the local database. Kodi itself is LibreELEC running on a fanless PC. Same issue also seen on a different Kodi running on a Raspberry Pi 3.

  • Is there any delay before playback starts once the previous file completes?

No, there is no perceptable delay.

  • Observed behaviour is that clicking "Continue Watching" results in the the next episode playing, but after that UpNext fails to activate at all. Does this happen consistently whenever "Continue Watching" is clicked?

Yes that's correct - it happens consistently with your build (tested a dozen or so times to be sure it wasn't an intermittent thing).

  • What happens when "Watch Now" is clicked from a regular UpNext notification, rather than waiting for playback to finish?

Just tested this and playback of next episode starts immediately as expected, but again UpNext doesn't trigger for that next episode. Again tried a few times and this is consistent.

I should note that with my original commits on this PR, which I've been using for a week or so now, everything is working fine for all my use cases 100% consistently but as you mention there is also autoPlayNextEpisode and playlists to consider (neither of which I use, so haven't tested behaviour).

I was also thinking about your comment that we should still be calling reset_queue() (but not reset state) in the onPlayBackEnded handler even when self.state.queued==True. I did code that up, and didn't encounter any change in behaviour, but was wondering if it could trigger yet another race condition in some corner case (potentially clearing the playlist before Kodi has queued up the next item?). I agree we should call reset_queue() at some point to clear the playlist but I'm thinking that will happen anyway...

Whatever happens subsequently (next episode is successfully played from the playlist, user presses STOP, some error occurs etc.), one of onPlayBackStarted, onPlayBackStopped or onPlayBackError should get triggered, all of which call reset_queue() anyway...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My setup is DAS via USB3 to Odroid N2 running CoreElec, so faster storage and client somewhere in the middle

It's interesting to get some datapoints to figure out why all these issues are occurring, the original issue from #139 was only an intermittent thing for me, but can still occur depending on how late I click the Still Watching? button with this original PR.

Just tested this and playback of next episode starts immediately as expected, but again UpNext doesn't trigger for that next episode. Again tried a few times and this is consistent.

I can only replicate this if the 5s delay onPlayBackStarted is reduced, but in doing so have noticed that there are some odd occasions where xbmc.Player().isPlaying() returns True, but xbmc.Player().getTotalTime() returns zero. Waiting for a bit longer results in getTotalTime() returning the correct duration. Perhaps caching has something to do with this? Have you changed the buffermode value to 1 or 4 by any chance? https://kodi.wiki/view/HOW-TO:Modify_the_video_cache#Cache_settings

Can you check if https://github.com/MoojMidge/service.upnext/tree/eacf14278b437720dbe84f77fabab5252ae9a3bf is any better?

I was also thinking about your comment that we should still be calling reset_queue() (but not reset state) in the onPlayBackEnded handler even when self.state.queued==True. I did code that up, and didn't encounter any change in behaviour, but was wondering if it could trigger yet another race condition in some corner case (potentially clearing the playlist before Kodi has queued up the next item?). I agree we should call reset_queue() at some point to clear the playlist but I'm thinking that will happen anyway...

This possible scenario should be covered, just didn't realise that the state reset was being done incorrectly as I never used the Still Watching? functionality. I'm guessing this was the reason for the issue reported in #135.

I can't trigger this possible scenario normally, but I did test something similar (was trying to see whether removing a playlist item would be enough to prevent the unwanted playback you were experiencing), and removing the playlist item doesn't seem to effect playback. If there was some scenario where the next item had not been queued yet, then the first playlist item gets removed (which does nothing), then the next item may or may not be queued, but reset_queue() should then get called on the next resulting onPlayBackStarted/onAVStarted.

Whatever happens subsequently (next episode is successfully played from the playlist, user presses STOP, some error occurs etc.), one of onPlayBackStarted, onPlayBackStopped or onPlayBackError should get triggered, all of which call reset_queue() anyway...

The idea behind reset_queue() was to only remove the playlist entry that was just added. If it is only called onPlayBackStopped or onPlayBackError then the playlist will continue to grow. reset_queue() during onPlayBackStarted should cover most normal scenarios, but not also the stop/error events. It would therefore need to be changed to keep track of every item added or clear the entire playlist to cover both scenarios.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have noticed that there are some odd occasions where xbmc.Player().isPlaying() returns True, but xbmc.Player().getTotalTime() returns zero. Waiting for a bit longer results in getTotalTime() returning the correct duration

Hmmm this also happens intermittently even after the onAVStarted event fires. It is also possible that this is what is preventing UpNext working with Emby/EmbyCon in some situations.

Another go at fixing the problem: https://github.com/MoojMidge/service.upnext/tree/b8edb738ff0d6fcd897d41de1d19aedb3361aab5


def onPlayBackError(self): # pylint: disable=invalid-name
"""Will be called when when playback stops due to an error"""
Expand Down