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

Add EIS control to video stream #11

Merged
merged 4 commits into from
Apr 9, 2025
Merged

Add EIS control to video stream #11

merged 4 commits into from
Apr 9, 2025

Conversation

lukin87
Copy link

@lukin87 lukin87 commented Mar 27, 2025

What

Adding in the EIS control flow to RNVC.

Changes

Tested on

Related issues

if (SystemProperties.get("persist.vendor.camera.enableEIS") == "0") {
Log.i(CameraSession.TAG, "not applying EIS change")
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

let's always mark end of stream (and return to prev value) so that maybe this solves the crash when video flash is on?

Copy link
Author

Choose a reason for hiding this comment

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

This actually introduces a fairly annoying edge case. If, due to a crash/edge case, the camera.enableEIS is left as "2" then a new CameraSession won't know which mode to restore to.

I think the neatest solution would be for Foxxconn to offer a new system prop persist.vendor.camera.markEndOfStream which we could toggle between 0 and 1. Then we never have to change the EIS value after boot (until we make it a user setting). Is that possible?

Copy link
Member

@hhff hhff Mar 28, 2025

Choose a reason for hiding this comment

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

@lukin87 the way to work around that is for us to introduce our own parent setting called Settings.Global.putInt(contentResolver, "eis_enabled", 0);

(When we make a user setting, this is the one we toggle, not camera.enableEIS)

User toggles that one, RNVC references it but never changes it, and internally handles camera.enableEIS based on that parent setting.

Copy link
Author

Choose a reason for hiding this comment

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

Ok great! I haven't done global settings yet but I assumed it'd be easy enough. I'll have a play with it now

Copy link
Member

@hhff hhff left a comment

Choose a reason for hiding this comment

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

One nitpick for safety


fun CameraSession.setEISMode(newMode: String) {
Log.i(CameraSession.TAG, "EIS: changed from: " + SystemProperties.get("persist.vendor.camera.enableEIS"));
SystemProperties.set("persist.vendor.camera.enableEIS", newMode);
Copy link
Member

@hhff hhff Apr 7, 2025

Choose a reason for hiding this comment

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

It's possible that the user updates to the LightOS version that runs this code, but they have old firmware (from deferring it, or bugs in the FOTA updater).

So, I believe this whole method needs to be wrapped in a try/catch in the case it's running on an earlier version of the firmware (that doesn't have the SELinux rules allowing us to access this system property)

Copy link
Author

Choose a reason for hiding this comment

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

Ahhh nice one - good catch.

I've put in a check now. I don't think there's any issue with just failing silently since toggling that bit would have no effect anyways but let me know if I've thought that through wrong

@lukin87 lukin87 requested a review from hhff April 9, 2025 11:54
@hhff hhff merged commit 3bbf813 into lightos Apr 9, 2025
@hhff hhff deleted the lightos-eis branch April 9, 2025 15:08
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