Skip to content
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.

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

implement RTCPeerConnectionFactory(encoderFactory, decoderFactory) using getSupportedVideoEncoder to enable VP8 and VP9 #416

Merged
merged 7 commits into from
Oct 29, 2019

Conversation

hthetiot
Copy link
Contributor

@hthetiot hthetiot commented Oct 24, 2019

Results in VP8, VP9 and H264 being available now.

a=rtpmap:96 VP9/90000
a=rtpmap:98 H264/90000
a=rtpmap:127 VP8/90000

const pc = new RTCPeerConnection(null);
pc.createOffer({
offerToReceiveAudio: true,
offerToReceiveVideo: true,
}).then((v) => console.log(v.sdp))

o=- 3095945614665419454 2 IN IP4 127.0.0.1
s=-
t=0 0
a=group:BUNDLE audio video
a=msid-semantic: WMS
m=audio 9 UDP/TLS/RTP/SAVPF 111 103 104 9 102 0 8 106 105 13 110 112 113 126
c=IN IP4 0.0.0.0
a=rtcp:9 IN IP4 0.0.0.0
a=ice-ufrag:BoM3
a=ice-pwd:9WQjGV6w/1I884V9dWKlpKMT
a=ice-options:trickle renomination
a=fingerprint:sha-256 32:C4:CE:92:6B:D5:8F:75:C1:06:E0:D9:EC:0F:58:10:6C:ED:A9:2D:7D:AD:27:F1:EC:1F:18:A7:31:FC:45:5B
a=setup:actpass
a=mid:audio
a=extmap:1 urn:ietf:params:rtp-hdrext:ssrc-audio-level
a=recvonly
a=rtcp-mux
a=rtpmap:111 opus/48000/2
a=rtcp-fb:111 transport-cc
a=fmtp:111 minptime=10;useinbandfec=1
a=rtpmap:103 ISAC/16000
a=rtpmap:104 ISAC/32000
a=rtpmap:9 G722/8000
a=rtpmap:102 ILBC/8000
a=rtpmap:0 PCMU/8000
a=rtpmap:8 PCMA/8000
a=rtpmap:106 CN/32000
a=rtpmap:105 CN/16000
a=rtpmap:13 CN/8000
a=rtpmap:110 telephone-event/48000
a=rtpmap:112 telephone-event/32000
a=rtpmap:113 telephone-event/16000
a=rtpmap:126 telephone-event/8000
m=video 9 UDP/TLS/RTP/SAVPF 96 97 98 99 100 101 127 123 125 122 124
c=IN IP4 0.0.0.0
a=rtcp:9 IN IP4 0.0.0.0
a=ice-ufrag:BoM3
a=ice-pwd:9WQjGV6w/1I884V9dWKlpKMT
a=ice-options:trickle renomination
a=fingerprint:sha-256 32:C4:CE:92:6B:D5:8F:75:C1:06:E0:D9:EC:0F:58:10:6C:ED:A9:2D:7D:AD:27:F1:EC:1F:18:A7:31:FC:45:5B
a=setup:actpass
a=mid:video
a=extmap:2 urn:ietf:params:rtp-hdrext:toffset
a=extmap:3 http://www.webrtc.org/experiments/rtp-hdrext/abs-send-time
a=extmap:4 urn:3gpp:video-orientation
a=extmap:5 http://www.ietf.org/id/draft-holmer-rmcat-transport-wide-cc-extensions-01
a=extmap:6 http://www.webrtc.org/experiments/rtp-hdrext/playout-delay
a=extmap:7 http://www.webrtc.org/experiments/rtp-hdrext/video-content-type
a=extmap:8 http://www.webrtc.org/experiments/rtp-hdrext/video-timing
a=recvonly
a=rtcp-mux
a=rtcp-rsize
a=rtpmap:96 VP9/90000
a=rtcp-fb:96 goog-remb
a=rtcp-fb:96 transport-cc
a=rtcp-fb:96 ccm fir
a=rtcp-fb:96 nack
a=rtcp-fb:96 nack pli
a=rtpmap:97 rtx/90000
a=fmtp:97 apt=96
a=rtpmap:98 H264/90000
a=rtcp-fb:98 goog-remb
a=rtcp-fb:98 transport-cc
a=rtcp-fb:98 ccm fir
a=rtcp-fb:98 nack
a=rtcp-fb:98 nack pli
a=fmtp:98 level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=640c2a
a=rtpmap:99 rtx/90000
a=fmtp:99 apt=98
a=rtpmap:100 H264/90000
a=rtcp-fb:100 goog-remb
a=rtcp-fb:100 transport-cc
a=rtcp-fb:100 ccm fir
a=rtcp-fb:100 nack
a=rtcp-fb:100 nack pli
a=fmtp:100 level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=42e02a
a=rtpmap:101 rtx/90000
a=fmtp:101 apt=100
a=rtpmap:127 VP8/90000
a=rtcp-fb:127 goog-remb
a=rtcp-fb:127 transport-cc
a=rtcp-fb:127 ccm fir
a=rtcp-fb:127 nack
a=rtcp-fb:127 nack pli
a=rtpmap:123 rtx/90000
a=fmtp:123 apt=127
a=rtpmap:125 red/90000
a=rtpmap:122 rtx/90000
a=fmtp:122 apt=125
a=rtpmap:124 ulpfec/90000

