h5p / h5p-video

10 stars 73 forks source link

Add support for EchoVideos from Echo360 as source #90

Closed echo360-damyon closed 5 months ago

echo360-damyon commented 1 year ago

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 commented 8 months ago

@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 commented 8 months ago

Thanks, that's fine with me.

echo360-damyon commented 8 months ago

Thanks Oliver,

Responses to the points listed as Functional:

  1. "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.

  1. 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 commented 8 months ago

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 commented 6 months ago

@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 https://github.com/echo360-damyon/h5p-video/pull/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:

echo360-damyon commented 6 months ago

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..."

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

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

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

"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 commented 6 months ago

@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.

"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.

devland commented 5 months ago

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