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

Fix for z-index / transparent background / stacking of UIViews issue #98

Closed
wants to merge 4 commits into from
Closed

Conversation

1N50MN14
Copy link

@1N50MN14 1N50MN14 commented Dec 2, 2015

This fix allows for:

  1. HTML View present above all video UI Views
  2. Video UI views stacked according to z-index
  3. Webhook now fixes MainViewController.m to allow for transparent background on webView

NOTE: video::-webkit-media-controls { display:none !important; } must be added to the CSS to hide video controllers, maybe this should be added to the documentation.

@ibc
Copy link
Collaborator

ibc commented Dec 3, 2015

It looks amazing! so thanks!

Just le me please a few days to go deeper into the changes.

@1N50MN14
Copy link
Author

1N50MN14 commented Dec 3, 2015

No worries, tested with iOS 8 & 9. On iOS 8 .clearColor() the background color wouldn't budge, same issue you encountered, the addon to the webhook forces it to work.

@gbhrdt
Copy link

gbhrdt commented Dec 9, 2015

Hi @1N50MN14
very nice, but I'm getting lots of these errors using your PR:

2015-12-09 14:33:04.640 videochatApp[274:12967] Failed to bind EAGLDrawable: <CAEAGLLayer: 0x15da79e00> to GL_RENDERBUFFER 1
2015-12-09 14:33:04.640 videochatApp[274:12967] Failed to make complete framebuffer object 8cd6
2015-12-09 14:33:04.668 videochatApp[274:12967] Failed to bind EAGLDrawable: <CAEAGLLayer: 0x15da79e00> to GL_RENDERBUFFER 1
2015-12-09 14:33:04.669 videochatApp[274:12967] Failed to make complete framebuffer object 8cd6
2015-12-09 14:33:04.702 videochatApp[274:12967] Failed to bind EAGLDrawable: <CAEAGLLayer: 0x15da79e00> to GL_RENDERBUFFER 1
2015-12-09 14:33:04.702 videochatApp[274:12967] Failed to make complete framebuffer object 8cd6
2015-12-09 14:33:04.735 videochatApp[274:12967] Failed to bind EAGLDrawable: <CAEAGLLayer: 0x15da79e00> to GL_RENDERBUFFER 1

I'm using iOS9.1.
The latest version of iosrtc from this branch works.

@1N50MN14
Copy link
Author

@gbhrdt I'm not getting any of those erros on iOS9.1, have you placed a z-index css property on the video element(s)?

@gbhrdt
Copy link

gbhrdt commented Dec 10, 2015

@1N50MN14 I did not change anything from my previous setup, so no, I did not add a z-index property. But shouldn't it work without it as well? I could go into some deeper testing tomorrow.

@1N50MN14
Copy link
Author

@gbhrdt You need to assign negative z-index properties on the video. If I were you I would wait until @ibc has confirmed this PR and merged it, then documentation would be updated and you can follow that. Otherwise it will be hard for me to figure out why or what is not working for you.

@gbhrdt
Copy link

gbhrdt commented Dec 10, 2015

@1N50MN14 Alright, I just realized I had some positive z-index set for one video element, because I wanted to place it above the other video element.
The plugin no more crashes with negative z-indexes, but I am not able anymore now to place one video behind the other. (I tried z-index: -2 (background video) and z-index: -1 (foreground video))

