ComunidadAylas / vorbis-rs

🔊 Rust bindings for the best-in-breed C libraries of the Vorbis audio codec and Ogg container encapsulation.
https://crates.io/crates/vorbis_rs
BSD 3-Clause "New" or "Revised" License
16 stars 4 forks source link

Give back Writer on Encoder::finish() #8

Closed algesten closed 1 year ago

algesten commented 1 year ago

If using a Cursor<Vec<u8>> to write to memory, or an auto-removing temp file (https://docs.rs/tempfile/3.5.0/tempfile/fn.tempfile.html#resource-leaking), there needs to be some way of getting the captured writer back.

I suggest adding the writer as output of the finish call, so we can get ownership of it back.

AlexTMjugador commented 1 year ago

Thank you for using this crate and submitting a PR! :heart:

Since finish does not currently return anything interesting on success, I think that it is a sensible idea to yield the sink back there.

However, the standard library provides a by_ref method for every Write, the purpose of which is to avoid transferring ownership of a writer to methods and data types that take a Write implementation, such as VorbisEncoder, by mutably borrowing it instead.

Does your PR solve any problem that by_ref does not? (Just asking, it could be merged even if it didn't :smile:)

algesten commented 1 year ago

@AlexTMjugador thanks for the prompt reply!

My problem with using a borrowed writer is that the borrow lifetime parameter must leak upward into any wrapping struct.

In my case I have a struct Output which holds both the vorbis encoder as well as a mixer since they are used together.

struct Output {
  encoder: Encoder<File>,
  mixer: Mixer,
}

If this was Encoder<&'a mut File> it would force me to also have Output<'a>. It's possible, but "taints" my abstractions and breaks my encapsulation.

algesten commented 1 year ago

Thanks!

AlexTMjugador commented 1 year ago

This change has just been published in vorbis_rs v0.3.0! :tada:

I also took the opportunity to do some minor tidy-ups to the codebase and yanked v0.1.0, as it contains a known soundness issue.