feat/gui video common play #2683
Conversation
devops-mycroft
added
the
CLA: Yes
|
Voight Kampff Integration Test Succeeded (Results) |
1 similar comment
|
Voight Kampff Integration Test Succeeded (Results) |
There was a problem hiding this comment.
I think this looks good I think. The thing I notice is that unlike the show_* this doesn't clear the current namespace when starting to show video.
We might want to think about how the show_* methods clears the namespace...It may cause skill creators some headache if they mix a show_* with normal pages. But that's outside of the scope of this PR.
I made a couple of minor comments, the missing docstring should be added before merge. And I'll leave the qml to be reviewed by @AIIX (since I'm terrible with it)
|
seems like we are moving to a centralize playback module per https://docs.google.com/document/d/1ebS60QLna4_AOQxQYpoaZrpkBzPXjL-7VFPJ7-xVvIQ/edit maybe this no longer makes sense if its going to be replaced soon? |
|
My thinking is that this could still be of use since the playback module described there would need to talk to the gui in some fashion and this is the best api for that. Edit: This is my humble opinion we should probably hear @krisgesling's thoughts on the matter |
|
IMHO, I think it will be safer to get the architecture nailed down first then we can be sure about the changes. |
i still think this should be considered, it's a simple and straightforward change, i don't see the SPEC going in any direction where this change will be invalid furthermore the SPEC is in very early stages and any implementation is not even being considered much less assigned, we are talking about delaying this change for months without any alternative available if this is the official stance i will probably be closing this PR and adding this to my jarbas_utils package and start importing it in my skills, otherwise i will be duplicating a lot of code and have to maintain it in sync across many repos when the SPEC gets off the idea stage into something concrete i will gladly update this EDIT: clarification, by update i mean that i will re-open the PR (if rejected now) or make a new one with any changes needed according to SPEC, essentially i am volunteering to maintain this functionality |
|
Voight Kampff Integration Test Succeeded (Results) |
|
Voight Kampff Integration Test Succeeded (Results) |
1 similar comment
|
Voight Kampff Integration Test Succeeded (Results) |
| function formatTime(timeInMs) { | ||
| if (!timeInMs || timeInMs <= 0) return "0:00" | ||
| var seconds = timeInMs / 1000; | ||
| var minutes = Math.floor(seconds / 60) |
There was a problem hiding this comment.
Should we add in an hours that is only shown if time > 1hr.
Would also need to pad the minutes if (hours > 0 && minutes < 10)
There was a problem hiding this comment.
still need to implement this, If i understand correctly an hour would be represented like "1:05:04" "H:M:S" format, and on duration change once it changes below an hour on displayed video would still be represented like "0:50:04" "H:M:S" format or "50.04" "M:S" format ?
There was a problem hiding this comment.
Yeah, good question, I hadn't thought of that but I like the idea of keeping the "H:M:S" format if the total duration is > 1hr but the time remaining drops below 1:00:00.
Where as if the total duration was < 1hr it would only ever show the "M:S" format.
I think it's less cognitive load for the user, it indicates that we're under an hour to go, and less chance of strange things happening when rendering the time remaining to screen.
@dschweppe do you have an opinion?
|
CC-licensed icons can be found at thenounproject.com. You can choose CC-BY
for free or commercial with no attribution resulted for a couple USD.
…On September 21, 2020 6:20:03 AM Aditya Mehra ***@***.***> wrote:
@AIIX commented on this pull request.
In mycroft/res/ui/SeekControl.qml:
> + highlighted: focus ? 1 : 0
+ z: 1000
+ + background: Rectangle {
+ radius: 200
+ color: Qt.rgba(0, 0, 0, 0.9)
+ border.width: 1
+ border.color: "white"
+ }
+ + contentItem: Item {
+ Image {
+ width: parent.width > 600 ? Kirigami.Units.iconSizes.medium :
Kirigami.Units.iconSizes.small
+ height: width
+ anchors.centerIn: parent
+ source: "images/back.png"
it can use an svg but are there any svg's available for this, not sure if
we can just pick something of the internet cause of potential license
issues or is there any creatives common license compatibility that too
should be good
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
Created a media player Icon Set that can be freely used with a CC0 1.0. license, free for commercial use and no attributions required. https://github.com/AIIX/SimpleMedia-Icons/ |
|
Voight Kampff Integration Test Failed (Results) |
|
Voight Kampff Integration Test Succeeded (Results) |
|
Voight Kampff Integration Test Succeeded (Results) |
|
Hey @AIIX I've just done some minor clean up and added a series of tests, if you could double check it, I think we're good to go. |
krisgesling
added
Status: To be reviewed
|
Voight Kampff Integration Test Succeeded (Results) |
1 similar comment
|
Voight Kampff Integration Test Succeeded (Results) |
|
@ken-mycroft - from our brief discussion today - here's the spec I mentioned: As suggested by the spec, I think we should be moving toward an integrated "media" playback service and that Skills should interact with that so the system can more easily know and express it's own state. Either way the GUI needs a way to play that video, so I think this is moving in the right direction however in this PR's current state the interface for playing a video would be directly in the GUI module. |
|
Please forgive me for my ignorance, but why could I not simply add a fourth and fifth type (.mov and .mp4) to the existing simple audio service and let it figure out which player to use like it does now with .mp3 vs .wav (and I believe ogg). In this way nothing above it would change and it would conform to existing commands like play, stop, etc. We would simply need to add a new config value (mp4_cmd=qvlc) and add the qvlc plugin (for QT environments) to the requirements.txt file. Skills would continue to play media the same way they do now, or am I missing something very basic here? |
Mycroft-Core handles audio playback externally via VLC or whatever different from the Qt implementation, The same is not applicable to videos and not even possible to do as any video playback directly requires a video surface creation and this is directly handled with Qt's official multimedia backend and video qml type https://doc.qt.io/qt-5/qml-qtmultimedia-video.html which assumes all control and responsibility for video playback of the streaming or local video using Linux's multimedia gstreamer provider. So the major difference between audio and video via CPS is:
Qt does not officially support qvlc and qml vlc extensions are not mature and don't seem even maintained anymore. Additionally there is no support for opening external application windows and controlling external Linux application window behavior. |
|
Additionally most of the spec sheet being mentioned here has already been implemented by @JarbasAl in another project and can be found here https://github.com/OpenVoiceOS/OVOS-workshop/blob/master/ovos_workshop/frameworks/cps/__init__.py |
|
Quick question, is this https://github.com/OpenVoiceOS/OVOS-workshop/blob/master/ovos_workshop/frameworks/cps/__init__.py included in this PR? My concern is backward compatibility in non qt environments and this code seems to handle that, albeit in an interesting way. |
No I believe that implementation by @JarbasAl https://github.com/OpenVoiceOS/OVOS-workshop/blob/master/ovos_workshop/frameworks/cps/__init__.py is a total refactor of the CPS framework based on the spec sheet with few more extensions, This PR is only meant to handle show_video in the Qt context currently based on the current CPS implementation in mycroft-core, any kinds of backward compatibility etc should probably be handled separately under some other PR. As @JarbasAl was the original author of this PR he might know better. There were also some additional PR's by Jarbas for CPS spec implementation but were closed. I do not know if the above refactor done under OVOS relates but some of them are: |
|
Just want to note that I tried to do this upstream across several prs, due to lack of interest and feedback I moved dev to ovos and am not following that spec, the ovos repo is not a great reference for this specific PR since it does so much more things Not sure on status of this pr, but it should be quite basic and one of the first pieces, no conflicts with any of the other work I did |
|
@ken-mycroft Here is what i could find, these are the PRs related to the better common play framework that i made before deciding to make this under https://github.com/OpenVoiceOS/OVOS-workshop . There are some more PRs in some of the official skills also (news skill ?)
These PRs partially informed the SPEC linked above |
|
closing this PR in favor of the new audio backend plugin this PR is very old and outdated |
Starts integrating gui video playback into the common play framework
see usage here https://github.com/JarbasSkills/skill-dagon/blob/feat/gui_common_play/__init__.py#L124