Edit: The problem is probably my overall CSS, which might be ovarlaying the videos (I'm using Ionic). We should add that one to the documentation.

@yocontra
Copy link

I think it should handle non-negative z-indexes, having it spew errors when the z-index is positive doesn't make any sense. This new behavior should land as non-breaking and should augment, not replace, the old behavior.

@1N50MN14
Copy link
Author

@gbhrdt Stacking of the videos on top of each other should work fine, make sure you set the background color of your HTML body to rgba(0,0,0,0) as those views are placed behind the HTML UI View. This too will be added to the documentation should the PR pass.

Edit: Yes you're right I haven't updated the documentation along with the PR, will do so once it passes.

@ibc
Copy link
Collaborator

ibc commented Dec 11, 2015

Hi all, sorry for the delay (holidays...).

So this PR goods nice but by reading comments above I would like to ask: are there any issues with the negative z-index?

@1N50MN14
Copy link
Author

@ibc There shouldn't. The PR does however expect the z-index style property to be present (as specified here self.elementView.tag = Int(zIndex)) so it knows how to arrange the videos. We could introduce a fallback in case zIndex is not present but then people won't understand why videos are not being stacked on top of each other so I personally think the plugin should fail in this case, which it does as it did for @gbhrdt.

@ibc
Copy link
Collaborator

ibc commented Dec 11, 2015

@1N50MN14 let me understand that. Currently (I mean, before this PR) nothing fails if the video has no CSS z-index. What does it mean that it now would "fail"?

@1N50MN14
Copy link
Author

@ibc That's correct, currently if no z-index is present all works fine. After this PR, if you don't specify z-index you get the log which @gbhrdt posted (scroll up to see his comment).

@1N50MN14
Copy link
Author

I mean it's really straight forward (https://github.com/1N50MN14/cordova-plugin-iosrtc/commit/386d0479f845a3b766f1649c08b3478a77bbedb0?diff=unified#diff-d70c63d9377ee3067943b8ec4cf9fda5R183) - there's no way for the plugin to know how to order the video UI views unless you tell it to via z-index, which would be a matter of documentation.

@saghul
Copy link
Collaborator

saghul commented Dec 11, 2015

@1N50MN14 that's unfortunate. Wouldn't a default value of 0 avoid that?

@1N50MN14
Copy link
Author

@saghul It should I was just thinking about that, in theory Int() of undefined should return 0? I'm going to check this maybe @gbhrdt's errors are due to something else. One minute I'll check this.

@1N50MN14
Copy link
Author

@saghul @ibc Yeah it does expect z-index to be present.. I'm not sure how it should behave otherwise though..

@ibc
Copy link
Collaborator

ibc commented Dec 11, 2015

With the current behavior (pre PR) videos do not need a z-index. If there are two videos then the last one is placed on top of the previous one (unless they have a z-index).

We cannot mandate the user to place a z-index. If not set, the library should default to z-index 0 and things should work as before. Is there any problem to behave like that?

@1N50MN14
Copy link
Author

@ibc Yes that would be problematic because a z-index of 0 (or positive z-index) referrs to a video element that we don't want to touch, whereas a negative z-index refers to a video element which we want to create a video UI view of, z-index will be used for sorting its order..

@1N50MN14
Copy link
Author

The app could still contain a "standard" video element that has nothing to do with the plugin, and we don't want to touch those. So the way to tell the plugin which videos we want to touch is by assigning a negative z-index on them and if none are present then naturally we'd have a scream.

@1N50MN14
Copy link
Author

(I realize that's not W3C compatible but I can't think of any other solution, any suggestions are welcome)

@ibc
Copy link
Collaborator

ibc commented Dec 11, 2015

Yes that would be problematic because a z-index of 0 (or positive z-index) refers to a video element that we don't want to touch, whereas a negative z-index refers to a video element which we want to create a video UI view of, z-index will be used for sorting its order..

Now I'm totally lost. What does it mean that we don't want to touch a video element with 0 or positive z-index?

The app could still contain a standard video element that has nothing to do with the plugin...

Of course. How is that a problem within this issue/PR?

I would need a detailed documentation about the PR behavior, so I will first explain how things work before this PR:

Current behavior

  • If a video element handles a MediaStream then a native view is created for it.
  • Such a view is placed on top of the HTML view.
  • If a video A is placed after another video B and both share the same position or part of it, then B becomes visible on top of A (this is just because its view was created later).
  • The previous rule changes if z-index is present in video A or B (greater value means more "visibility").

Now I would ask for a similar "documentation" regarding the PR exposed in here :)

@1N50MN14
Copy link
Author

New behaviour

  • If no z-index are given to a video element (handling a MediaStream OR NOT) then z-index is considered to be zero.
  • The plugin goes through all video elements having a negative z-index and creates matching native views. They are sorted by z-index.
  • The HTML view sits on top of all native video views (it also contains any videos elements having a z-index of zero or greater).

Edit: Finally, the HTML View now has transparent background color by default, so you could see the native video views beneath it.

