Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add media audio image #6070

Merged
merged 6 commits into from
May 31, 2023
Merged

Add media audio image #6070

merged 6 commits into from
May 31, 2023

Conversation

keianhzo
Copy link
Contributor

@keianhzo keianhzo commented May 11, 2023

This PR adds support for the media audio background image that already exists in AFRAME. It does it by implementing a new VideoTexture class that supports an optional image to show instead of the video similarly to what the HTMLVIdeoElement.poster property does.

This also consolidates overrides that we had for the THREE VideoTexture here: https://github.com/mozilla/hubs/blob/ad175eee477637d3a3b0e5dd1609e85deb74aa1e/src/utils/threejs-video-texture-pause.js

I considered other options like creating an audio element and keeping track of it but I thought that this simplifies the code as both media types are handled in the same way using a video element.

Does it makes more sense to rename this to HubsVideoTexture to avoid confusion?

Copy link
Contributor

@takahirox takahirox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't have long enough time to review the entire change. Just my rough thoughts.

  • I suppose it is more straightforward and maintainable if video is in component data? Because, for example, this line requires a lot to know things that are not clear in this file like VideoMenu entity's Mesh must have VideoTexture in .map.
const video = (world.eid2obj.get(videoEid) as any).material.map.video as HTMLVideoElement;
  • But if this change is a temporary solution and we will revisit later, it may be acceptable because workable may be better than broken.

  • rename this to HubsVideoTexture to avoid confusion

Yes, I like renaming to avoid conflicts. I know video texture extension is under discussion in glTF. If Three.js glTF loader will support it and create Three.js Video texture, our same name VideoTexture may be confusing.

@keianhzo
Copy link
Contributor Author

That sounds good. I've renamed it to HubsVideoTexture and exposed the video element in a MediaVideoData map.

@keianhzo keianhzo merged commit 39b189a into master May 31, 2023
10 of 12 checks passed
@keianhzo keianhzo deleted the audio-image branch May 31, 2023 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants