make accommodations necessary for iOS 10 HLS #2597

machenmusik · 2017-04-18T21:40:02Z

Provides core support needed for iOS 10 HLS playback discussed in #1416.

Note that this leaves video flipped in Y dimension and also with B and R color channels reversed.
It is expected that corrections can be applied using custom shaders.
(Accommodating those corrections using the built-in shaders is outside the scope of this PR.)

machenmusik · 2017-04-18T21:42:29Z

(Due to the current Mobile Safari CORS issue, this is a bit of a PITA to diagnose / address / test as the A-Frame page and the HLS content must have the same domain.)

dmarcos · 2017-04-19T15:30:53Z

So with PR the video at least plays but flipped and with wrong color? :)

dmarcos · 2017-04-19T15:31:11Z

Is that better than the video not playing at all?

machenmusik · 2017-04-19T15:36:36Z

Is that better than the video not playing at all?

Without this change, there is no way to make it work at all.
With this change, it is actually simple to use custom shader to fix flip and channel order.

So yes, this change is much better than the video not playing at all... see discussion in #1416

dmarcos · 2017-04-19T22:16:20Z

Can we add add a custom shader that it's set automatically when playing on iOS?

machenmusik · 2017-04-19T23:35:30Z

ok, the switching could probably use a little better guard (e.g. auto-switch if it's a shader like standard or flat we know won't work) if you wanted to touch up the PR

dmarcos · 2017-04-19T23:49:07Z

src/systems/material.js

+    if (utils.device.isIOS() && isHLS(videoEl)) {
+      // really it's BGRA, so this needs correction in shader
+      texture.format = THREE.RGBAFormat;
+      texture.needsCorrectionBGRA = true;


Can we add this to the this instead of the texture?

I don't think so - this is the system, not a given video texture, right? Complicating things is the fact that iOS 10 MP4 playback does not need any of these workarounds, so needs to be done on a per-video-texture basis.

dmarcos · 2017-04-19T23:49:18Z

src/systems/material.js

+      texture.needsCorrectionBGRA = true;
+      // apparently this is needed for HLS, so this needs correction in shader
+      texture.flipY = false;
+      texture.needsCorrectionFlipY = true;


Can we add this to the this instead of the texture?

(see above)

machenmusik · 2017-04-20T00:03:03Z

due to CORS issues with Mobile Safari, in order to test this, your HTML page must be in the same exact domain as the IDevice-compatible video content (m3u8 manifests and ts / aac files), Subdomains will not work at present.

cvan · 2017-05-03T11:37:24Z

due to CORS issues with Mobile Safari, in order to test this, your HTML page must be in the same exact domain as the IDevice-compatible video content (m3u8 manifests and ts / aac files), Subdomains will not work at present.

can you point me to a sample video you're using as a test case?

cvan · 2017-05-03T11:39:45Z

src/systems/material.js

@@ -195,6 +205,12 @@ module.exports.System = registerSystem('material', {
  }
 });

+function isHLS (videoEl) {
+  if (videoEl.type === 'application/x-mpegurl') return true;


do MIME types get lowercased automatically when calling the type getter?

I notice in Apple's docs, it appears as application/x-mpegURL.

also, do you want to check vnd.apple.mpegURL as well?

and what about video/MP2T (.ts)?

i've only seen lowercase, but good question.

not sure on your other two points

cvan · 2017-05-03T11:40:07Z

src/systems/material.js

@@ -195,6 +205,12 @@ module.exports.System = registerSystem('material', {
  }
 });

