rust-mobile / ndk

Rust bindings to the Android NDK
Apache License 2.0
1.14k stars 112 forks source link

ndk/media_codec: Return `MaybeUninit` bytes in `buffer_mut()` #403

Closed MarijnS95 closed 1 year ago

MarijnS95 commented 1 year ago

Depends on #401, CC @zarik5

The byte-array mapped/returned by AMediaCodec_getInputBuffer() is not known to be initialized as the caller requires this API to subsequently initialize the data, and mapping/casting uninitialized memory segments as safe data in Rust is Undefined Behaviour. Instead, return a MaybeUninit<u8> type to be safe (as in: the user cannot safely read data in the returned slice, only write to it) and document the intent back to the user.

Unfortunately, as of writing many useful API around slices of MaybeUninit are still unstable.

rib commented 1 year ago

Just an aside comment, but for this kind of PR where you're stacking changes on other PRs it can be nice to set the base branch to the preceding PR branch.

That's only possible when you can push your PR branch to the upstream repo but it looks like you've done that here. That way, when reviewing the changes it's easier to focus on the ones that really belong to this PR.

In github if you then merge from the bottom the the stack it will actually automatically update the base branch to main for each other PR.

MarijnS95 commented 1 year ago

In github if you then merge from the bottom the the stack it will actually automatically update the base branch to main for each other PR.

Did they fix that? In the past it would close the PR because the target branch was gone after merging, with no way to recover.

rib commented 1 year ago

In github if you then merge from the bottom the the stack it will actually automatically update the base branch to main for each other PR.

Did they fix that? In the past it would close the PR because the target branch was gone after merging, with no way to recover.

I've been doing this recently and haven't had that happen to me so I guess it's fixed yeah

MarijnS95 commented 1 year ago

Note that the diff is now useless since I force-pushed that PR, and will probably only bother to rebase this once #401 is approved and squash-merged in.

rib commented 1 year ago

yeah, I guessed what just happend :)

rib commented 1 year ago

another aside but I can highly recommend update-refs in newer versions of git: https://andrewlock.net/working-with-stacked-branches-in-git-is-easier-with-update-refs/

rib commented 1 year ago

another aside but I can highly recommend update-refs in newer versions of git: https://andrewlock.net/working-with-stacked-branches-in-git-is-easier-with-update-refs/

makes this kind of stacking a breeze

MarijnS95 commented 1 year ago

Neat, I should've looked that up earlier! It's just a 2-deep stack now though, but two different PRs (both depend on #401).

MarijnS95 commented 1 year ago

The combination of pull.rebase = true and rebase.updateRefs = true in gitconfig is pretty powerful :grimacing:

rib commented 1 year ago

The combination of pull.rebase = true and rebase.updateRefs = true in gitconfig is pretty powerful 😬

yeah I'm a full convert.

I tend to pile several branches for different PRs in a rib/stack/foo branch and will just always do my rebasing from that and can then just periodically re-push the updated PR branches.

there are a few cases where you can get caught out I've found but nothing major.

e.g. sometimes if I'm about to make some big changes to a branch but I'm not sure if I might need to come back to the current version I'll create a second branch name for my current head. If I'm not thinking though I end up just doing an rebase that just makes the new branch name follow the new work - defeating the point of that. Easy enough to repair after you diverge but something that can happen.

rib commented 1 year ago

well I suppose I don't use pull.rebase - I still like to fetch as a separate step

MarijnS95 commented 1 year ago

well I suppose I don't use pull.rebase - I still like to fetch as a separate step

I end up combing both; git pull origin main with an automatic rebase is a nice way to stay up to date for me. And of course fetch -p to get rid of remote tracking branches :)

rib commented 1 year ago

ah, I don't use -p to prune but that's maybe handy when github auto deletes merged PR branches. dunno

MarijnS95 commented 1 year ago

If you work on a project that sees tens to hundreds of PRs and branches a week, it's pretty much a necessity :)

rib commented 1 year ago

yeah I guess I'd want to differentiate pruning ones related to reviewing other peoples work vs branches that I originally created and would want to keep even if the remote eventually gets deleted via a merged PR

MarijnS95 commented 1 year ago

For the few of those I intentionally --unset-upstream them so that that alias keeps working... The volume of branches I otherwise create across repos is too high to manually sort out and delete every time :)