Testing

To test task/preferredCodec branch

cordova plugin remove cordova-plugin-iosrtc --verbose
cordova plugin add https://github.com/cordova-rtc/cordova-plugin-iosrtc#task/preferredCodec --verbose
cordova platform remove ios --no-save
cordova platform add ios --no-save

…ing getSupportedVideoEncoder to enable VP8 and VP9
@hthetiot
Copy link
Contributor Author

hthetiot commented Oct 24, 2019

VP9 performance is not really good, we may need to change priority order or disable VP9.

@hthetiot
Copy link
Contributor Author

hthetiot commented Oct 24, 2019

Arf using VP8 or VP9 cause video size changes loop that call MediaStreamRenderer_refresh to much with videoViewWidth 997.0000000000001

PluginMediaStreamRenderer#refresh() [elementLeft:0.0, elementTop:0.0, elementWidth:1024.0, elementHeight:748.0, videoViewWidth:997.0000000000001, videoViewHeight:748.0, visible:true, opacity:1.0, zIndex:-1.0, mirrored:false, clip:true, borderRadius:0.0]
PluginMediaStreamRenderer#refresh() [elementLeft:0.0, elementTop:0.0, elementWidth:1024.0, elementHeight:748.0, videoViewWidth:997.0000000000001, videoViewHeight:748.0, visible:true, opacity:1.0, zIndex:-1.0, mirrored:false, clip:true, borderRadius:0.0]
PluginMediaStreamRenderer#refresh() [elementLeft:0.0, elementTop:0.0, elementWidth:1024.0, elementHeight:748.0, videoViewWidth:997.0000000000001, videoViewHeight:748.0, visible:true, opacity:1.0, zIndex:-1.0, mirrored:false, clip:true, borderRadius:0.0]

I need to invesstigate before merging this.

@hthetiot
Copy link
Contributor Author

Refresh loop fixed by c2139a4

…dow resize event to resize peer MediaStreamRenderer while local MediaStreamRenderer will resize due device video size change in most of the cases
…oderFactory.preferredCodec to prioritize H264 as VP9 can results in bad performance
@hthetiot
Copy link
Contributor Author

hthetiot commented Oct 24, 2019

VP9 sucks:

@menelike

This comment was marked as off-topic.

@menelike

This comment was marked as off-topic.

@hthetiot
Copy link
Contributor Author

hthetiot commented Oct 24, 2019

@menelike Sorry I'm confused what did you say, I need your help to understand.
you patched v4 with https://github.com/cordova-rtc/cordova-plugin-iosrtc/pull/416/files#diff-d60c1f113017b14d32febffb7b73e125L893-R902 and it solve io13?
The only diff with v5 and v4 is javascript not swift (or really minor)

See final: 07d00dc

This has nothing to do with ios13 it's a refresh size jiggle related to my understanding.

@hthetiot
Copy link
Contributor Author

hthetiot commented Oct 24, 2019

@menelike

  1. This is not related to this issue, please create separate issue, it's due that i did not make a perfect use of LayoutConstraints on v5, help is welcome to improve ([Resolved] use safeAreaLayoutGuide for ios 11 and NSLayoutConstraint for ios 10 to fix elementView bad position depending status bar visibility #367) but this was proven only to be a warning and do not cause issue so fare.
