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

fn rav1d_log: Replace with safe write! calls to a fully safe Rav1dLogger #619

Merged
merged 7 commits into from
Dec 19, 2023

Conversation

kkysen
Copy link
Collaborator

@kkysen kkysen commented Dec 10, 2023

The existing variadic and unsafe 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_APIs. This is done by storing the Std{out,err} variants as special Dav1dLogger::callback fns. During the conversion back, if the callback is one of those special fns, 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.

This also makes it fairly easy to eliminate #![feature(c_variadics)] (it's only used for the special marker fns now, but those can be implemented a bit differently).

@CrazyboyQCD
Copy link

Since rust-lang/rust#117472 has been merged, I think cstr crate could be removed sometime.

@kkysen
Copy link
Collaborator Author

kkysen commented Dec 11, 2023

Since rust-lang/rust#117472 has been merged, I think cstr crate could be removed sometime.

Oh, that's awesome! Thanks for letting me know! I saw the RFC got accepted earlier but I didn't realize it would be stabilized so soon. We'll remove cstr once 1.76 is released.

src/log.rs Outdated Show resolved Hide resolved
src/log.rs Outdated Show resolved Hide resolved
src/log.rs Outdated Show resolved Hide resolved
src/log.rs Outdated Show resolved Hide resolved
src/log.rs Outdated Show resolved Hide resolved
src/log.rs Show resolved Hide resolved
src/log.rs Outdated Show resolved Hide resolved
src/log.rs Outdated Show resolved Hide resolved
src/log.rs Show resolved Hide resolved
…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.
This was the wrong default.
It was the zero value, not the actual default.
And users of the C API will just set the fields directly,
so go back to the that in the binaries.
…until `c"strings"`'s stabilization reaches stable.
… if`, ... with a `match` with `if guards.
…nd into the binaries.

The binaries aren't important, so it's useful to fully remove these from the important `rav1d` lib.
Base automatically changed from kkysen/tiles-Vec to main December 19, 2023 00:46
@kkysen kkysen merged commit ca3923a into main Dec 19, 2023
18 checks passed
@kkysen kkysen deleted the kkysen/logging branch December 19, 2023 00:46
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.

Use log crate for debug logging
3 participants