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

feat: use background-color from video element css #586

Merged
merged 2 commits into from
Oct 28, 2020
Merged

feat: use background-color from video element css #586

merged 2 commits into from
Oct 28, 2020

Conversation

calebboyd
Copy link
Contributor

@calebboyd calebboyd commented Oct 17, 2020

reopening due to #580 being closed

This adds a new subview to the webView which we ensure is always behind the webView and video views.
In the refresh cycle it updates the background color, setting the alpha channel to 0 on the HTML video
and the Element View and element background color to the passed rgb value.

@hthetiot hthetiot added this to the 6.0.15 milestone Oct 17, 2020
@hthetiot hthetiot changed the title feat: use background-color from element css feat: use background-color from video element css Oct 18, 2020
@hthetiot
Copy link
Contributor

hthetiot commented Oct 18, 2020

Not working as expected see comment and video here: #583

@hthetiot
Copy link
Contributor

hthetiot commented Oct 18, 2020

Please remove your "This adds a new subview to the webView which we ensure is always behind the webView and video views." This is unnecessary, just change the color of the videoView. As it break HTML over video capability using zIndex -1.

src/PluginMediaStreamRenderer.swift Outdated Show resolved Hide resolved
@hthetiot
Copy link
Contributor

Please test with https://github.com/cordova-rtc/cordova-plugin-iosrtc-sample and provide capture, you can thank @AleksandarTokarev for testing the initial PR.

@AleksandarTokarev
Copy link

Let me know if i need to do any testing when changes are made.

@hthetiot
Copy link
Contributor

Thank you @calebboyd , if you can rebase/merge master of not done already, like that @AleksandarTokarev can test :)

Thank you.

@AleksandarTokarev
Copy link

Maybe u guys can make a custom branch out of these newest changes

@hthetiot
Copy link
Contributor

Once this PR is sync with master, to include 6.0.15 fixes you can do that do test it:

cordova plugin remove cordova-plugin-iosrtc --verbose
cordova plugin add https://github.com/calebboyd/cordova-plugin-iosrtc#background --verbose
cordova platform remove ios --no-save
cordova platform add ios --no-save

That the beauty of git and npm/cordova in action :)

@AleksandarTokarev
Copy link

Sure, let me know when master is merged into this branch

@calebboyd
Copy link
Contributor Author

I had rebased it already, should be good to go

@calebboyd
Copy link
Contributor Author

I removed the superview concept. It had been working with the sample (as well as in my own application with HTML over video). I think I did have a bug with the zPositioning not applying the global minimum to the superview "fill" layer (If one iosrtcVideo was behind the webview and one was in front), this is likely what you saw @AleksandarTokarev

I had added the superview originally to support setting the plugin's background to a specified color (other than clear). Because we have to make that concession in order to support HTML over the video.

It might make more sense to add this with a different api altogether, rather than the videos' css

@hthetiot
Copy link
Contributor

Thank you @calebboyd for your contribution.
Once it's tested and match community expected behavior we can merge.

@AleksandarTokarev
Copy link

AleksandarTokarev commented Oct 18, 2020

@calebboyd @hthetiot
It seems that i am getting the black screen yet again in this rebased version on top of master
https://www.dropbox.com/s/h0zmaxmykyh1b6y/20201018_220940.mp4?dl=0
The streams (videos) load on both sides, but the black stuff is there (as u can see in the video)

In this second video - i have put background-color: white; as class in both the local and remote video elements
https://www.dropbox.com/s/trz225tqa6hsnoz/20201018_222813.mp4?dl=0
Here i am still getting the black video initially.

Also notice the 'stretching' of the video that is being done in the above 2 videos initially - this is not happening on 6.0.11

What i think would be the best UI/UX would be version 6.0.11 - see video in #583 (comment) (which does not have any black screen at any time), combined with the video stream fix on both sides in 6.0.15.

@calebboyd
Copy link
Contributor Author

calebboyd commented Oct 18, 2020

@hthetiot @AleksandarTokarev what do you think if we were to add a new plugin method for setting the background color of a fullScreen UIView, and then initializing the video elementViews to clear. Then the video css would kick in once the stream was available (maybe we can do that earlier?). In the meantime the UIViews configured background color would show through.

This was my intent with the superview piece.. since we're forcing the webview to be clear we need something that we can configure to replace it

CDView       PluginBackground (new layer)    PluginVideos or WebView
  |                |                                   |
  |                |                                   |      
  |                |                                   |
  |                |                                   |
  |                | black initially                   |   initially: clear,
  |                | iosrtc.setBackgroundColor(...)    |   rendered: background-color
  |                |                                   |
  |                |                                   |
  |                |                                   |

If we can get the video css earlier -- It might be most frictionless (no api needed) to use the bottommost (zPos) video's configured background color... 🤔.. or even use the body's background-color since it gets cleard

@AleksandarTokarev
Copy link

@calebboyd I am not really versed into Native Code and inner workings of Webview so i dont think i can give a proper comment on this one. But from your drawing, i think we would want initially to be clear and after that set the background color (or in my case dont do anything - just fill the video element with the stream)

@hthetiot
Copy link
Contributor

The PR look good, let me check again.

Off topic: On master I made a SHIM of canvas context drawimage video tag, I think you also.used render.save before you may want to check that @calebboyd

@hthetiot hthetiot modified the milestones: 6.0.15, 6.0.x Oct 23, 2020
Copy link
Contributor

@hthetiot hthetiot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@calebboyd I'm not sure about the full feature set that you want to implement in the end. But ideally you should if not change current behavior for the user/ community.

I'm sure you can make change to have the iosRTC sample app working with same behavior with html over video, that all that matters to me.

If a flag is needed check the implementation of the flag ANUAL_INIT_AUDIO_DEVICE

995eaaf#diff-6813833f33103dfcaa583d4f09e06dc84c0ce8ddd34c8b567b56c9728be58389

@@ -233,6 +234,9 @@ class PluginMediaStreamRenderer : NSObject, RTCEAGLVideoViewDelegate {
}

self.elementView.layer.cornerRadius = CGFloat(borderRadius)
let rgb = backgroundColor.components(separatedBy: ",").map{ CGFloat(($0 as NSString).floatValue) / 256.0 }
let color = UIColor(red: rgb[0], green: rgb[1], blue: rgb[2], alpha: 1)
self.elementView.backgroundColor = color
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

backgroundColorRgba[3] = '0';
this.element.style.backgroundColor = 'rgba(' + backgroundColorRgba.join(',') + ')';
backgroundColorRgba.length = 3;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@calebboyd
Copy link
Contributor Author

Sounds good. I don't plan on changing any behavior. Mostly just trying to make the plugin more congruent with browser behavior. The defaults here will remain the same (black) or more specifically, computedStyle for backgroundColor if not set, is black rgb(0,0,0). Which is the default that was established in #179 (comment)

@hthetiot
Copy link
Contributor

Let see how it goes, if it cause issue I will revert.

@hthetiot hthetiot merged commit b01a23c into cordova-rtc:master Oct 28, 2020
@hthetiot
Copy link
Contributor

work as expected on master and sample.

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