-
Notifications
You must be signed in to change notification settings - Fork 22
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
fn rav1d_log: Replace with safe write! calls to a fully safe `Rav…
…1dLogger`.
The existing variadic `rav1d_log` calls are replaced with `write!` calls
to an `Option<Rav1dLogger>` field.
`enum Rav1dLogger`'s interface is `fn write_fmt`, which `write!` calls.
These are defined as part of `trait Rav1dLog`, similar to `trait {io,fmt}::Write`.
The difference is that no errors are returned (we don't really need them)
and crucially, they are `&self` methods as opposed to `&mut self` methods,
which keeps things much simpler for the borrow checker.
`enum Rav1dLogger` can be `Dav1d` or `Std{out,err}` for now.
The `Dav1d` variant is necessary to convert to/from the `Dav1dLogger`,
and `Stderr` is the default logger used (and `Stdout` is trivially similar).
There will presumably be other loggers desired,
but it's easier to leave those for later once we know their desired behaviors better.
The `Dav1d` variant is called by implementing `fmt::Write`,
which lets us just implement `fn write_str` and have `fn write_fmt` auto-implemented.
`fn write_str` is implemented correctly but with poor performance by printing byte by byte.
This is to avoid dealing with intermediary null characters
and having to allocate a new null-terminated `CString`.
This is for logging so perf isn't important,
and if better perf is desired, users can switch
to the Rust API, which has no such limitation.
Then we construct a bijection between `{D,R}av1dLogger`
to allow the bidirectional conversions needed in the `DAV1D_API`s.
This is done by storing the `Std{out,err}` variants as special `Dav1dLogger::callback` `fn`s.
During the conversion back, if the callback is one of those special `fn`s,
we can decode it back into the same `Rav1dLogger` it originally was.
With use of the `Dav1dLogger::cookie` field, this can be extended
to other `Rav1dLogger` variants in the future as well.- Loading branch information
Showing
12 changed files
with
190 additions
and
139 deletions.
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,47 +1,152 @@ | ||
| use crate::include::common::validate::validate_input; | ||
| use crate::include::dav1d::dav1d::Rav1dLogger; | ||
| use crate::src::internal::Rav1dContext; | ||
| use crate::stderr; | ||
| use cstr::cstr; | ||
| use std::ffi::c_char; | ||
| use std::ffi::c_int; | ||
| use std::ffi::c_uint; | ||
| use std::ffi::c_void; | ||
| use std::fmt; | ||
| use std::fmt::Write as _; | ||
| use std::io::stderr; | ||
| use std::io::stdout; | ||
| use std::io::Write as _; | ||
| use std::ptr; | ||
|
|
||
| extern "C" { | ||
| fn vfprintf(_: *mut libc::FILE, _: *const c_char, _: ::core::ffi::VaList) -> c_int; | ||
| pub type Dav1dLoggerCallback = unsafe extern "C" fn( | ||
| // The above `cookie` field. | ||
| cookie: *mut c_void, | ||
| // A `printf`-style format specifier. | ||
| fmt: *const c_char, | ||
| // `printf`-style variadic args. | ||
| args: ... | ||
| ); | ||
|
|
||
| #[derive(Clone)] | ||
| #[repr(C)] | ||
| pub struct Dav1dLogger { | ||
| /// A cookie that's passed as the first argument to the callback below. | ||
| cookie: *mut c_void, | ||
| /// A `printf`-style function except for an extra first argument that will always be the above `cookie`. | ||
| callback: Option<Dav1dLoggerCallback>, | ||
| } | ||
|
|
||
| #[cold] | ||
| pub unsafe extern "C" fn rav1d_log_default_callback( | ||
| _cookie: *mut c_void, | ||
| format: *const c_char, | ||
| mut ap: ::core::ffi::VaList, | ||
| ) { | ||
| vfprintf(stderr, format, ap.as_va_list()); | ||
| impl Dav1dLogger { | ||
| /// # Safety | ||
| /// | ||
| /// `callback`, if non-[`None`]/`NULL` must be safe to call when: | ||
| /// * the first argument is `cookie` | ||
| /// * the rest of the arguments would be safe to call `printf` with | ||
| pub const unsafe fn new(cookie: *mut c_void, callback: Option<Dav1dLoggerCallback>) -> Self { | ||
| Self { cookie, callback } | ||
| } | ||
| } | ||
|
|
||
| impl Default for Rav1dLogger { | ||
| impl Default for Dav1dLogger { | ||
| fn default() -> Self { | ||
| Self { | ||
| cookie: ptr::null_mut(), | ||
| callback: Some(rav1d_log_default_callback), | ||
| // Safety: `callback` is [`None`]. | ||
| unsafe { Self::new(ptr::null_mut(), None) } | ||
| } | ||
| } | ||
|
|
||
| /// A [`Dav1dLogger`] from C that's not | ||
| /// just a [`Rav1dLogger`] converted to a [`Dav1dLogger`]. | ||
| #[derive(Clone)] | ||
| pub(crate) struct OnlyDav1dLogger { | ||
| cookie: *mut c_void, | ||
| callback: Dav1dLoggerCallback, | ||
| } | ||
|
|
||
| impl fmt::Write for OnlyDav1dLogger { | ||
| fn write_str(&mut self, s: &str) -> fmt::Result { | ||
| // `s` doesn't have a terminating nul-byte, | ||
| // and it may have internal nul-bytes, | ||
| // so it's easiest just to print one byte at a time. | ||
| // This may be slow, but logging can be disabled if it's slow, | ||
| // or the Rust API can be used instead. | ||
| let fmt = cstr!("%c"); | ||
| for &byte in s.as_bytes() { | ||
| // # Safety | ||
| // | ||
| // The first argument is `self.cookie` | ||
| // and the rest are safe to call `printf` with, | ||
| // as required by [`Self::new`]. | ||
| unsafe { | ||
| (self.callback)(self.cookie, fmt.as_ptr(), byte as c_uint); | ||
| } | ||
| } | ||
| Ok(()) | ||
| } | ||
| } | ||
|
|
||
| #[cold] | ||
| pub unsafe extern "C" fn rav1d_log(c: *mut Rav1dContext, format: *const c_char, args: ...) { | ||
| if validate_input!(!c.is_null()).is_err() { | ||
| return; | ||
| #[derive(Clone, Default)] | ||
| pub(crate) enum Rav1dLogger { | ||
| Dav1d(OnlyDav1dLogger), | ||
| Stdout, | ||
| #[default] | ||
| Stderr, | ||
| } | ||
|
|
||
| /// Any type implementing [`Rav1dLog`] can be used with [`write!`]. | ||
| pub trait Rav1dLog { | ||
| fn write_fmt(&self, args: fmt::Arguments); | ||
| } | ||
|
|
||
| impl Rav1dLog for Rav1dLogger { | ||
| /// Logging doesn't have to be fast when it's on, | ||
| /// but we don't want it slow things down when it's off, | ||
| /// so ensure the logging code isn't inlined everywhere, bloating call sites. | ||
| #[inline(never)] | ||
| fn write_fmt(&self, args: fmt::Arguments) { | ||
| match self { | ||
| // The `dav1d.clone()` is because [`fmt::Write::write_fmt`] takes `&mut` | ||
| // even though we don't need it to. | ||
| // [`OnlyDav1dLogger`] is trivial to [`Clone`], though, so we can just do that. | ||
| Self::Dav1d(dav1d) => dav1d.clone().write_fmt(args).unwrap(), | ||
| Self::Stdout => stdout().write_fmt(args).unwrap(), | ||
| Self::Stderr => stderr().write_fmt(args).unwrap(), | ||
| } | ||
| } | ||
| if ((*c).logger.callback).is_none() { | ||
| return; | ||
| } | ||
|
|
||
| impl Rav1dLog for Option<Rav1dLogger> { | ||
| /// When a logger isn't set, we don't want to have to run any code here, | ||
| /// so force this to be inlined so a [`None`] can be seen. | ||
| #[inline(always)] | ||
| fn write_fmt(&self, args: fmt::Arguments) { | ||
| if let Some(logger) = self { | ||
| logger.write_fmt(args); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Used as a marker for [`Rav1dLogger::Stdout`]. Still a valid (i.e. safe) [`Dav1dLoggerCallback`], though. | ||
| unsafe extern "C" fn rav1d_logger_stdout(_cookie: *mut c_void, _fmt: *const c_char, ...) {} | ||
|
|
||
| /// Used as a marker for [`Rav1dLogger::Stderr`]. Still a valid (i.e. safe) [`Dav1dLoggerCallback`], though. | ||
| unsafe extern "C" fn rav1d_logger_stderr(_cookie: *mut c_void, _fmt: *const c_char, ...) {} | ||
|
|
||
| impl From<Dav1dLogger> for Option<Rav1dLogger> { | ||
| fn from(logger: Dav1dLogger) -> Self { | ||
| let Dav1dLogger { cookie, callback } = logger; | ||
| let callback = callback?; | ||
| Some(if callback == rav1d_logger_stdout { | ||
| Rav1dLogger::Stdout | ||
| } else if callback == rav1d_logger_stderr { | ||
| Rav1dLogger::Stderr | ||
| } else { | ||
| Rav1dLogger::Dav1d(OnlyDav1dLogger { cookie, callback }) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| impl From<Option<Rav1dLogger>> for Dav1dLogger { | ||
| fn from(logger: Option<Rav1dLogger>) -> Self { | ||
| let cookie = match &logger { | ||
| Some(Rav1dLogger::Dav1d(dav1d)) => dav1d.cookie, | ||
| _ => ptr::null_mut(), | ||
| }; | ||
| let callback = logger.map(|logger| match logger { | ||
| Rav1dLogger::Dav1d(dav1d) => dav1d.callback, | ||
| Rav1dLogger::Stdout => rav1d_logger_stdout, | ||
| Rav1dLogger::Stderr => rav1d_logger_stderr, | ||
| }); | ||
| Self { cookie, callback } | ||
| } | ||
| let mut ap: ::core::ffi::VaListImpl; | ||
| ap = args.clone(); | ||
| ((*c).logger.callback).expect("non-null function pointer")( | ||
| (*c).logger.cookie, | ||
| format, | ||
| ap.as_va_list(), | ||
| ); | ||
| } |
Oops, something went wrong.