From 0f3bed413de18f0070c4851f8f2ce0e90df4903a Mon Sep 17 00:00:00 2001 From: Alexander Capehart Date: Sat, 15 Feb 2025 13:04:50 -0700 Subject: [PATCH] musikr: improve lifecycles --- musikr/src/main/jni/src/lib.rs | 1 + .../main/jni/src/taglib/audioproperties.rs | 8 ++--- musikr/src/main/jni/src/taglib/bridge.rs | 2 +- musikr/src/main/jni/src/taglib/file.rs | 18 +++++----- musikr/src/main/jni/src/taglib/file_ref.rs | 10 +++--- musikr/src/main/jni/src/taglib/flac.rs | 35 ++++++++++--------- musikr/src/main/jni/src/taglib/iostream.rs | 14 ++++---- musikr/src/main/jni/src/taglib/ogg.rs | 29 ++++++--------- musikr/src/main/jni/src/taglib/tk.rs | 24 ++++++------- musikr/src/main/jni/src/taglib/xiph.rs | 33 ++++++++++------- 10 files changed, 89 insertions(+), 85 deletions(-) diff --git a/musikr/src/main/jni/src/lib.rs b/musikr/src/main/jni/src/lib.rs index efd0b9ad3..45b0417e0 100644 --- a/musikr/src/main/jni/src/lib.rs +++ b/musikr/src/main/jni/src/lib.rs @@ -8,6 +8,7 @@ mod taglib; mod jstream; use taglib::file_ref::FileRef; +use taglib::file::File; use jstream::JInputStream; type SharedEnv<'local> = Rc>>; diff --git a/musikr/src/main/jni/src/taglib/audioproperties.rs b/musikr/src/main/jni/src/taglib/audioproperties.rs index 68eba51c7..5b26ecff8 100644 --- a/musikr/src/main/jni/src/taglib/audioproperties.rs +++ b/musikr/src/main/jni/src/taglib/audioproperties.rs @@ -1,12 +1,12 @@ use super::bridge::CppAudioProperties; use std::pin::Pin; -pub struct AudioProperties<'a> { - this: Pin<&'a CppAudioProperties> +pub struct AudioProperties<'file_ref> { + this: Pin<&'file_ref CppAudioProperties> } -impl<'a> AudioProperties<'a> { - pub(super) fn new(this: Pin<&'a CppAudioProperties>) -> Self { +impl<'file_ref> AudioProperties<'file_ref> { + pub(super) fn new(this: Pin<&'file_ref CppAudioProperties>) -> Self { Self { this } } diff --git a/musikr/src/main/jni/src/taglib/bridge.rs b/musikr/src/main/jni/src/taglib/bridge.rs index e95ce224f..d2f90f14f 100644 --- a/musikr/src/main/jni/src/taglib/bridge.rs +++ b/musikr/src/main/jni/src/taglib/bridge.rs @@ -78,7 +78,7 @@ mod bridge_impl { #[cxx_name = "XiphComment"] type CPPXiphComment; #[cxx_name = "fieldListMap"] - fn fieldListMap(self: Pin<&CPPXiphComment>) -> &CPPFieldListMap; + fn fieldListMap(self: Pin<& CPPXiphComment>) -> &CPPFieldListMap; #[namespace = "TagLib"] #[cxx_name = "SimplePropertyMap"] diff --git a/musikr/src/main/jni/src/taglib/file.rs b/musikr/src/main/jni/src/taglib/file.rs index 7e7ad384f..b6d49ccf9 100644 --- a/musikr/src/main/jni/src/taglib/file.rs +++ b/musikr/src/main/jni/src/taglib/file.rs @@ -5,17 +5,17 @@ use super::ogg::OpusFile; use super::ogg::VorbisFile; use super::flac::FLACFile; -pub struct File<'a> { - this: Pin<&'a mut CPPFile> +pub struct File<'file_ref> { + this: Pin<&'file_ref mut CPPFile> } -impl<'a> File<'a> { - pub(super) fn new(this: Pin<&'a mut CPPFile>) -> Self { +impl<'file_ref> File<'file_ref> { + pub(super) fn new(this: Pin<&'file_ref mut CPPFile>) -> Self { Self { this } } - pub fn audio_properties(&self) -> Option> { + pub fn audio_properties(&self) -> Option> { let props_ptr = self.this.as_ref().audioProperties(); let props_ref = unsafe { // SAFETY: @@ -30,7 +30,7 @@ impl<'a> File<'a> { props_pin.map(|props| AudioProperties::new(props)) } - pub fn as_opus(&mut self) -> Option> { + pub fn as_opus(&mut self) -> Option> { let opus_file = unsafe { // SAFETY: // This FFI function will be a simple C++ dynamic_cast, which checks if @@ -51,7 +51,7 @@ impl<'a> File<'a> { opus_pin.map(|opus| OpusFile::new(opus)) } - pub fn as_vorbis(&mut self) -> Option> { + pub fn as_vorbis(&mut self) -> Option> { let vorbis_file = unsafe { // SAFETY: // This FFI function will be a simple C++ dynamic_cast, which checks if @@ -72,7 +72,7 @@ impl<'a> File<'a> { vorbis_pin.map(|vorbis| VorbisFile::new(vorbis)) } - pub fn as_flac(&mut self) -> Option> { + pub fn as_flac(&mut self) -> Option> { let flac_file = unsafe { // SAFETY: // This FFI function will be a simple C++ dynamic_cast, which checks if @@ -94,7 +94,7 @@ impl<'a> File<'a> { } } -impl<'a> Drop for File<'a> { +impl<'file_ref> Drop for File<'file_ref> { fn drop(&mut self) { unsafe { std::ptr::drop_in_place(&mut self.this); diff --git a/musikr/src/main/jni/src/taglib/file_ref.rs b/musikr/src/main/jni/src/taglib/file_ref.rs index 38b5f2a0f..d10ea8fea 100644 --- a/musikr/src/main/jni/src/taglib/file_ref.rs +++ b/musikr/src/main/jni/src/taglib/file_ref.rs @@ -4,13 +4,13 @@ use super::iostream::{BridgedIOStream, IOStream}; use cxx::UniquePtr; use std::pin::Pin; -pub struct FileRef<'a> { - stream: BridgedIOStream<'a>, +pub struct FileRef<'io> { + stream: BridgedIOStream<'io>, this: UniquePtr, } -impl<'a> FileRef<'a> { - pub fn new(stream: T) -> FileRef<'a> { +impl<'io> FileRef<'io> { + pub fn new(stream: T) -> FileRef<'io> { let stream = BridgedIOStream::new(stream); let cpp_stream = stream.cpp_stream().as_mut_ptr(); let file_ref = unsafe { bridge::new_FileRef(cpp_stream) }; @@ -20,7 +20,7 @@ impl<'a> FileRef<'a> { } } - pub fn file(&self) -> Option> { + pub fn file<'file_ref>(&'file_ref self) -> Option> { let file_ptr = unsafe { // SAFETY: // - This pin is only used in this unsafe scope. diff --git a/musikr/src/main/jni/src/taglib/flac.rs b/musikr/src/main/jni/src/taglib/flac.rs index 7886dce78..3e3a21c99 100644 --- a/musikr/src/main/jni/src/taglib/flac.rs +++ b/musikr/src/main/jni/src/taglib/flac.rs @@ -7,16 +7,16 @@ use std::marker::PhantomData; use cxx::UniquePtr; use std::pin::Pin; -pub struct FLACFile<'a> { - this: Pin<&'a mut CPPFLACFile> +pub struct FLACFile<'file_ref> { + this: Pin<&'file_ref mut CPPFLACFile> } -impl<'a> FLACFile<'a> { - pub(super) fn new(this: Pin<&'a mut CPPFLACFile>) -> Self { +impl<'file_ref> FLACFile<'file_ref> { + pub(super) fn new(this: Pin<&'file_ref mut CPPFLACFile>) -> Self { Self { this } } - pub fn xiph_comments(&mut self) -> Option { + pub fn xiph_comments(&mut self) -> Option> { let this = self.this.as_mut(); let tag = this.xiphComment(false); let tag_ref = unsafe { @@ -28,25 +28,26 @@ impl<'a> FLACFile<'a> { tag_pin.map(|tag| XiphComment::new(tag)) } - pub fn picture_list(&mut self) -> PictureList { + pub fn picture_list(&mut self) -> PictureList<'file_ref> { let pictures = FLACFile_pictureList(self.this.as_mut()); PictureList::new(pictures) } } -pub struct PictureList<'a> { - // PictureList is implicitly tied to the lifetime of the parent that owns it, - // so we need to track that lifetime. - _data: PhantomData<&'a CPPFLACPicture>, +pub struct PictureList<'file_ref> { + // PictureList is implicitly tied to the lifetime of the file_ref, despite us technically + // """""owning"""" it. + _data: PhantomData<&'file_ref CPPFLACPicture>, + // Only in a UniquePtr because we can't marshal over ownership of the PictureList by itself over cxx. this: UniquePtr, } -impl<'a> PictureList<'a> { +impl<'file_ref> PictureList<'file_ref> { pub(super) fn new(this: UniquePtr) -> Self { Self { _data: PhantomData, this } } - pub fn to_vec(&self) -> Vec { + pub fn to_vec(&self) -> Vec> { let pictures = PictureList_to_vector(unsafe { // SAFETY: This pin is only used in this unsafe scope. // The pin is used as a C++ this pointer in the ffi call, which does @@ -69,16 +70,16 @@ impl<'a> PictureList<'a> { } } -pub struct Picture<'a> { - this: Pin<&'a CPPFLACPicture> +pub struct Picture<'file_ref> { + this: Pin<&'file_ref CPPFLACPicture> } -impl<'a> Picture<'a> { - pub(super) fn new(this: Pin<&'a CPPFLACPicture>) -> Self { +impl<'file_ref> Picture<'file_ref> { + pub(super) fn new(this: Pin<&'file_ref CPPFLACPicture>) -> Self { Self { this } } - pub fn data(&self) -> ByteVector<'a> { + pub fn data(&self) -> ByteVector<'file_ref> { let data = Picture_data(self.this); ByteVector::new(data) } diff --git a/musikr/src/main/jni/src/taglib/iostream.rs b/musikr/src/main/jni/src/taglib/iostream.rs index 4c7169c2b..a6795524b 100644 --- a/musikr/src/main/jni/src/taglib/iostream.rs +++ b/musikr/src/main/jni/src/taglib/iostream.rs @@ -10,13 +10,13 @@ pub trait IOStream : Read + Write + Seek { } -pub(super) struct BridgedIOStream<'a> { - rs_stream: Pin>>, +pub(super) struct BridgedIOStream<'io_stream> { + rs_stream: Pin>>, cpp_stream: UniquePtr } -impl<'a> BridgedIOStream<'a> { - pub fn new(stream: T) -> Self { +impl<'io_stream> BridgedIOStream<'io_stream> { + pub fn new(stream: T) -> Self { let mut rs_stream = Box::pin(DynIOStream(Box::new(stream))); let cpp_stream = bridge::wrap_RsIOStream(rs_stream.as_mut()); BridgedIOStream { @@ -30,7 +30,7 @@ impl<'a> BridgedIOStream<'a> { } } -impl<'a> Drop for BridgedIOStream<'a> { +impl<'io_stream> Drop for BridgedIOStream<'io_stream> { fn drop(&mut self) { unsafe { // CPP stream references the rust stream, so it must be dropped first @@ -41,9 +41,9 @@ impl<'a> Drop for BridgedIOStream<'a> { } #[repr(C)] -pub(super) struct DynIOStream<'a>(Box); +pub(super) struct DynIOStream<'io_stream>(Box); -impl<'a> DynIOStream<'a> { +impl<'io_stream> DynIOStream<'io_stream> { // Implement the exposed functions for cxx bridge pub fn name(&mut self) -> String { diff --git a/musikr/src/main/jni/src/taglib/ogg.rs b/musikr/src/main/jni/src/taglib/ogg.rs index df71a886b..8709bbcd3 100644 --- a/musikr/src/main/jni/src/taglib/ogg.rs +++ b/musikr/src/main/jni/src/taglib/ogg.rs @@ -2,16 +2,16 @@ pub use super::bridge::{CPPOpusFile, CPPVorbisFile}; use super::xiph::XiphComment; use std::pin::Pin; -pub struct VorbisFile<'a> { - this: Pin<&'a mut CPPVorbisFile> +pub struct VorbisFile<'file_ref> { + this: Pin<&'file_ref mut CPPVorbisFile> } -impl<'a> VorbisFile<'a> { - pub(super) fn new(this: Pin<&'a mut CPPVorbisFile>) -> Self { +impl<'file_ref> VorbisFile<'file_ref> { + pub(super) fn new(this: Pin<&'file_ref mut CPPVorbisFile>) -> Self { Self { this } } - pub fn xiph_comments(&self) -> Option { + pub fn xiph_comments(&self) -> Option> { let this = self.this.as_ref(); let tag = this.vorbisTag(); let tag_ref = unsafe { @@ -24,25 +24,18 @@ impl<'a> VorbisFile<'a> { } } -pub struct OpusFile<'a> { - this: Pin<&'a mut CPPOpusFile> +pub struct OpusFile<'file_ref> { + this: Pin<&'file_ref mut CPPOpusFile> } -impl <'a> OpusFile<'a> { - pub(super) fn new(this: Pin<&'a mut CPPOpusFile>) -> Self { +impl<'file_ref> OpusFile<'file_ref> { + pub(super) fn new(this: Pin<&'file_ref mut CPPOpusFile>) -> Self { Self { this } } - pub fn xiph_comments(&self) -> Option> { + pub fn xiph_comments(&self) -> Option> { let this = self.this.as_ref(); - let tag = unsafe { - // SAFETY: - // - This pin is only used in this unsafe scope. - // - The pin is used as a C++ this pointer in the ffi call, which does - // not change address by C++ semantics. - // - The value is a pointer that does not depend on the address of self. - this.opusTag() - }; + let tag = this.opusTag(); let tag_ref = unsafe { // SAFETY: This pointer is a valid type, and can only used and accessed // via this function and thus cannot be mutated, satisfying the aliasing rules. diff --git a/musikr/src/main/jni/src/taglib/tk.rs b/musikr/src/main/jni/src/taglib/tk.rs index 53585cb72..bfa426e08 100644 --- a/musikr/src/main/jni/src/taglib/tk.rs +++ b/musikr/src/main/jni/src/taglib/tk.rs @@ -4,17 +4,17 @@ use std::marker::PhantomData; use std::pin::Pin; use std::{ffi::CStr, string::ToString}; -pub struct String<'a> { - this: Pin<&'a CPPString>, +pub struct String<'file_ref> { + this: Pin<&'file_ref CPPString>, } -impl<'a> String<'a> { - pub(super) fn new(this: Pin<&'a CPPString>) -> Self { +impl<'file_ref> String<'file_ref> { + pub(super) fn new(this: Pin<&'file_ref CPPString>) -> Self { Self { this } } } -impl<'a> ToString for String<'a> { +impl<'file_ref> ToString for String<'file_ref> { fn to_string(&self) -> std::string::String { let c_str = self.this.toCString(true); unsafe { @@ -34,12 +34,12 @@ impl<'a> ToString for String<'a> { } } -pub struct StringList<'a> { - this: Pin<&'a CPPStringList>, +pub struct StringList<'file_ref> { + this: Pin<&'file_ref CPPStringList>, } -impl<'a> StringList<'a> { - pub(super) fn new(this: Pin<&'a CPPStringList>) -> Self { +impl<'file_ref> StringList<'file_ref> { + pub(super) fn new(this: Pin<&'file_ref CPPStringList>) -> Self { Self { this } } @@ -57,15 +57,15 @@ impl<'a> StringList<'a> { } } -pub struct ByteVector<'a> { +pub struct ByteVector<'file_ref> { // ByteVector is implicitly tied to the lifetime of the parent that owns it, // so we need to track that lifetime. Only reason why it's a UniquePtr is because // we can't marshal over ownership of the ByteVector by itself over cxx. - _data: PhantomData<&'a CPPByteVector>, + _data: PhantomData<&'file_ref CPPByteVector>, this: UniquePtr, } -impl<'a> ByteVector<'a> { +impl<'file_ref> ByteVector<'file_ref> { pub(super) fn new(this: UniquePtr) -> Self { Self { _data: PhantomData, diff --git a/musikr/src/main/jni/src/taglib/xiph.rs b/musikr/src/main/jni/src/taglib/xiph.rs index a02ace3f0..3d2030359 100644 --- a/musikr/src/main/jni/src/taglib/xiph.rs +++ b/musikr/src/main/jni/src/taglib/xiph.rs @@ -5,38 +5,47 @@ use super::tk; use std::pin::Pin; use std::collections::HashMap; -pub struct XiphComment<'a> { - this: Pin<&'a mut CPPXiphComment> +pub struct XiphComment<'file_ref> { + this: Pin<&'file_ref mut CPPXiphComment> } -impl<'a> XiphComment<'a> { - pub(super) fn new(this: Pin<&'a mut CPPXiphComment>) -> Self { +impl<'file_ref> XiphComment<'file_ref> { + pub(super) fn new(this: Pin<&'file_ref mut CPPXiphComment>) -> Self { Self { this } } - pub fn field_list_map(&'a self) -> FieldListMap<'a> { - let map = self.this.as_ref().fieldListMap(); + pub fn field_list_map<'slf>(&'slf self) -> FieldListMap<'file_ref> { + // To call the method we need, we have to get our mut reference down to an immutable + // reference. The safe API can do this, but shortens the lifecycle to at most self, even + // though the reference really lives as long as file_ref. Sadly, this requires us to transmute + // to extend the lifecycle back. This new pointer is really unsafe (we now have both a mut + // and an immutable reference to the same object), but it's dropped after this call. + // The value returned is unable to actually mutate this object, so it's safe. + let this_ref: &'slf CPPXiphComment = self.this.as_ref().get_ref(); + let extended_ref: &'file_ref CPPXiphComment = unsafe { std::mem::transmute(this_ref) }; + let this: Pin<&'file_ref CPPXiphComment> = unsafe { Pin::new_unchecked(extended_ref) }; + let map = this.fieldListMap(); let map_pin = unsafe { Pin::new_unchecked(map) }; FieldListMap::new(map_pin) } - pub fn picture_list(&mut self) -> PictureList<'a> { + pub fn picture_list(&mut self) -> PictureList<'file_ref> { let pictures = XiphComment_pictureList(self.this.as_mut()); PictureList::new(pictures) } } -pub struct FieldListMap<'a> { - this: Pin<&'a CPPFieldListMap>, +pub struct FieldListMap<'file_ref> { + this: Pin<&'file_ref CPPFieldListMap>, } -impl<'a> FieldListMap<'a> { - pub(super) fn new(this: Pin<&'a CPPFieldListMap>) -> Self { +impl<'file_ref> FieldListMap<'file_ref> { + pub(super) fn new(this: Pin<&'file_ref CPPFieldListMap>) -> Self { Self { this } } } -impl<'a> FieldListMap<'a> { +impl<'file_ref> FieldListMap<'file_ref> { pub fn to_hashmap(&self) -> HashMap> { let cxx_vec = FieldListMap_to_entries(self.this); cxx_vec