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

Lifecycle binding #19

Closed
BulatMukhutdinov opened this issue Jun 6, 2019 · 6 comments
Closed

Lifecycle binding #19

BulatMukhutdinov opened this issue Jun 6, 2019 · 6 comments

Comments

@BulatMukhutdinov
Copy link

BulatMukhutdinov commented Jun 6, 2019

in CameraFragment you call

CameraX.bindToLifecycle(this, preview, imageCapture, imageAnalyzer)

meaning you bind it to fragment's lifecycle. But you call this method on onViewCreated() which means you would receive the old lifecycleOwner. To avoid crashing you every time also call CameraX.unbindAll() which is some sort of work around.
Instead it would be better to call CameraX.bindToLifecycle() with viewLifecycleOwner. Then you can delete CameraX.unbindAll()

Thus, it seems like you have misused fragment's lifecycle with view's lifecycle. Correct me I am wrong.
Thanks

@owahltinez
Copy link
Contributor

I'm not sure what you mean by "the old lifecycleOwner". The current sample is using the fragment's lifecycle, which usually is the same as the view's lifecycle except CameraFragment sets the retainInstance flag.

By attaching CameraX to the fragment lifecycle, we avoid CameraX being fully torn down (and thus closing the camera) upon config changes like rotation. The intent is to allow for CameraX to maintain some of its state by doing: CameraX.unbindAll()CameraX.bindToLifecycle(), which happens during onViewCreated.

By using the view's lifecycle, the order of events is: CameraX *teardown*CameraX *initialize*CameraX.bindToLifecycle(). However, I just tested this and it seems that this results in snappier transitions during device rotations on a Pixel 3 device. I'm guessing it's due to the teardown starting as the view gets destroyed, rather than doing everything during onViewCreated.

I will investigate the potential implications of switching to the view's lifecycle. It's likely that devices with slow camera open/close will have degraded performance using this path. We have to take into consideration other things such as navigation, multi-window environments, etc.

@BulatMukhutdinov
Copy link
Author

BulatMukhutdinov commented Jun 7, 2019

Could you please explicitly specify where in the sample CameraX *initialize* happens?

@owahltinez
Copy link
Contributor

CameraX initialization is not triggered by app code. Some of it happens upon app startup, some of it when the first use case is bound. It's up to the CameraX library and subject to change in the future.

@BulatMukhutdinov
Copy link
Author

Then, returning to original question, it seems that if you would bind to fragment's lifecycle the call stack is
CameraX *initialize*CameraX.unbindAll()CameraX.bindToLifecycle() → (rotate) → CameraX.unbindAll()CameraX.bindToLifecycle()
While with viewLifecycleOwner it is
CameraX *initialize*CameraX.bindToLifecycle() → (rotate) → CameraX.bindToLifecycle()

The main concern is when you doing CameraX.unbindAll() the concept of lifecycle awareness is lost since you bind and unbind it each time manually.

@owahltinez
Copy link
Contributor

While with viewLifecycleOwner it is
CameraX initialize → CameraX.bindToLifecycle() → (rotate) → CameraX.bindToLifecycle()

I don't think this is entirely true. I'm counting camera open as CameraX *initialize* although maybe I'm not making things very clear. Essentially, by binding to the view's lifecycle, we will be closing and reopening the camera every time we rotate. Whereas binding to the fragment's lifecycle we can even keep some use cases bound across config changes (which we aren't doing yet).

The main concern is when you doing CameraX.unbindAll() the concept of lifecycle awareness is lost since you bind and unbind it each time manually.

That's fair feedback, although only partly true. We are just making sure that no other use cases are bound, which is not necessarily a bad practice but, as you say, it kinda defeats the point of making everything lifecycle-aware.

As I said earlier, we will investigate switching to using the view's lifecycle as the owner. Personally, assuming there are no major drawbacks, I think it would be the right choice. It simplifies the code and sets a good example for developers to follow. Otherwise, we are chasing diminishing returns for a slight performance gain, whereas some of the optimizations could be done by CameraX itself. For example, CameraX could wait a bit to close the camera during a config change, since it's likely that an app will bind the same use cases and open the same camera again.

@BulatMukhutdinov
Copy link
Author

Thank you for your response. I close the issue but pls ping me if there would be any updates on this

owahltinez added a commit that referenced this issue Jun 28, 2019
- Fixes #18
- Fixes #19
- Fixes #26
- Implement safe args
- Crop circles using Glide
- Delete unneeded ImageUtils

Change-Id: I928aef500771c0ba9122203a2295cd2f141e3e41
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

No branches or pull requests

2 participants