[LayoutConstraints] Unable to simultaneously satisfy constraints.
	Probably at least one of the constraints in the following list is one you don't want. 
	Try this: 
		(1) look at each constraint and try to figure out which you don't expect; 
		(2) find the code that added the unwanted constraint or constraints and fix it. 
	(Note: If you're seeing NSAutoresizingMaskLayoutConstraints that you don't understand, refer to the documentation for the UIView property translatesAutoresizingMaskIntoConstraints) 
(
    "<NSAutoresizingMaskLayoutConstraint:0x282300eb0 h=-&- v=-&- WKWebView:0x105010e00'rise'.height == UIView:0x108300f90.height   (active)>",
    "<NSAutoresizingMaskLayoutConstraint:0x2823112c0 h=--& v=--& UIView:0x104c7e450.midY == 604.5   (active)>",
    "<NSAutoresizingMaskLayoutConstraint:0x282312b20 h=--& v=--& UIView:0x104c7e450.height == 323   (active)>",
    "<NSLayoutConstraint:0x2823cf6b0 UIView:0x104c7e450.bottom == UILayoutGuide:0x283904000'UIViewSafeAreaLayoutGuide'.bottom   (active)>",
    "<NSLayoutConstraint:0x282300e10 'UIView-Encapsulated-Layout-Height' UIView:0x108300f90.height == 375   (active)>",
    "<NSLayoutConstraint:0x282301a90 'UIViewSafeAreaLayoutGuide-bottom' V:[UILayoutGuide:0x283904000'UIViewSafeAreaLayoutGuide']-(34)-|   (active, names: '|':WKWebView:0x105010e00'rise' )>"
)
  1. H264 works initially, but after a few seconds (random) please create separate issue
    This i think it related to the Capturer that change size from 640 to 1280 when you start the peer stream. I have not reproduced on Freeswitch or Firefox but i did have issue on Chrome and Safari 11, Can you confirm that when you st with: 640, height: 480 on GetUserMedia it works that will confirm that the issue is from chrome that dont like stream size change (specially 1280).

@menelike The topic is VP9/VP8 here not LayoutConstraints, not H264 that are both on 6 and 5 master. Please try to keep the subject of the issue contains, feel free to create other dedicated issue that to avoid noise and fix one problem at the time. Thank you.

@menelike

This comment was marked as off-topic.

@hthetiot
Copy link
Contributor Author

hthetiot commented Oct 24, 2019

@menelike Quick notes on H264 works initially, but after a few seconds (random) please create separate issue for more help.

You will see this:

Cordova iOSRTC Sample[30058:1037578] PluginMediaStreamRenderer | video size changed [width:960.0, height:540.0]
Cordova iOSRTC Sample[30058:1037578] PluginMediaStreamRenderer | video size changed [width:1280.0, height:720.0]
Cordova iOSRTC Sample[30058:1037578] PluginMediaStreamRenderer | video size changed [width:1280.0, height:960.0]

That like RTCCameraVideoCapturer.startCapture ramp up to the right resolution and Chrome does not like the change of resolution when reaching 1280x960 but all other are ok, could be a H264 profile issue. If you set GetUserMedia with and height to anything bellow 1080 or even only height 720 it will not freeze.

https://github.com/cordova-rtc/cordova-plugin-iosrtc/blob/master/src/PluginRTCVideoCaptureController.swift#L141

@hthetiot hthetiot modified the milestones: 6.0.x, 6.0.0 Oct 25, 2019
@hthetiot
Copy link
Contributor Author

@hthetiot
Copy link
Contributor Author

@menelike this should mitigate "H264 works initially, but after a few seconds (random)"
0a90699

@menelike

This comment was marked as off-topic.

@DaveLomber
Copy link

Hi there

I faced the black screen instead of remote video stream on iOS 13

With this MR it started to work well, great job guys 🥇
I continue do more testing

@hthetiot
Copy link
Contributor Author

Thank you for feedback @DaveLomber

@hthetiot
Copy link
Contributor Author

Let land this on master so we support Janus and Jisti

@hthetiot hthetiot merged commit 156f9a2 into master Oct 29, 2019
@hthetiot hthetiot deleted the task/preferredCodec branch June 30, 2020 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants