Skip to content

Add Timed Playlist feature and minor refactors#217

Merged
hburgund merged 6 commits intoroundware:developfrom
13rac1:187/duration-audio
Mar 14, 2015
Merged

Add Timed Playlist feature and minor refactors#217
hburgund merged 6 commits intoroundware:developfrom
13rac1:187/duration-audio

Conversation

@13rac1
Copy link
Copy Markdown
Member

@13rac1 13rac1 commented Feb 10, 2015

Initial pull request for discussions of new feature. This code works, but needs some additional features and unit tests.

@hburgund
Copy link
Copy Markdown
Member

OK, did some more testing on this and the behavior seems to still be what I indicated before:

  • works like a charm when there are other assets that cause the timed playlist assets to be checked for and triggered
  • but when there are no assets and only move_listeners happening, the timed playlist assets are never triggered

I’ll have a closer look at the code tomorrow, but I’m thinking this is just something simple that needs tweaking so that move_listener triggers the code that checks for the timed assets…. _update_playlist_timed.

@hburgund
Copy link
Copy Markdown
Member

My testing methodology:

Start a stream with request_stream in a location where there were no assets located and then send a bunch of move_listener calls during the timed assets ranges and I got no assets playing nor did the elapsed time log statement print.

@hburgund
Copy link
Copy Markdown
Member

OK, I think I may have made some progress. I changed the move_listener function in recording_collection to this:

def move_listener(self, request):
    self.lock.acquire()
    self._update_playlist_proximity(request)
    self._update_playlist_timed()
    self.lock.release()

Just adding the line to update the playlist_timed. The timed playlist assets do now play in some very quick tests, but need to test more. It now looks like the `_get_recording`` function is being run twice as I see the “Elapsed Time:” log statement twice when I didn’t see it at all before.

22:38:16 DEBUG <roundwared.stream.move_listener:131> move_listener(1.1,1.0)
22:38:16 DEBUG <roundwared.recording_collection._update_playlist_proximity:184> Timeout banned assets and seconds remaining: {}
22:38:16 DEBUG <roundwared.recording_collection._update_playlist_proximity:186> Proximity banned assets: []
22:38:16 DEBUG <roundwared.recording_collection._update_playlist_proximity:195> Ordering is: random
22:38:16 DEBUG <roundwared.asset_sorters.order_assets_randomly:45> Ordering Assets Randomly. Input: []
22:38:16 DEBUG <roundwared.asset_sorters.order_assets_randomly:47> Ordering Assets Randomly. Output: []
22:38:16 DEBUG <roundwared.recording_collection._update_playlist_timed:228> Elapsed Time: 70.4130589962
22:38:16 DEBUG <roundwared.recording_collection._update_playlist_timed:239> Found timed assets: [<Asset: 26: audio at 2.0/1.0>]
22:38:16 DEBUG <roundwared.recording_collection._update_playlist_timed:228> Elapsed Time: 70.4187948704
22:38:16 DEBUG <roundwared.recording_collection._update_playlist_timed:239> Found timed assets: [<Asset: 26: audio at 2.0/1.0>]
22:38:16 DEBUG <roundwared.recording_collection.get_recording:92> Found 20150107-130030-1.wav
22:38:16 INFO <roundwared.audiotrack.add_file:135> Session 1 - Playing asset 26 filename: 20150107-130030-1.wav, duration: 10.01 secs

So it seems some progress, but clearly still at the very least cleaning up to do. I’ll look more tomorrow, but let me know what you think. Is my new line of code somehow triggering the code you thought would already update the timed playlist items due to move_listener? And hence it’s getting run twice now?!

@hburgund
Copy link
Copy Markdown
Member

from @eosrei:

Ok. I understand what you are doing, but this doesn't make it work any better.​

I didn't put the code in move_listener, because the timed playlist has nothing to do with moving the listener.

Here is how it works:

  • dbus_receive.add_signal_receiver - Gets the call from dbus. It calls stream.move_listener()
  • stream.move_listener - Calls recording_collection.move_listener() which calls recording_collection._update_playlist_proximity()
  • stream.move_listener - Calls audiotrack.move_listener() which calls recording_collection.add_file()
  • recording_collection.add_file - Calls recording_collection.get_recording()
  • recording_collection.get_recording - Calls recording_collection._get_recording()
  • recording_collection._get_recording - Calls recording_collection._update_playlist_timed()
  • If recording_collection.playlist_timed contains anything, it's returned.
  • If not, if recording_collection.playlist_proximity contains anything, it's returned.

Yada yada yada, recording_collection._update_playlist_timed is called.

@hburgund
Copy link
Copy Markdown
Member

My change very likely is not the right approach to take, but it does in my testing cause the timed playlist assets to play when a move_listener happens during their time range (and nothing else is playing currently). I guess I’m circumventing the proper signal chain that you laid out below (super helpful, thanks), but for some reason, move_listener is otherwise not triggering the timed playlist items in my tests.

We also need to add something later to handle the situations that I think would still not work properly even when move_listener triggering is all set.

  • global listen projects in which there are no move_listeners, so we have to rely entirely on other assets playing to trigger the timed playlist check, which may have some issues
  • geo-listen projects in which the listener isn’t moving and there aren’t lots of assets where they are located

I am not worried about the global-listen projects now since that’s not the immediate use case and there are some other issues with global listening left over from earlier. But the specific situation with geo-listen projects should eventually be addressed, though once I start using what we’ve got more, I’ll have a lot more info with which to inform how we handle this.

@hburgund
Copy link
Copy Markdown
Member

from @eosrei:

This con​cerns me, because the only reason that would make a difference is if there is something drastically wrong. It's quite explicit in the code.

​There is an issue with Geo Listen in general. IDK how it worked correctly before. Seems like it could never work.​

It seems like we should replace the audio playing code. It currently runs after after an audio file stops playing or after move_listener. To correctly support this use case of no move_listeners and non-continuous tracks, I think it should just be a constant timer.

Once a second:

  • Is something playing? Good! Exit.
  • No? Has it been less seconds than a random number between the ranges I just made up? Good! Exit.
  • No? OMG! PLAY SOMETHING!!!!

​This would allow you to easily force "constant timing" too.

@hburgund
Copy link
Copy Markdown
Member

Well, maybe I’ve just done something totally wacko with my testing…always a possibility. I can try to do another fresh test.

I like your timer idea. Seems to me since all of RW is related to time since it’s audio, that having some kind of overall timing mechanism in place would be helpful for a bunch of different features. But we don’t need to do this right now as that seems like a bit of a drastic code change.

@13rac1
Copy link
Copy Markdown
Member Author

13rac1 commented Feb 10, 2015

Eh.. It's not that drastic IMO and it's also the only reasonable solution I'm seeing for this specific issue.

@13rac1
Copy link
Copy Markdown
Member Author

13rac1 commented Feb 11, 2015

track_timer audiotrack manager should make this all work correctly, but unit tests are still needed before this can/should be merged. You can use it in a one-off project though.

hburgund added a commit that referenced this pull request Mar 14, 2015
Add Timed Playlist feature and minor refactors
@hburgund hburgund merged commit c20539a into roundware:develop Mar 14, 2015
@13rac1 13rac1 deleted the 187/duration-audio branch March 14, 2015 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants