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

Video renders in landscape, but not portrait orientation #360

Closed
osnoww opened this issue Jul 23, 2019 · 21 comments
Closed

Video renders in landscape, but not portrait orientation #360

osnoww opened this issue Jul 23, 2019 · 21 comments

Comments

@osnoww
Copy link

osnoww commented Jul 23, 2019

I am using the latest version of this plugin

If a video call is started in landscape mode, the call is successful on a variety of Apple phones and OS versions
if the call is started in portrait mode, the call fails. The console log contains error messages like
"failed to bind EAGLDrawable: <CAEAGLLayer: 0x60000271a480> to GL_RENDERBUFFER 1
Failed to make complete framebuffer object 8cd6"

@hthetiot
Copy link
Contributor

hthetiot commented Aug 11, 2019

I do not reproduce this issue.
Can you provide the iOS version used.

@hthetiot
Copy link
Contributor

hthetiot commented Aug 13, 2019

It may be related to the style of the video tag that you use in portrait causing the issue.
Can you share the CSS applied to the video tag you use in portrait? @osnoww

@hthetiot hthetiot added the bug label Aug 13, 2019
@hthetiot
Copy link
Contributor

will test and confirm.

@hthetiot
Copy link
Contributor

hthetiot commented Aug 15, 2019

Can you try with latest v5.0.0 @osnoww see upgrade instructions here:

@hthetiot
Copy link
Contributor

Cannot reproduce

@hthetiot hthetiot added this to the 5.0.2 milestone Sep 13, 2019
@hthetiot hthetiot self-assigned this Sep 13, 2019
@osnoww
Copy link
Author

osnoww commented Sep 13, 2019 via email

@hthetiot
Copy link
Contributor

hthetiot commented Sep 15, 2019

Thank you @osnoww that a great news.
I hope you can make a PR, let me know if you want to give me a patch and do the cleaning for you.

Here is the initial draft and I have exposed RTCPeerConnection.addTrack|removeTrack already for the future migration in 5.0.2 next release.

PluginRTCPeerConnection.(addTrack|removeTrack) https://github.com/cordova-rtc/cordova-plugin-iosrtc/blob/task/getReceiversOrSenders-and-addOrRemoveTracks/src/PluginRTCPeerConnection.swift#L276

RTCPeerConnection_(addTrack|removeTrack):
https://github.com/cordova-rtc/cordova-plugin-iosrtc/blob/task/getReceiversOrSenders-and-addOrRemoveTracks/src/iosrtcPlugin.swift#L304

@cah-kyle-dunn
Copy link
Contributor

cah-kyle-dunn commented Oct 8, 2019

I'm now experiencing this issue after iOS 13 (specifically, iOS 13.1.2). The attributes don't really change between orientations. It works in iOS 12.

@cah-kyle-dunn
Copy link
Contributor

It appears to be fixed by #399

@hthetiot
Copy link
Contributor

hthetiot commented Oct 9, 2019

Yes @cah-dunn I can confirm on iOS #399 works fine, but the fact is I did not change anything regarding orientation detection beside calling Renderer every 500ms.

An alternative to it would be this:

window.addEventListener('resize', function () { 
    console.log('orientationChange');
    cordova.plugins.iosrtc.refreshVideos();

})

I'm not sure I will keep the Interval 500ms on release, you may want to check #master and confirm using resize event + cordova.plugins.iosrtc.refreshVideos does fix master.

@hthetiot
Copy link
Contributor

hthetiot commented Oct 9, 2019

May be this get trigger on #399 rotate but I have not check yet:

videoView(_ videoView: RTCEAGLVideoView!, didChangeVideoSize size: CGSize)
https://github.com/cordova-rtc/cordova-plugin-iosrtc/blob/master/src/PluginMediaStreamRenderer.swift

@hthetiot hthetiot modified the milestones: 5.0.2, 6.0.x Oct 9, 2019
@hthetiot
Copy link
Contributor

hthetiot commented Oct 9, 2019

@osnoww check #399 it's M69 if you have above M75 please let me know I can take your change and integrate them may be.

@hthetiot
Copy link
Contributor

hthetiot commented Oct 9, 2019

Confirm "func videoView(_ videoView: RTCVideoRenderer, didChangeVideoSize size: CGSize) {" can call on orientation change on M69. cc @cah-dunn

@cah-kyle-dunn
Copy link
Contributor

cah-kyle-dunn commented Oct 9, 2019

@hthetiot I did a good amount of debugging with master and I think it was likely an issue with the old libWebRTC-LATEST-Universal-Release.a and updating to WebRTC.framework fixed it. This specific issue actually caused issues in Safari after iOS 13 Beta 4 and had to be fixed in WebKit (https://bugs.webkit.org/show_bug.cgi?id=199519). Could be unrelated, but very coincidental.

I can confirm that the video view is being updated on orientation change. I think we could/should remove the 500ms interval prior to release.

Thanks for all of your work on this project by the way, you're making some awesome updates.

@hthetiot
Copy link
Contributor

hthetiot commented Oct 9, 2019

@cah-dunn Thank you for great feedback, much apreciated.

@osnoww
Copy link
Author

osnoww commented Oct 10, 2019 via email

@akilude
Copy link
Contributor

akilude commented Oct 30, 2019

@hthetiot the issue still persists when using the plugin (version 6.0).

Occurs when using the below css:
min-width: 100%; min-height: 100vh;

Simplest fix is to not specify min-width and min-height css.

I've tested on iOS 10, iphone XR

@hthetiot
Copy link
Contributor

Do you call refreshVideos on rotate ?
Please provide full example, this issue has been close because I do not reproduce.

@hthetiot
Copy link
Contributor

Note: this has nothing to do with Metal anyway.

@hthetiot
Copy link
Contributor

hthetiot commented Oct 30, 2019

Simplest fix is to not specify min-width and min-height css.

That not the same issue then @akilude
Here we talked about non rendering not issue with different CSS. READ issue please don't comment out of topic #360 (comment)

Please create separate issue with full example and will had support, alternatively don't do bad css like that use flexbox and with:100% height: 100%

@hthetiot
Copy link
Contributor

See related function that get the size, should work with any proper CSS,...

https://github.com/cordova-rtc/cordova-plugin-iosrtc/blob/master/js/MediaStreamRenderer.js

Unless full example is provided, I will not reopen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants