Add method for updating playback information from skills #2619

forslund · 2020-06-17T10:00:22Z

Description

This adds the method used by the npr-news-skill, spotify-skill to communicate their info to the playback control showing the default playback UI.

The method has the common parameters specified but is setup to be able to add new functionallity without core changes.

Points of discussion:

Is this the way we want to go or should we re-think it somewhat?
There are a couple of extra fields that might be added such as track length and current position to be able to show a progress bar or similar.
The status should be implemented to be able to say that playback has stopped clearing the information. Should this be a string, enum or just a bool?
Status that could be of interest is playing, paused and stopped and maybe exited so a bool
probably not fine grained enough I'd personally would probably prefer an enum for this.

How to test

Remove the matching methods from the npr-news skill and check that messages are still sent when playback starts.

Contributor license agreement signed?

CLA [ Yes ]

devops-mycroft · 2020-06-17T10:48:42Z

Voight Kampff Integration Test Succeeded (Results)

JarbasAl · 2020-06-17T15:25:52Z

as mentioned in #2614 it would be nice to send this info to audio service, not only to common play, because skills (and other stuff connected to bus) request this info from here

common_play needs this info for the gui, audio service needs this info to provide to the messagebus when requested

I would just make (all) the audio services also listen to this message and update their info, or this method can emit a seconday message to update audioservice

i like using Enum for status, but we need to think about serialization to json, it should be understandable to humans so i like using a string

i think track length and current position should be added to the method

forslund · 2020-06-17T18:24:58Z

Yes this doesn't cover the issue you describe in #2614, this is mainly to make the message that's currently being used an actual standard (or possibly not if the discussion deems this method not good enough).

I have an initial idea for a solution for the issue you described and will make a PR showing that off as soon as I get another free hour or two to prepare it.

For the enum I was thinking of using IntEnum, those can be serialized to json safely and can be de-serialized easily.

I will update this PR with the suggested additions.

JarbasAl · 2020-06-17T19:01:29Z

proposal

class CPSTrackStatus(Enum):
    DISAMBIGUATION = 1  # not queued for playback, show in gui
    PLAYING = 2  # Skill is handling playback internally
    PLAYING_AUDIOSERVICE = 3  # Skill forwarded playback to audio service
    QUEUED = 4  # Waiting playback to be handled inside skill
    QUEUED_AUDIOSERVICE = 5  # Waiting playback in audio service
    BUFFERING = 6  # Incase it's an online source the buffering state or
    STALLED = 7  # stalled state helps to know when to show the buffering ui
    END_OF_MEDIA = 8  # helps to know if we want to do autoplay or something

class CommonPlaySkill(MycroftSkill, ABC):
	# (...)
    def CPS_send_status(self, artist='', track='', album='', image='',
                        track_length="", current_position="",
                        status=CPSTrackStatus.DISAMBIGUATION, **kwargs):
        data = {'skill': self.name,
                'artist': artist,
                'album': album,
                'track': track,
                'image': image,
                'track_length': track_length,
                'current_position': current_position,
                'status': status
                }
        data = {**data, **kwargs}  # Merge extra arguments
        self.bus.emit(Message('play:status', data))

    def CPS_send_tracklist(self, tracklist=None):
        tracklist = tracklist or []
        if not isinstance(tracklist, list):
            tracklist = [tracklist]
        for track in tracklist:
            self.CPS_send_status(**track)

JarbasAl · 2020-06-17T19:03:45Z

devops-mycroft · 2020-06-17T20:43:49Z

Voight Kampff Integration Test Failed (Results)

AIIX · 2020-06-18T07:49:58Z

Just trying to get a better idea/understanding about what additional data methods are being proposed on CPS side for sending to the GUI in this discussion, as I would like to upgrade the current UI for playback skill to add support for a seekbar, thumbnail, background image and more controls like mute, repeat, autoplay and maybe a swipeable second page for searched results / playlist.

on GUI side the Mycroft.AudioPlayer component has its own state and signal handlers, but it seems the playback skill that handles common play itself does not use the media player component, so assuming we want to handle seeking by voice "fast forward 10 seconds" and seek bar in the UI to sync graphically to that position, is the above data message going to be emitted on change/update of every key field like current position ?

might be more related to playback skill itself that handles the ui, how does that interaction work in reverse for updating the data message, example on seeking physically on the ui ?

devops-mycroft · 2020-06-18T08:18:43Z

Voight Kampff Integration Test Failed (Results)

JarbasAl · 2020-06-18T08:18:56Z

Regarding AudioService and CommonPlaySkill

CommonPlaySkills should be able to manipulate tracklist
- add / reorder / update info / change track status
- self.audioservice + self.CPS_send_status
AudioService should implement all state management
Some individual Audio Backends may need refactor
- playlist needs to be kept in sync between external players and python
- same for media status + playback time
- Maybe we need a AudioBackend class for GUI Mediaplayer ?
Some Skills might want to handle their own audio and playback (spotify)
- track status should reflect this
- GUI Mediaplayer may also fall under this category

Regarding GUI

I see the GUI has having 3 swipeable pages:

Currently Playing

gui actions / intents:

these should emit mycroft.audio.service.XXX messages, see here

previous
next
pause / resume
seekbar

Playlist

A view of queued tracks selected for playback, allow user selecting a specific track + track_info

Disambiguation

Analog to playlist but with extra tracks populated during search

user can select playback, but not queued

WIP branch here

Usage Examples:

Changes:
(will edit post with list)

TODO:

new audio service method goto_track_num in addition to next/prev
new audio service method to change repeat flag
new audio service method to change shuffle/play_random flag
allow reordering tracks

forslund · 2020-06-18T08:47:21Z

Very well summarized @JarbasAI, I like your proposed ui layout.

One option would be if a skill could notify the audioservice that it's doing the playback and that the audioservice should query it for playlist and status instead of the audioservice, basically an audioservice->skill interface, this could be implemented as a separate service so the audioservice itself doesn't need to do anything special to handle the communication.

Also to note initially the playback skill ui was meant as a simple fallback if the skill didn't implement the media playback ui by itself. Now it's growing to a complete multimedia thing so the more advanced ui should definitely be based on @AIIX's ui.

devops-mycroft · 2020-06-18T15:02:46Z

Voight Kampff Integration Test Failed (Results)

AIIX · 2020-06-18T18:52:01Z

Most of the UI side stuff done using mock data and playlist model for playback skill, new UI needs wiring with CPS and new model data

For smaller type displays 400x400:

For mark-2 type displays 480x800:

For Bigscreen type displays 1920x1080:

Playlist Page

AIIX · 2020-06-18T19:09:40Z

this is the example list entry i used for playlist model:

{   
"TrackInformation": [ 
{
         "Title":"Get Lucky (feat. Pharrell Williams)",
         "Artist":"Daft Punk",
         "ImageUrl":"https://upload.wikimedia.org/wikipedia/en/7/71/Get_Lucky.jpg",
         "Duration":"3:40",
         "MusicSource":"Soundcloud"
} 
  ] }

AIIX · 2020-06-19T11:31:19Z

Have pushed the new UI to playback skill MycroftAI/skill-playback-control#31, it isn't wired up so will need to manually edit the skill to use the new player UI for now. It also consist of the mock test models in both the qml files and can be referenced to check how data is expected and assumptions.

devops-mycroft · 2020-06-22T07:19:51Z

Voight Kampff Integration Test Failed (Results)

krisgesling · 2020-07-07T12:39:32Z

mycroft/audio/services/__init__.py

+                    index = idx
+                    break
+        elif index is None:
+            index = len(self.track_data) - 1


Can we trust the length of self.track_data here?

I assume a dict is used because it may receive data on any track without the preceding track data necessarily existing?

So imagining:

it receives only data about tracks with index's of 1 and 2

then for some reason receives an index of None

Would it therefore overwrite the track_data at index 1?

I've cut this part of the PR since this was part of @JarbasAI WIP updates to the track handling in the audioservices.

