Add support for EchoVideos from Echo360 as source #90

echo360-damyon · 2023-09-20T05:49:30Z

At Echo360 (https://echo360.com) we have been asked by our customers to allow videos captured in our EchoVideo platform to be able to be included in H5P content. We have added these features in our test environment and they will be released generally at the end of the year.

For this to work in the same way as other supported video platforms it will require changes to both this repository as well as the H5P Editor php library. Our changes are modelled after the other video platforms.

otacke · 2024-02-13T14:30:47Z

@echo360-damyon Hi! I just wanted to let you know that I have been asked to review your pull request(s). I am going to use github's regular review features unless you prefer a different way.

echo360-damyon · 2024-02-13T15:10:00Z

Thanks, that's fine with me.

otacke

Hi again!

Thanks for your contribution in the name of H5P Group. They asked me to do an initial check up, so I deployed your code, ran some real world tests and had a look at your code for obvious things that might need to be taken care of. Here are my observations, leading to a potential need for changes. I am also going to report my observations to H5P Group and they will ultimately need to decide what changes feel to be necessary.

Functional

I used the new code in two different scenarios: As subcontent in H5P.Column, which leaves controls to H5P.Video and as pseudo subcontent in H5P.Interactive Video which has its own set of controls, so H5P.Video does not need to provide any.

The player does not only show the video, but (depending on the display ratio) also displays a black margin around the video, a name in the upper left-hand corner and some video chooser in the upper right-hand corner. If you compare this to the other video handlers, e.g. for HTML (https://h5p.org/interactive-video) or YouTube, then you will notice that the video should take up the whole width (in most cases). Extra visual elements (such as the video name or extra controls) should be put in a layer on top of the video (to be shown/hidden at your pleasing, e.g. on hover). I am not aware whether H5P Group would accept the way videos appear in your case - still waiting on an answer from them.
Your handler does not support all of the features that H5P.Video offers to authors (and needs to ensure that every handler can be used in every context). Your handler will be passed a couple of parameters in the 2nd arguments of your constructor function:

{boolean} controls: If true (default), then the handler should display controls to the user, typically play/pause, a seekbar, a time code, a volume control and a playbackrate control. If false, then those must not be present and then and only then the content type that instantiated H5P.Video is responsible for offering controls and then uses the handler functions to relay commands to the actual player. Your handler (which leads to the extra H5P sharing URL) does never have controls (unlike your "public" sharing URL). The user can only click on the video to play/pause it.
{boolean} autoplay: If true, the handler should try to auto play the video - which will of course cannot overcome a browser's media policies. Your handler does not handle that parameter and never plays automatically.
{boolean} loop: If true, the handler should restart the video once it ended. Your handler does not handle that parameter and never loops the video.
{boolean} fit: Seems to be supported.
{object} poster: Image that should be displayed instead of the video before it is played. Your handler doesn't support this, but I think it's not mandatory.
{object}[] tracks: If set, the handler should display the respective subtitle tracks in order to allow people with hearing disabilities to follow the video content. This one is probably optional, because the YouTube handler cannot support tracks.
{boolean} disableRemotePlayback: If true, the player is not allowed to have a remote playback UI. You're not handling this, but probably don't have to anyway. And this should be optional.
{boolean} disableFullscreen: If true and the player's controls allow the user to go to full screen, that very control should be disabled. Your H5P sharing URL doesn't show a full screen button, but your regular sharing URL does. So, if you implement the controls, you should add an option to disable the full screen button.
{boolean} deactivateSound: If true the video should be played muted and if the player's controls allow the user to unmute the video or change the volume, the respective controls should be disabled. So, if you implement the controls, you may want to take care of this.
{number} startAt: If set, the video should start at the given time in seconds. Your handler seems to take care of that, but I am not entirely sure about some details, see below.

If I try to play/pause the video by clicking on it, I sometimes need to click twice or even multiple times to see an effect. Using the handler's play function seems to work reliably though.
There's a slight but noticeable delay before the player reacts to a click; not with the YouTube handler, for instance, which also deals with remotely hosted videos.
Resizing works fine, but one can see quite some flickering.
Resuming (restarting at the time code that the user left off) works fine in Interactive Video, but not when run in Column. I didn't try to locate the issue, but potentially handling the options.startAt value does not work as expected while in Interactive Video, the latter probably calls seek to achieve the same effect.
Changing the playbackrate from within Interactive Video triggers some weird effects. The rate between the player and Interactive Video seems to get out of sync, the video suddenly stops, etc. Could be an async issue.
When using the play button of Interactive Video or the handler's play function directly, the video will not play with audio and calling unmute also doesn't have an effect. Audio will only play if the user clicks on the video to play it.

Code

I left a couple of comments in the code. Nothing serious. Given that there's some functionality that will need changing, a 2nd round may be necessary though. We can cross that bridge when we get to it.

The H5P coding guidelines seems to not have been consulted (https://h5p.org/documentation/for-developers/coding-guidelines). Yes, I am aware that older code of H5P Group may not adhere to it fully :-) If you want some eslint configuration that should enable eslint to fix most of the things automatically: https://github.com/NDLANO/eslint-config-ndla-h5p/blob/master/.eslintrc.js
Using jQuery. This is something that you could not have known, but H5P Group is about to remove jQuery from H5P core and thus doesn't want newly added code to use it so it won't have to be migrated to vanilla JavaScript later on.
There are some unused variables/functions left in the code.
You're using arrow functions which is more then fine, but you are mixing in some traditional "function" functions as well. Those can be replaced if not really needed (the constructor and the self-invoking function would cause trouble though or require more work, those should be fine as is).
Please add some line breaks in between functions and maybe in some other places to help distinguish blocks of code when reading it.
A public function isLoaded(): boolean is missing, see comments. But again, this is not something that's really documented by H5P Group ...
Some JSDoc nitpicking ;-)

Hope my comments help you!

otacke · 2024-02-14T13:53:12Z

scripts/echo360.js

+   * @param {Object} l10n Localization strings
+   */
+  function EchoPlayer(sources, options, l10n) {
+    const self = this;


Keeping track of this by introducing self (or similar) is not required when using arrow functions.

I removed all references to self.

otacke · 2024-02-14T13:56:56Z

scripts/echo360.js

+   * @private
+   * @returns {Promise} Echo Player SDK object
+   */
+  const loadEchoPlayerSDK = async () => {


This function does not seem to be used anywhere.

otacke · 2024-02-14T14:02:18Z

scripts/echo360.js

+    let ratio = 9/16;
+    const LOADING_TIMEOUT_IN_SECONDS = 30;
+    const id = `h5p-echo-${++numInstances}`;
+    const $wrapper = $('<div/>');


Please do not use jQuery but vanilla JavaScript unless you're answering a call from H5P core that still expects it or if you get it from H5P core as a parameter, e.g. in the attach function. H5P Group is about to remove jQuery from H5P core and wants to limit places throughout their code base that they have to touch to remove usage of it.

Not many uses of jQuery should remain here, and the jQuery object doesn't need to be passed down as $ in the self-evoking function wrapper, but it can be called as H5P.jQuery directly.

I removed all uses of JQuery except where it's passed in as a parameter to a method.

otacke · 2024-02-14T14:05:49Z

scripts/echo360.js

+    let currentTextTrack;
+    let currentTime = 0;
+    let duration = 0;
+    let isMuted = 0;


This should be a boolean.

It is a boolean now.

otacke · 2024-02-14T14:06:08Z

scripts/echo360.js

+    let currentTime = 0;
+    let duration = 0;
+    let isMuted = 0;
+    let volume = 0;


The default volume should be 1.

otacke · 2024-02-14T14:42:31Z

scripts/echo360.js

+     *
+     * @public
+     */
+    self.play = async () => {


Why async? I don't think it will ever be called asynchronously.

I removed the async

otacke · 2024-02-14T14:49:26Z

scripts/echo360.js

+    };
+    /**
+     * @public
+     * @returns {Number} Video duration in seconds


Can also return undefined.

Updated the JSDoc.

otacke · 2024-02-14T14:52:00Z

scripts/echo360.js

+      isMuted = false;
+    };
+    /**
+     * Whether the video is muted.


Please use common "command" scheme for function descriptions, so here e.g. Determine whether ...

I added better function descriptions / params and return types for all functions.

otacke · 2024-02-14T15:21:38Z

scripts/echo360.js

+    self.getCaptionsTrack = () => {
+      return currentTextTrack;
+    };
+    self.on('resize', () => {


A public function isLoaded(): boolean is missing. It's supposed to return true if the player that the handler introduces is done loading (in your case when the loadingPromise resolves) and false otherwise.

I know, this is not documented anywhere - but you might have noticed some error message when using your code in InteractiveVideo without that function.

I added isLoaded which resolves true when the promise is completed.

otacke · 2024-02-14T16:28:48Z

scripts/echo360.js

+    const $wrapper = $('<div/>');
+    const $placeholder = $('<div/>', {
+      id: id,
+      html: `<div class="h5p-video-loading" style="height: 100%; min-height: 200px; display: block; z-index: 100;" aria-label="${l10n.loading}"></div>`


Setting border: none; seems advisable to get rid of the (potential) default border that a browser may put there.

I added this style.

echo360-damyon · 2024-02-22T03:33:59Z

Thanks Oliver,

Responses to the points listed as Functional:

"video chooser in the upper right-hand corner"

This is because Echo 360 Videos can have multiple video tracks and the user can switch between them while they watch the recording. E.g. talking head and powerpoint slides.

Features / options

{boolean} controls:

I have added this to the query string in the iframe url - in future we can update the Echo360 to follow this option.

{boolean} autoplay:

I have added this.

{boolean} loop:

I have added this.

{object} poster:

Echo Video has it's own media editor where the poster image can be set.

{object}[] tracks:

Echo Video has it's own system for handling captioning for media including paid for captioning by third party providers.

{boolean} disableFullscreen:

I have added this to the query string in the iframe url - in future we can update the Echo360 to follow this option.

{boolean} deactivateSound:

I have added this to the query string in the iframe url - in future we can update the Echo360 to follow this option.

{number} startAt:

The handler performs a seek when the video is loaded if this option is present.

Resuming (restarting at the time code that the user left off) works fine in Interactive Video, but not when run in Column.

I'll investigate column mode in more detail today.

Changing the playbackrate

The timing sync of the playback rates is under active development at the moment and this is not specific to h5p.

When using the play button of Interactive Video or the handler's play function directly, ... Audio will only play if the user clicks on the video to play it.

I have addressed this by adding allow="fullscreen, autoplay" to the iframe which grants permissions to play the audio.

echo360-damyon · 2024-02-22T06:26:46Z

With regards to the Column mode - when testing I noticed that the patch for echo-h5p-editor-php-library looks correct against the current version on master branch - but when used with an older version (e.g. on h5p.org directly) - it requires additional changes to load the video the same way as youtube does it.

If you search for Youtube in the scripts/h5peditor-av.js file - you can see it has special handling if the source provider is youtube. If used against the older library - those checks will need to be repeated for EchoVideo.

otacke

@echo360-damyon Thanks for all the changes in the name of H5P Group. Just letting you know: I don't know yet if I will continue working on this review or of they're going to take over. Either way, someone should reach out to you soon.

Use updated quality options

…logic PLT-2090: Update quality option mapping

numInstances is supposed to reflect the number of instances of the handler for a unique element id, so it cannot be an instance property

Would cause trouble downstream, e.g. in Interactive Video

otacke · 2024-03-25T16:05:12Z

@echo360-damyon I was asked by H5P Group to have a 2nd look. I think I found some issued that I decided to fix directly with a short comment per commit instead of leaving extra comments that take longer to write. Please find echo360-damyon#1.

Apart from that, I think that it's your player that needs changing now. Unless you agreed on something else with H5P Group, I think you may want to check:

Video should use the full player canvas. Additional platform features should be added in an overlay as in YouTube or Vimeo, not next to the video.
When videos run standalone, e.g. in Column, then they should receive proper controls (unless deactivated in the options), so “play”, “pause”, “seek”, “mute”, “full screen” + ”current time / duration” (optionally playback rate)
When videos run inside Interactive Book (or any other parent content type which requests to deactivate the full screen button), no full screen button should be visible.
When videos are run inside Interactive Video, when seeking, the knob jumps back to the original position briefly and then jumps back to the requested position. It feels a little odd (doesn’t happen for YouTube, for instance) and should probably be fixed, but that’s up to the UX team of H5P Group to decide.
When resizing, there’s some noticeable flickering going on and should probably be fixed, but that’s up to the UX team of H5P Group to decide.
When running the video standalone where one currently needs to click on the video to play/pause it, this click doesn’t always seem to be registered properly. One sometimes needs to click multiple times to see an effect.

Fix 2nd code review issues

echo360-damyon · 2024-04-09T05:48:27Z

Hi @otacke, thanks for the review again.

I have merged in your changes to the PR.

Also any testing against our QA server would have been hard recently because our QA system is changed between development and release regression testing as needed. It has just been reset to the latest release and will be acceptable to test against for a short time (not determined - probably a few weeks).

Here are responses to your further comments:

"Video should use the full player canvas..."

This is not reasonable. Our player has a lot of features you aren't aware of and it's not feasible to re-implement the controls as an overlay.

"When videos run standalone, e.g. in Column, then they should receive proper controls..."

They are if the option is set to display controls - this may not have appeared not to work because of the version on our QA environment when you tested.

"When videos run inside Interactive Book ... no full screen button should be visible."

It is not visible - this may not have appeared not to work because of the version on our QA environment when you tested.

"When videos are run ...seeking jumps back and forward"

This happens in our normal player even outside of H5p and is in the backlog of things which we will address in time.

"When resizing, there’s some noticeable flickering"

There is is minimal flickering while resizing - this can be addressed in future but is not in scope of this H5P change.

"When running the video standalone...this click doesn’t always seem to be registered properly. "

This was addressed and is to do with the iframe permissions.

otacke · 2024-04-09T08:17:01Z

@echo360-damyon Thanks for getting back to me.

"Video should use the full player canvas..."

This is not reasonable. Our player has a lot of features you aren't aware of and it's not feasible to re-implement the controls as an overlay.

That's not up for me to decide, and I was not part of your discussions with H5P Group. I merely compared your solution to the four other handlers and those fill the whole Canvas. That's what users of H5P are used to. If that's something that's not reasonable on your end, then you probably either have discussed this with H5P Group already (that's why I mentioned "Unless you agreed on something else with H5P Group [...]"), or their UX team will probably reach out to you anyway if they have an issue with that.

"When videos run standalone, e.g. in Column, then they should receive proper controls..."

They are if the option is set to display controls - this may not have appeared not to work because of the version on our QA environment when you tested.

Yup, seems to be working nicely now.

One thing that H5P Group may not be too happy about is using the title attribute on the buttons. While this is a normal HTML feature, it seems to cause trouble with some screen readers. If this is a custom view for H5P, then it might not cause to much trouble to remove it. But it's your side of the implementation. And again, that's not up for me to decide.
When used with the keyboard, the play button works as it should, but all the other buttons don't trigger their respective function, but also trigger play/pause.

"When videos run inside Interactive Book ... no full screen button should be visible."

It is not visible - this may not have appeared not to work because of the version on our QA environment when you tested.

Still doesn't seem to work, but this seems in fact to be an issue with H5P.InteractiveBook/H5P.Column/H5P.Video.

"When videos are run ...seeking jumps back and forward"

This happens in our normal player even outside of H5p and is in the backlog of things which we will address in time.

Will let H5P Group know.

"When resizing, there’s some noticeable flickering"

There is is minimal flickering while resizing - this can be addressed in future but is not in scope of this H5P change.

Will let H5P Group know. This is up to their UX team.

"When running the video standalone...this click doesn’t always seem to be registered properly. "

This was addressed and is to do with the iframe permissions.

Will also let H5P Group know. Also, while checking again, I noticed that double clicking on the video toggles the full screen mode. Again, um to H5P Group.

Use updated quality options

numInstances is supposed to reflect the number of instances of the handler for a unique element id, so it cannot be an instance property

Would cause trouble downstream, e.g. in Interactive Video

When the message event is "timeline", message.data.duration is `undefined`, the duration value which is used by `getDuration()`, too, would be overwritten and be `undefined` (bug there then), and here looping would not work.

document.featurePolicy is experimental (no support in Firefox/Safari). Prevent potential crash.

The parameter is passed as a float in seconds

devland · 2024-05-02T11:31:05Z

When used in h5p-course-presentation, the video plays but it doesn't take up all the space the editor allows it.

Add support for EchoVideos from Echo360 as source

f46ed6b

This was referenced Oct 31, 2023

Support for Echo360 Videos in H5P #109

Open

Support for Echo360 Videos in H5P h5p/h5p-editor-php-library#183

Open

otacke suggested changes Feb 14, 2024

View reviewed changes

echo360-damyon added 7 commits February 21, 2024 09:59

Clean eslint warnings

63dada9

Remove (most) jQuery

98abfa5

Consistent arrow functions

5c020c3

Add isLoaded function

3f4cb36

Iframe permission policy delegation

ac0447f

JSDoc improvements and coding style

313e5d1

Duration is a number or undefined

f6b47e2

otacke reviewed Feb 22, 2024

View reviewed changes

Comparison operator

261340b

echo360-damyon force-pushed the echo-video branch from fd8fd8e to 261340b Compare February 26, 2024 01:54

echo360-damyon added 3 commits February 27, 2024 09:04

Cancel resizing on timeline updates

cc460c1

Consistent playback rate handling

671326e

Improve start time handling

45e03c5

echo360-damyon force-pushed the echo-video branch from 0ead985 to 45e03c5 Compare February 29, 2024 00:19

MHod-101 and others added 8 commits March 7, 2024 13:44

PLT-2090: Update quality option mapping

d4a217c

Use updated quality options

Merge pull request #1 from Echo360/plt-2090-update-quality-selection-…

fc1bb89

…logic PLT-2090: Update quality option mapping

Fix startAt option

fc1ccb9

Load player controls before the video starts

6fd6021

JI-5798 fix auto pause when video ended (h5p#121)

2f9f86d

Bump patch version

120b5cc

Fix numInstances

5fa6536

numInstances is supposed to reflect the number of instances of the handler for a unique element id, so it cannot be an instance property

Stop triggering "paused" before video has been "played"

ea3989b

Would cause trouble downstream, e.g. in Interactive Video

Merge pull request #1 from otacke/amend-echo-video

ed6fd8b

Fix 2nd code review issues

echo360-damyon and others added 22 commits May 2, 2024 12:22

Add support for EchoVideos from Echo360 as source

4b25857

Clean eslint warnings

58242c2

Remove (most) jQuery

fc3fa74

Consistent arrow functions

0d5e843

Add isLoaded function

d12e05a

Iframe permission policy delegation

852212a

JSDoc improvements and coding style

2333668

Duration is a number or undefined

029ca09

Comparison operator

05ad569

Cancel resizing on timeline updates

f4c57fe

Consistent playback rate handling

6547fd3

Improve start time handling

11195fe

PLT-2090: Update quality option mapping

9716994

Use updated quality options

Fix startAt option

ee55eed

Load player controls before the video starts

1ecaaff

Fix numInstances

0159b5e

numInstances is supposed to reflect the number of instances of the handler for a unique element id, so it cannot be an instance property

Stop triggering "paused" before video has been "played"

4b747c0

Would cause trouble downstream, e.g. in Interactive Video

Fix updating duration

27e310e

When the message event is "timeline", message.data.duration is `undefined`, the duration value which is used by `getDuration()`, too, would be overwritten and be `undefined` (bug there then), and here looping would not work.

Pass query string with iframe source

bd79cb2

Make eslint happy

543a135

Make code robust for document.featurePolicy

89ffe81

document.featurePolicy is experimental (no support in Firefox/Safari). Prevent potential crash.

Fix startAt handling

10d0b23

The parameter is passed as a float in seconds

devland added 2 commits May 2, 2024 14:04

HFP-3869 fix resizing in CP editor

df8e267

HFP-3869 fix conflict

21e4d19

devland merged commit c047a8e into h5p:master May 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for EchoVideos from Echo360 as source #90

Add support for EchoVideos from Echo360 as source #90

echo360-damyon commented Sep 20, 2023

otacke commented Feb 13, 2024

echo360-damyon commented Feb 13, 2024

otacke left a comment •

edited

Loading

otacke Feb 14, 2024

echo360-damyon Feb 22, 2024

otacke Feb 14, 2024

echo360-damyon Feb 22, 2024

otacke Feb 14, 2024

echo360-damyon Feb 22, 2024

otacke Feb 14, 2024

echo360-damyon Feb 22, 2024

otacke Feb 14, 2024

echo360-damyon Feb 22, 2024

otacke Feb 14, 2024

echo360-damyon Feb 22, 2024

otacke Feb 14, 2024

echo360-damyon Feb 22, 2024

otacke Feb 14, 2024

echo360-damyon Feb 22, 2024

otacke Feb 14, 2024

echo360-damyon Feb 22, 2024

otacke Feb 14, 2024

echo360-damyon Feb 22, 2024

echo360-damyon commented Feb 22, 2024

echo360-damyon commented Feb 22, 2024

otacke left a comment

otacke commented Mar 25, 2024

echo360-damyon commented Apr 9, 2024

otacke commented Apr 9, 2024

devland commented May 2, 2024

Add support for EchoVideos from Echo360 as source #90

Add support for EchoVideos from Echo360 as source #90

Conversation

echo360-damyon commented Sep 20, 2023

otacke commented Feb 13, 2024

echo360-damyon commented Feb 13, 2024

otacke left a comment • edited Loading

Choose a reason for hiding this comment

Functional

Code

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

echo360-damyon commented Feb 22, 2024

echo360-damyon commented Feb 22, 2024

otacke left a comment

Choose a reason for hiding this comment

otacke commented Mar 25, 2024

echo360-damyon commented Apr 9, 2024

otacke commented Apr 9, 2024

devland commented May 2, 2024

otacke left a comment •

edited

Loading