From bae6e605d13ac4d54137b1db305e6d695ab27ffa Mon Sep 17 00:00:00 2001 From: Antonis Kalou Date: Mon, 26 Mar 2018 18:37:47 +0200 Subject: [PATCH 1/2] Remove unused reference to file The Tag and AudioProperties structs were keeping a reference to the file object, making it hard to use them in isolation. For example doing the following complains about `f` not living long enough: ```rust taglib::File::new(path).and_then(|f| f.tag()) ``` --- Cargo.toml | 2 +- src/lib.rs | 27 +++++++++++---------------- 2 files changed, 12 insertions(+), 17 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 5b94889..d4f00c3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -14,7 +14,7 @@ name = "taglib" path = "src/lib.rs" [dependencies] -libc = "0.1" +libc = "0.2" [dependencies.taglib-sys] path = "taglib-sys" diff --git a/src/lib.rs b/src/lib.rs index 9d76868..e65e078 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -32,8 +32,7 @@ use sys as ll; fn c_str_to_str(c_str: *const c_char) -> String { if c_str.is_null() { String::new() - } - else { + } else { let bytes = unsafe { CStr::from_ptr(c_str).to_bytes() }; String::from_utf8_lossy(bytes).to_string() } @@ -49,9 +48,8 @@ pub struct File { /// Each `Tag` instance can only be created by the `taglib::File::tag()` /// method. #[allow(dead_code)] -pub struct Tag<'a> { - raw: *mut ll::TagLib_Tag, - file: &'a File, +pub struct Tag { + raw: *mut ll::TagLib_Tag } /// Common audio file properties. @@ -59,12 +57,11 @@ pub struct Tag<'a> { /// Instances of `AudioProperties` can only be created through the /// `taglib::File::audioproperties()` method. #[allow(dead_code)] -pub struct AudioProperties<'a> { +pub struct AudioProperties { raw: *const ll::TagLib_AudioProperties, - file: &'a File, } -impl<'a> Tag<'a> { +impl Tag { /// Returns the track name, or an empty string if no track name is present. pub fn title(&self) -> String { let res = unsafe { ll::taglib_tag_title(self.raw) }; @@ -152,7 +149,7 @@ impl<'a> Tag<'a> { } } -impl<'a> AudioProperties<'a> { +impl AudioProperties { /// Returns the length, in seconds, of the track. pub fn length(&self) -> u32 { unsafe { ll::taglib_audioproperties_length(self.raw) as u32 } @@ -227,7 +224,7 @@ impl File { Ok(s) => s, _ => return Err(FileError::InvalidFileName) }; - + let filename_c_ptr = filename_c.as_ptr(); let f = unsafe { ll::taglib_file_new(filename_c_ptr) }; @@ -261,9 +258,8 @@ impl File { if res.is_null() { Err(FileError::NoAvailableTag) - } - else { - Ok(Tag { raw: res, file: self }) + } else { + Ok(Tag { raw: res }) } } @@ -278,9 +274,8 @@ impl File { if res.is_null() { Err(FileError::NoAvailableAudioProperties) - } - else { - Ok(AudioProperties { raw: res, file: self }) + } else { + Ok(AudioProperties { raw: res }) } } From 885a5b003416ef01c538f4360375fbbc2a81b3d9 Mon Sep 17 00:00:00 2001 From: Antonis Kalou Date: Tue, 27 Mar 2018 20:55:58 +0200 Subject: [PATCH 2/2] Store tag and property content directly in struct Previously a handle to `File` was kept around in `Tag` and `AudioProperties` so that the raw file pointer wouldn't be dropped while it was being used. This made it difficult to just pass the tag around as a user of the API, due to having to keep the `file` object alive as well. For example, this would result in a lifetime error because the file object does not live long enough: ``` taglib::File::new(path).and_then(|f| f.tag()) ``` In this change the `Tag` and `AudioProperties` are created when request from the file, E.g. `file.tag()` and there is no raw pointers being passed around. If the user wishes to save new tags they pass the tag struct in to the `save` function directly instead (`AudioProperties` can't be changed). --- examples/tagreader.rs | 24 ++--- src/lib.rs | 228 +++++++++++++++++------------------------- 2 files changed, 104 insertions(+), 148 deletions(-) diff --git a/examples/tagreader.rs b/examples/tagreader.rs index bda8ca9..029df79 100644 --- a/examples/tagreader.rs +++ b/examples/tagreader.rs @@ -21,13 +21,13 @@ pub fn main() { match file.tag() { Ok(t) => { println!("-- TAG --"); - println!("title - {}", t.title()); - println!("artist - {}", t.artist()); - println!("album - {}", t.album()); - println!("year - {}", t.year()); - println!("comment - {}", t.comment()); - println!("track - {}", t.track()); - println!("genre - {}", t.genre()); + println!("title - {}", t.title.unwrap_or_default()); + println!("artist - {}", t.artist.unwrap_or_default()); + println!("album - {}", t.album.unwrap_or_default()); + println!("comment - {}", t.comment.unwrap_or_default()); + println!("genre - {}", t.genre.unwrap_or_default()); + println!("year - {}", t.year.unwrap_or(0)); + println!("track - {}", t.track.unwrap_or(0)); }, Err(e) => { println!("No available tags for {} (error: {:?})", arg, e); @@ -36,13 +36,13 @@ pub fn main() { match file.audioproperties() { Ok(p) => { - let secs = p.length() % 60; - let mins = (p.length() - secs) / 60; + let secs = p.length % 60; + let mins = (p.length - secs) / 60; println!("-- AUDIO --"); - println!("bitrate - {}", p.bitrate()); - println!("sample rate - {}", p.samplerate()); - println!("channels - {}", p.channels()); + println!("bitrate - {}", p.bitrate); + println!("sample rate - {}", p.samplerate); + println!("channels - {}", p.channels); println!("length - {}m:{}s", mins, secs); }, Err(e) => { diff --git a/src/lib.rs b/src/lib.rs index e65e078..106ba28 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -29,148 +29,89 @@ use std::ffi::{CString, CStr}; use sys as ll; -fn c_str_to_str(c_str: *const c_char) -> String { +fn c_str_to_str(c_str: *const c_char) -> Option { if c_str.is_null() { - String::new() + return None; + } + + let bytes = unsafe { CStr::from_ptr(c_str).to_bytes() }; + if bytes.is_empty() { + None } else { - let bytes = unsafe { CStr::from_ptr(c_str).to_bytes() }; - String::from_utf8_lossy(bytes).to_string() + Some(String::from_utf8_lossy(bytes).to_string()) } } +fn u32_to_option(n: u32) -> Option { + if n == 0 { None } else { Some(n) } +} + +fn str_to_c_str(s: Option) -> *const c_char { + let s = s.unwrap_or("".into()); + + CString::new(s.as_str()).unwrap().as_ptr() +} + /// A representation of an audio file, with meta-data and properties. pub struct File { raw: *mut ll::TagLib_File, } -/// The abstract meta-data container for audio files -/// -/// Each `Tag` instance can only be created by the `taglib::File::tag()` -/// method. -#[allow(dead_code)] +/// Represents audio tag metadata. pub struct Tag { - raw: *mut ll::TagLib_Tag -} - -/// Common audio file properties. -/// -/// Instances of `AudioProperties` can only be created through the -/// `taglib::File::audioproperties()` method. -#[allow(dead_code)] -pub struct AudioProperties { - raw: *const ll::TagLib_AudioProperties, + /// The title of the track, if available. + pub title: Option, + /// The album name, if available. + pub album: Option, + /// The artist name, if available. + pub artist: Option, + /// An additional comment, if available. + pub comment: Option, + /// The genre, if any. + pub genre: Option, + /// The year the track was created, if available. + pub year: Option, + /// The track number, if available. + pub track: Option, } impl Tag { - /// Returns the track name, or an empty string if no track name is present. - pub fn title(&self) -> String { - let res = unsafe { ll::taglib_tag_title(self.raw) }; - c_str_to_str(res) - } - - /// Sets the track name. - pub fn set_title(&mut self, title: &str) { - let cs = CString::new(title).unwrap(); - let s = cs.as_ptr(); - unsafe { ll::taglib_tag_set_title(self.raw, s); } - } - - /// Returns the artist name, or an empty string if no artist name is present. - pub fn artist(&self) -> String { - let res = unsafe { ll::taglib_tag_artist(self.raw) }; - c_str_to_str(res) - } - - /// Sets the artist name. - pub fn set_artist(&mut self, artist: &str) { - let cs = CString::new(artist).unwrap(); - let s = cs.as_ptr(); - unsafe { ll::taglib_tag_set_artist(self.raw, s); } - } - - /// Returns the album name, or an empty string if no album name is present. - pub fn album(&self) -> String { - let res = unsafe { ll::taglib_tag_album(self.raw) }; - c_str_to_str(res) - } - - /// Sets the album name. - pub fn set_album(&mut self, album: &str) { - let cs = CString::new(album).unwrap(); - let s = cs.as_ptr(); - unsafe { ll::taglib_tag_set_album(self.raw, s); } - } - - /// Returns the track comment, or an empty string if no track comment is - /// present. - pub fn comment(&self) -> String { - let res = unsafe { ll::taglib_tag_comment(self.raw) }; - c_str_to_str(res) - } - - /// Sets the track comment. - pub fn set_comment(&mut self, comment: &str) { - let cs = CString::new(comment).unwrap(); - let s = cs.as_ptr(); - unsafe { ll::taglib_tag_set_comment(self.raw, s); } - } - - /// Returns the genre name, or an empty string if no genre name is present. - pub fn genre(&self) -> String { - let res = unsafe { ll::taglib_tag_genre(self.raw) }; - c_str_to_str(res) - } - - /// Sets the genre name. - pub fn set_genre(&mut self, genre: &str) { - let cs = CString::new(genre).unwrap(); - let s = cs.as_ptr(); - unsafe { ll::taglib_tag_set_genre(self.raw, s); } - } - - /// Returns the year, or 0 if no year is present. - pub fn year(&self) -> u32 { - unsafe { ll::taglib_tag_year(self.raw) as u32 } - } - - /// Sets the year. - pub fn set_year(&mut self, year: u32) { - unsafe { ll::taglib_tag_set_year(self.raw, year); } - } - - /// Returns the track number, or 0 if no track number is present. - pub fn track(&self) -> u32 { - unsafe { ll::taglib_tag_track(self.raw) as u32 } + unsafe fn new(raw: *const ll::TagLib_Tag) -> Tag { + Tag { + title: c_str_to_str(ll::taglib_tag_title(raw)), + album: c_str_to_str(ll::taglib_tag_album(raw)), + artist: c_str_to_str(ll::taglib_tag_artist(raw)), + comment: c_str_to_str(ll::taglib_tag_comment(raw)), + genre: c_str_to_str(ll::taglib_tag_genre(raw)), + year: u32_to_option(ll::taglib_tag_year(raw) as u32), + track: u32_to_option(ll::taglib_tag_track(raw) as u32) + } } +} - /// Sets the track number. - pub fn set_track(&mut self, track: u32) { - unsafe { ll::taglib_tag_set_track(self.raw, track); } - } +/// Common audio file properties. +pub struct AudioProperties { + /// The length, in seconds, of the track. + pub length: u32, + /// The most appropriate bit rate for the track, in KB/s. + /// For constant bit rate formats, the value is the bit rate of the file; + /// for variable bit rate formats this is either the average or the nominal + /// bit rate. + pub bitrate: u32, + /// The sample rate in Hz. + pub samplerate: u32, + /// The number of audio channels. + pub channels: u32, } impl AudioProperties { - /// Returns the length, in seconds, of the track. - pub fn length(&self) -> u32 { - unsafe { ll::taglib_audioproperties_length(self.raw) as u32 } - } - - /// Returns the most appropriate bit rate for the track, in kB/s. - /// For constant bit rate formats, the returned value is the bit - /// rate of the file; for variable bit rate formats this is either - /// the average or the nominal bit rate. - pub fn bitrate(&self) -> u32 { - unsafe { ll::taglib_audioproperties_bitrate(self.raw) as u32 } - } - - /// Returns the sample rate, in Hz. - pub fn samplerate(&self) -> u32 { - unsafe { ll::taglib_audioproperties_samplerate(self.raw) as u32 } - } - - /// Returns the number of audio channels. - pub fn channels(&self) -> u32 { - unsafe { ll::taglib_audioproperties_channels(self.raw) as u32 } + unsafe fn new(raw: *const ll::TagLib_AudioProperties) -> AudioProperties { + AudioProperties { + length: ll::taglib_audioproperties_length(raw) as u32, + bitrate: ll::taglib_audioproperties_bitrate(raw) as u32, + samplerate: ll::taglib_audioproperties_samplerate(raw) as u32, + channels: ll::taglib_audioproperties_channels(raw) as u32, + } } } @@ -252,35 +193,50 @@ impl File { Ok(File { raw: f }) } + /// Returns whether the file is valid. + pub fn is_valid(&self) -> bool { + unsafe { ll::taglib_file_is_valid(self.raw) != 0 } + } + /// Returns the `taglib::Tag` instance for the given file. pub fn tag(&self) -> Result { - let res = unsafe { ll::taglib_file_tag(self.raw) }; + let raw = unsafe { ll::taglib_file_tag(self.raw) }; - if res.is_null() { + if raw.is_null() { Err(FileError::NoAvailableTag) } else { - Ok(Tag { raw: res }) + let tag = unsafe { Tag::new(raw) }; + Ok(tag) } } - /// Returns whether the file is valid. - pub fn is_valid(&self) -> bool { - unsafe { ll::taglib_file_is_valid(self.raw) != 0 } - } /// Returns the `taglib::AudioProperties` instance for the given file. pub fn audioproperties(&self) -> Result { - let res = unsafe { ll::taglib_file_audioproperties(self.raw) }; + let raw = unsafe { ll::taglib_file_audioproperties(self.raw) }; - if res.is_null() { + if raw.is_null() { Err(FileError::NoAvailableAudioProperties) } else { - Ok(AudioProperties { raw: res }) + let props = unsafe { AudioProperties::new(raw) }; + Ok(props) } } - /// Updates the meta-data of the file. - pub fn save(&self) -> bool { - unsafe { ll::taglib_file_save(self.raw) != 0 } + /// Write the given tag to the audio file. + pub fn save(&self, tag: Tag) -> bool { + unsafe { + let raw_tag = ll::taglib_file_tag(self.raw); + // if the user managed to get a tag to pass in then this should work + assert!(!raw_tag.is_null()); + ll::taglib_tag_set_title(raw_tag, str_to_c_str(tag.title)); + ll::taglib_tag_set_album(raw_tag, str_to_c_str(tag.album)); + ll::taglib_tag_set_artist(raw_tag, str_to_c_str(tag.artist)); + ll::taglib_tag_set_genre(raw_tag, str_to_c_str(tag.genre)); + ll::taglib_tag_set_comment(raw_tag, str_to_c_str(tag.comment)); + ll::taglib_tag_set_year(raw_tag, tag.year.unwrap_or(0)); + ll::taglib_tag_set_track(raw_tag, tag.track.unwrap_or(0)); + ll::taglib_file_save(self.raw) != 0 + } } }