+function isHLS (videoEl) {
+  if (videoEl.type === 'application/x-mpegurl') return true;
+  if (videoEl.src && videoEl.src.indexOf('.m3u8') > 0) return true;


I would check videoEl.src.toLowerCase()

Can we check for toLowerCase and add braces to the return?

if (videoEl.src && videoEl.src.toLowerCase().indexOf('.m3u8') > 0) { return true; }

cvan · 2017-05-03T11:41:00Z

src/shaders/ios10hls.js

+var registerShader = require('../core/shader').registerShader;
+
+/**
+ * Custom shader for iOS 10 HLS.


can you add a comment with a URL to a doc/spec for this API?

sorry which API?

some Apple docs (or Google's Web Fundamentals docs) about what HLS is

cvan · 2017-05-03T11:41:38Z

@machenmusik nice work on supporting this 👍 if you can point me to some test case URLs, I'll try this out on my end.

machenmusik · 2017-05-03T15:16:36Z

i think i used hls urls from the prior threejs tickets on the topic

maybe should just convert the aframe 360 video from MP4 to iPhone-safe HLS?

cvan · 2017-05-09T03:13:25Z

maybe should just convert the aframe 360 video from MP4 to iPhone-safe HLS?

how about including both as <source>s?

machenmusik · 2017-05-09T14:49:37Z

if including both as <source> works, that's fine although it complicates the example since you can't do with inline src in a-videosphere for example -- I was just trying to say that if we want HLS source, should probably make a version of the recently-iDevice-friendly MP4 example that is HLS format

cvan · 2017-05-10T13:46:22Z

awesome, thanks for making the updates 👍 btw, is there a simple test or two you can add?

wjhtinger · 2017-05-10T15:14:32Z

How about play the live 360 stream from webrtc?

cvan · 2017-05-10T16:30:47Z

See my comment in issue #2663. If @machenmusik has other insights, would be glad to know and discuss. Thanks, everyone 👍

dmarcos · 2017-05-12T01:00:03Z

@machenmusik I want to merge this but I would like to see it working first. Do you have any example I can try?

machenmusik · 2017-05-12T02:39:30Z

so per discussion with @dmarcos, it seems that recent changes may have broken this somehow.., may need to do a little more investigation before merging this one :-/

machenmusik · 2017-05-12T16:35:52Z

found the problem :-)

dmarcos · 2017-05-12T16:56:30Z

Thanks!

cvan · 2017-05-10T13:45:23Z

src/systems/material.js

@@ -206,8 +206,8 @@ module.exports.System = registerSystem('material', {
 });

 function isHLS (videoEl) {
-  if (videoEl.type === 'application/x-mpegurl') return true;
-  if (videoEl.src && videoEl.src.indexOf('.m3u8') > 0) return true;
+  if (videoEl.type.toLowerCase() === 'application/x-mpegurl') { return true; }


per my earlier comment re: Apple's docs, I'd also check for vnd.apple.mpegurl and video/mp2t

cvan · 2017-05-10T13:45:37Z

src/systems/material.js

-  if (videoEl.type === 'application/x-mpegurl') return true;
-  if (videoEl.src && videoEl.src.indexOf('.m3u8') > 0) return true;
+  if (videoEl.type.toLowerCase() === 'application/x-mpegurl') { return true; }
+  if (videoEl.src && videoEl.src.toLowerCase().indexOf('.m3u8') > 0) { return true; }


per my earlier comment re: Apple's docs, I'd also check for .ts

cvan · 2017-05-10T13:45:52Z

src/shaders/ios10hls.js

@@ -1,7 +1,8 @@
 var registerShader = require('../core/shader').registerShader;

 /**
- * Custom shader for iOS 10 HLS.
+ * Custom shader for iOS 10 HTTP Live Streaming (HLS).
+ * For more information on HLS, see https://datatracker.ietf.org/doc/draft-pantos-http-live-streaming/


thanks for adding link. I'd also link to https://developer.apple.com/library/content/documentation/NetworkingInternet/Conceptual/StreamingMediaGuide/DeployingHTTPLiveStreaming/DeployingHTTPLiveStreaming.html

cvan · 2017-05-13T01:30:57Z

src/utils/material.js

@@ -127,3 +131,9 @@ function handleTextureEvents (el, texture) {
  });
 }
 module.exports.handleTextureEvents = handleTextureEvents;
+
+module.exports.isHLS = function (videoEl) {
+  if (videoEl.type && videoEl.type.toLowerCase() in ['application/x-mpegurl', 'application/vnd.apple.mpegurl']) { return true; }


arrays don't work like this. you've got to use includes or indexOf (which has better compatibility across the board). test for this?

interesting, as it does work?

maybe it only works based on src. i do owe some tests (as subsequent PR) so will fix there as needed

it doesn't work for me in Chrome nor Safari on iOS - you sure?

here's a test case I tested on iOS.

i think the test page i was using was matching on src (masking broken type check) - will fix when i add isHLS tests as mentioned above

awesome! thanks

cvan · 2017-05-13T01:31:24Z

src/utils/material.js

+
+module.exports.isHLS = function (videoEl) {
+  if (videoEl.type && videoEl.type.toLowerCase() in ['application/x-mpegurl', 'application/vnd.apple.mpegurl']) { return true; }
+  if (videoEl.src && videoEl.src.toLowerCase().indexOf('.m3u8') > 0) { return true; }


what about checking for .ts too?

.ts is not HLS, it is not clear that either .ts playback works or that it needs the workaround (MP4 does not)

if you'd like to add .ts, and it needs this workaround, perhaps you'd like to do in separate PR as this one is merged now?

I'll test it some time to see - I just need to do some investigation there

cvan · 2017-05-13T01:34:10Z

src/systems/material.js

@@ -144,6 +145,16 @@ module.exports.System = registerSystem('material', {
    texture.minFilter = THREE.LinearFilter;
    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)) {


have you tested this on Edge (which supports WebVR in release channel)? Edge supports HLS too; it may or may not have the same bugs.

it does not.

it does not have the bugs, or it does not have proper HLS support?

it does not appear to have the bugs.

dmarcos reviewed Apr 19, 2017

View reviewed changes

cvan reviewed May 3, 2017

View reviewed changes

dmarcos added the needs changes label May 9, 2017

cvan assigned machenmusik May 9, 2017

cvan mentioned this pull request May 10, 2017

Do aframe can support play 360 live stream video? #2663

Closed

dmarcos added the needs answers / feedback label May 12, 2017

machenmusik added 5 commits May 11, 2017 22:11

make accommodations necessary for iOS 10 HLS

7fc0ee7

add candidate ios10hls shader

ec887c3

use ios10hls shader automatically if needed

7e69802

minor edits per discussion on PR

730188c

move isHLS into utils/material

cb9cf70

machenmusik force-pushed the fix-ios-10-hls branch from 65e8677 to cb9cf70 Compare May 12, 2017 02:21

fix isHLS issue

7e8be9a

dmarcos merged commit bb2fa5a into aframevr:master May 12, 2017

cvan reviewed May 13, 2017

View reviewed changes

machenmusik mentioned this pull request May 13, 2017

Add iOS HLS tests #2668

Merged

make accommodations necessary for iOS 10 HLS #2597

make accommodations necessary for iOS 10 HLS #2597

Conversation

machenmusik commented Apr 18, 2017

machenmusik commented Apr 18, 2017

dmarcos commented Apr 19, 2017

dmarcos commented Apr 19, 2017

machenmusik commented Apr 19, 2017 • edited Loading

dmarcos commented Apr 19, 2017

machenmusik commented Apr 19, 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

machenmusik commented Apr 20, 2017

cvan commented May 3, 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

cvan commented May 3, 2017

machenmusik commented May 3, 2017

cvan commented May 9, 2017

machenmusik commented May 9, 2017

cvan commented May 10, 2017

wjhtinger commented May 10, 2017

cvan commented May 10, 2017

dmarcos commented May 12, 2017

machenmusik commented May 12, 2017

machenmusik commented May 12, 2017

dmarcos commented May 12, 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

cvan 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

machenmusik commented Apr 19, 2017 •

edited

Loading

cvan May 13, 2017 •

edited

Loading