@@ -99,6 +100,13 @@ module.exports = function (context) {
fs.appendFileSync(xcconfigPath, swiftOptions.join('\n'));
debug('file correctly fixed: ' + xcconfigPath);

var result = fs.readFileSync(MainViewControllerPath, 'utf8')
var rep ='theWebView.backgroundColor = [UIColor blackColor];';
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is that unused var rep for?

Copy link
Author

Choose a reason for hiding this comment

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

That line should be stripped out, sorry about that.

@ibc
Copy link
Collaborator

ibc commented Dec 11, 2015

Is the PR tested when a MediaStream is closed (so PluginMediaStreamRenderer.close() is called)?

I mean this method in PluginMediaStreamRenderer.swift:

func close() {
    NSLog("PluginMediaStreamRenderer#close()")

    self.reset()
    self.elementView.removeFromSuperview()
}

@1N50MN14
Copy link
Author

@ibc I don't believe so

@ibc
Copy link
Collaborator

ibc commented Dec 11, 2015

I'd like to clarify that I am not a iOS expert, nor a UIView stuff expert. So I cannot properly manage future issues or limitations due to changes in the video layers stuff. So please, ensure the code runs perfect in all the expected usecases :)

@1N50MN14
Copy link
Author

Ok that works for me, I mean I've been using the changes for 2 weeks without any issues. Can you guys also try to give this PR a shot to make sure all works as expected?

@gbhrdt
Copy link

gbhrdt commented Dec 13, 2015

I tested it over the weekend and everything seems working fine when using the negative z-indexes. Also, if no or a positive z-index is given, it works like before. I don't really know where these errors came from. Maybe I forgot to re-add the ios platform to my project when migrating to the new PR in order to re-run the hook, at first.
A small problem I see here: I used to have a black background on the video elements before, which made it look a bit nicer, if the video is not loaded yet, now the videos are 'overlayed' by their backgrounds. Would it be possible to apply the background-color to the UIView somehow?

@1N50MN14
Copy link
Author

@gbhrdt Yes I have a PR for that, but I can not do anything just yet until this PR is merged.

@ibc
Copy link
Collaborator

ibc commented Dec 15, 2015

@1N50MN14 thanks a lot for your work and your time. It happens that I do need to fix #107 before adding any other feature in the plugin, so I will come back here when that is done.

@1N50MN14
Copy link
Author

@ibc No worries, I see why #107 is a PITA, want me to look into this with my build?

@ibc
Copy link
Collaborator

ibc commented Dec 15, 2015

@1N50MN14 sure, thanks! (let's continue in #107, I will comment what I'm doing right now to figure out where the bug is).

@1N50MN14
Copy link
Author

@ibc Are you planning on merging this PR?

@@ -1,3 +1,7 @@
/node_modules/
/TODO
/NO_GIT/
/lib/libWebRTC-LATEST-Universal-Release.a
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand why libWebRTC-LATEST-Universal-Release.a is removed from the repository.

@ibc
Copy link
Collaborator

ibc commented Jan 28, 2016

@1N50MN14 some points:

  • May you please merge all your commits into a single one so ti will look nice once merged into the main project?
  • Please remove the changes to .gitignore?

Please do that and I'll merge it today :)

@azachar
Copy link

azachar commented Feb 10, 2016

Hello @1N50MN14,
I merged & compiled your changes into the master - https://github.com/azachar/cordova-plugin-iosrtc (that I just forked) and it still doesn't work.

While debugging I discovered that z-index is not read correctly from css on a video element.

Could you please provide with some examples? The following one, doesn't work:

#videos {
  video::-webkit-media-controls { display:none !important; }
  #remote-video {
    video {
      opacity: 0.5;
      object-fit: cover;
      z-index: -3;
    }
  }
  #mini-video {
    z-index: -2;
  }
}
#videos     
     video#mini-video(autoplay muted)
     #remote-video
          video // i cannot access this element directly since it is injected by a 3rd party library

Thank you for your great job!

@jsampedro77
Copy link

Hello @1N50MN14, I am also trying to test this feature, but no success. The video always remains in the top. Could you provide a sample to code to test it?

thank you!

@1N50MN14
Copy link
Author

1N50MN14 commented Mar 8, 2016

Hi guys, this PR was submitted four months ago since then the code base of the plugin has changed, if you see earlier comments it used to work. In the meantime I stopped using this repo in my project, I've been relying on my own private fork which I have been maintaining separately for the past three months with a code base that is very divergent by now. I can try to fork this repo again and revisit this this issue next week when I have some free time and a submit a new PR.

