-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Support sources in video (fix #3173) #3176
Changes from all commits
4013b78
976aafa
b320c2d
9f8ae0c
23f95bc
00b2929
8e3a160
c303a8e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,15 +9,41 @@ | |
<body> | ||
<a-scene stats> | ||
<a-assets> | ||
<video id="lego" preload="true" | ||
src="https://s3-us-west-2.amazonaws.com/s.cdpn.io/t-197/walking_the_dog_360_video_short.webm" | ||
<!-- Test anonymous video with source element. --> | ||
<!-- Normally, one would give the video an id, e.g. id="lego" --> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment's a bit confusing, without seeing the JavaScript |
||
<video preload="auto" | ||
width="360" height="360" autoplay loop="true" | ||
crossOrigin="anonymous"></video> | ||
crossOrigin="anonymous"> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I know you didn't modify it, but can you lowercase the |
||
<source src="https://s3-us-west-2.amazonaws.com/s.cdpn.io/t-197/walking_the_dog_360_video_short.webm"></source> | ||
</video> | ||
</a-assets> | ||
|
||
<a-entity material="shader: flat; src: #lego" geometry="primitive: sphere; radius: 100" | ||
<!-- Normally, one would use the video id, e.g. material="src:#lego" --> | ||
<a-entity material="shader: flat" geometry="primitive: sphere; radius: 100" | ||
scale="-1 1 1"> | ||
</a-entity> | ||
</a-scene> | ||
|
||
<!-- Normally, one would use video id, e.g. material="src:#lego" above. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is a repeat of the comment on line 20. I'd remove this sentence. |
||
For this example however, we want to show that an anonymous video | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
element can be utilized successfully. --> | ||
<script> | ||
// Call onceSceneLoaded() once the scene is loaded. | ||
var scene = document.querySelector('a-scene'); | ||
if (scene.hasLoaded) { | ||
onceSceneLoaded(); | ||
} else { | ||
scene.addEventListener('loaded', onceSceneLoaded); | ||
} | ||
|
||
function onceSceneLoaded() { | ||
// Get the video element we'd like to show. | ||
// It's anonymous (no id), so we access it by element name. | ||
var video = document.querySelector('video'); | ||
|
||
// Set the material's src texture to the video element. | ||
var entity = document.querySelector('a-entity[material]'); | ||
entity.setAttribute('material', 'src', video); | ||
} | ||
</script> | ||
</body> | ||
</html> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,8 +57,8 @@ module.exports.System = registerSystem('material', { | |
|
||
// Video element. | ||
if (src.tagName === 'VIDEO') { | ||
if (!src.hasAttribute('src') && !src.hasAttribute('srcObject')) { | ||
warn('Video element was defined without `src` nor `srcObject` attributes.'); | ||
if (!src.src && !src.srcObject && !src.childElementCount) { | ||
warn('Video element was defined with neither `source` elements nor `src` / `srcObject` attributes.'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should
be
? |
||
} | ||
this.loadVideo(src, data, cb); | ||
return; | ||
|
@@ -180,7 +180,7 @@ module.exports.System = registerSystem('material', { | |
if (data.src.tagName) { | ||
// Since `data.src` can be an element, parse out the string if necessary for the hash. | ||
data = utils.extendDeep({}, data); | ||
data.src = data.src.getAttribute('src'); | ||
data.src = data.src.src; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do these calls in
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. L45 is for images so no. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure this should be.
This seems like a larger issue that probably warrants its own GitHub issue. What are your thoughts?
Personally, I think requiring developers to add |
||
} | ||
return JSON.stringify(data); | ||
}, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -191,6 +191,24 @@ suite('material system', function () { | |
}); | ||
}); | ||
|
||
test('loads image given a <video> element with <source>', function (done) { | ||
var videoEl = document.createElement('video'); | ||
var system = this.system; | ||
var data = {}; | ||
|
||
videoEl.insertAdjacentHTML('beforeend', | ||
'<source src="' + VIDEO1 + '"></source>'); | ||
system.loadVideo(videoEl, data, function (texture) { | ||
var hash = Object.keys(system.textureCache)[0]; | ||
assert.equal(texture.image, videoEl); | ||
system.textureCache[hash].then(function (result) { | ||
assert.equal(texture, result.texture); | ||
assert.equal(texture.image, result.videoEl); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 awesome! |
||
done(); | ||
}); | ||
}); | ||
}); | ||
|
||
test('sets texture flags appropriately when given a <video> element that isHLS on iOS', function (done) { | ||
var videoEl = document.createElement('video'); | ||
var system = this.system; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
anonymous
->cross-origin (anonymous)
perhaps? even as someone familiar with browsers' cross-origin policies, this comment doesn't help me a whole lot. perhaps adding a link to MDN might be helpful?