Add CreateVideoTextureOptions to support crossOrigin videos #4635
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
Yes, please add this info to the |
|
@elalish Thanks for reviewing. Docs updated. Let me know if anything else is needed. |
| "name": "createVideoTexture(uri, options?)", | ||
| "htmlName": "createVideoTexture", | ||
| "description": "Returns a Texture object for use with the <code>setTexture</code> method on <code>TextureInfo</code> objects in the Materials API. The video element created is available through <code>texture.source.element</code> for controlling playback.", | ||
| "description": "Returns a Texture object for use with the <code>setTexture</code> method on <code>TextureInfo</code> objects in the Materials API. The video element created is available through <code>texture.source.element</code> for controlling playback. for cross origin resources, you can set `options.crossOrigin` to `anonymous`.", |
There was a problem hiding this comment.
Looking at this more closely, I wonder if we need a new parameter at all. We already have an attribute withCredentials that is used for all of our image loaders. In three.js it's doing: credentials: this.withCredentials ? 'include' : 'same-origin'. Could we just do video.crossOrigin = this.withCredentials ? 'with-credentials' : 'anonymous' to be consistent?
There was a problem hiding this comment.
There was a problem hiding this comment.
This is what I understand from your comment:
<model-viewer with-credentials src="https://modelviewer.dev/shared-assets/models/sphere.glb" ... >videoTexture = modelViewerAnimated.createVideoTexture("https://cdn.glitch.global/68b42b4e-5a36-47f5-ab00-09026a7a73d0/lottie-logo.mp4?v=1705067796906");
// video.crossOrigin would be 'anonymous' as you saidTesting it locally gave me this error:
Access to fetch at 'https://modelviewer.dev/shared-assets/models/sphere.glb' from origin 'http://localhost:8002' has been blocked by CORS policy: The value of the 'Access-Control-Allow-Origin' header in the response must not be the wildcard '*' when the request's credentials mode is 'include'.
Which addresses the problem in a scenario that the glb file is on the same origin but the video is not.
There was a problem hiding this comment.
@elalish For improving the consistency in the code, we can change:
interface CreateVideoTextureOptions {
withCredentials?: boolean
}There was a problem hiding this comment.
Hmm, how common do you think it is that different assets are coming from different origins for MV users? I do see your point about generality, but the fact is that we've already got this single with-credentials switch that affects src, createTexture, and lottieLoader, without any complaints yet. I think I'd rather add video to that pattern for now, or else do a complete refactor if it proves to be a user pain point. What do you think?
There was a problem hiding this comment.
how common do you think it is that different assets are coming from different origins for MV users?
I'm not really sure as I'm a newbie in this library, but I believe most users would serve their assets alongside their application, and it would be okay for them. However, it may be concerning for users who have an application on origin A and are using a CMS, which may be on origin B, to serve an image, a video, or a Lottie file.
I think I'd rather add video to that pattern for now, or else do a complete refactor if it proves to be a user pain point.
I think that each resource having its option to be with credentials or not would be a more bulletproof solution. You're right; it may not be a user pain point now, and for the sake of consistency, we can do as you suggested, which covers the case for users that have all assets on a cross-origin source.
I will update the MR ASAP.
|
@elalish Hi again. Thanks! |
elalish
left a comment
There was a problem hiding this comment.
Thanks! Agreed regarding a more general solution - let's see how it goes. Out of curiosity, what is stopping your testing?
|
@elalish I appreciate that 😃🤩 . Actually, I wasn't be able to find a cross origin server that serves both glb and video assets with the right cors headers and credential checking which I can be able to use error (which tells us to change the cors configurations on the server that I don't have access to). Not that I'm thinking maybe there would be some way for me to configure a new server in my localhost and use it as the assets server and configure the cors headers so that it requires credentials for serving assets, making it like a cross origin server... But I Also would be glad if you had any idea and guide me through how it should be tested. Thanks for your amazing work! |
|
I see - I figured you would just test with a local server. Any generic file server should work - a Google Cloud bucket or Amazon S3, etc. Thanks! |
|
Helped a lot |
Reference Issue
Fixes #4634
Features
option: CreateVideoTextureOptionsargument tocreateVideoTexture. Allowing to call it likecreateVideoTexture(url, {crossOrigin: "anonymous"})Please let me know if there is any change needed if the solution is accepted (like updating documents, etc).