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 SurfaceTexture bindings #267

Merged
merged 37 commits into from
Jun 9, 2022
Merged

Add SurfaceTexture bindings #267

merged 37 commits into from
Jun 9, 2022

Conversation

lattice0
Copy link
Contributor

@lattice0 lattice0 commented Apr 23, 2022

Support for ASurfaceTexture from NDK

Closes #262

Copy link
Contributor

@dvc94ch dvc94ch left a comment

Choose a reason for hiding this comment

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

looks good, I assume @MarijnS95 will have some comments too

ndk/src/surface_texture.rs Outdated Show resolved Hide resolved
ndk/src/surface_texture.rs Outdated Show resolved Hide resolved
ndk/src/surface_texture.rs Show resolved Hide resolved
ndk/src/surface_texture.rs Show resolved Hide resolved
@lattice0
Copy link
Contributor Author

I'm not sure about

unsafe impl Send for SurfaceTexture {}
unsafe impl Sync for SurfaceTexture {}

though, have to think about it

@dvc94ch
Copy link
Contributor

dvc94ch commented Apr 23, 2022

good point, it shouldn't be Sync as it will get released mutliple times on drop. Send is fine.

@lattice0
Copy link
Contributor Author

Shouldn't clippy complain about unsafe impl without justifications?

Anyways, what about Send? As far as I know, the SurfaceTexture is created in the main thread by flutter's method channel. On java, stuff from Activity and etc, usually require you to call things from the main thread on it. I know almost nothing about SurfaceTexture but I remember having a hard time before when trying to modify Activity related objects from other threads.

@JasperDeSutter
Copy link
Contributor

This is missing api level guards, which I think should be 28 for all methods. Perhaps also a general "surface-texture" feature flag can be added which enables this module?

Copy link
Member

@MarijnS95 MarijnS95 left a comment

Choose a reason for hiding this comment

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

looks good, I assume @MarijnS95 will have some comments too

Looks like yall covered most of it, and I do agree with all the comments that are still outstanding.

ndk/src/surface_texture.rs Outdated Show resolved Hide resolved
ndk/src/surface_texture.rs Outdated Show resolved Hide resolved
ndk/src/surface_texture.rs Outdated Show resolved Hide resolved
@MarijnS95
Copy link
Member

MarijnS95 commented May 12, 2022

good point, it shouldn't be Sync as it will get released mutliple times on drop. Send is fine.

Sync implies that you can have multiple references to an object: references are not ownership, hence cannot drop the object multiple times.

This only becomes a problem if we allow safely cloning/copying the SurfaceTexture (implying multiple ownership) which is not possible for this since there are no reference counting functions in the NDK (like we have in NativeWindow).
(Note: you can unsafely alias it by calling from_ptr(other.ptr()))

As such, we should implement:

  • Send, if it's safe to hand over this structure from one thread to another: I don't know if this is true for GL contexts as those are afaik also bound to a thread if I remember correctly. That may or may not impose a limitation here;
  • Sync - presuming we can implement Send - if the NDK allows calling these function from multiple threads, concurrently.

It does not seem like the NDK docs clarify any of this, so should perhaps look at Surface and if any of this is allowed on the Java side? How would that work?

@MarijnS95 MarijnS95 changed the title adds SurfaceTexture support Add SurfaceTexture support May 12, 2022
@lattice0
Copy link
Contributor Author

Is it ok to merge now?

PS: what is happening at the format task?

Copy link
Member

@MarijnS95 MarijnS95 left a comment

Choose a reason for hiding this comment

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

Is it ok to merge now?

There are some unresolved comments still open, and quite a few marked resolved which haven't actually been addressed - I've unmarked those as unresolved.

PS: what is happening at the format task?

You can click on it to see what it wants. Basically make sure you run cargo fmt before pushing your contribution upstream.


After that, all that remains is checking what's up with the Send/Sync.

I presume Send is safe to have here unless there's some GL context magic going on that disallows a bound texture to be used on a different thread before it is unbound.

Sync requires at least the above constraints too.

ndk/src/surface_texture.rs Outdated Show resolved Hide resolved
ndk/src/surface_texture.rs Outdated Show resolved Hide resolved
ndk/src/surface_texture.rs Outdated Show resolved Hide resolved
@lattice0
Copy link
Contributor Author

is it ok now?

@MarijnS95 MarijnS95 requested a review from dvc94ch May 28, 2022 08:40
Copy link
Member

@MarijnS95 MarijnS95 left a comment

Choose a reason for hiding this comment

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

I think we're good here. We can add back Sync once we're sure the NDK/Java internally synchronize these threads, but the documentation on SurfaceTexture mentions that certain operations are only valid when called on the thread that has an OpenGL context bound (which was previously used elsewhere), to me implying that it is safe to Send a SurfaceTexture across threads.

It seems like you've only copied the documentation summary of every function. Please make sure the entire paragraph is available or otherwise link to the appropriate function in the NDK docs: https://developer.android.com/ndk/reference/group/surface-texture

ndk/src/surface_texture.rs Show resolved Hide resolved
Copy link
Member

@MarijnS95 MarijnS95 left a comment

Choose a reason for hiding this comment

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

That shouldn't have been an approval in light of missing documentation.

@lattice0
Copy link
Contributor Author

What about now? :)

Copy link
Member

@MarijnS95 MarijnS95 left a comment

Choose a reason for hiding this comment

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

What about now? :)

No need to keep pinging/asking, I get notifications for your pushes and handle them as soon as appropriate. Especially when you ask, make sure that your PR is in tip-top shape first... Which it really isn't :(

ndk/src/surface_texture.rs Outdated Show resolved Hide resolved
ndk/src/surface_texture.rs Outdated Show resolved Hide resolved
ndk/src/surface_texture.rs Outdated Show resolved Hide resolved
ndk/src/surface_texture.rs Outdated Show resolved Hide resolved
ndk/src/surface_texture.rs Outdated Show resolved Hide resolved
ndk/src/surface_texture.rs Outdated Show resolved Hide resolved
ndk/src/surface_texture.rs Outdated Show resolved Hide resolved
@MarijnS95
Copy link
Member

@lattice0 Mind if I take over?

@lattice0
Copy link
Contributor Author

lattice0 commented Jun 2, 2022

Ok, do you want to finish the PR? I can apply the changes otherwise, haven't seen your message

@MarijnS95 MarijnS95 changed the title Add SurfaceTexture support Add SurfaceTexture bindings Jun 8, 2022
@MarijnS95
Copy link
Member

MarijnS95 commented Jun 9, 2022

Ok, do you want to finish the PR? I can apply the changes otherwise, haven't seen your message

Not sure what was going on with your last push, but master was merged in and subsequently reverted (resulting in this PR undoing recent merges, if I were to click the merge button), and lots of links were corrupted.

I've reverted those changes and redone the docs on this PR. Still looking for a balance to make doc links more consistent across the ndk crate (when to link to the wrapper type, when to link to the ffi type, and when to link to the Android docs) but I think I struck the right path now. Will be updated for other types separately.

@MarijnS95 MarijnS95 merged commit 8e1148f into rust-mobile:master Jun 9, 2022
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.

SurfaceTexture NDK bindings
4 participants