diff --git a/musikr/src/main/jni/src/taglib/ffi.rs b/musikr/src/main/jni/src/taglib/ffi.rs index 54c1845da..65a649558 100644 --- a/musikr/src/main/jni/src/taglib/ffi.rs +++ b/musikr/src/main/jni/src/taglib/ffi.rs @@ -17,8 +17,10 @@ pub(crate) mod bindings { #[namespace = "TagLib"] type FileRef; - fn isNull(self: Pin<&FileRef>) -> bool; - fn file(self: Pin<&FileRef>) -> *mut BaseFile; + #[cxx_name = "isNull"] + fn thisIsNull(self: Pin<&FileRef>) -> bool; + #[cxx_name = "file"] + fn thisFile(self: Pin<&FileRef>) -> *mut BaseFile; #[namespace = "taglib_shim"] type RustIOStream; @@ -34,14 +36,19 @@ pub(crate) mod bindings { #[namespace = "TagLib"] #[cxx_name = "File"] type BaseFile; - fn audioProperties(self: Pin<&BaseFile>) -> *mut AudioProperties; + #[cxx_name = "audioProperties"] + fn thisAudioProperties(self: Pin<&BaseFile>) -> *mut AudioProperties; #[namespace = "TagLib"] type AudioProperties; - fn lengthInMilliseconds(self: Pin<&AudioProperties>) -> i32; - fn bitrate(self: Pin<&AudioProperties>) -> i32; - fn sampleRate(self: Pin<&AudioProperties>) -> i32; - fn channels(self: Pin<&AudioProperties>) -> i32; + #[cxx_name = "lengthInMilliseconds"] + fn thisLengthInMilliseconds(self: Pin<&AudioProperties>) -> i32; + #[cxx_name = "bitrate"] + fn thisBitrate(self: Pin<&AudioProperties>) -> i32; + #[cxx_name = "sampleRate"] + fn thisSampleRate(self: Pin<&AudioProperties>) -> i32; + #[cxx_name = "channels"] + fn thisChannels(self: Pin<&AudioProperties>) -> i32; #[namespace = "TagLib::Ogg"] #[cxx_name = "File"] @@ -49,19 +56,20 @@ pub(crate) mod bindings { #[namespace = "TagLib::Ogg"] type XiphComment; - unsafe fn fieldListMap(self: Pin<&XiphComment>) -> &SimplePropertyMap; + #[cxx_name = "fieldListMap"] + unsafe fn thisFieldListMap(self: Pin<&XiphComment>) -> &SimplePropertyMap; #[namespace = "TagLib::Ogg::Vorbis"] #[cxx_name = "File"] type VorbisFile; #[cxx_name = "tag"] - unsafe fn tag(self: Pin<&VorbisFile>) -> *mut XiphComment; + unsafe fn vorbisThisTag(self: Pin<&VorbisFile>) -> *mut XiphComment; #[namespace = "TagLib::Ogg::Opus"] #[cxx_name = "File"] type OpusFile; #[cxx_name = "tag"] - unsafe fn opusTag(self: Pin<&OpusFile>) -> *mut XiphComment; + unsafe fn opusThisTag(self: Pin<&OpusFile>) -> *mut XiphComment; #[namespace = "TagLib::FLAC"] #[cxx_name = "File"] @@ -115,94 +123,166 @@ pub(crate) mod bindings { #[namespace = "taglib_shim"] type Property; - fn key(self: Pin<&Property>) -> &TagString; - unsafe fn value(self: Pin<&Property>) -> &StringList; - - #[namespace = "TagLib"] - type StringList; - #[namespace = "taglib_shim"] - fn StringList_to_vector(string_list: Pin<&StringList>) -> UniquePtr>; + #[cxx_name = "key"] + fn thisKey(self: Pin<&Property>) -> &TString; + #[cxx_name = "value"] + unsafe fn thisValue(self: Pin<&Property>) -> &TStringList; #[namespace = "TagLib"] #[cxx_name = "String"] - type TagString; + type TString; + #[cxx_name = "toCString"] + unsafe fn thisToCString(self: Pin<&TString>, unicode: bool) -> *const c_char; + + #[namespace = "TagLib"] + #[cxx_name = "StringList"] + type TStringList; #[namespace = "taglib_shim"] - unsafe fn toCString(self: Pin<&TagString>, unicode: bool) -> *const c_char; + fn StringList_to_vector(string_list: Pin<&TStringList>) -> UniquePtr>; } } impl bindings::FileRef { pub fn file_or(&self) -> Option<&bindings::BaseFile> { - unsafe { - // SAFETY: This pin only lasts for the scope of this function. - // Nothing that can change the memory address of self is returned, - // only the address of the file pointer. - let pinned_self = Pin::new_unchecked(&*self); - if !pinned_self.isNull() { - pinned_self.file().as_ref() + let file = 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 file data is a pointer that does not depend on the + // address of self. + let this = Pin::new_unchecked(&*self); + // Note: This is not the rust ptr "is_null", but a taglib isNull method + // that checks for file validity. Without this check, we can get corrupted + // file ptrs. + if !this.thisIsNull() { + Some(this.thisFile()) } else { None } - } + }; + file.and_then(|file| unsafe { + // SAFETY: + // - This points to a C++ FFI type ensured to be aligned by cxx's codegen. + // - The null-safe version is being used. + // - This points to a C++FFI type ensured to be valid by cxx's codegen. + // - There are no datapaths that will yield any mutable pointers or references + // to this, ensuring that it will not be mutated as per the aliasing rules. + file.as_ref() + }) } } impl bindings::BaseFile { pub fn audio_properties(&self) -> Option<&bindings::AudioProperties> { let props = unsafe { - let pinned_self = Pin::new_unchecked(self); - pinned_self.audioProperties() + // 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 audio properties data is a pointer that does not depend on the + // address of self. + let this: Pin<&bindings::BaseFile> = Pin::new_unchecked(self); + this.thisAudioProperties() }; unsafe { + // SAFETY: + // - This points to a C++ FFI type ensured to be aligned by cxx's codegen. + // - The null-safe version is being used. + // - This points to a C++FFI type ensured to be valid by cxx's codegen. + // - There are no datapaths that will yield any mutable pointers or references + // to this, ensuring that it will not be mutated as per the aliasing rules. props.as_ref() } } pub fn as_opus(&self) -> Option<&bindings::OpusFile> { + let ptr_self = self as *const Self; let opus_file = unsafe { - bindings::File_asOpus(self as *const Self) + // SAFETY: + // This FFI function will be a simple C++ dynamic_cast, which checks if + // the file can be cased down to an opus file. If the cast fails, a null + // pointer is returned, which will be handled by as_ref's null checking. + bindings::File_asOpus(ptr_self) }; unsafe { + // SAFETY: + // - This points to a C++ FFI type ensured to be aligned by cxx's codegen. + // - The null-safe version is being used. + // - This points to a C++FFI type ensured to be valid by cxx's codegen. + // - There are no datapaths that will yield any mutable pointers or references + // to this, ensuring that it will not be mutated as per the aliasing rules. opus_file.as_ref() } } pub fn as_vorbis(&self) -> Option<&bindings::VorbisFile> { + let ptr_self = self as *const Self; let vorbis_file = unsafe { - bindings::File_asVorbis(self as *const Self) + // SAFETY: + // This FFI function will be a simple C++ dynamic_cast, which checks if + // the file can be cased down to an opus file. If the cast fails, a null + // pointer is returned, which will be handled by as_ref's null checking. + bindings::File_asVorbis(ptr_self) }; unsafe { + // SAFETY: + // - This points to a C++ FFI type ensured to be aligned by cxx's codegen. + // - The null-safe version is being used. + // - This points to a C++FFI type ensured to be valid by cxx's codegen. + // - There are no datapaths that will yield any mutable pointers or references + // to this, ensuring that it will not be mutated as per the aliasing rules. vorbis_file.as_ref() } } } impl bindings::AudioProperties { - pub fn length_ms(&self) -> i32 { + pub fn length_in_milliseconds(&self) -> i32 { unsafe { - let pinned_self = Pin::new_unchecked(self); - pinned_self.lengthInMilliseconds() + // 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 copied and thus not dependent on the address of self. + let this = Pin::new_unchecked(self); + this.thisLengthInMilliseconds() } } - pub fn bitrate_kbps(&self) -> i32 { + pub fn bitrate(&self) -> i32 { unsafe { - let pinned_self = Pin::new_unchecked(self); - pinned_self.bitrate() + // 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 copied and thus not dependent on the address of self. + let this = Pin::new_unchecked(self); + this.thisBitrate() } } - pub fn sample_rate_hz(&self) -> i32 { + pub fn sample_rate(&self) -> i32 { unsafe { - let pinned_self = Pin::new_unchecked(self); - pinned_self.sampleRate() + // 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 copied and thus not dependent on the address of self. + let this = Pin::new_unchecked(self); + this.thisSampleRate() } } - pub fn channel_count(&self) -> i32 { + pub fn channels(&self) -> i32 { unsafe { - let pinned_self = Pin::new_unchecked(self); - pinned_self.channels() + // 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 copied and thus not dependent on the address of self. + let this = Pin::new_unchecked(self); + this.thisChannels() } } } @@ -210,11 +290,13 @@ impl bindings::AudioProperties { impl bindings::OpusFile { pub fn xiph_comments(&self) -> Option<&bindings::XiphComment> { let tag = unsafe { - // SAFETY: This will not exist beyond the scope of this function, - // and will only be used over ffi as a c++ this pointer (which is - // also pinned) - let pinned_self = Pin::new_unchecked(self); - pinned_self.opusTag() + // 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. + let this = Pin::new_unchecked(self); + this.opusThisTag() }; unsafe { // SAFETY: This pointer is a valid type, and can only used and accessed @@ -227,11 +309,13 @@ impl bindings::OpusFile { impl bindings::VorbisFile { pub fn xiph_comments(&self) -> Option<&bindings::XiphComment> { let tag = unsafe { - // SAFETY: This will not exist beyond the scope of this function, - // and will only be used over ffi as a c++ this pointer (which is - // also pinned) - let pinned_self = Pin::new_unchecked(self); - pinned_self.tag() + // 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. + let this = Pin::new_unchecked(self); + this.vorbisThisTag() }; unsafe { // SAFETY: This pointer is a valid type, and can only used and accessed @@ -244,11 +328,13 @@ impl bindings::VorbisFile { impl bindings::XiphComment { pub fn field_list_map(&self) -> &bindings::SimplePropertyMap { unsafe { - // SAFETY: This will not exist beyond the scope of this function, - // and will only be used over ffi as a c++ this pointer (which is - // also pinned) - let pinned_self = Pin::new_unchecked(self); - pinned_self.fieldListMap() + // 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 reference that does not depend on the address of self. + let this = Pin::new_unchecked(self); + this.thisFieldListMap() } } } @@ -256,11 +342,14 @@ impl bindings::XiphComment { impl bindings::SimplePropertyMap { pub fn to_hashmap(&self) -> HashMap> { let cxx_vec = unsafe { - // SAFETY: This will not exist beyond the scope of this function, - // and will only be used over ffi as a c++ this pointer (which is - // also pinned) - let pinned_self = Pin::new_unchecked(self); - bindings::SimplePropertyMap_to_vector(pinned_self) + // 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 unique_ptr to a copied vector that is not dependent + // on the address of self. + let this = Pin::new_unchecked(self); + bindings::SimplePropertyMap_to_vector(this) }; cxx_vec.iter().map(|property| property.to_tuple()).collect() } @@ -269,29 +358,41 @@ impl bindings::SimplePropertyMap { impl bindings::Property { pub fn to_tuple(&self) -> (String, Vec) { unsafe { - // SAFETY: This will not exist beyond the scope of this function, - // and will only be used over ffi as a c++ this pointer (which is - // also pinned) - let pinned_self = Pin::new_unchecked(self); - let key = pinned_self.key().to_string(); - let value = pinned_self.value().to_vec(); + // 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 values returned are copied and thus not dependent on the address + // of self. + let this = Pin::new_unchecked(self); + let key = this.thisKey().to_string(); + let value = this.thisValue().to_vec(); (key, value) } } } -impl ToString for bindings::TagString { +impl ToString for bindings::TString { fn to_string(&self) -> String { let c_str = unsafe { - // SAFETY: This will not exist beyond the scope of this function, - // and will only be used over ffi as a c++ this pointer (which is - // also pinned) - let this = Pin::new_unchecked(self); - this.toCString(true) + // 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 returned are pointers and thus not dependent on the address + // of self. + let this: Pin<&bindings::TString> = Pin::new_unchecked(self); + this.thisToCString(true) }; unsafe { - // SAFETY: This is an output from C++ with a null pointer - // by design. It will not be mutated and is instantly copied - // into rust. + // SAFETY: + // - This is a C-string returned by a C++ method guaranteed to have + // a null terminator. + // - This C-string is fully allocated and owned by the TagString instance, + // in a continous block from start to null terminator. + // - This C-string will be non-null even if empty. + // - This pointer will not be mutated before it's entirely copied into + // rust. + // - This C-string is copied to a rust string before TagString is destroyed. CStr::from_ptr(c_str) } .to_string_lossy() @@ -299,14 +400,17 @@ impl ToString for bindings::TagString { } } -impl bindings::StringList { +impl bindings::TStringList { pub fn to_vec(&self) -> Vec { let cxx_values = unsafe { - // SAFETY: This will not exist beyond the scope of this function, - // and will only be used over ffi as a c++ this pointer (which is - // also pinned) - let pinned_self = Pin::new_unchecked(self); - bindings::StringList_to_vector(pinned_self) + // 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 returned is a unique ptr to a copied vector that is not + // dependent on the address of self. + let this = Pin::new_unchecked(self); + bindings::StringList_to_vector(this) }; cxx_values.iter().map(|value| value.to_string()).collect() } diff --git a/musikr/src/main/jni/src/taglib/mod.rs b/musikr/src/main/jni/src/taglib/mod.rs index 3156e0889..41fe95181 100644 --- a/musikr/src/main/jni/src/taglib/mod.rs +++ b/musikr/src/main/jni/src/taglib/mod.rs @@ -75,10 +75,10 @@ impl FileRef { // Extract data from C++ objects let file = file_ref.file_or().and_then(|file| { let audio_properties = file.audio_properties().map(|props| AudioProperties { - length_in_milliseconds: props.length_ms(), - bitrate_in_kilobits_per_second: props.bitrate_kbps(), - sample_rate_in_hz: props.sample_rate_hz(), - number_of_channels: props.channel_count(), + length_in_milliseconds: props.length_in_milliseconds(), + bitrate_in_kilobits_per_second: props.bitrate(), + sample_rate_in_hz: props.sample_rate(), + number_of_channels: props.channels(), }); if let Some(vorbis_file) = file.as_vorbis() {