Add iOS HLS tests #2668

machenmusik · 2017-05-13T16:43:00Z

Add tests and do minor refactoring per discussion on #2597

…n to materialtextureloaded to make it testable

machenmusik · 2017-05-13T16:53:41Z

Doing the tests was a bit ugly, but they are now fully functional; in particular, there is not a mechanism to mock things like AFRAME.utils.device.isIOS() without the ugly hack of temporarily modifying AFRAME.utils.device i.e. the scene.isIOS flag discussed in #2597 to make the tests easy does not exist.

ngokevin · 2017-05-13T18:16:21Z

I meant In the system, if you change the utils.device.isIOS call to check the scene value, it should be easily mockable by setting the scene.isIOS.

…bute; fix shader substitution tests

machenmusik · 2017-05-13T18:28:48Z

aaargh, for some reason even though shader substitution test works on firefox and chrome on my machine, it fails on firefox in the cloud. :-/ gonna have to disable those tests for now as I don't have more time to spend on that

ngokevin · 2017-05-14T20:33:00Z

src/systems/material.js

@@ -146,7 +146,7 @@ module.exports.System = registerSystem('material', {
    setTextureProperties(texture, data);

    // if we're on iOS, and the video is HLS, we currently need to do some hacks
-    if (utils.device.isIOS() && isHLS(videoEl)) {
+    if (utils.device.isIOS() && isHLS(videoEl.src || videoEl.getAttribute('src'), videoEl.type || videoEl.getAttribute('type'))) {


Just to confirm, you tried changing utils.device.isIOS() here to this.el.sceneEl.isIOS, and in the test, try mocking sceneEl.isIOS = true before the assertions?

no, i thought you were saying the substitution had already been done, so only changing the flag was necessary. if the function has to be monkey patched, not sure it is any clearer to do it as you suggest rather than directly manipulating to return desired value

here the check for the property and if none the getAttribute value was unexpectedly required for both synthetic tests and actual real-world usage to work correctly.

Use sceneEl.iIOS here so it doesn't have to run the user agent parse again, and you can stub it in the tests.

ngokevin · 2017-05-14T20:33:51Z

docs/core/utils.md

@@ -151,6 +151,13 @@ Checks if device is Gear VR. Returns a `boolean`.

 Checks if device is a smartphone. Returns a `boolean`.

+## `AFRAME.utils.material`
+
+### `AFRAME.utils.material.isHLS (src, type)`


I don't think this has to be public API

it did to be accessible in tests...?

Public API meaning documented

ah, I understand you now.

This reverts commit 8c43f5b.

machenmusik · 2017-05-15T19:55:22Z

@ngokevin good now?

ngokevin · 2017-05-15T20:50:16Z

src/utils/material.js

@@ -118,11 +120,16 @@ function handleTextureEvents (el, texture) {

  // Video events.
  if (!texture.image || texture.image.tagName !== 'VIDEO') { return; }
+
  texture.image.addEventListener('loadeddata', function emitVideoTextureLoadedDataAll () {
    // Check to see if we need to use iOS 10 HLS shader.
    if (texture.needsCorrectionBGRA && texture.needsCorrectionFlipY) {


Can do:

if (texture.needsCorrectionBGRA && texture.needsCorrectionFlipY && ['standard', 'flat', 'testShader'].indexOf(el.components.material.data.shader) !== -1)

indexOf to be safe

One thing I was considering is passing needsCorrectionBGRA and needsCorrectionFlipY into shaders in general when true, so someone with a custom shader would know when corrections are necessary. Thoughts?

ngokevin · 2017-05-15T20:50:43Z

tests/core/shader.test.js

@@ -152,6 +152,114 @@ suite('shader', function () {
                                           instance.attributes['src']));
    });

+    // Skip since this fails Travis CI Firefox run, even though it works on Chrome, and with local Firefox.


Do we know where Firefox fails? Might be a timing issue.

no, but it is irrelevant since the actual substitution code won't be ever really be fired using Firefox, and I don't have the time to spare to investigate that

ngokevin · 2017-05-15T20:51:07Z

tests/core/shader.test.js

+      assert.notOk(initSpy.called);
+      assert.notOk(updateSpy.called);
+
+      // Mock iOS.  NOTE: this doesn't work... el.sceneEl.isIOS = true;


Any ideas why this wouldn't work? We set the boolean and it checks the boolean

it may work now (I didn't clean up the skipped tests per the last changes) but I don't have the time to spare to investigate that

ngokevin · 2017-05-15T21:02:12Z

tests/core/shader.test.js

+
+      // Mock iOS.  NOTE: this doesn't work... el.sceneEl.isIOS = true;
+      var realIsIOS = AFRAME.utils.device.isIOS;
+      AFRAME.utils.device.isIOS = function () { return true; };


this.sinon.stub(AFRAME.utils.device.isIOS, () => true) might work, if we use sinon, the teardown will automatically restore it.

perhaps, but I don't have the time to spare to investigate that

ngokevin · 2017-05-15T21:04:36Z

tests/core/shader.test.js

+          done();
+        });
+      });
+      el.setAttribute('material', {shader: 'testShader', src: videoEl});


Do we need to specify testShader?

I don't know, the original test on which this was based did so kept it, I don't have the time to spare to investigate that

ngokevin · 2017-05-15T21:05:09Z

tests/core/shader.test.js

+      el.setAttribute('material', {shader: 'testShader', src: videoEl});
+      var material = el.components.material;
+      var instance = material.shader;
+      assert.ok(instance);


I don't think we need to assert these, should keep focused on what we want to assert (e.g., the shader is switched)

the original test on which this was based did so kept it

ngokevin · 2017-05-15T21:05:35Z

tests/core/shader.test.js

+    });
+
+    // Skip since this fails Travis CI Firefox run, even though it works on Chrome, and with local Firefox.
+    test.skip('iOS HLS video uses appropriate shader (setAttribute)', function (done) {


This is a large test, hard to spot what's different from the previous one

agreed, maybe we should just remove the skipped tests for now?

I think one of the two tests should suffice, I can help try to simplify.

machenmusik · 2017-05-16T14:24:17Z

@ngokevin if you have the time to tidy up whichever of the skipped tests you prefer and get rid of the other one, that should wrap this one up

ngokevin · 2017-05-16T23:37:47Z

Will do right now, thanks!

ngokevin · 2017-05-17T00:39:21Z

Cleaned up at a2d641a

The isIOS check worked for me
Rather than waiting for the video to load, I just mock the event
Remove extraneous assertions and other setup meant for other tests
FF works for me without waiting for the vid

machenmusik · 2017-05-17T00:54:06Z

nice @ngokevin, thank you!

machenmusik added 3 commits May 13, 2017 12:40

change isHLS to directly accept src and type; move shader substitutio…

7b8ceba

…n to materialtextureloaded to make it testable

add tests for iOS HLS functionality

84b8894

add isHLS to docs

8c43f5b

machenmusik force-pushed the add-ios-hls-tests branch from b504f88 to 8c43f5b Compare May 13, 2017 16:48

machenmusik mentioned this pull request May 13, 2017

make accommodations necessary for iOS 10 HLS #2597

Merged

machenmusik changed the title ~~Add ios hls tests~~ [WIP] Add ios hls tests May 13, 2017

need to check video element for both direct property and via getAttri…

ccd5e23

…bute; fix shader substitution tests

machenmusik force-pushed the add-ios-hls-tests branch from 73d4802 to ccd5e23 Compare May 13, 2017 18:18

skip shader substitution tests due to Travis CI Firefox issue

801058c

machenmusik force-pushed the add-ios-hls-tests branch from a79a0cb to 801058c Compare May 13, 2017 18:32

machenmusik changed the title ~~[WIP] Add ios hls tests~~ Add iOS HLS tests May 13, 2017

ngokevin reviewed May 14, 2017

View reviewed changes

machenmusik added 2 commits May 15, 2017 15:42

Revert "add isHLS to docs"

4acdbca

This reverts commit 8c43f5b.

use sceneEl.isIOS per discussion on PR

94b5c21

ngokevin requested changes May 15, 2017

View reviewed changes

minor edit per PR discussion

622dc98

ngokevin merged commit da71c5f into aframevr:master May 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add iOS HLS tests #2668

Add iOS HLS tests #2668

machenmusik commented May 13, 2017

machenmusik commented May 13, 2017

ngokevin commented May 13, 2017

machenmusik commented May 13, 2017 •

edited

Loading

ngokevin May 14, 2017

machenmusik May 15, 2017

machenmusik May 15, 2017

ngokevin May 15, 2017

ngokevin May 14, 2017

machenmusik May 15, 2017

ngokevin May 15, 2017

machenmusik May 15, 2017

machenmusik commented May 15, 2017

ngokevin May 15, 2017

machenmusik May 16, 2017

ngokevin May 15, 2017

machenmusik May 15, 2017

ngokevin May 15, 2017

machenmusik May 15, 2017

ngokevin May 15, 2017

machenmusik May 15, 2017

ngokevin May 15, 2017

machenmusik May 15, 2017

ngokevin May 15, 2017

machenmusik May 15, 2017

ngokevin May 15, 2017

machenmusik May 15, 2017

ngokevin May 15, 2017

machenmusik commented May 16, 2017

ngokevin commented May 16, 2017

ngokevin commented May 17, 2017 •

edited

Loading

machenmusik commented May 17, 2017

Add iOS HLS tests #2668

Add iOS HLS tests #2668

Conversation

machenmusik commented May 13, 2017

machenmusik commented May 13, 2017

ngokevin commented May 13, 2017

machenmusik commented May 13, 2017 • edited Loading

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

machenmusik commented May 15, 2017

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

machenmusik commented May 16, 2017

ngokevin commented May 16, 2017

ngokevin commented May 17, 2017 • edited Loading

machenmusik commented May 17, 2017

machenmusik commented May 13, 2017 •

edited

Loading

ngokevin commented May 17, 2017 •

edited

Loading