Skip to content

Various performance improvements#2046

Open
Mike-the-one wants to merge 1 commit intopedroSG94:masterfrom
Mike-the-one:master
Open

Various performance improvements#2046
Mike-the-one wants to merge 1 commit intopedroSG94:masterfrom
Mike-the-one:master

Conversation

@Mike-the-one
Copy link

These are the performance improvements I mentioned in the following issues

#2044
#2043
#2040

For AndroidViewFilterRender I also add a render flag, so if this flag is false then this view will not be rendered at all, so if we have more than one view filters we can turn it on or off easily.

Thanks!

@pedroSG94
Copy link
Owner

Hello,

Thank you for the PR!

I was doing tests and all seems good except the AndroidViewFilterRender and that you need move ObservableWebView to common module because you can't use it from encoder module.
AndroidViewFilterRender stopped to work with the default code example. I'm not sure if the way to use it changed but with the old example in the rotation activty using the menu, you should see 4 buttons but nothing is renderer:
https://github.com/pedroSG94/RootEncoder/blob/master/app/src/main/java/com/pedro/streamer/utils/FilterMenu.kt#L101

Also, I'm not sure if the color change in the VideoEncoder is secure for old devices (encoders and players that will reproduce the video) and should be disabled by default, but Anyway, I think that the code should check that if the version is older than Android N and the patch never should be done because you can't change it in the MediaFormat anyway:
https://github.com/pedroSG94/RootEncoder/pull/2046/changes#diff-69c1199aafc51d208f38b0aca826b15f07117951962299d31c6b456b8824f8bcR175

What do you think about it?

@Mike-the-one
Copy link
Author

Yes, I completely agree to have the color profile set to false by default, I think the best is to have a flag and turn it on by the user?

Yes, if the ObservableWebView should be in a different module then sure.

I am wondering if you can do it?

thanks

@pedroSG94
Copy link
Owner

Hello,

Ok, I will do it.

@Mike-the-one
Copy link
Author

Thanks! Once this is in I will try it out in my app, and let you know.

Thanks again!!!

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.

2 participants