Skip to content

Support sources in video (fix #3173) #3176

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

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 4 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions examples/boilerplate/360-video/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
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
27 changes: 22 additions & 5 deletions examples/test/videosphere/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,32 @@
<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

@cvan cvan Oct 19, 2017

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

@cvan cvan Oct 19, 2017

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="true"
Copy link
Contributor

@cvan cvan Oct 18, 2017

Choose a reason for hiding this comment

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

this should be preload="auto" (per #3175)

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

@cvan cvan Oct 19, 2017

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

@cvan cvan Oct 18, 2017

Choose a reason for hiding this comment

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

I'm getting this error in macOS desktop Safari:

image

(all the other video examples seem to work in Safari when I have Develop > Cross-Origin Restrictions enabled.)

Copy link
Contributor

@cvan cvan Oct 18, 2017

Choose a reason for hiding this comment

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

same on iOS Safari too:

image

Copy link
Contributor Author

@machenmusik machenmusik Oct 18, 2017

Choose a reason for hiding this comment

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

does Safari support webm format? I assumed that was why this example fails there

Copy link
Contributor Author

@machenmusik machenmusik Oct 18, 2017

Choose a reason for hiding this comment

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

(That failure should be present with or without this PR.)

</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>

<script>
// Call onceSceneLoaded() once the scene is loaded.
var scene = document.querySelector('a-scene');
if (scene.hasLoaded) { onceSceneLoaded(); } else {
Copy link
Contributor

@cvan cvan Oct 18, 2017

Choose a reason for hiding this comment

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

you can break this out onto separate lines

scene.addEventListener('loaded', onceSceneLoaded);
}

function onceSceneLoaded() {
// Set the material's src texture to the first video element.
Copy link
Contributor

@cvan cvan Oct 18, 2017

Choose a reason for hiding this comment

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

perhaps it'd be better to just provide an id to the <a-entity> so the selector is self explanatory?

Copy link
Contributor Author

@machenmusik machenmusik Oct 18, 2017

Choose a reason for hiding this comment

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

The goal of my edits to this one is to show usage of an anonymous video, added comments to reflect. Boilerplate example already shows usage of video with id.

var entity = document.querySelector('a-entity[material]');
entity.setAttribute('material', 'src', document.querySelector('video'));
}
</script>
</body>
</html>
18 changes: 15 additions & 3 deletions src/systems/material.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,10 @@ 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) {
if (!src.querySelector('source[src],source[srcObject]')) {
Copy link
Contributor

@cvan cvan Oct 18, 2017

Choose a reason for hiding this comment

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

feel free to add a comma between the selectors

Copy link
Contributor Author

@machenmusik machenmusik Oct 18, 2017

Choose a reason for hiding this comment

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

there is one?

Copy link
Contributor

@cvan cvan Oct 18, 2017

Choose a reason for hiding this comment

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

feel free to remove this level of nesting

Copy link
Contributor Author

@machenmusik machenmusik Oct 18, 2017

Choose a reason for hiding this comment

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

Is that necessary? The outer level is the check for traditional src / srcObject attribute, the inner is a check for child source elements with them; there is no need to do querySelector in the more common case of src/srcObject.

warn('Video element was defined without `src` nor `srcObject` attributes.');
}
}
this.loadVideo(src, data, cb);
return;
Expand Down Expand Up @@ -180,7 +182,13 @@ 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

@cvan cvan Oct 18, 2017

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

@machenmusik machenmusik Oct 18, 2017

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

@cvan cvan Oct 19, 2017

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.

if (!data.src && data.src.tagName === 'VIDEO') {
// Video elements can have source elements as well
Copy link
Contributor

@cvan cvan Oct 18, 2017

Choose a reason for hiding this comment

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

can you end this comment with a period?

data.sources = Array.prototype.slice.call(
Copy link
Contributor

@cvan cvan Oct 18, 2017

Choose a reason for hiding this comment

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

nit: could remove one function call like this:

data.sources = Array.prototype.map.call(data.src.querySelectorAll('source'), function (vid) {
  return video.src || video.srcObject;
}).join(', ');

Copy link
Contributor Author

@machenmusik machenmusik Oct 18, 2017

Choose a reason for hiding this comment

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

I specifically did not do this because on some browsers, querySelectorAll results are not usable as an Array, but need to be converted with Array.prototype.slice.call first.

data.src.querySelectorAll('source'))
.map(function (v) { return v.src || v.srcObject; }).join(',');
}
}
return JSON.stringify(data);
},
Expand Down Expand Up @@ -270,6 +278,10 @@ function calculateVideoCacheHash (data, videoEl) {
for (i = 0; i < videoEl.attributes.length; i++) {
videoAttributes[videoEl.attributes[i].name] = videoEl.attributes[i].value;
}
// Video elements can have source elements as well
Copy link
Contributor

@cvan cvan Oct 18, 2017

Choose a reason for hiding this comment

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

same here

videoAttributes.sources = Array.prototype.slice.call(
Copy link
Contributor

@cvan cvan Oct 18, 2017

Choose a reason for hiding this comment

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

same here

videoEl.querySelectorAll('source'))
.map(function (v) { return v.src || v.srcObject; }).join(',');
Object.keys(videoAttributes).sort().forEach(function (name) {
hash += name + ':' + videoAttributes[name] + ';';
});
Expand Down
18 changes: 18 additions & 0 deletions tests/systems/material.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
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

@cvan cvan Oct 18, 2017

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