@jsampedro77
Copy link

@1N50MN14 would there be any chance of having access to that private fork? :)

@1N50MN14
Copy link
Author

1N50MN14 commented Mar 8, 2016

@jsampedro77 Not at the moment simply because there are constantly breaking changes at this point (updated the APIs, cordova-ios4 support, Android support which have been adding some complexity etc) and I won't be able to help you, but I will open source it as an independent project down the line possibly in 12 weeks from now.

@ibc
Copy link
Collaborator

ibc commented Mar 8, 2016

I can spend some time on this if somebody really guarantees that the PR works as expected and provides HTML example codes and/or proper documentation of the z-index based feature.

I just cannot merge it as it is because I don't have such requested information and I've read (here) some users commenting that it does not work as expected. Unfortunately I don't have time to test it and I cannot merge something that I cannot even properly document in the plugin API and could break some running apps.

@mbanting
Copy link

@1N50MN14 I too am looking forward to trying your full screen solution when you make it public. For now I've resized the vids to 90% height.

@saghul
Copy link
Collaborator

saghul commented Jun 10, 2016

Ohai!

I finally found some time to test the patch out. It does not work in its current form. But it does with some tweaks.

Result: https://www.dropbox.com/s/wcnbrqy0yg18d6a/2016-06-10%2015.54.41.png?dl=0

I tested it with both UIWebView and WKWebView and the result is the same, so that's great. The adapted patch is here: https://github.com/saghul/cordova-plugin-iosrtc/tree/stacked-uiview

The result you can see in the screenshot works as follows:

  • the large video view has z-index of -2
  • the small video view has a z-index of -1
  • the HTML body has a background-color "transparent"

If all those are met then the trick works. The current state of the patch doesn't seem to properly handle positive z-indexes, I'll try to get that sorted out and send a PR (if @1N50MN14 is OK with that).

@1N50MN14
Copy link
Author

1N50MN14 commented Jun 10, 2016

@saghul The PR intentionally does not handle positive z-indexes and it shouldn't because you might have video elements in the DOM that aren't necessarily meant to be used in the context of this plugin (aka playing a local video file etc). The negative z-index lets the plugin know which video sources should be picked to dynamically create the views. The PR as is works on both UIWebView and WKWebview.

video::-webkit-media-controls { display:none !important; } should be added to the CSS manually, I didn't want the plugin to deal with this. Otherwise it's been working for me as is for at least 6 months. Anyway, feel free to publish a separate PR I don't mind, @ibc makes the final call ;)

@saghul
Copy link
Collaborator

saghul commented Jun 10, 2016

The PR intentionally does not handle positive z-indexes and it shouldn't because you might have video elements in the DOM that aren't necessarily meant to be used in the context of this plugin (aka playing a local video file etc).

I don't necessarily disagree, but int its current form it's very weird that if no z-index is set then the plugin's videos are not displayed. (principle of least astonishment and all that).

The PR as is works on both UIWebView and WKWebview.

I know. I just pointed it out because someone might have thought changes are required after migrating to cordova-ios 4.

should be added to the CSS manually, I didn't want the plugin to deal with this.

Agreed.

Anyway, feel free to publish a separate PR I don't mind

Thanks, appreciate it!

@1N50MN14
Copy link
Author

@saghul Yup you make a good point, awesome, well done!

@1N50MN14
Copy link
Author

@saghul I have a patch for #116 as well (it's just few lines of code really) would you like to test it and submit it as a part of your PR?

@saghul
Copy link
Collaborator

saghul commented Jun 10, 2016

Ok, got it working! Please see: #179

I took a different approach for ordering the views from the original patch by @1N50MN14 which makes it (IMHO) simpler: add the video elements to the webView's superview, and use the layer's zPosition to take care of the ordering. See the (detailed) commit message in the pull request for a more thorough explanation.

@saghul
Copy link
Collaborator

saghul commented Jun 10, 2016

I have a patch for #116 as well (it's just few lines of code really) would you like to test it and submit it as a part of your PR?

@1N50MN14 I think that's not related to this, better submit a separate PR. Thanks!

@1N50MN14
Copy link
Author

Closing this PR in favor of @saghul's

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

Successfully merging this pull request may close these issues.

8 participants