AgoraIO-Community / AgoraWebSDK-NG

The Next Generation of Agora Web SDK
https://agoraio-community.github.io/AgoraWebSDK-NG/
161 stars 75 forks source link

Some feedback #50

Open Jarred-Sumner opened 4 years ago

Jarred-Sumner commented 4 years ago

I'm using Agora inside a browser-based game for in-game voice chat. Overall, I really like Agora and Agora Web SDK NG is an easier API than Agora Web SDK – good job!

I have a few points of feedback that I hope is helpful to you.

Bundle size

The minified source is 547 KB, which is large for any library.

I think there are a number of opportunities to reduce bundle size. From looking at the code, you could release 2-3 bundles instead:

If you provided a CommonJS or ESM build and then used export & import in your source code to import files, this would make it more eligible for tree shaking. Additionally, for any libraries you use, you'd require them and add them as a dependency in your package.json, rather than inline them into the source.

Different companies use Agora differently – some might use the DOM-related functionality (showing a video stream, adding an <audio /> tag, etc) and some might not. Tree shaking would allow integrators to only import the parts of Agora's SDK that are relevant to their application. This should significantly reduce bundle size for most integrators.

Performance

Agora makes many requests, seemingly for analytics rather than VOIP. There is a meaningful performance cost to sending lots of requests, and browsers have connection limits for how many requests they send at a time per tab.

If the only thing happening on the page is VOIP, then its probably fine. But, if there's lots of other stuff too (e.g. rendering a 3D game), it can cause performance problems.

It would be great if integrators could disable analytics and voice quality-related data collection.

Additionally, allocating new objects or pushing onto arrays inside of setInterval (or any timer) is an easy way to trigger Garbage Collection and briefly freeze the webpage. If you reuse objects (where possible) inside of timers, it will improve performance.

CORP & COEP

Currently, this SDK does not work when using Cross-Origin-Resource-Policy & Cross-Origin-Embedder-Policy together. Requests to Agora's servers are blocked by the browser.

These headers enable SharedArrayBuffer in Firefox 79 and soon will be required by Chrome to use SharedArrayBuffer too.

These standards are pretty new, so it makes sense why this wouldn't be supported yet. I'm honestly not sure of a solution to this, unless it involves proxying.

Audio Context

Web browsers cannot create many AudioContext objects per page without stuff breaking. It would be great if the SDK allowed a developer to pass in their own AudioContext object, instead of creating one during initialization.

disoul commented 4 years ago

Thanks for your feedback! 👍

Your suggestions are very nice and helpful, I think we can add some of them to our team's roadmap.

Bundle size

There are a lot of babel-polyfill codes in the current SDK bundle. However, these codes are redundant in most scenarios. We build SDK with the polyfill codes because some customers want to import(not use!) our SDK in some very old browsers. (maybe they just want to import our SDK in <script> tag and call checkSystemRequirements to display an unsupported warning.)

Adding an ESM bundle is a good solution. In the ESM bundle we can remove these unuseful codes and support tree shaking.

Performance

We are planning to merge these analytics requests or use WebSocket to report analytics data.

CORP & COEP

I think it is necessary for Agora service to support CORP & COEP, SharedArrayBuffer is very useful in some WASM applications. But this is not our team's work, I will try to push our backend service team.

Audio Context

This sounds good, I will add this API in the future version.

But please be attention that If developers pass their own AudioContext to SDK, they have to write many iOS/Safari AudioContext workarounds by themself. Such as no sound in iOS silent mode, no sound in iOS 12 randomly.

disoul commented 4 years ago

I have tested our demo page in cross-origin isolated mode on Firefox 79, it works fine and I can access the SharedArrayBuffer API.

image

Then I read the spec and find this line:

"require-corp" When this value is used, fetching cross-origin resources requires the server's explicit permission through the CORS protocol or the Cross-Origin-Resource-Policy header.

My understanding is that our service now supports the CORS protocol, so there is no need to add the Cross-Origin-Resource-Policy header, right?

Jarred-Sumner commented 4 years ago

Thanks for the detailed response! Also, your English is really good.

But please be attention that If developers pass their own AudioContext to SDK, they have to write many iOS/Safari AudioContext workarounds by themself. Such as no sound in iOS silent mode, no sound in iOS 12 randomly.

Yeah, manually managing AudioContext is not fun (especially in Safari).

Chrome has a little-known API that tells you if creating an AudioContext will work before you actually create it:

if (
  navigator?.userActivation?.isActive ||
  navigator?.userActivation?.hasBeenActive
) {
  this.getAudioContext();
}

I don't see documentation for navigator.userActivation other than old blog posts though and it's Chrome only. But it might help reduce cases where an AudioContext is created prematurely in Chrome

My understanding is that our service now supports the CORS protocol, so there is no need to add the Cross-Origin-Resource-Policy header, right?

You're right – I misread the document, I thought it required both CORS & CORP rather than either CORS or CORP. I was also missing the Cross-Origin-Opener-Policy header.

Edit: It works in Firefox correctly, but once these headers are added, it no longer works in Chrome – at least, on localhost. image

This is with the following headers on the page: image

I'm working around this issue by proxying these domains and editing the code to request relative to / rather than https:// (e.g. /webrtc2-ap-web-1.agora.io/url):

const PROXY_DOMAINS = [
  "webrtc2-ap-web-1.agora.io",
  "webrtc2-ap-web-2.agoraio.cn",
  "webrtc2-ap-web-3.agora.io",
  "webrtc2-ap-web-4.agoraio.cn",
  "ap-proxy-1.agora.io",
  "ap-proxy-2.agora.io",
  "cds-ap-web-1.agora.io",
  "cds-ap-web-2.agoraio.cn",
  "cds-ap-web-3.agora.io",
  "cds-ap-web-4.agoraio.cn",
  "sua-ap-web-1.agora.io",
  "sua-ap-web-2.agoraio.cn",
  "sua-ap-web-3.agora.io",
  "sua-ap-web-4.agoraio.cn",
  "uap-ap-web-1.agora.io",
  "uap-ap-web-2.agoraio.cn",
  "uap-ap-web-3.agora.io",
  "uap-ap-web-4.agoraio.cn",
];

BTW, have you seen RNNoise? Its better than native noise cancellation and fast enough to run in real-time on browsers. I integrated it into the game to cover up laptop fan noise in voice chat. I'd be a little wary of using it in browsers that don't support Audio Worklet or any older browsers, but it might be a cool feature to add sometime later since it would (theoretically) improve audio call quality & make the volume level stuff better at distinguishing between speech and ambient noise.


On an unrelated note, do you know how to get pushing a livestream to CDN to work? I'm trying to make it push a MediaStreamTrack from a Canvas element to an RTMP url and I get invalid_appid. I filed a support ticket about this: CUS-7128 and was able to reproduce the issue in both Agora Web SDK and Agora Web SDK NG.

disoul commented 4 years ago

It seems that Chrome and Firefox have different CORE strategies. But I think it is still necessary for Agora Service to add this header(there are no side effects in this header I thought).


😃 RNNoise and other software 3A algorithms are in our plan. Chrome's AEC module will not process the audio from WebAudio(https://bugs.chromium.org/p/chromium/issues/detail?id=687574). So we need a way to implement audio 3A processing through JavaScript or WASM.


Have you enabled the RTMP Converter service for your APPID in your Agora Console? image