Support playing External videos #127
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hiroshisuga
suggested changes
Aug 22, 2021
| } | ||
|
|
||
| orchestrator () { | ||
| const { events, active, primaryPlaybackRate } = this.props; |
There was a problem hiding this comment.
We need to import a timestamp update function here.
const {getCurrentPlayerTime } = this.props;
| const { playing, playbackRate } = this.state; | ||
|
|
||
| let primaryPlayerPlaying = true; | ||
|
|
There was a problem hiding this comment.
It seems to me this.time has to be updated here, otherwise it stays always zero.
this.time = getCurrentPlayerTime();
| } | ||
|
|
||
There was a problem hiding this comment.
We can probably create a function to obtain the time stamp here.
getCurrentPlayerTime() {
const { time } = this.state;
return time;
}
And this function should be registered at the constructor like:
this.getCurrentPlayerTime = this.getCurrentPlayerTime.bind(this);
| videoUrl={url} | ||
| onPlayerReady={this.handlePlayerReady} | ||
| events={events} | ||
| primaryPlaybackRate={primaryPlaybackRate} |
There was a problem hiding this comment.
export the function to the component.
getCurrentPlayerTime={this.getCurrentPlayerTime}
|
Hello,
this.time is a reference to time managed in primary player. Look at
*/utils/synchronizer.js/*. handleUpdateTime is a function that is called
everytime primary
player is updated. If you stop the primary player, then this.time is not
updated and externalPlayer detects than must also stop.
this.primary.on('timeupdate', () => this.handleUpdateTime());
handleUpdateTime() {
const currentTime = this.primary.currentTime();
if (this.externalVideos && this.externalVideos.time !== currentTime)
{
this.externalVideos.time = currentTime;
}
}
You can use the url (with debug at the end):
https://bbbdemo23.f-integra.org/playback/presentation/2.3/1facfc84a74fb0957f2f1ceb4760a984925bc61b-1629192064279*?debug*
If you active verbose in your console (in the devolop tools of your
browser) you can check that this.time is updated:
El 22/08/2021 a las 7:36, hiroshisuga escribió:
…
***@***.**** requested changes on this pull request.
It worked well in my place, after adding some code (please look at my
review comments). Without these modifications, the external video
appears but does not start.
------------------------------------------------------------------------
In src/components/external-video-player/index.js
<#127 (comment)>:
> + // Seek if viewer has drifted too far away from presenter
+ if (Math.abs(this.getCurrentTime() - time) > SYNC_INTERVAL_SECOND * 0.75) {
+ player.seekTo(time, true);
+ }
+ }
+
+ componentDidMount () {
+ this.timer = setInterval(() => this.orchestrator(), ORCHESTRATOR_INTERVAL_MILLISECOND);
+ }
+
+ componentWillUnmount () {
+ clearInterval(this.timer);
+ }
+
+ orchestrator () {
+ const { events, active, primaryPlaybackRate } = this.props;
We need to bring the timestamp update function here.
|const {getCurrentPlayerTime } = this.props;|
------------------------------------------------------------------------
In src/components/external-video-player/index.js
<#127 (comment)>:
> + }
+
+ componentDidMount () {
+ this.timer = setInterval(() => this.orchestrator(), ORCHESTRATOR_INTERVAL_MILLISECOND);
+ }
+
+ componentWillUnmount () {
+ clearInterval(this.timer);
+ }
+
+ orchestrator () {
+ const { events, active, primaryPlaybackRate } = this.props;
+ const { playing, playbackRate } = this.state;
+
+ let primaryPlayerPlaying = true;
+
It seems to me this.time has to be updated here, otherwise it stays
always zero.
|this.time = getCurrentPlayerTime();|
------------------------------------------------------------------------
In src/components/player.js
<#127 (comment)>:
> }
-
+
We can probably create a function to obtain the time stamp here.
|getCurrentPlayerTime() {|
|const { time } = this.state;|
|return time;|
|}|
And this function should be registered at the constructor like:
|this.getCurrentPlayerTime = this.getCurrentPlayerTime.bind(this);|
------------------------------------------------------------------------
In src/components/player.js
<#127 (comment)>:
> + const events = video.events;
+
+ let primaryPlaybackRate = 1;
+
+ if (this.player.video) {
+ primaryPlaybackRate = this.player.video.playbackRate();
+ }
+
+ return (
+ <ExternalVideoPlayer
+ active={active}
+ intl={intl}
+ videoUrl={url}
+ onPlayerReady={this.handlePlayerReady}
+ events={events}
+ primaryPlaybackRate={primaryPlaybackRate}
send the function to the component.
|getCurrentPlayerTime={this.getCurrentPlayerTime}|
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#127 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/APQOHH5KOWDRP7SR6QHSF2TT6CEGDANCNFSM5CQFKSNA>.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>.
|
|
Hello,
remember than you have to create or update manually files similar and
copy them in your folder record (in my example
/var/bigbluebutton/published/presentation/1facfc84a74fb0957f2f1ceb4760a984925bc61b-1629192064279):
https://negra.f-integra.org/external_videos.xml
https://negra.f-integra.org/shapes.svg.txt
Anyway if it doesn't work you can send me your record files (a zip file
with all files) and I can check what it is happening in my server.
I am not a professional programmer and I greatly appreciate all the
comments to improve the code. Insist that it is an alpha version and
that it can surely be improved, optimized or rewritten in a better way.
Thank you very much for the review.
Regards
El 22/08/2021 a las 7:36, hiroshisuga escribió:
…
***@***.**** requested changes on this pull request.
It worked well in my place, after adding some code (please look at my
review comments). Without these modifications, the external video
appears but does not start.
------------------------------------------------------------------------
In src/components/external-video-player/index.js
<#127 (comment)>:
> + // Seek if viewer has drifted too far away from presenter
+ if (Math.abs(this.getCurrentTime() - time) > SYNC_INTERVAL_SECOND * 0.75) {
+ player.seekTo(time, true);
+ }
+ }
+
+ componentDidMount () {
+ this.timer = setInterval(() => this.orchestrator(), ORCHESTRATOR_INTERVAL_MILLISECOND);
+ }
+
+ componentWillUnmount () {
+ clearInterval(this.timer);
+ }
+
+ orchestrator () {
+ const { events, active, primaryPlaybackRate } = this.props;
We need to bring the timestamp update function here.
|const {getCurrentPlayerTime } = this.props;|
------------------------------------------------------------------------
In src/components/external-video-player/index.js
<#127 (comment)>:
> + }
+
+ componentDidMount () {
+ this.timer = setInterval(() => this.orchestrator(), ORCHESTRATOR_INTERVAL_MILLISECOND);
+ }
+
+ componentWillUnmount () {
+ clearInterval(this.timer);
+ }
+
+ orchestrator () {
+ const { events, active, primaryPlaybackRate } = this.props;
+ const { playing, playbackRate } = this.state;
+
+ let primaryPlayerPlaying = true;
+
It seems to me this.time has to be updated here, otherwise it stays
always zero.
|this.time = getCurrentPlayerTime();|
------------------------------------------------------------------------
In src/components/player.js
<#127 (comment)>:
> }
-
+
We can probably create a function to obtain the time stamp here.
|getCurrentPlayerTime() {|
|const { time } = this.state;|
|return time;|
|}|
And this function should be registered at the constructor like:
|this.getCurrentPlayerTime = this.getCurrentPlayerTime.bind(this);|
------------------------------------------------------------------------
In src/components/player.js
<#127 (comment)>:
> + const events = video.events;
+
+ let primaryPlaybackRate = 1;
+
+ if (this.player.video) {
+ primaryPlaybackRate = this.player.video.playbackRate();
+ }
+
+ return (
+ <ExternalVideoPlayer
+ active={active}
+ intl={intl}
+ videoUrl={url}
+ onPlayerReady={this.handlePlayerReady}
+ events={events}
+ primaryPlaybackRate={primaryPlaybackRate}
send the function to the component.
|getCurrentPlayerTime={this.getCurrentPlayerTime}|
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#127 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/APQOHH5KOWDRP7SR6QHSF2TT6CEGDANCNFSM5CQFKSNA>.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>.
|
|
Hi @amguirado73, thanks for the reply. I am neither a professional programmer (not even a computer person) and not sure at all if my modification makes sense. The facts are:
So, I still don't know if I am doing something wrong, or least likely, your PR does not contain every modification that you applied in your demo. @pedrobmarin will tell us something. Last but not least, I am very grateful with your PR. This has been very awaited function in the community. I've heard that it's technically difficult but your PR seems to have solved the issue. I cannot believe you are not professional;-) We will see if somebody changes the BBB's side. |
|
Hello,
I have just updated the code (a new commit). I used handedUpdate
function in syncronizer file but your suggestion is better and cleaner.
I have tested in my develop server and it seems to work.
Thank you very much. You can check it.
Regards.
El 22/08/2021 a las 9:50, hiroshisuga escribió:
…
Hi @amguirado73 <https://github.com/amguirado73>, thanks for the reply.
I am neither a professional programmer (not even a computer person)
and not sure at all if my modification makes sense. The facts are:
1. I know that this.time is updated in your demo. Working amazingly.
2. But when I locally implemented your code (I also created the
external_videos.xml manually but not shapes.svg which seems not
essential for external video) in my server, this.time had never
been updated. Thus, the external video appears in the playback
screen but never started.
3. Looking at the code, I found that this.time is not changed
anywhere in src/components/external-video-player/index.js (staying
zero). As this.time is a constructor variable, it has to be
modified within src/components/external-video-player/index.js (if
I am not terribly wrong..). Thus I tried to get the player's time
from src/components/player.js.
4. After these modifications, this.time started to be updated and the
external videos are played back very well.
So, I still don't know if I am doing something wrong, or least likely,
your PR does not contain every modification that you applied in your
demo. @pedrobmarin <https://github.com/pedrobmarin> will tell us
something.
Last but not least, I am very grateful with your PR. This has been
very awaited function in the community. I've heard that it's
technically difficult but your PR seems to have solved the issue. I
cannot believe you are not professional;-) We will see if somebody
changes the BBB's side.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#127 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/APQOHH3ANLQTLEVTTHV4CXTT6CT45ANCNFSM5CQFKSNA>.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>.
|
|
Would it be possible to show the control bar on the YouTube panel? Currently changing the sound volume is not possible in YouTube and we cannot hear the presenter when the YouTube video is played with a large sound. |
|
Hello,
yes, you can change youtube controls in external_video_player/index.js:
this.opts = {
// default option for all players, can be overwritten
playerOptions: {
autoplay: false,
playsinline: true,
controls: false,
},
file: {
attributes: {
controls: false,
autoPlay: false,
playsInline: true,
},
},
youtube: {
playerVars: {
autoplay: 0,
modestbranding: 1,
autohide: 1,
rel: 0,
ecver: 2,
_*controls: 0, (change to 1)*_
enablejsapi: 0,
showinfo: 0
},
},
preload: true,
};
Regards
El 23/08/2021 a las 2:32, hiroshisuga escribió:
…
Would it be possible to show the control bar on the YouTube panel?
Currently changing the sound volume is not possible and we cannot hear
what the presenter is saying when the YouTube video is played with a
large sound.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#127 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/APQOHH4MD6TSR3AJXEICSJTT6GJJFANCNFSM5CQFKSNA>.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>.
|
|
Great! Setting this value to 1 (with autohide option on) seems to be a good default setting to me. |
|
Hi,
could give me some feedback about your experience?
Regards
Thanks
El lun., 23 ago. 2021 7:53, hiroshisuga ***@***.***> escribió:
… Great! Setting this value to 1 (with autohide option on) seems to be a
good default setting to me.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#127 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/APQOHH2RBYLWHE2K7TQ6ANLT6HO33ANCNFSM5CQFKSNA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
|
|
Hi @amguirado73 , From what I saw and experieced I think this is going to the correct direction. Yet, there are several points that I would do different:
Anyway, just points to have in mind. You don't have to feel obligated to achieve all of them, I'll try to help with some of those. Thanks a lot for this initial work! |
|
Hello,
thank you very much for your feedback. I will try to revise code.
Thanks.
Regards
El mar., 7 sept. 2021 21:32, Pedro Beschorner Marin <
***@***.***> escribió:
… Hi @amguirado73 <https://github.com/amguirado73> ,
From what I saw and experieced I think this is going to the correct
direction. Yet, there are several points that I would do different:
- I'd rather use a json data file than an xml. Having to parse,
deconstruct and rebuild it's not ideal;
- using the medias' synchronizer to handle volume sync may not be the
best choice because we only use this structure when there are multiple
medias to play (webcams and screenshare);
- as we already ship a simple version of external videos (that only
adds a chat message) we'll have to be extra careful to avoid breaking the
recordings for those who already publish them with it;
- I think we will need to check if the external video is available
before playing it. Not sure what could happen if, for instance, the video
was removed from the source and we keep trying to sync it;
- The live session allows external videos from other sources. We would
have to check on those too;
- We are still missing the recording process scripts (the ones that
will build de external_video.xml/json file)
Anyway, just points to have in mind. You don't have to feel obligated to
achieve all of them, I'll try to help with some of those.
Thanks a lot for this initial work!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#127 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/APQOHH2UE57REZKXH2PVDKTUAZSFPANCNFSM5CQFKSNA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
Hello,
I have studied your feedback.
1. JSON files. Could we agree the JSON format to be compatible with
simple and this version of playback?. For example:
{
"external_videos": [
{
"start_timestamp": "173",
"stop_timestamp": "264",
"url": "htttps://www.youtube.com/watch?v=NUsoVlDFqZg",
"events": [
{ "timestamp": "174", "type": "presenterReady", "time": "0",
"rate": "0", "playing": "false" },
{ "timestamp": "174", "type": "play", "time": "0", "rate":
"0", "playing": "false" },
{ "timestamp": "178", "type": "playerUpdate", "time": "4",
"rate": "1", "playing": "true" }
]
},
{
"start_timestamp": "297",
"stop_timestamp": "337",
"url": "https://negra.f-integra.org/FredDixon.mp4",
"event": [
{ "timestamp": "298", "type": "presenterReady", "time": "0",
"rate": "0", "playing": "false" },
{ "timestamp": "299", "type": "play", "time": "0", "rate":
"0", "playing": "false" },
{ "timestamp": "302", "type": "playerUpdate", "time": "3",
"rate": "1", "playing": "true" },
]
},
]
}
2. Using the medias' synchronizer to handle volume sync. Yes this is a
error. It is easy solve in a similar way that playback rate is controlled.
3. Simple version of external videos. It is true. The code must be
backward compatible.
4. Check if external video is available before playing it. I will study
what is maden in live playing to check if a url is available.
5. Missing the recording process script. I need help is this point
because this scripts are in ruby.
I would like to add another consideration. If the external video is
buffering, the primay player still is playing and can be out of sync
with external video. The next
event playerUpdate can syncronize again the video but if your connection
is too slow, and the external video is buffering again, then can be a
problem.
Thanks.
Regards.
El 07/09/2021 a las 21:32, Pedro Beschorner Marin escribió:
…
Hi @amguirado73 <https://github.com/amguirado73> ,
From what I saw and experieced I think this is going to the correct
direction. Yet, there are several points that I would do different:
* I'd rather use a json data file than an xml. Having to parse,
deconstruct and rebuild it's not ideal;
* using the medias' synchronizer to handle volume sync may not be
the best choice because we only use this structure when there are
multiple medias to play (webcams and screenshare);
* as we already ship a simple version of external videos (that only
adds a chat message) we'll have to be extra careful to avoid
breaking the recordings for those who already publish them with it;
* I think we will need to check if the external video is available
before playing it. Not sure what could happen if, for instance,
the video was removed from the source and we keep trying to sync it;
* The live session allows external videos from other sources. We
would have to check on those too;
* We are still missing the recording process scripts (the ones that
will build de external_video.xml/json file)
Anyway, just points to have in mind. You don't have to feel obligated
to achieve all of them, I'll try to help with some of those.
Thanks a lot for this initial work!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#127 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/APQOHH2UE57REZKXH2PVDKTUAZSFPANCNFSM5CQFKSNA>.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
Hi @amguirado73 , I have found that this PR is outdated due to the massive refactoring done in the version 3 player. Would you by chance consider to refactor this PR as well so that it works on the version 3 player? I am actually using your PR on the 2.3.4 player, typing manually the xml file. Students like it very much. I hope I continue to use it in the latest player. |
|
Hi,
yes, I will try to refactor the PR but I can't specify how long it will
take me. I would like to use the same JSON format file
(external_videos.json) that 2.4 introduced.
Regards
El 17/01/2022 a las 3:35, hiroshisuga escribió:
…
Hi @amguirado73 <https://github.com/amguirado73> , I have found that
this PR is outdated due to the massive refactoring done in the version
3 player.
Would you by chance consider to refactor this PR as well so that it
works on the version 3 player?
I am actually using your PR on the 2.3.4 player, typing manually the
xml file. Students like it very much. I hope I continue to use it in
the latest player.
—
Reply to this email directly, view it on GitHub
<#127 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/APQOHH2YPUQ4U4AUZRMSTELUWN57ZANCNFSM5CQFKSNA>.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Very exciting work! Thank you! |
1 task
|
Hey guys! @amguirado73, could you please merge the |
|
Hi @GuiLeme , I have my own branch based on the one from @amguirado73 . It's working but still needs some (or a lot of) refactoring, as the implementation is old fashioned and not elegant. I made it just quickly for my students which need that function right now. If you are interested, I can post it as PR. |
|
@hiroshisuga, yeah, that would be great!! Let's just wait for a few days, maybe, and if @amguirado73 don't respond back, we follow through with your PR, and tag this PR for credits! What do you say? |
|
@GuiLeme , whatever is just fine for me. |
|
Hello, Sorry for the delay in answering. My availability to update the code is very limited. Maybe in September I can have some time but I can't guarantee it. Regards |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR gives the ability to playback external videos (youtube, mp4, ...).
It reproduces events generated in a meeting (start/stop/playbackrate). It need a xml file similar to:
https://negra.f-integra.org/external_videos.xml
Now, bbb is not able to generate this type of file but it may be adapted. I have generated this file changing bbb-html5 and include logs with external video redis messages. File shapes.svg must also be modified.
Besides events generated in meeting, this PR also lets control external video with primary controls. So if yoy start/stop/change rate or volume, the external video change.
It has been only tested with youtube and mp4 files.
You can test it in:
https://bbbdemo23.f-integra.org/playback/presentation/2.3/1facfc84a74fb0957f2f1ceb4760a984925bc61b-1629192064279