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

ndk/native_window: Add lock() to blit raw pixel data #404

Merged
merged 1 commit into from
Jun 23, 2023

Conversation

MarijnS95
Copy link
Member

@rib
Copy link
Contributor

rib commented Jun 16, 2023

Hehe, did you maybe also see me using ANativeWindow_lock in my recent rust-android-examples changes per chance?

https://github.com/rust-mobile/rust-android-examples/pull/4/files#diff-80528ea769093da879768e086770c9b11f81b2cd8aa51b8218f5183772eb8b18R93

Yeah, in particular it was too much of a faff in that context to also deal with getting the bytes per-pixel which you really need to do anything with the buffers from the CPU since the API is pretty ghastly and doesn't tell you the size of the buffer in bytes - you have to work it out based on the pixel format.

Blob doesn't need to be todo!() it is a byte format with the extra constraint that the buffer will have a height of 1 so the width can be read as a size in bytes.

@MarijnS95
Copy link
Member Author

No I did not, this is actually a commit from almost 1 year ago (d104d01) that I just kept postponing to contribute back, because I didn't know what to do with the pack_rgba() (see that commit) helper and ended up scrapping that.

@MarijnS95
Copy link
Member Author

Regarding BLOB, just 1 byte per pixel, that's cool.

Also bpp is generally in bits, shall I change this to return bits instead of bytes?

@rib
Copy link
Contributor

rib commented Jun 16, 2023

bpp is just ambiguous - definitely wouldn't assume either way without looking for context.

bytes seems fine here.

There are some missing format that could be good to fill in for completeness here.

e.g. AHARDWAREBUFFER_FORMAT_R10G10B10A10_UNORM = VK_FORMAT_R10X6G10X6B10X6A10X6_UNORM_4PACK16

which is 8 bytes per pixel:

specifies a four-component, 64-bit unsigned normalized format that has a 10-bit R component in the top 10 bits of the word in bytes 0..1, a 10-bit G component in the top 10 bits of the word in bytes 2..3, a 10-bit B component in the top 10 bits of the word in bytes 4..5, and a 10-bit A component in the top 10 bits of the word in bytes 6..7, with the bottom 6 bits of each word unused.
(from https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VkFormat.html)

@rib
Copy link
Contributor

rib commented Jun 16, 2023

No I did not, this is actually a commit from almost 1 year ago (d104d01) that I just kept postponing to contribute back, because I didn't know what to do with the pack_rgba() (see that commit) helper and ended up scrapping that.

nice coincidence then

I wanted to be able to clear the framebuffer at least when doing that but also didn't want to be inlining a bunch of byte-per-pixel format mappings just for that.

@MarijnS95
Copy link
Member Author

AHARDWAREBUFFER_FORMAT_R10G10B10A10_UNORM doesn't seem to be in NDK 25.2, but there are a few others that we should definitely add.

@rib
Copy link
Contributor

rib commented Jun 16, 2023

AHARDWAREBUFFER_FORMAT_R10G10B10A10_UNORM doesn't seem to be in NDK 25.2, but there are a few others that we should definitely add.

oh, curious.

I was looking here https://developer.android.com/ndk/reference/group/a-hardware-buffer#ahardwarebuffer_format

@MarijnS95
Copy link
Member Author

Docs are typically ahead, guess they come with NDK r26. There's no Android version compatibility listed in the docs, though, so I assume 26 because that's when the non-legacy formats were added.

@rib
Copy link
Contributor

rib commented Jun 16, 2023

because I didn't know what to do with the pack_rgba() (see that commit) helper and ended up scrapping that.

right that's probably the appropriate thing here. that might be a handy utility for testing but probably doesn't belong as part of the ndk crate.

ndk/src/native_window.rs Outdated Show resolved Hide resolved
ndk/src/native_window.rs Outdated Show resolved Hide resolved
Copy link

@notgull notgull left a comment

Choose a reason for hiding this comment

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

I'm not familiar enough with Android to comment, but overall this looks good to me.

/// returns it is updated (commonly enlarged) with the actual area the caller needs to redraw.
pub fn lock(
&self,
dirty_bounds: Option<&mut ffi::ARect>,
Copy link

Choose a reason for hiding this comment

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

I would avoid having a mutable reference like this if all possible. I would pass in a rectangle and then return a rectangle as a tuple with the lock.

Copy link
Member Author

Choose a reason for hiding this comment

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

I instead considered putting it in the returned guard.

Copy link
Member Author

@MarijnS95 MarijnS95 Jun 21, 2023

Choose a reason for hiding this comment

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

Done that now, but it might make it less clear that when the user passes this to the function, they have to read back the updated value from NativeWindowBufferLockGuard. But that would be the same (except for still having &mut) when they already define their local variable mut for other reasons.

}

/// The actual bits.
pub fn bits(&mut self) -> *mut std::ffi::c_void {
Copy link

Choose a reason for hiding this comment

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

Maybe it would be possible to have a function that returns a mutable slice instead of a pointer?

Copy link
Member Author

@MarijnS95 MarijnS95 Jun 16, 2023

Choose a reason for hiding this comment

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

I had considered that, and especially in the context of softbuffer it is UB to cast a pointer to a safe slice, since these APIs are mostly for writing to memory.

It'll have to be &mut [MaybeUninit<u8>].

Copy link
Member Author

@MarijnS95 MarijnS95 Jun 21, 2023

Choose a reason for hiding this comment

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

@notgull done this now. Perhaps it is also interesting to add API's that give access to individual scanlines, by offsetting the pointer by stride() * num_rows * bpp and giving the user only a slice of length width() * bpp? And then wrap that in an iterator again so that it becomes very natural to update pixels?

That won't help for softbuffer though which currently assumes contiguous memory.

@MarijnS95 MarijnS95 force-pushed the window-lock branch 3 times, most recently from da75940 to c8ba8e6 Compare June 21, 2023 13:12
@MarijnS95 MarijnS95 requested review from notgull and rib June 22, 2023 21:39
Copy link

@notgull notgull 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 to me!

Copy link
Contributor

@rib rib left a comment

Choose a reason for hiding this comment

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

I'm not entirely convinced that having the separate API for getting the modified dirty bounds from the lock ends up being better than the &mut argument.

My nagging concern would probably be that it's now easier to miss the fact that if you do pass a dirty bounds you need to additionally remember to query back the modified bounds - whereas with the &mut argument, even if a developer hadn't fully appreciated that the region might get expanded they will any way probably reference the correct region in their code.

Either way is workable though and this is a fairly low-level detail so this seems reasonable enough to me.

Apart from that, everything else looks good to me.

@MarijnS95
Copy link
Member Author

MarijnS95 commented Jun 23, 2023

Thanks @rib. I've reverted that suggestion from @notgull now as I indeed agree that when a user is forced to pass Some(&mut dirty_bounds) over Some(dirty_bounds) this hopefully nags/pushes them towards reading the documentation (or understanding from the get-go) that their bounds will be updated.

It is only a problem when someone requests explicit bounds anyway, most might just pass None and touch the whole window.

I've also added the per-line iterator API hinted at above and tested it out in:

https://github.com/rust-mobile/rust-android-examples/compare/na-pixels-lock

Rust's chunks_mut and subslicing does make it rather trivial to implement, without messing around with pointer offsets :)

@MarijnS95 MarijnS95 merged commit b520751 into rust-mobile:master Jun 23, 2023
@MarijnS95 MarijnS95 deleted the window-lock branch June 23, 2023 22:36
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.

3 participants