This PR is now only the proposed standard methods for the the common play skills. Jarbas is/was working on a follow-up PR with those things.

I haven't had much time, but im still working on this. I think its better if thats a follow up PR 👍

This adds the function used by the npr-news-skill and spotify-skill to communicate their info to the playback control showing the default playback UI. The function has the common parameters specified but is setup to be able to add new functionallity without core changes.

devops-mycroft · 2020-07-20T12:25:09Z

Voight Kampff Integration Test Succeeded (Results)

krisgesling

Sorry it's taken me a while to get back to this. Looks like a great step forward to standardising this with the flexibility to add extra parameters as we flesh it out further.

All seems to be working well, and good documentation. Just a tiny nitpick I noticed.

krisgesling · 2020-08-10T05:35:28Z

mycroft/skills/common_play_skill.py

+            image (str): url for image to show
+        """
+        data = {'skill': self.name,
+                "uri": uri,


nitpick - double quotes

devops-mycroft · 2020-08-10T15:36:12Z

Voight Kampff Integration Test Succeeded (Results)

devops-mycroft · 2020-08-10T20:37:11Z

Voight Kampff Integration Test Failed (Results)

krisgesling · 2020-08-11T00:56:39Z

Ah looks like we're missing current_position from the default args list. Yay for automated tooling!

This commit adds the status, extended track info as well as a tracklist information as proposed by Jarbasal.

forslund · 2020-08-11T04:59:56Z

Doh! And yes very good to have test suites picking these errors up :) Fixing

devops-mycroft · 2020-08-11T05:37:25Z

Voight Kampff Integration Test Succeeded (Results)

JarbasAl · 2020-08-11T11:41:16Z

mycroft/skills/common_play_skill.py

+        Provides track data for playlist
+
+        Arguments:
+            tracklist (str/list): Tracklist data


this should be dict or list of dicts, not str/list

JarbasAl · 2020-08-11T11:42:29Z

mycroft/skills/common_play_skill.py

+            uri (str): uri for track
+            track_length (float): track length in seconds
+            elapsed_time (float): current offset into track in seconds
+            playlist_postion (int):


typo, position not postion, also missing an explanation of this argument

AIIX · 2020-09-22T13:37:14Z

Most of the UI side stuff done using mock data and playlist model for playback skill, new UI needs wiring with CPS and new model data

For smaller type displays 400x400:

For mark-2 type displays 480x800:

For Bigscreen type displays 1920x1080:

Playlist Page

@dschweppe screenshots for the horizontal media player

devops-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Jun 17, 2020

forslund force-pushed the feature/common-play-inform-gui branch 2 times, most recently from 7161fca to 8bc56d7 Compare June 18, 2020 07:17

AIIX mentioned this pull request Jun 19, 2020

Add updated player and playlist UI MycroftAI/skill-playback-control#31

Closed

krisgesling reviewed Jul 7, 2020

View reviewed changes

forslund force-pushed the feature/common-play-inform-gui branch from 8bc56d7 to 116b2e5 Compare July 20, 2020 11:49

krisgesling approved these changes Aug 10, 2020

View reviewed changes

forslund force-pushed the feature/common-play-inform-gui branch from 116b2e5 to 7142664 Compare August 10, 2020 14:59

forslund force-pushed the feature/common-play-inform-gui branch from 7142664 to d7116b9 Compare August 10, 2020 19:34

krisgesling added the Status: To be reviewed Concept accepted and PR has sufficient information for full review label Aug 11, 2020

Status and track list suggested by @JarbasAl

8f4847f

This commit adds the status, extended track info as well as a tracklist information as proposed by Jarbasal.

forslund force-pushed the feature/common-play-inform-gui branch from d7116b9 to 8f4847f Compare August 11, 2020 05:00

krisgesling merged commit 55cd624 into MycroftAI:dev Aug 11, 2020

forslund deleted the feature/common-play-inform-gui branch August 11, 2020 07:22

JarbasAl reviewed Aug 11, 2020

View reviewed changes

This was referenced Aug 18, 2020

cps media type MycroftAI/skill-npr-news#90

Closed

cps track status MycroftAI/skill-playback-control#35

Closed

JarbasAl mentioned this pull request Jun 4, 2021

feat/gui video common play #2683

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add method for updating playback information from skills #2619

Add method for updating playback information from skills #2619

forslund commented Jun 17, 2020

devops-mycroft commented Jun 17, 2020

JarbasAl commented Jun 17, 2020 •

edited

Loading

forslund commented Jun 17, 2020

JarbasAl commented Jun 17, 2020 •

edited

Loading

JarbasAl commented Jun 17, 2020

devops-mycroft commented Jun 17, 2020

AIIX commented Jun 18, 2020

devops-mycroft commented Jun 18, 2020

JarbasAl commented Jun 18, 2020 •

edited

Loading

forslund commented Jun 18, 2020

devops-mycroft commented Jun 18, 2020

AIIX commented Jun 18, 2020

AIIX commented Jun 18, 2020

AIIX commented Jun 19, 2020

devops-mycroft commented Jun 22, 2020

krisgesling Jul 7, 2020

forslund Jul 20, 2020 •

edited

Loading

JarbasAl Jul 20, 2020

devops-mycroft commented Jul 20, 2020

krisgesling left a comment

krisgesling Aug 10, 2020

devops-mycroft commented Aug 10, 2020

devops-mycroft commented Aug 10, 2020

krisgesling commented Aug 11, 2020

forslund commented Aug 11, 2020

devops-mycroft commented Aug 11, 2020

JarbasAl Aug 11, 2020

JarbasAl Aug 11, 2020

AIIX commented Sep 22, 2020

For smaller type displays 400x400:

For mark-2 type displays 480x800:

For Bigscreen type displays 1920x1080:

Playlist Page

Add method for updating playback information from skills #2619

Add method for updating playback information from skills #2619

Conversation

forslund commented Jun 17, 2020

Description

How to test

Contributor license agreement signed?

devops-mycroft commented Jun 17, 2020

JarbasAl commented Jun 17, 2020 • edited Loading

forslund commented Jun 17, 2020

JarbasAl commented Jun 17, 2020 • edited Loading

JarbasAl commented Jun 17, 2020

devops-mycroft commented Jun 17, 2020

AIIX commented Jun 18, 2020

devops-mycroft commented Jun 18, 2020

JarbasAl commented Jun 18, 2020 • edited Loading

WIP branch here

forslund commented Jun 18, 2020

devops-mycroft commented Jun 18, 2020

AIIX commented Jun 18, 2020

For smaller type displays 400x400:

For mark-2 type displays 480x800:

For Bigscreen type displays 1920x1080:

Playlist Page

AIIX commented Jun 18, 2020

AIIX commented Jun 19, 2020

devops-mycroft commented Jun 22, 2020

krisgesling Jul 7, 2020

Choose a reason for hiding this comment

forslund Jul 20, 2020 • edited Loading

Choose a reason for hiding this comment

JarbasAl Jul 20, 2020

Choose a reason for hiding this comment

devops-mycroft commented Jul 20, 2020

krisgesling left a comment

Choose a reason for hiding this comment

krisgesling Aug 10, 2020

Choose a reason for hiding this comment

devops-mycroft commented Aug 10, 2020

devops-mycroft commented Aug 10, 2020

krisgesling commented Aug 11, 2020

forslund commented Aug 11, 2020

devops-mycroft commented Aug 11, 2020

JarbasAl Aug 11, 2020

Choose a reason for hiding this comment

JarbasAl Aug 11, 2020

Choose a reason for hiding this comment

AIIX commented Sep 22, 2020

For smaller type displays 400x400:

For mark-2 type displays 480x800:

For Bigscreen type displays 1920x1080:

Playlist Page

JarbasAl commented Jun 17, 2020 •

edited

Loading

JarbasAl commented Jun 17, 2020 •

edited

Loading

JarbasAl commented Jun 18, 2020 •

edited

Loading

forslund Jul 20, 2020 •

edited

Loading