Support sources in video (fix #3173) #3176

machenmusik · 2017-10-18T03:59:06Z

Fixes #3173. Added test and converted 360 video examples to ensure visibility of correct behavior.

…and src attributes

cvan

this is looking good. thanks for taking this on - I appreciate it.

all the examples work on

Chrome on all platforms
Firefox on desktop, Windows and macOS
all but /examples/test/videosphere work in Safari on desktop and iOS (error: WebGL: INVALID_VALUE: texImage2D: no video)

let me know when you're ready for me to test this again. thanks!

cvan · 2017-10-18T07:36:22Z

src/systems/material.js

-      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]')) {


feel free to add a comma between the selectors

there is one?

cvan · 2017-10-18T07:36:40Z

src/systems/material.js

-      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]')) {


feel free to remove this level of nesting

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.

cvan · 2017-10-18T07:38:09Z

src/systems/material.js

+      data.src = data.src.src;
+      if (!data.src && data.src.tagName === 'VIDEO') {
+        // Video elements can have source elements as well
+        data.sources = Array.prototype.slice.call(


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(', ');

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.

cvan · 2017-10-18T07:38:41Z

src/systems/material.js

@@ -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
+  videoAttributes.sources = Array.prototype.slice.call(


cvan · 2017-10-18T07:38:51Z

src/systems/material.js

-      data.src = data.src.getAttribute('src');
+      data.src = data.src.src;
+      if (!data.src && data.src.tagName === 'VIDEO') {
+        // Video elements can have source elements as well


can you end this comment with a period?

cvan · 2017-10-18T07:43:07Z

src/systems/material.js

@@ -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;


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

https://github.com/chenzlabs/aframe/blob/9f8ae0c/src/core/a-assets.js#L45

https://github.com/chenzlabs/aframe/blob/9f8ae0c/src/core/a-assets.js#L81

https://github.com/chenzlabs/aframe/blob/9f8ae0c/src/core/a-assets.js#L103

https://github.com/chenzlabs/aframe/blob/9f8ae0c/src/core/a-assets.js#L162

https://github.com/chenzlabs/aframe/blob/9f8ae0c/src/core/a-assets.js#L205

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.

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.

cvan · 2017-10-18T07:44:01Z

examples/test/videosphere/index.html

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


you can break this out onto separate lines

cvan · 2017-10-18T07:44:35Z

examples/test/videosphere/index.html

-               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" -->
+        <video preload="true"


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

cvan · 2017-10-18T07:45:25Z

tests/systems/material.test.js

+          assert.equal(texture.image, videoEl);
+          system.textureCache[hash].then(function (result) {
+            assert.equal(texture, result.texture);
+            assert.equal(texture.image, result.videoEl);


👍 awesome!

cvan · 2017-10-18T10:43:54Z

examples/test/videosphere/index.html

               width="360" height="360" autoplay loop="true"
-               crossOrigin="anonymous"></video>
+               crossOrigin="anonymous">
+          <source src="https://s3-us-west-2.amazonaws.com/s.cdpn.io/t-197/walking_the_dog_360_video_short.webm"></source>


I'm getting this error in macOS desktop Safari:

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

same on iOS Safari too:

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

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

ngokevin · 2017-10-18T17:30:26Z

Thanks. Is it awkward since while videos can be loaded, the a-assets doesn't match the support? It also doesn't seem like just a simpler .src replace, that seemed fine to me since it wouldn't add anything. But need to do query selectors and walk through the sources when we consider a video element. And later for audio as well.

For multi-asset support, I might consider this which sets the src based on an available list of srcs. This normalizes the API so each asset is an element in a-assets with a src attribute, and the rest of the codebase doesn't need to do checks. Importantly, this also lets us reliably preload files through FileLoader since that's what THREE.AudioLoader will use, not audio elements.

<a-asset-item id="aSound" srcs="a.wav, a.mp3, a.ogg"></a-asset-item>

machenmusik · 2017-10-18T18:01:42Z

Is it awkward since while videos can be loaded, the a-assets doesn't match the support?

Well, given that the source element for video is often used when the browser should pick from given alternatives, I'm not sure how a-assets should work, which one should it load and wait for, and what if it doesn't match the browser? I think a note that video using source elements can't be preloaded via a-assets would be fine. Perhaps that warning should still fire if no src/srcObject attributes (i.e. eliminate that querySelector addition).

For media assets of considerable size, I don't think that preloading every possible variant because we don't know which will be used is a good idea. Also, streaming assets may not be preloadable, since they may not terminate. But having a generic way for a-asset-item to specify multiple explicit URLs to load would be OK in my book, and might also help the gilt plus associated files problem, if a component could be used on a-asset-item to generate the srcs list.

ngokevin · 2017-10-18T18:09:25Z

Yeah, it's awkward to officially "support" it since it doesn't play with <a-assets> as expected, and they can't preload well so that weakens the point in using <a-assets> at all. <a-asset-item> supporting multiple would fit better with A-Frame and it enables efficient audio preloading. Wouldn't really be a component, but a feature to <a-asset-item> which is open for expansion.

Streaming, yeah, we wouldn't use <a-assets> at all since <a-assets> is there purely for preloading. Still think it's a better fit in a component. I wonder how that plays well with uploading to the GPU, does it have to decode and upload each frame?

machenmusik · 2017-10-18T18:16:39Z

Streaming plays like any other video texture.

You don't need to use a-assets unless you want to block the entire UI to load (large) video files, which IMO is actually a bad thing to do in general even with existing src-attribute MP4 files. (Or giant model files, etc.)

ngokevin · 2017-10-18T18:22:36Z

Yeah, for large videos or streaming videos, often prefer to use inline URLs (either in material or other components) versus <a-assets>, only used for preloading files you want to preload. Hopefully, network fetching does not affect rendering too much?

machenmusik · 2017-10-18T18:35:26Z

IMO, using inline URL tends to be part of the problem, because no one can figure out where the actual media element is to be controlled (which is a FAQ) -- better to append a child video element with whatever you want to scene (outside a-assets) and reference by id/selector

ngokevin · 2017-10-18T18:43:54Z

We can document grabbing a handle to the video element if three.js creates it (I believe el.getObject3D('mesh').material.map.image).

machenmusik · 2017-10-18T19:01:00Z

it's A-Frame that creates it
https://github.com/aframevr/aframe/blob/master/src/systems/material.js#L144

ngokevin · 2017-10-18T19:04:00Z

Ah, right. Still can get the video handle that way above.

machenmusik · 2017-10-18T20:47:24Z

True, but that's kinda gross. (Maybe an accessor function on the material component would be nicer.)

Are you requesting more changes here (e.g. reinstate a-assets warning) ?

I think the value of being able to play videos that use source elements, even though they won't be preloaded, is worth the small disconnect (especially if the warning is reinstated and/or tweaked to explain)

ngokevin · 2017-10-18T21:04:26Z

The disconnect may or may not be small. It's another edge case to consider and document. Not comfortable supporting <source> in <a-assets> when we can't preload, yet that's the purpose of <a-assets>.

I'm thinking again, is there a common need to support multiple video formats? A-Frame is a future-facing project, most people will just use one of the formats that are supported in all modern browsers, versus going through the work of converting to multiple formats. For the people that somehow need it, it'd still be possible to do with a bit of JS. I haven't come across projects that have attempted or asked for multiple formats (@cvan seemed to list A-Blast and Puzzle Rain, but they just use one format).

cvan · 2017-10-18T21:10:36Z

my memory was wrong - it was A Saturday Night

machenmusik · 2017-10-18T21:13:07Z

is there a common need to support multiple video formats?

Sadly, yes at the moment, as there is not quite a common set of codecs and formats that are supported by all browsers. For example, no one seemed to notice that examples/test/videosphere is using webm which apparently fails in Safari. H.264 and MP4 is probably closest at the moment for video, but even then there are outliers like experimental Chromium which do not support.

ngokevin · 2017-10-18T21:19:40Z

Sound is a slightly different problem. <source> would be less effective there as three.js AudioLoader does not even use the <audio> element, but rather the FileLoader. Unprescribing <audio> and prescribing something like <a-asset-item srcs> would be effective there.

For video, I don't know if A-Frame should bend for Chromium. I'd prioritize mainline and VR-supported browsers.

machenmusik · 2017-10-18T21:20:09Z

So A-Frame and THREE are perfectly capable of supporting video that does not appear in a-assets, and as we have discussed a significant number of video use cases really don't want to incur preload for hundreds of megabytes or more of video, especially inadvertently on mobile.

My suggestion is to continue to document how to use video with src attribute as has been common A-Frame practice for a while, but also to point out that non-preloaded video elements also work (once we merge this PR) with examples including WebRTC and videos with source elements.

I would also suggest we think about whether we want to continue encouraging use of a-video and a-videosphere components, or would rather discourage in favor of something else like a third party component.

machenmusik · 2017-10-18T21:27:25Z

@cvan brought up audio, i agree that is different, as preload mistakes are in the single to double digits of megabytes which is less fatal. Multiple video source support is useful and often needed, it's not about Chromium specifically.

Question, do we find browser caching (often on mass storage and not purely RAM) to be unreliable so we want to use THREE caching (which is in RAM by necessity and thus bloats footprint) ?

ngokevin · 2017-10-18T21:30:21Z

Is it possible to remove all the query selector logic? For hashing, just use the video.src provided as is. Then the PR is neutral and video <source> works (for now) at no cost. Just to make the PR a pure getAttribute('src') -> .src change? Then a warning in <a-assets> for videos with sources maybe.

machenmusik · 2017-10-18T21:38:24Z

There is no video.src provided when you use source elements. That being said, we can require use of element id in those cases

ngokevin · 2017-10-18T21:42:14Z

In A-Frame, I guess they'd always have IDs? So I'm understanding the fix more, is just to get past A-Frame warnings/checks/hashing so we can pass the video to three.js uninterrupted. Then the fix is mostly making the hash functions not mess it up (don't think they're too important anyways for video).

cvan · 2017-10-18T22:35:56Z

Chromium is used by the official Oculus Browser and Samsung Internet Browser. Those are browsers that A-Frame should handle as first-class citizens, yes?

dmarcos · 2017-10-18T22:53:08Z

@cvan Yes we should support those. Even though Chromium does not ship with proprietary codecs, Oculus Browser and Samsung Internet both support H.264 and MP4 as far as I know, they're not just vanilla Chromium. The only affected one would be Chromium WebVR experimental builds.

machenmusik · 2017-10-18T23:10:47Z

@ngokevin yup.

machenmusik · 2017-10-18T23:14:26Z

The problem with MP4 is that it doesn't do live. (But that's the other discussion)

ngokevin · 2017-10-19T02:57:56Z

needschanges to remove the querySelector logic, use IDs for the hash.

… weak check for child source elements

machenmusik · 2017-10-19T03:26:10Z

Latest version correctly gives a-assets warning for examples/test/videosphere (which still works anyway if you're not using Safari), and also correctly gives no warnings for examples/test/video.

cvan

a few minor things - thanks 👍

cvan · 2017-10-19T06:30:12Z

examples/test/videosphere/index.html

                scale="-1 1 1">
      </a-entity>
    </a-scene>
+
+    <!-- Normally, one would use video id, e.g. material="src:#lego" above. 


this is a repeat of the comment on line 20. I'd remove this sentence.

cvan · 2017-10-19T06:30:35Z

examples/test/videosphere/index.html

                scale="-1 1 1">
      </a-entity>
    </a-scene>
+
+    <!-- Normally, one would use video id, e.g. material="src:#lego" above. 
+         For this example however, we want to show that an anonymous video


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

cvan · 2017-10-19T06:31:52Z

src/systems/material.js

-      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.');


should

Video

be

`video`

?

cvan · 2017-10-19T06:35:58Z

src/systems/material.js

@@ -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;


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.

cvan · 2017-10-19T06:36:31Z

examples/test/videosphere/index.html

               width="360" height="360" autoplay loop="true"
-               crossOrigin="anonymous"></video>
+               crossOrigin="anonymous">


nit: I know you didn't modify it, but can you lowercase the O in Origin?

cvan · 2017-10-19T06:37:27Z

examples/test/videosphere/index.html

@@ -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. -->


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?

cvan · 2017-10-19T06:38:21Z

examples/test/videosphere/index.html

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


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?

ngokevin · 2017-10-19T07:49:45Z

Let's make sure the approach is alright before going full into nits/comments/typos, just to reduce noise.

ngokevin · 2017-10-19T08:13:05Z

Thanks! That one took a lot of energy 😅

cvan · 2017-10-19T08:20:34Z

Agreed, thanks for helping with this. In the future, would you like me to mark reviews as approved but with Comments?

Beyond my admittedly noisy comments here, the important line I wanted to double check on was this: #3176 (comment)

Doesn’t regress anything to my knowledge, but I think the loading of the videos are being timed out as a result. Am uncaught Promise, but assets and scene still load. So ok 👌🏽!

Thanks for this - I appreciate it, both of you.

ngokevin · 2017-10-19T08:29:51Z

The preloading abilities <source> video will be limited (it's not great to preload large videos anyways). The PR will enable at least the use of <source> without weird warnings (by passing an ID or the video element directly thru setAttribute).

ngokevin · 2017-10-19T08:31:22Z

Yeah, perhaps it might time out if you use <source>. Could be a separate fix to skip media elements that don't have a src attribute defined directly.

NameIsSoDiao · 2017-10-25T11:08:45Z

Can not play VR video on ios#3192

machenmusik added 4 commits October 17, 2017 23:53

modify video to use source element instead of src attribute

4013b78

add test for video with source element instead of src attribute

976aafa

modify to use anonymous video with source element instead of with id …

b320c2d

…and src attributes

support video with source element(s) rather than src attribute

9f8ae0c

machenmusik mentioned this pull request Oct 18, 2017

Support <source>s in <video>s (use videoEl.src in a-assets and calculateVideoCacheHash) #3173

Closed

cvan suggested changes Oct 18, 2017

View reviewed changes

machenmusik added 2 commits October 18, 2017 08:01

fix pre-existing preload usage per discussion on PR

23f95bc

editorial changes per discussion on PR

00b2929

ngokevin added the needs changes label Oct 19, 2017

machenmusik added 2 commits October 18, 2017 23:21

add warning when audio/video asset has neither src nor srcObject

8e3a160

remove querySelector per discussion on PR; use childElementCount as a…

c303a8e

… weak check for child source elements

cvan suggested changes Oct 19, 2017

View reviewed changes

ngokevin merged commit 187f520 into aframevr:master Oct 19, 2017

delapuente mentioned this pull request Oct 31, 2017

Allow MediaStream to be sound sources. #3042

Closed

This was referenced Oct 31, 2017

make the example 360 video page redirect to example Glitch #3210

Closed

fix <video autoplay> playback (when audio is present) on mobile for both iOS + Android #2657

Closed

Support sources in video (fix #3173) #3176

Support sources in video (fix #3173) #3176

Conversation

machenmusik commented Oct 18, 2017

cvan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngokevin commented Oct 18, 2017

machenmusik commented Oct 18, 2017

ngokevin commented Oct 18, 2017

machenmusik commented Oct 18, 2017

ngokevin commented Oct 18, 2017

machenmusik commented Oct 18, 2017

ngokevin commented Oct 18, 2017

machenmusik commented Oct 18, 2017

ngokevin commented Oct 18, 2017

machenmusik commented Oct 18, 2017

ngokevin commented Oct 18, 2017

cvan commented Oct 18, 2017

machenmusik commented Oct 18, 2017 • edited

ngokevin commented Oct 18, 2017 • edited

machenmusik commented Oct 18, 2017

machenmusik commented Oct 18, 2017

ngokevin commented Oct 18, 2017

machenmusik commented Oct 18, 2017

ngokevin commented Oct 18, 2017

cvan commented Oct 18, 2017

dmarcos commented Oct 18, 2017

machenmusik commented Oct 18, 2017

machenmusik commented Oct 18, 2017

ngokevin commented Oct 19, 2017

machenmusik commented Oct 19, 2017

cvan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngokevin commented Oct 19, 2017

ngokevin commented Oct 19, 2017

cvan commented Oct 19, 2017

ngokevin commented Oct 19, 2017

ngokevin commented Oct 19, 2017

NameIsSoDiao commented Oct 25, 2017

machenmusik commented Oct 18, 2017 •

edited

ngokevin commented Oct 18, 2017 •

edited