Skip to content

Commit

Permalink
ndk: Mark all enums as non_exhaustive and fix repr types
Browse files Browse the repository at this point in the history
We're currently in a painful situation, both when it comes to
correctness of the wrapper functions that take or return enum values,
as well as convenience and ability to upgrade. This is a summation of
issues, and reasons why external crates like `android-activity` are
copy-pasting and fixing up our implementation:
- False mapping of unknown values to NDK `_UNKNOWN` constants.  This
  makes the API lossy as the original unknown value does not have the
  **KNOWN** value that is assigned to the `_UNKNOWN` constant (typically
  `0`);
- Lacking `#[non_exhaustive]` in most places, not allowing us to add new
  **and missing** constants to `enum`;
- `#[repr(u32)]` that follows the default `u32` type used by `bindgen`
  when processing an "untyped" enum, rather than considering the type
  (`i32`, `i8`, etc) that is actually used by every function taking or
  returning this "integer constant";
- Inconsistent use of either:
  - Panicking on unknown values;
  - Wrapping unknown values in a custom `Err` type (i.e.
    `AudioError::UnknownValue` or `TryFromPrimitiveError`);
  - Assigning unknown values to an `Unknown = ffi::XXX_UNKNOWN` variant;
  - Sometimes returning them in a typed `Unknown(xxx)` variant.

To solve this we have to make the following changes to every `pub enum`
with custom discriminants in the NDK crate:

- Drop all builtin type conversions and update `repr(u32)` to use the
  common type used by _every_ function that takes or returns this enum
  value. Conversions always happen via `.into()` from `IntoPrimitive`
  (needed to automatically process `__Unknown`);
- Annotate all those `enum`s with `#[non_exhaustive]`.  This requires
  callers to have a `x => todo!("New constant {x:?}")` or similar
  handling in a `match` block.  It will neatly print the value for
  `__Unknown` added below;
- Replace `TryFromPrimitive` with `IntoPrimitive`;
- Have a catch-all `__Unknown` variant, that is `#[doc(hidden)]`.  This:
  - Saves us from having a `TryIntoPrimitiveError` or custom `Err` type;
  - Allows the caller to pass in custom/unknown values;
  - Entices contributions/additions in the crate by having the type
    `#[doc(hidden)]`, as a lot of enums are currently lagging behind
    their upstream representation.

- TODO: For error types like `AudioResult`/`AudioError`, implement
  `Error` on them immediately like on `MediaError`.  This is possible
  now that the custom `UnknownValue` variant is gone.

Note that for some (currently) input-only `enum`s IntoPrimitive
  • Loading branch information
MarijnS95 committed Dec 25, 2023
1 parent 485b34e commit 399dd50
Show file tree
Hide file tree
Showing 7 changed files with 899 additions and 825 deletions.
345 changes: 173 additions & 172 deletions ndk/src/audio.rs

Large diffs are not rendered by default.

94 changes: 48 additions & 46 deletions ndk/src/bitmap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,12 @@ pub enum BitmapError {
BadParameter = ffi::ANDROID_BITMAP_RESULT_BAD_PARAMETER,
#[doc(alias = "ANDROID_BITMAP_RESULT_JNI_EXCEPTION")]
JniException = ffi::ANDROID_BITMAP_RESULT_JNI_EXCEPTION,
// Use the OK discriminant, as no-one will be able to call `as i32` and only has access to the
// constants via `From` provided by `IntoPrimitive` which reads the contained value.

// Use the SUCCESS discriminant, as no-one will be able to call `as i32` and only has access to
// the constants via `From` provided by `IntoPrimitive` which reads the contained value.
#[doc(hidden)]
#[num_enum(catch_all)]
Unknown(i32) = ffi::AAUDIO_OK,
__Unknown(i32) = ffi::ANDROID_BITMAP_RESULT_SUCCESS,
}

