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

Refactor codec::Parameters #88

Merged

Conversation

FreezyLemon
Copy link
Contributor

Currently, Parameters tries to differentiate its states (owned, shared) by setting the owner field. With the way the API works at the moment, there really isn't any advantage to this over using different structs for owned, shared, and mutable shared states. It takes up an extra Rc and complicates the differentiation between immutable and mutable borrows (since it's all the same type).

I'm also pretty sure that there is some potential for nasty UB via Stream::parameters because the returned Parameters type can be accessed mutably1 even though Stream is supposed to be an immutable reference to AVStream.

Some notes regarding the new implementation:

  • I want to use the pattern Something (owned), SomethingRef, SomethingMut for other wrapper types in the future
  • I don't want to use impl<'a> AsRef<SomethingRef<'a>> for Something because it returns a &'b SomethingRef<'a> with two levels of indirection, which seems unnecessary
  • The macros are required to keep the boilerplate under control. I might even want to add more macros to implement common field types and/or traits with less code
  • I might add safety docs to from_raw. At the moment, the only safety concern is that you need to bound the returned lifetime correctly (since it can otherwise be arbitrarily chosen). The best way to do this is by bounding it on a borrow of another type:
    pub fn something<'s>(&'s self) -> SomethingRef<'s> { /* ... */ }
  • StreamMut::copy_parameters_from_context seems a bit unnecessary, but it is more efficient. The old implementation always allocated a new AVCodecParameters from a context, and then copied that AVCodecParameters into the stream. The new one just uses parameters_from_context(stream.codecpar, context) which avoids the in-between step

Footnotes

  1. See examples/transcode-x264.rs: (*ost.parameters().as_mut_ptr()).codec_tag = 0;

fn copy_parameters_from_context is more verbose but avoids an
allocation.

fn parameters_mut is necessary to ensure UB (before the refactor,
fn parameters(&self) allowed mutation via unsafe code).
@FreezyLemon
Copy link
Contributor Author

At the moment, the only safety concern is ...

Well, that was obviously not true. I pushed some documentation which should cover everything

Copy link
Owner

@shssoichiro shssoichiro left a comment

Choose a reason for hiding this comment

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

Sorry, kept forgetting to look at this one.

@shssoichiro shssoichiro merged commit 6a541ca into shssoichiro:master Dec 5, 2024
20 checks passed
@FreezyLemon FreezyLemon deleted the refactor-codec-parameters branch December 5, 2024 17:54
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.

2 participants