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.

Sign up for GitHub

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

Jump to bottom

Support sources in video (fix #3173) #3176

Merged
merged 8 commits into from
Oct 19, 2017
Merged

Support sources in video (fix #3173) #3176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4013b78
modify video to use source element instead of src attribute
machenmusik Oct 18, 2017
976aafa
add test for video with source element instead of src attribute
machenmusik Oct 18, 2017
b320c2d
modify to use anonymous video with source element instead of with id …
machenmusik Oct 18, 2017
9f8ae0c
support video with source element(s) rather than src attribute
machenmusik Oct 18, 2017
23f95bc
fix pre-existing preload usage per discussion on PR
machenmusik Oct 18, 2017
00b2929
editorial changes per discussion on PR
machenmusik Oct 18, 2017
8e3a160
add warning when audio/video asset has neither src nor srcObject
machenmusik Oct 19, 2017
c303a8e
remove querySelector per discussion on PR; use childElementCount as a…
machenmusik Oct 19, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions examples/boilerplate/360-video/index.html
View file
Open in desktop
Expand Up @@ -9,8 +9,9 @@
<body>
<a-scene>
<a-assets>
<video id="video" src="https://ucarecdn.com/fadab25d-0b3a-45f7-8ef5-85318e92a261/"
autoplay loop crossorigin="anonymous"></video>
<video id="video" autoplay loop crossorigin="anonymous">
<source src="https://ucarecdn.com/fadab25d-0b3a-45f7-8ef5-85318e92a261/"></source>
</video>
</a-assets>

<a-videosphere src="#video" rotation="0 180 0"></a-videosphere>
Expand Down
36 changes: 31 additions & 5 deletions examples/test/videosphere/index.html
View file
Open in desktop
Expand Up @@ -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. -->
Copy link
Contributor

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?

<!-- Normally, one would give the video an id, e.g. id="lego" -->
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment's a bit confusing, without seeing the JavaScript <script> below. It seems like this example is being modified to test <source>. Could we just have a separate example that tests <source>s?

<video preload="auto"
width="360" height="360" autoplay loop="true"
crossOrigin="anonymous"></video>
crossOrigin="anonymous">
Copy link
Contributor

Choose a reason for hiding this comment

The 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 O in Origin?

<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.
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

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

anonymous video? I'm not sure I'd expect people to know what that means

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>
3 changes: 3 additions & 0 deletions src/core/a-assets.js
View file
Open in desktop
Expand Up @@ -52,6 +52,9 @@ module.exports = registerElement('a-assets', {
mediaEls = this.querySelectorAll('audio, video');
for (i = 0; i < mediaEls.length; i++) {
mediaEl = fixUpMediaElement(mediaEls[i]);
if (!mediaEl.src && !mediaEl.srcObject) {
warn('Audio/video asset has neither `src` nor `srcObject` attributes.');
}
loaded.push(mediaElementLoaded(mediaEl));
}

Expand Down
6 changes: 3 additions & 3 deletions src/systems/material.js
View file
Open in desktop
Expand Up @@ -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.');
Copy link
Contributor

Choose a reason for hiding this comment

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

should

Video

be

`video`

?

}
this.loadVideo(src, data, cb);
return;
Expand Down Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

do these calls in src/core/a-assets.js need to be adjusted as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

L45 is for images so no.
L81 maybe, but see below... so at present I would argue no.
L103 is problematic because we don't know which source the browser would select, and I'm not in favor of trying to duplicate browser selection logic here as that is a losing battle longer term.
L162 is problematic as I'm not sure whether we want to rely on THREE level caching rather than browser level caching (or instructions from the sources not to cache)... so at present I would argue no.
L205 maybe, but is it harmful to add crossorigin? We don't know which source the browser would select, and I think we would be better served to ask authors to add crossorigin themselves, as they would have to do outside A-Frame -- just adding the attribute doesn't make the video delivery and browser honor it, and I think that's a contributor to the number of "why doesn't my video play" questions.

Copy link
Contributor

Choose a reason for hiding this comment

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

L81 maybe, but see below... so at present I would argue no.

I'm pretty sure this should be.

L162 is problematic as I'm not sure whether we want to rely on THREE level caching rather than browser level caching (or instructions from the sources not to cache)... so at present I would argue no.

This seems like a larger issue that probably warrants its own GitHub issue. What are your thoughts?

L205 maybe, but is it harmful to add crossorigin?

Personally, I think requiring developers to add crossorigin="anonymous" is probably security theatre. If it's in the examples or added by A-Frame automatically, whatever developers see or have to do to make it work, they'll probably just do it blindly. I'm not certain it helps.

}
return JSON.stringify(data);
},
Expand Down
18 changes: 18 additions & 0 deletions tests/systems/material.test.js
View file
Open in desktop
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The 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;
Expand Down