impl fmt::Display for BitmapError {
Expand Down Expand Up @@ -57,26 +59,33 @@ fn construct<T>(with_ptr: impl FnOnce(*mut T) -> i32) -> Result<T> {
BitmapError::from_status(status).map(|()| unsafe { result.assume_init() })
}

#[repr(u32)]
#[derive(Clone, Copy, Debug, PartialEq, Eq, IntoPrimitive, TryFromPrimitive)]
#[repr(i32)]
#[derive(Clone, Copy, Debug, PartialEq, Eq, IntoPrimitive, FromPrimitive)]
#[allow(non_camel_case_types)]
#[doc(alias = "AndroidBitmapFormat")]
#[non_exhaustive]
pub enum BitmapFormat {
#[doc(alias = "ANDROID_BITMAP_FORMAT_NONE")]
NONE = ffi::AndroidBitmapFormat::ANDROID_BITMAP_FORMAT_NONE.0,
NONE = ffi::AndroidBitmapFormat::ANDROID_BITMAP_FORMAT_NONE.0 as i32,
#[doc(alias = "ANDROID_BITMAP_FORMAT_RGBA_8888")]
RGBA_8888 = ffi::AndroidBitmapFormat::ANDROID_BITMAP_FORMAT_RGBA_8888.0,
RGBA_8888 = ffi::AndroidBitmapFormat::ANDROID_BITMAP_FORMAT_RGBA_8888.0 as i32,
#[doc(alias = "ANDROID_BITMAP_FORMAT_RGB_565")]
RGB_565 = ffi::AndroidBitmapFormat::ANDROID_BITMAP_FORMAT_RGB_565.0,
RGB_565 = ffi::AndroidBitmapFormat::ANDROID_BITMAP_FORMAT_RGB_565.0 as i32,
#[deprecated = "Deprecated in API level 13. Because of the poor quality of this configuration, it is advised to use ARGB_8888 instead."]
#[doc(alias = "ANDROID_BITMAP_FORMAT_RGBA_4444")]
RGBA_4444 = ffi::AndroidBitmapFormat::ANDROID_BITMAP_FORMAT_RGBA_4444.0,
RGBA_4444 = ffi::AndroidBitmapFormat::ANDROID_BITMAP_FORMAT_RGBA_4444.0 as i32,
#[doc(alias = "ANDROID_BITMAP_FORMAT_A_8")]
A_8 = ffi::AndroidBitmapFormat::ANDROID_BITMAP_FORMAT_A_8.0,
A_8 = ffi::AndroidBitmapFormat::ANDROID_BITMAP_FORMAT_A_8.0 as i32,
#[doc(alias = "ANDROID_BITMAP_FORMAT_RGBA_F16")]
RGBA_F16 = ffi::AndroidBitmapFormat::ANDROID_BITMAP_FORMAT_RGBA_F16.0,
RGBA_F16 = ffi::AndroidBitmapFormat::ANDROID_BITMAP_FORMAT_RGBA_F16.0 as i32,
#[doc(alias = "ANDROID_BITMAP_FORMAT_RGBA_1010102")]
RGBA_1010102 = ffi::AndroidBitmapFormat::ANDROID_BITMAP_FORMAT_RGBA_1010102.0,
RGBA_1010102 = ffi::AndroidBitmapFormat::ANDROID_BITMAP_FORMAT_RGBA_1010102.0 as i32,

// Use the SUCCESS discriminant, as no-one will be able to call `as i32` and only has access to
// the constants via `From` provided by `IntoPrimitive` which reads the contained value.
#[doc(hidden)]
#[num_enum(catch_all)]
__Unknown(i32),
}

/// An immediate wrapper over [`android.graphics.Bitmap`]
Expand Down Expand Up @@ -279,7 +288,7 @@ impl Bitmap {
.try_into()
.expect("i32 overflow in DataSpace"),
pixels,
format as i32,
format.into(),
quality,
<*mut _>::cast(&mut cb_state),
Some(compress_cb::<F>),
Expand All @@ -295,19 +304,25 @@ impl Bitmap {
}

/// Possible values for [`ffi::ANDROID_BITMAP_FLAGS_ALPHA_MASK`] within [`BitmapInfoFlags`]
#[repr(u32)]
#[cfg(feature = "api-level-30")]
#[derive(Clone, Copy, Debug)]
#[derive(Clone, Copy, Debug, IntoPrimitive, FromPrimitive)]
#[doc(alias = "ANDROID_BITMAP_FLAGS_ALPHA_MASK")]
#[non_exhaustive]
pub enum BitmapInfoFlagsAlpha {
/// Pixel components are premultiplied by alpha.
#[doc(alias = "ANDROID_BITMAP_FLAGS_ALPHA_PREMUL")]
Premultiplied,
Premultiplied = ffi::ANDROID_BITMAP_FLAGS_ALPHA_PREMUL,
/// Pixels are opaque.
#[doc(alias = "ANDROID_BITMAP_FLAGS_ALPHA_OPAQUE")]
Opaque,
Opaque = ffi::ANDROID_BITMAP_FLAGS_ALPHA_OPAQUE,
/// Pixel components are independent of alpha.
#[doc(alias = "ANDROID_BITMAP_FLAGS_ALPHA_UNPREMUL")]
Unpremultiplied,
Unpremultiplied = ffi::ANDROID_BITMAP_FLAGS_ALPHA_UNPREMUL,

#[doc(hidden)]
#[num_enum(catch_all)]
__Unknown(u32),
}

/// Bitfield containing information about the bitmap.
Expand Down Expand Up @@ -335,13 +350,7 @@ impl BitmapInfoFlags {
#[doc(alias = "ANDROID_BITMAP_FLAGS_ALPHA_MASK")]
pub fn alpha(self) -> BitmapInfoFlagsAlpha {
// Note that ffi::ANDROID_BITMAP_FLAGS_ALPHA_SHIFT is 0 and hence irrelevant.
match self.0 & ffi::ANDROID_BITMAP_FLAGS_ALPHA_MASK {
ffi::ANDROID_BITMAP_FLAGS_ALPHA_PREMUL => BitmapInfoFlagsAlpha::Premultiplied,
ffi::ANDROID_BITMAP_FLAGS_ALPHA_OPAQUE => BitmapInfoFlagsAlpha::Opaque,
ffi::ANDROID_BITMAP_FLAGS_ALPHA_UNPREMUL => BitmapInfoFlagsAlpha::Unpremultiplied,
3 => todo!("ALPHA_MASK value 3"),
_ => unreachable!(),
}
(self.0 & ffi::ANDROID_BITMAP_FLAGS_ALPHA_MASK).into()
}

/// Returns [`true`] when [`ffi::ANDROID_BITMAP_FLAGS_IS_HARDWARE`] is set, meaning this
Expand Down Expand Up @@ -369,7 +378,7 @@ impl std::fmt::Debug for BitmapInfo {
f.field("width", &self.width())
.field("height", &self.height())
.field("stride", &self.stride())
.field("format", &self.try_format());
.field("format", &self.format());

#[cfg(feature = "api-level-30")]
f.field("flags", &self.flags());
Expand All @@ -385,7 +394,7 @@ impl BitmapInfo {
width,
height,
stride,
format: u32::from(format) as i32,
format: format.into(),
flags: 0,
},
}
Expand Down Expand Up @@ -423,22 +432,8 @@ impl BitmapInfo {
}

/// Convert the internal, native [`ffi::AndroidBitmapInfo::format`] into a [`BitmapFormat`].
///
/// # Panics
///
/// This function panics if the underlying value does not have a corresponding variant in
/// [`BitmapFormat`]. Use [`try_format()`][BitmapInfo::try_format()] for an infallible version
/// of this function.
pub fn format(&self) -> BitmapFormat {
self.try_format().unwrap()
}

/// Attempt to convert the internal, native [`ffi::AndroidBitmapInfo::format`] into a
/// [`BitmapFormat`]. This may fail if the value does not have a corresponding Rust enum
/// variant.
pub fn try_format(&self) -> Result<BitmapFormat, TryFromPrimitiveError<BitmapFormat>> {
let format = self.inner.format as u32;
format.try_into()
self.inner.format.into()
}

/// Bitfield containing information about the bitmap.
Expand All @@ -451,33 +446,40 @@ impl BitmapInfo {
/// Specifies the formats that can be compressed to with [`Bitmap::compress()`] and
/// [`Bitmap::compress_raw()`].
#[cfg(feature = "api-level-30")]
#[repr(u32)]
#[repr(i32)]
#[derive(Clone, Copy, Debug, Hash, PartialEq, Eq, TryFromPrimitive, IntoPrimitive)]
#[doc(alias = "AndroidBitmapCompressFormat")]
#[non_exhaustive]
pub enum BitmapCompressFormat {
/// Compress to the JPEG format.
///
/// quality of `0` means compress for the smallest size. `100` means compress for max visual
/// quality.
#[doc(alias = "ANDROID_BITMAP_COMPRESS_FORMAT_JPEG")]
Jpeg = ffi::AndroidBitmapCompressFormat::ANDROID_BITMAP_COMPRESS_FORMAT_JPEG.0,
Jpeg = ffi::AndroidBitmapCompressFormat::ANDROID_BITMAP_COMPRESS_FORMAT_JPEG.0 as i32,
/// Compress to the PNG format.
///
/// PNG is lossless, so quality is ignored.
#[doc(alias = "ANDROID_BITMAP_COMPRESS_FORMAT_PNG")]
Png = ffi::AndroidBitmapCompressFormat::ANDROID_BITMAP_COMPRESS_FORMAT_PNG.0,
Png = ffi::AndroidBitmapCompressFormat::ANDROID_BITMAP_COMPRESS_FORMAT_PNG.0 as i32,
/// Compress to the WEBP lossless format.
///
/// quality refers to how much effort to put into compression. A value of `0` means to
/// compress quickly, resulting in a relatively large file size. `100` means to spend more time
/// compressing, resulting in a smaller file.
#[doc(alias = "ANDROID_BITMAP_COMPRESS_FORMAT_WEBP_LOSSY")]
WebPLossy = ffi::AndroidBitmapCompressFormat::ANDROID_BITMAP_COMPRESS_FORMAT_WEBP_LOSSY.0,
WebPLossy =
ffi::AndroidBitmapCompressFormat::ANDROID_BITMAP_COMPRESS_FORMAT_WEBP_LOSSY.0 as i32,
/// Compress to the WEBP lossy format.
///
/// quality of `0` means compress for the smallest size. `100` means compress for max visual quality.
#[doc(alias = "ANDROID_BITMAP_COMPRESS_FORMAT_WEBP_LOSSLESS")]
WebPLossless = ffi::AndroidBitmapCompressFormat::ANDROID_BITMAP_COMPRESS_FORMAT_WEBP_LOSSLESS.0,
WebPLossless =
ffi::AndroidBitmapCompressFormat::ANDROID_BITMAP_COMPRESS_FORMAT_WEBP_LOSSLESS.0 as i32,

#[doc(hidden)]
#[num_enum(catch_all)]
__Unknown(i32),
}

/// Encapsulates possible errors returned by [`Bitmap::compress()`] or [`Bitmap::compress_raw()`].
Expand Down
Loading

0 comments on commit 399dd50

Please sign in to comment.