From f3f349847a53105a5470ec58b98bbff0ec5e5b2d Mon Sep 17 00:00:00 2001 From: Alexander Capehart Date: Wed, 12 Feb 2025 19:14:25 -0700 Subject: [PATCH] Revert "musikr: use no_std in crate" This reverts commit 7f84349f2e4efaf9f2fc77d16ad3aa259ea024b4. --- musikr/src/main/jni/Cargo.lock | 1 - musikr/src/main/jni/Cargo.toml | 10 +- musikr/src/main/jni/shim/iostream_shim.cpp | 18 ++-- musikr/src/main/jni/src/jni_stream.rs | 77 +++++++++----- musikr/src/main/jni/src/lib.rs | 10 +- musikr/src/main/jni/src/taglib/ffi.rs | 13 +-- musikr/src/main/jni/src/taglib/mod.rs | 113 +++++++++------------ musikr/src/main/jni/src/taglib/stream.rs | 59 ++++++++--- 8 files changed, 160 insertions(+), 141 deletions(-) diff --git a/musikr/src/main/jni/Cargo.lock b/musikr/src/main/jni/Cargo.lock index 22745bd0e..a899bbbfc 100644 --- a/musikr/src/main/jni/Cargo.lock +++ b/musikr/src/main/jni/Cargo.lock @@ -196,7 +196,6 @@ dependencies = [ "cxx", "cxx-build", "jni", - "link-cplusplus", ] [[package]] diff --git a/musikr/src/main/jni/Cargo.toml b/musikr/src/main/jni/Cargo.toml index 15a10d8d0..9cfda0d2b 100644 --- a/musikr/src/main/jni/Cargo.toml +++ b/musikr/src/main/jni/Cargo.toml @@ -8,14 +8,8 @@ name = "metadatajni" crate-type = ["cdylib"] [dependencies] -link-cplusplus = { version = "1.0.9", features = ["nothing"] } -cxx = { version = "1.0.137", default-features = false, features = ["alloc"] } -jni = { version = "0.21.1", default-features = false } +cxx = "1.0.137" +jni = "0.21.1" [build-dependencies] cxx-build = "1.0.137" - -[profile.release] -lto = true -codegen-units = 1 -panic = "abort" diff --git a/musikr/src/main/jni/shim/iostream_shim.cpp b/musikr/src/main/jni/shim/iostream_shim.cpp index 9d0051d9b..c4bcd392c 100644 --- a/musikr/src/main/jni/shim/iostream_shim.cpp +++ b/musikr/src/main/jni/shim/iostream_shim.cpp @@ -8,12 +8,12 @@ extern "C" { const char *rust_stream_name(const void *stream); size_t rust_stream_read(void *stream, uint8_t *buffer, size_t length); - // void rust_stream_write(void *stream, const uint8_t *data, size_t length); + void rust_stream_write(void *stream, const uint8_t *data, size_t length); void rust_stream_seek(void *stream, int64_t offset, int32_t whence); - // void rust_stream_truncate(void *stream, int64_t length); + void rust_stream_truncate(void *stream, int64_t length); int64_t rust_stream_tell(const void *stream); int64_t rust_stream_length(const void *stream); - // bool rust_stream_is_readonly(const void *stream); + bool rust_stream_is_readonly(const void *stream); } namespace taglib_shim @@ -47,9 +47,11 @@ namespace taglib_shim return TagLib::ByteVector(reinterpret_cast(buffer.data()), bytes_read); } - void RustIOStream::writeBlock(const TagLib::ByteVector &_) + void RustIOStream::writeBlock(const TagLib::ByteVector &data) { - throw std::runtime_error("Stream is read-only"); + rust_stream_write(rust_stream, + reinterpret_cast(data.data()), + data.size()); } void RustIOStream::insert(const TagLib::ByteVector &data, TagLib::offset_t start, size_t replace) @@ -125,9 +127,9 @@ namespace taglib_shim seek(0); } - void RustIOStream::truncate(TagLib::offset_t _) + void RustIOStream::truncate(TagLib::offset_t length) { - throw std::runtime_error("Stream is read-only"); + rust_stream_truncate(rust_stream, length); } TagLib::offset_t RustIOStream::tell() const @@ -142,7 +144,7 @@ namespace taglib_shim bool RustIOStream::readOnly() const { - return true; + return rust_stream_is_readonly(rust_stream); } bool RustIOStream::isOpen() const diff --git a/musikr/src/main/jni/src/jni_stream.rs b/musikr/src/main/jni/src/jni_stream.rs index e82644afe..5077a2bcb 100644 --- a/musikr/src/main/jni/src/jni_stream.rs +++ b/musikr/src/main/jni/src/jni_stream.rs @@ -1,7 +1,7 @@ -use crate::taglib::{IOStream, Whence}; +use crate::taglib::TagLibStream; use jni::objects::{JObject, JValue}; use jni::JNIEnv; -use alloc::string::String; +use std::io::{Read, Seek, SeekFrom, Write}; pub struct JInputStream<'local, 'a> { env: &'a mut JNIEnv<'local>, @@ -20,7 +20,7 @@ impl<'local, 'a> JInputStream<'local, 'a> { } } -impl<'local, 'a> IOStream for JInputStream<'local, 'a> { +impl<'local, 'a> TagLibStream for JInputStream<'local, 'a> { fn name(&mut self) -> String { // Call the Java name() method safely let name = self @@ -35,12 +35,18 @@ impl<'local, 'a> IOStream for JInputStream<'local, 'a> { .into() } - fn read(&mut self, buf: &mut [u8]) -> Result { + fn is_readonly(&self) -> bool { + true // JInputStream is always read-only + } +} + +impl<'local, 'a> Read for JInputStream<'local, 'a> { + fn read(&mut self, buf: &mut [u8]) -> std::io::Result { // Create a direct ByteBuffer from the Rust slice let byte_buffer = unsafe { self.env .new_direct_byte_buffer(buf.as_mut_ptr(), buf.len()) - .map_err(|_| String::from("Failed to create byte buffer"))? + .map_err(|e| std::io::Error::new(std::io::ErrorKind::Other, e.to_string()))? }; // Call readBlock safely @@ -53,20 +59,38 @@ impl<'local, 'a> IOStream for JInputStream<'local, 'a> { &[JValue::Object(&byte_buffer)], ) .and_then(|result| result.z()) - .map_err(|_| String::from("Failed to read block"))?; + .map_err(|e| std::io::Error::new(std::io::ErrorKind::Other, e.to_string()))?; if !success { - return Err(String::from("Failed to read block")); + return Err(std::io::Error::new( + std::io::ErrorKind::Other, + "Failed to read block", + )); } Ok(buf.len()) } +} - fn seek(&mut self, pos: Whence) -> Result<(), String> { +impl<'local, 'a> Write for JInputStream<'local, 'a> { + fn write(&mut self, _buf: &[u8]) -> std::io::Result { + Err(std::io::Error::new( + std::io::ErrorKind::PermissionDenied, + "JInputStream is read-only", + )) + } + + fn flush(&mut self) -> std::io::Result<()> { + Ok(()) // Nothing to flush in a read-only stream + } +} + +impl<'local, 'a> Seek for JInputStream<'local, 'a> { + fn seek(&mut self, pos: SeekFrom) -> std::io::Result { let (method, offset) = match pos { - Whence::Start(offset) => ("seekFromBeginning", offset as i64), - Whence::Current(offset) => ("seekFromCurrent", offset), - Whence::End(offset) => ("seekFromEnd", offset), + SeekFrom::Start(offset) => ("seekFromBeginning", offset as i64), + SeekFrom::Current(offset) => ("seekFromCurrent", offset), + SeekFrom::End(offset) => ("seekFromEnd", offset), }; // Call the appropriate seek method safely @@ -74,28 +98,29 @@ impl<'local, 'a> IOStream for JInputStream<'local, 'a> { .env .call_method(&self.input, method, "(J)Z", &[JValue::Long(offset)]) .and_then(|result| result.z()) - .map_err(|e| String::from("Failed to seek"))?; + .map_err(|e| std::io::Error::new(std::io::ErrorKind::Other, e.to_string()))?; if !success { - return Err(String::from("Failed to seek")); + return Err(std::io::Error::new( + std::io::ErrorKind::Other, + "Failed to seek", + )); } - Ok(()) - } - - fn tell(&mut self) -> Result { - self + // Return current position safely + let position = self .env .call_method(&self.input, "tell", "()J", &[]) .and_then(|result| result.j()) - .map_err(|_| String::from("Failed to get position")) - } + .map_err(|e| std::io::Error::new(std::io::ErrorKind::Other, e.to_string()))?; - fn length(&mut self) -> Result { - self - .env - .call_method(&self.input, "length", "()J", &[]) - .and_then(|result| result.j()) - .map_err(|_| String::from("Failed to get length")) + if position == i64::MIN { + return Err(std::io::Error::new( + std::io::ErrorKind::Other, + "Failed to get position", + )); + } + + Ok(position as u64) } } diff --git a/musikr/src/main/jni/src/lib.rs b/musikr/src/main/jni/src/lib.rs index 37028ec3e..f6378b13a 100644 --- a/musikr/src/main/jni/src/lib.rs +++ b/musikr/src/main/jni/src/lib.rs @@ -1,7 +1,3 @@ -#![no_std] -extern crate core; -extern crate alloc; - use jni::objects::{JClass, JObject}; use jni::sys::jstring; use jni::JNIEnv; @@ -12,10 +8,6 @@ mod jni_stream; pub use taglib::*; use jni_stream::JInputStream; -use alloc::string::String; - -type Map = alloc::collections::BTreeMap; - #[no_mangle] pub extern "C" fn Java_org_oxycblt_musikr_metadata_MetadataJNI_openFile<'local>( mut env: JNIEnv<'local>, @@ -26,7 +18,7 @@ pub extern "C" fn Java_org_oxycblt_musikr_metadata_MetadataJNI_openFile<'local>( let stream = match JInputStream::new(&mut env, input) { Ok(stream) => stream, Err(e) => { - let error = String::from("Failed to create input stream"); + let error = format!("Failed to create input stream: {}", e); let error_str = env.new_string(error).expect("Couldn't create error string!"); return error_str.into_raw(); } diff --git a/musikr/src/main/jni/src/taglib/ffi.rs b/musikr/src/main/jni/src/taglib/ffi.rs index 311339523..65a649558 100644 --- a/musikr/src/main/jni/src/taglib/ffi.rs +++ b/musikr/src/main/jni/src/taglib/ffi.rs @@ -1,9 +1,7 @@ -use core::ffi::CStr; -use core::pin::Pin; - -use alloc::collections::BTreeMap; -use alloc::string::{String, ToString}; -use alloc::vec::Vec; +use std::ffi::CStr; +use std::pin::Pin; +use std::string::ToString; +use std::collections::HashMap; #[cxx::bridge] pub(crate) mod bindings { @@ -342,7 +340,7 @@ impl bindings::XiphComment { } impl bindings::SimplePropertyMap { - pub fn to_hashmap(&self) -> BTreeMap> { + pub fn to_hashmap(&self) -> HashMap> { let cxx_vec = unsafe { // SAFETY: // - This pin is only used in this unsafe scope. @@ -373,7 +371,6 @@ impl bindings::Property { } } } - impl ToString for bindings::TString { fn to_string(&self) -> String { let c_str = unsafe { diff --git a/musikr/src/main/jni/src/taglib/mod.rs b/musikr/src/main/jni/src/taglib/mod.rs index e960b4928..41fe95181 100644 --- a/musikr/src/main/jni/src/taglib/mod.rs +++ b/musikr/src/main/jni/src/taglib/mod.rs @@ -2,12 +2,52 @@ mod ffi; mod stream; use ffi::bindings; -use alloc::boxed::Box; -use alloc::string::String; -use alloc::collections::BTreeMap; -use alloc::vec::Vec; -pub use stream::RustStream; +use std::collections::HashMap; +pub use stream::{RustStream, TagLibStream}; +type XiphComments = HashMap>; + +pub enum File { + Unknown { + audio_properties: Option, + }, + MP3 { + audio_properties: Option, + }, + FLAC { + audio_properties: Option, + xiph_comments: Option, + }, + MP4 { + audio_properties: Option, + }, + OGG { + audio_properties: Option, + xiph_comments: Option, + }, + Opus { + audio_properties: Option, + xiph_comments: Option, + }, + WAV { + audio_properties: Option, + }, + WavPack { + audio_properties: Option, + }, + APE { + audio_properties: Option, + }, +} + +/// Audio properties of a media file +#[derive(Default)] +pub struct AudioProperties { + pub length_in_milliseconds: i32, + pub bitrate_in_kilobits_per_second: i32, + pub sample_rate_in_hz: i32, + pub number_of_channels: i32, +} // Safe wrapper for FileRef that owns extracted data pub struct FileRef { @@ -16,7 +56,7 @@ pub struct FileRef { impl FileRef { /// Create a new FileRef from a stream implementing TagLibStream - pub fn from_stream<'a, T: IOStream + 'a>(stream: T) -> Option { + pub fn from_stream<'a, T: TagLibStream + 'a>(stream: T) -> Option { // Create the RustStream wrapper let rust_stream = stream::RustStream::new(stream); @@ -74,64 +114,3 @@ impl FileRef { &self.file } } - -// Trait that must be implemented by Rust streams to be used with TagLib -pub trait IOStream { - fn name(&mut self) -> String; - fn read(&mut self, buf: &mut [u8]) -> Result; - fn seek(&mut self, whence: Whence) -> Result<(), String>; - fn tell(&mut self) -> Result; - fn length(&mut self) -> Result; - // No tag editing support, only doing read-only streams for now - // fn write(&mut self, _buf: &[u8]) -> std::io::Result; -} - -pub enum Whence { - Start(i64), - Current(i64), - End(i64), -} - -/// Audio properties of a media file -#[derive(Default)] -pub struct AudioProperties { - pub length_in_milliseconds: i32, - pub bitrate_in_kilobits_per_second: i32, - pub sample_rate_in_hz: i32, - pub number_of_channels: i32, -} - -type XiphComments = BTreeMap>; - -pub enum File { - Unknown { - audio_properties: Option, - }, - MP3 { - audio_properties: Option, - }, - FLAC { - audio_properties: Option, - xiph_comments: Option, - }, - MP4 { - audio_properties: Option, - }, - OGG { - audio_properties: Option, - xiph_comments: Option, - }, - Opus { - audio_properties: Option, - xiph_comments: Option, - }, - WAV { - audio_properties: Option, - }, - WavPack { - audio_properties: Option, - }, - APE { - audio_properties: Option, - }, -} diff --git a/musikr/src/main/jni/src/taglib/stream.rs b/musikr/src/main/jni/src/taglib/stream.rs index ecd37c6b0..80b6b1366 100644 --- a/musikr/src/main/jni/src/taglib/stream.rs +++ b/musikr/src/main/jni/src/taglib/stream.rs @@ -1,15 +1,19 @@ -use core::ffi::{c_void, c_char}; -use alloc::ffi::CString; -use alloc::boxed::Box; -use alloc::slice; -use super::{IOStream, Whence}; +use std::ffi::{c_void, CString}; +use std::io::{Read, Seek, SeekFrom, Write}; +use std::os::raw::c_char; + +// Trait that must be implemented by Rust streams to be used with TagLib +pub trait TagLibStream: Read + Write + Seek { + fn name(&mut self) -> String; + fn is_readonly(&self) -> bool; +} // Opaque type for C++ #[repr(C)] -pub struct RustStream<'a>(Box); +pub struct RustStream<'a>(Box); impl<'a> RustStream<'a> { - pub fn new(stream: T) -> Self { + pub fn new(stream: T) -> Self { RustStream(Box::new(stream)) } } @@ -30,10 +34,21 @@ pub extern "C" fn rust_stream_read( length: usize, ) -> usize { let stream = unsafe { &mut *(stream as *mut RustStream<'_>) }; - let buffer = unsafe { slice::from_raw_parts_mut(buffer, length) }; + let buffer = unsafe { std::slice::from_raw_parts_mut(buffer, length) }; stream.0.read(buffer).unwrap_or(0) } +#[no_mangle] +pub extern "C" fn rust_stream_write( + stream: *mut c_void, + data: *const u8, + length: usize, +) { + let stream = unsafe { &mut *(stream as *mut RustStream<'_>) }; + let data = unsafe { std::slice::from_raw_parts(data, length) }; + stream.0.write_all(data).unwrap(); +} + #[no_mangle] pub extern "C" fn rust_stream_seek( stream: *mut c_void, @@ -42,22 +57,38 @@ pub extern "C" fn rust_stream_seek( ) { let stream = unsafe { &mut *(stream as *mut RustStream<'_>) }; let pos = match whence { - 0 => Whence::Start(offset), - 1 => Whence::Current(offset), - 2 => Whence::End(offset), + 0 => SeekFrom::Start(offset as u64), + 1 => SeekFrom::Current(offset), + 2 => SeekFrom::End(offset), _ => panic!("Invalid seek whence"), }; stream.0.seek(pos).unwrap(); } +#[no_mangle] +pub extern "C" fn rust_stream_truncate(stream: *mut c_void, length: i64) { + let stream = unsafe { &mut *(stream as *mut RustStream<'_>) }; + stream.0.seek(SeekFrom::Start(length as u64)).unwrap(); + // TODO: Actually implement truncate once we have a better trait bound +} + #[no_mangle] pub extern "C" fn rust_stream_tell(stream: *mut c_void) -> i64 { - let stream: &mut RustStream<'_> = unsafe { &mut *(stream as *mut RustStream<'_>) }; - stream.0.tell().unwrap() + let stream = unsafe { &mut *(stream as *mut RustStream<'_>) }; + stream.0.seek(SeekFrom::Current(0)).unwrap() as i64 } #[no_mangle] pub extern "C" fn rust_stream_length(stream: *mut c_void) -> i64 { let stream = unsafe { &mut *(stream as *mut RustStream<'_>) }; - stream.0.length().unwrap() + let current = stream.0.seek(SeekFrom::Current(0)).unwrap(); + let end = stream.0.seek(SeekFrom::End(0)).unwrap(); + stream.0.seek(SeekFrom::Start(current)).unwrap(); + end as i64 } + +#[no_mangle] +pub extern "C" fn rust_stream_is_readonly(stream: *const c_void) -> bool { + let stream = unsafe { &*(stream as *const RustStream<'_>) }; + stream.0.is_readonly() +} \ No newline at end of file