[vlc-devel] [RFC: PATCH 2/2] stream filter: Add rust based cuesheet module

Alexandre Janniaux ajanni at videolabs.io
Fri Sep 11 03:20:45 CEST 2020


Hi,

Thank you for submitting this work!

Here are a few review points on the Rust part.

At the same time, I have a bunch of questions, some of which
are also in the comments below:

 - how do we run the tests in the CI? Is it planned to
   integrate with automake?
 - do we need cbindgen to compile the module?
 - can the cbindgen file be mutualized between plugins?
 - since we don't have Rust code yet, maybe the code can
   be formatted by software before following ten differents
   convention, so that anyone can use their preferred one
   locally?

On Thu, Sep 10, 2020 at 10:17:30PM +0530, Kartik Ohri wrote:
> The module is written using Rust API and serves as
> a proof of concept for the same.
> ---
>  modules/demux/Makefile.am                     |  26 +++
>  modules/demux/playlist/cuesheet.c             | 179 ++++++++++++++++++
>  modules/demux/playlist/cuesheet/Cargo.toml    |  13 ++
>  modules/demux/playlist/cuesheet/cbindgen.toml |  16 ++
>  modules/demux/playlist/cuesheet/src/capi.rs   |  96 ++++++++++
>  modules/demux/playlist/cuesheet/src/config.rs | 103 ++++++++++
>  modules/demux/playlist/cuesheet/src/lib.rs    |  18 ++
>  modules/demux/playlist/cuesheet/src/parse.rs  |  75 ++++++++
>  8 files changed, 526 insertions(+)
>  create mode 100644 modules/demux/playlist/cuesheet.c
>  create mode 100644 modules/demux/playlist/cuesheet/Cargo.toml
>  create mode 100644 modules/demux/playlist/cuesheet/cbindgen.toml
>  create mode 100644 modules/demux/playlist/cuesheet/src/capi.rs
>  create mode 100644 modules/demux/playlist/cuesheet/src/config.rs
>  create mode 100644 modules/demux/playlist/cuesheet/src/lib.rs
>  create mode 100644 modules/demux/playlist/cuesheet/src/parse.rs
>
> diff --git a/modules/demux/Makefile.am b/modules/demux/Makefile.am
> index 675b2d10af..31591a768b 100644
> --- a/modules/demux/Makefile.am
> +++ b/modules/demux/Makefile.am
> @@ -1,3 +1,4 @@
> +
>  demuxdir = $(pluginsdir)/demux
>  demux_LTLIBRARIES =
>
> @@ -256,6 +257,31 @@ libplaylist_plugin_la_SOURCES = \
>  	demux/playlist/playlist.c demux/playlist/playlist.h
>  demux_LTLIBRARIES += libplaylist_plugin.la
>
> +if BUILD_RUST
> +CARGO_C = cargo capi install --release
> +RUST_RUNTIME_LIBS = -ldl -lrt -lpthread -lgcc_s -lc -lm -lrt -lpthread -lutil
> +libcuesheet_rust_plugin_la_SOURCES = \
> +	demux/playlist/cuesheet/src/capi.rs \
> +	demux/playlist/cuesheet/src/config.rs \
> +	demux/playlist/cuesheet/src/lib.rs \
> +	demux/playlist/cuesheet/src/parse.rs
> +
> +abs_src_dir = $(shell readlink -f $(srcdir))
> +
> + at abs_top_builddir@/modules/demux/playlist/cuesheet/cuesheet.h: $(libcuesheet_rust_plugin_la_SOURCES)
> +	mkdir -p demux/playlist/cuesheet && cd demux/playlist/cuesheet && \
> +	$(CARGO_C) --library-type=staticlib --destdir=. --includedir=.. --libdir=. \
> +	--manifest-path=$(abs_src_dir)/demux/playlist/cuesheet/Cargo.toml
> +
> +BUILT_SOURCES += @abs_top_builddir@/modules/demux/playlist/cuesheet/cuesheet.h
> +CUESHEET_LIBS = @abs_top_builddir@/modules/demux/playlist/cuesheet/libcuesheet.a
> +CUESHEET_LIBS += $(RUST_RUNTIME_LIBS)
> +libcuesheet_plugin_la_SOURCES = demux/playlist/cuesheet.c @abs_top_builddir@/modules/demux/playlist/cuesheet/cuesheet.h
> +libcuesheet_plugin_la_LIBADD = $(CUESHEET_LIBS)
> +
> +demux_LTLIBRARIES += libcuesheet_plugin.la
> +endif
> +
>  libts_plugin_la_SOURCES = demux/mpeg/ts.c demux/mpeg/ts.h \
>          demux/mpeg/ts_pid.h demux/mpeg/ts_pid_fwd.h demux/mpeg/ts_pid.c \
>          demux/mpeg/ts_psi.h demux/mpeg/ts_psi.c \
> diff --git a/modules/demux/playlist/cuesheet.c b/modules/demux/playlist/cuesheet.c
> new file mode 100644
> index 0000000000..aa83e58455
> --- /dev/null
> +++ b/modules/demux/playlist/cuesheet.c
> @@ -0,0 +1,179 @@
> +/*****************************************************************************
> + * cuesheet.c : cue sheet playlist import format
> + *****************************************************************************
> + * Copyright (C) 2020 VLC authors and VideoLAN
> + *
> + * Authors: Kartik Ohri <kartikohri13 at gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU Lesser General Public License as published by
> + * the Free Software Foundation; either version 2.1 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public License
> + * along with this program; if not, write to the Free Software Foundation,
> + * Inc., 51 Franklin Street, Fifth Floor, Boston MA 02110-1301, USA.
> + *****************************************************************************/
> +
> +#ifdef HAVE_CONFIG_H
> +# include "config.h"
> +#endif
> +
> +#include <string.h>
> +
> +#include <vlc_common.h>
> +#include <vlc_plugin.h>
> +#include <vlc_access.h>
> +#include <vlc_url.h>
> +
> +#include "cuesheet/cuesheet.h"
> +#include "playlist.h"
> +
> +typedef struct {
> +    struct CCuesheet *cuesheet;
> +} cuesheet_sys_t;
> +
> +int Import_Cue_Sheet(vlc_object_t *);
> +static int ReadDir(stream_t *, input_item_node_t *);
> +static char* rust_string_to_c_string(char*);
> +static char* get_cuesheet_property(CCuesheet *, const char*);
> +static char* get_cuesheet_comment(CCuesheet *, const char*);
> +static char* get_track_property(CCuesheet *, const char*, int);
> +
> +int Import_Cue_Sheet( vlc_object_t *p_this )
> +{
> +    stream_t *p_demux = (stream_t *) p_this;
> +    CHECK_FILE ( p_demux );
> +
> +    if ( !stream_IsMimeType ( p_demux->s, "application/x-cue" )
> +    && !stream_HasExtension( p_demux->s, ".cue" ))
> +        return VLC_EGENERIC;
> +
> +    p_demux->pf_control = access_vaDirectoryControlHelper;
> +    p_demux->pf_readdir = ReadDir;
> +    return VLC_SUCCESS;
> +}
> +
> +/**
> + * free should not be called on strings allocated by rust compiler. There is no
> + * way to ensure this without duplicating the string before passing it to
> + * input_item_t.
> + */
> +static char* rust_string_to_c_string(char *psz_rust) {
> +    if (!psz_rust)
> +        return NULL;
> +    char *psz_c = strdup(psz_rust);
> +    unref_cuesheet_string(psz_rust);
> +    return psz_c;
> +}
> +
> +static char* get_cuesheet_property(CCuesheet *cuesheet, const char* psz_property) {
> +    char* psz_val = cuesheet_get_property(cuesheet, psz_property);
> +    return rust_string_to_c_string(psz_val);
> +}
> +
> +static char* get_cuesheet_comment(CCuesheet *cuesheet, const char* psz_property) {
> +    char* psz_val = cuesheet_get_comment_value(cuesheet, psz_property);
> +    return rust_string_to_c_string(psz_val);
> +}
> +
> +static char* get_track_property(CCuesheet *cuesheet, const char* psz_property, int track_num) {
> +    char* psz_val = cuesheet_get_track_property(cuesheet, track_num, psz_property);
> +    return rust_string_to_c_string(psz_val);
> +}
> +
> +static int ReadDir(stream_t* p_demux, input_item_node_t* p_subitems)
> +{
> +    CCuesheet *cuesheet = cuesheet_from_demux(p_demux->s);
> +
> +    int tracks = cuesheet_get_tracks_number(cuesheet);
> +    int idx = 0;
> +    char *psz_val = NULL;
> +    char *ppsz_option[2];
> +
> +    char *psz_audio_file = get_cuesheet_property(cuesheet, "FILE_NAME");
> +    if (!psz_audio_file)
> +        goto error;
> +
> +    char *psz_audio_file_uri = vlc_uri_encode(psz_audio_file);
> +    char *psz_url = vlc_uri_resolve(p_demux->psz_url, psz_audio_file_uri);
> +    free(psz_audio_file);
> +    free(psz_audio_file_uri);
> +
> +    char *psz_album = get_cuesheet_property(cuesheet, "TITLE");
> +    char *psz_album_artist = get_cuesheet_property(cuesheet, "PERFORMER");
> +    char *psz_description = get_cuesheet_comment(cuesheet, "COMMENT");
> +    char *psz_genre = get_cuesheet_comment(cuesheet, "GENRE");
> +    char *psz_date = get_cuesheet_comment(cuesheet, "DATE");


Doesn't all those strings leak?

> +
> +    while ( idx < tracks )
> +    {
> +        int i_options = 0;
> +        input_item_t *p_item;
> +
> +        psz_val = get_track_property(cuesheet, "TITLE", idx);
> +        if (psz_val)
> +        {
> +            p_item = input_item_New(psz_url, psz_val);
> +            p_item->i_type = ITEM_TYPE_FILE;
> +        }
> +        else
> +            goto error;
> +
> +        if (psz_album)
> +            input_item_SetAlbum(p_item, psz_album);
> +
> +        if (psz_album_artist)
> +            input_item_SetAlbumArtist(p_item, psz_album_artist);
> +
> +        if (psz_description)
> +            input_item_SetDescription(p_item, psz_description);
> +
> +        if (psz_date)
> +            input_item_SetDate(p_item, psz_date);
> +
> +        if (psz_genre)
> +            input_item_SetGenre(p_item, psz_genre);
> +
> +        psz_val = get_track_property(cuesheet, "PERFORMER", idx);
> +        if (psz_val)
> +            input_item_SetArtist(p_item, psz_val);
> +
> +        int i_time = cuesheet_get_track_start(cuesheet, idx);
> +        if (asprintf(ppsz_option, ":start-time=%d", i_time))
> +            i_options++;
> +
> +        if (idx < tracks - 1)
> +        {
> +            i_time = cuesheet_get_track_start(cuesheet, idx + 1);
> +            if (asprintf(ppsz_option + i_options, ":stop-time=%d", i_time))
> +                i_options++;
> +        }
> +
> +        input_item_AddOptions(p_item, i_options, (const char **)ppsz_option, VLC_INPUT_OPTION_TRUSTED);
> +        input_item_node_AppendItem(p_subitems, p_item);
> +        input_item_Release(p_item);
> +        idx++;
> +    }
> +    unref_cuesheet(cuesheet);
> +    return VLC_SUCCESS;
> +error:
> +    unref_cuesheet(cuesheet);
> +    return VLC_EGENERIC;
> +}
> +
> +vlc_module_begin()
> +    add_shortcut( "playlist" )
> +    set_category( CAT_INPUT )
> +    set_subcategory( SUBCAT_INPUT_DEMUX )
> +    set_description ( N_("Cue Sheet importer") )
> +    set_capability ( "stream_filter", 320 )
> +    set_callback ( Import_Cue_Sheet )
> +vlc_module_end()
> +
> +
> diff --git a/modules/demux/playlist/cuesheet/Cargo.toml b/modules/demux/playlist/cuesheet/Cargo.toml
> new file mode 100644
> index 0000000000..597076aa13
> --- /dev/null
> +++ b/modules/demux/playlist/cuesheet/Cargo.toml
> @@ -0,0 +1,13 @@
> +[package]
> +name = "cuesheet"
> +version = "0.1.0"
> +authors = ["Kartik Ohri <kartikohri13 at gmail.com>"]
> +edition = "2018"
> +license = "LGPL-2.1-or-later"
> +
> +[dependencies]
> +vlccore-rs = {path = "../../../../src/vlccore-rs"}
> +vlccore-sys = {path = "../../../../src/vlccore-sys"}
> +
> +[lib]
> +name = "cuesheet"
> \ No newline at end of file
> diff --git a/modules/demux/playlist/cuesheet/cbindgen.toml b/modules/demux/playlist/cuesheet/cbindgen.toml
> new file mode 100644
> index 0000000000..bc8fd14ae8
> --- /dev/null
> +++ b/modules/demux/playlist/cuesheet/cbindgen.toml
> @@ -0,0 +1,16 @@
> +header = "// SPDX-License-Identifier: LGPL-3.0-or-later"
> +sys_includes = ["stddef.h", "stdint.h", "stdlib.h"]
> +no_includes = true
> +include_guard = "CUESHEET_H"
> +tab_width = 4
> +language = "C"
> +documentation_style = "C"
> +style = "type"
> +cpp_compat = true
> +
> +[export]
> +item_types = ["enums", "structs", "unions", "typedefs", "opaque", "functions"]
> +
> +[enum]
> +rename_variants = "ScreamingSnakeCase"
> +prefix_with_name = true
> \ No newline at end of file
> diff --git a/modules/demux/playlist/cuesheet/src/capi.rs b/modules/demux/playlist/cuesheet/src/capi.rs
> new file mode 100644
> index 0000000000..b2e3e9e683
> --- /dev/null
> +++ b/modules/demux/playlist/cuesheet/src/capi.rs
> @@ -0,0 +1,96 @@
> +use crate::config::Cuesheet;
> +use std::os::raw::{c_char, c_int};
> +use std::ffi::{CStr, CString};
> +use std::convert::TryFrom;
> +use std::io::{BufReader, BufRead};
> +use std::ptr::null_mut;
> +
> +use vlccore_rs::stream as stream;
> +use vlccore_sys::stream::stream_t;
> +
> +pub struct CCuesheet {
> +    cuesheet : Cuesheet
> +}
> +
> +#[no_mangle]
> +pub extern fn cuesheet_from_demux(s: *mut stream_t) -> *mut CCuesheet {
> +    let stream = stream::Stream::from(s);

We usually use:

    let stream : stream::Stream = s.into();

Mainly because the Into trait is automatically implemented
from a From trait, but the reverse is not true. Most of the
time, the `: stream::Stream` can also be omitted too (but
probably not here).

> +    let mut cuesheet = Cuesheet::default();
> +    let reader = BufReader::new(stream);
> +    reader.lines().for_each(|line| {
> +        line.map(|l| cuesheet.process_line(l.as_str()));
> +    });

In its current form, the playlist demux will open regardless
of whether the line processing failed or not and the error is
dropped.

It means that if a line fails to proceed, it might just be
dropped silently, instead of failing to open the module. I'm
not sure this is what we want here, though I haven't checked
the other playlist modules.

> +    let c_cuesheet = Box::new(CCuesheet {cuesheet});
> +    Box::into_raw(c_cuesheet)
> +}
> +
> +#[no_mangle]
> +pub unsafe extern fn cuesheet_get_property(c_cuesheet : *mut CCuesheet,
> +    line : *const c_char) -> *mut c_char {
> +
> +    let cuesheet = & (*c_cuesheet).cuesheet;
> +
> +    CStr::from_ptr(line).to_str().ok()
> +        .and_then(|line| cuesheet.get_property(line))
> +        .map(|property| convert_string_to_ptr(property.to_string()))
> +        .unwrap_or(null_mut())
> +}
> +
> +#[no_mangle]
> +pub unsafe extern fn cuesheet_get_comment_value(c_cuesheet : *mut CCuesheet,
> +    line : *const c_char) -> *mut c_char {
> +    let cuesheet = & (*c_cuesheet).cuesheet;
> +
> +    CStr::from_ptr(line).to_str().ok()
> +        .and_then(|line| cuesheet.comments.get(line))
> +        .map(|comment| convert_string_to_ptr(comment.to_string()))
> +        .unwrap_or(null_mut())
> +}
> +
> +#[no_mangle]
> +pub unsafe extern fn cuesheet_get_tracks_number(c_cuesheet : *mut CCuesheet)
> +    -> c_int {
> +    (*c_cuesheet).cuesheet.tracks.len() as i32

It's weird that you cast into i32 to return a c_int, neaning
that it will probably error out if both are not the same.

> +}
> +
> +#[no_mangle]
> +pub unsafe extern fn cuesheet_get_track_property(c_cuesheet : *mut CCuesheet,
> +    index : c_int, line : *const c_char) -> *mut c_char {
> +
> +    let cuesheet = & (*c_cuesheet).cuesheet;
> +
> +    usize::try_from(index).ok()
> +        .and_then(|index| cuesheet.get_track_at_index(index))
> +        .zip(CStr::from_ptr(line).to_str().ok())
> +        .and_then(|(track, property)| track.get_property(property))
> +        .map(|value| convert_string_to_ptr(value.to_string()))
> +        .unwrap_or(null_mut())
> +}
> +
> +#[no_mangle]
> +pub unsafe extern fn cuesheet_get_track_start(c_cuesheet: *mut CCuesheet,
> +    index : c_int) -> c_int {
> +    let cuesheet = &(*c_cuesheet).cuesheet;
> +
> +    usize::try_from(index).ok()
> +        .and_then(|index| cuesheet.get_track_at_index(index))
> +        .map(|track| track.begin_time_in_seconds())
> +        .unwrap_or(-1)
> +}
> +
> +unsafe fn convert_string_to_ptr(string: String) -> *mut c_char {
> +    if string.is_empty() {
> +        return std::ptr::null_mut();
> +    }
> +    CString::new(string).map(|ptr| ptr.into_raw()).unwrap_or(null_mut())
> +}
> +
> +#[no_mangle]
> +pub unsafe extern fn unref_cuesheet_string(text : *mut c_char) {
> +    let _ = CString::from_raw(text);
> +}
> +
> +#[no_mangle]
> +pub unsafe extern fn unref_cuesheet(cuesheet : *mut CCuesheet) {
> +    let _ = Box::from_raw(cuesheet);
> +}
> diff --git a/modules/demux/playlist/cuesheet/src/config.rs b/modules/demux/playlist/cuesheet/src/config.rs
> new file mode 100644
> index 0000000000..118f046711
> --- /dev/null
> +++ b/modules/demux/playlist/cuesheet/src/config.rs
> @@ -0,0 +1,103 @@
> +use std::collections::HashMap;
> +use std::borrow::Borrow;
> +
> +#[derive(Clone, Debug)]
> +pub struct CuesheetTrack {
> +    pub item_type : String,
> +    pub item_position : u8,
> +    pub item_performer : String,
> +    pub item_title : String,
> +    pub item_begin_duration : String
> +}
> +
> +impl Default for CuesheetTrack {
> +    fn default() -> Self {
> +        CuesheetTrack {
> +            item_type: "".to_owned(),
> +            item_position: 0,
> +            item_performer: "".to_string(),
> +            item_title: "".to_string(),
> +            item_begin_duration: "".to_string()
> +        }
> +    }
> +}
> +
> +impl CuesheetTrack {
> +    pub fn begin_time_in_seconds(&self) -> i32 {
> +        let start: &str = &self.item_begin_duration;
> +        let mut duration_parts: Vec<&str> = start.split(":").collect();
> +
> +        let frame_rate = 75;
> +        let convert_min_to_s = 60;
> +        let mut start_time = 0;
> +
> +        let time = duration_parts.pop()
> +            .and_then(|part| part.parse::<u32>().ok())
> +            .map(|part| start_time = part / frame_rate)
> +            .and_then(|_| duration_parts.pop())
> +            .and_then(|part| part.parse::<u32>().ok())
> +            .map(|part| start_time += part)
> +            .and_then(|_| duration_parts.pop())
> +            .and_then(|part| part.parse::<u32>().ok())
> +            .map(|part| start_time += part * convert_min_to_s);

It seems weird to mix a functional syntax with mutation from outside
the input parameters. Splitting here would probably help, but it might
need an intermediate private function to benefit from early return.

> +
> +        match time {
> +            Some(()) => start_time as i32,
> +            None => -1
> +        }

Especially given this error handling here, which feels a bit alien
since the matched Option doesn't actually store the value.

> +    }
> +
> +    pub fn get_property(&self, key : &str) -> Option<&str> {
> +        match key.to_ascii_lowercase().as_str() {
> +            "type" => Some(self.item_type.as_str()),
> +            "begin_duration" => Some(self.item_begin_duration.as_str()),
> +            "performer" => Some(self.item_performer.as_str()),
> +            "title" => Some(self.item_title.as_str()),
> +            _ => None
> +        }
> +    }
> +}
> +
> +#[derive(Clone, Debug)]
> +pub struct Cuesheet {
> +    pub file_name : String,
> +    pub file_type : String,
> +    pub tracks : Vec<CuesheetTrack>,
> +    pub performer : String,
> +    pub title : String,
> +    pub comments : HashMap<String, String>,
> +    pub is_processing_tracks : bool
> +}
> +
> +impl Default for Cuesheet {
> +    fn default() -> Self {
> +        Cuesheet {
> +            file_name: "".to_string(),
> +            file_type: "".to_string(),
> +            tracks: Vec::new(),
> +            performer: "".to_string(),
> +            title: "".to_string(),
> +            comments: HashMap::new(),
> +            is_processing_tracks: false
> +        }
> +    }
> +}
> +
> +impl Cuesheet {
> +    pub fn get_track_at_index(&self, index : usize) -> Option<&CuesheetTrack> {
> +        if index >= self.tracks.len() {
> +            return None;
> +        }
> +        Some(self.tracks[index].borrow())
> +    }
> +
> +    pub fn get_property(&self, key : &str) -> Option<&str> {
> +        match key.to_ascii_lowercase().as_str() {
> +            "file_name" => Some(self.file_name.as_str()),
> +            "file_type" => Some(self.file_type.as_str()),
> +            "performer" => Some(self.performer.as_str()),
> +            "title" => Some(self.title.as_str()),
> +            _ => None
> +        }
> +    }
> +}
> \ No newline at end of file
> diff --git a/modules/demux/playlist/cuesheet/src/lib.rs b/modules/demux/playlist/cuesheet/src/lib.rs
> new file mode 100644
> index 0000000000..c768d23825
> --- /dev/null
> +++ b/modules/demux/playlist/cuesheet/src/lib.rs
> @@ -0,0 +1,18 @@
> +mod parse;
> +mod config;
> +
> +#[cfg(test)]
> +mod tests {
> +    use crate::parse::parse_line_alt;
> +    #[test]
> +    fn test_parse_line_alt() {
> +        let parts = parse_line_alt("FILE \"Live in Berlin\" MP3");
> +        assert_eq!(3, parts.len());
> +        assert_eq!("FILE", parts[0]);
> +        assert_eq!("Live in Berlin", parts[1]);
> +        assert_eq!("MP3", parts[2]);
> +    }

Maybe tests with duration could be added? And incorrect
duration / case that should be leading to errors.

It seems that the test suite is not integrated with
automake too. How is it planned to execute it?

> +}
> +
> +#[cfg(cargo_c)]
> +mod capi;
> \ No newline at end of file
> diff --git a/modules/demux/playlist/cuesheet/src/parse.rs b/modules/demux/playlist/cuesheet/src/parse.rs
> new file mode 100644
> index 0000000000..b570638325
> --- /dev/null
> +++ b/modules/demux/playlist/cuesheet/src/parse.rs
> @@ -0,0 +1,75 @@
> +use crate::config::*;
> +
> +pub fn parse_line(line: &str) -> Vec<String> {
> +    let characters = line.trim().chars();
> +    let mut parts: Vec<String> = Vec::new();
> +    let mut inside_quotes: bool = false;
> +    let mut temp_str: String = String::new();
> +    for char in characters {
> +        if char == '"' {
> +            inside_quotes = !inside_quotes;
> +        } else if inside_quotes {
> +            temp_str.push(char);
> +        } else if char.is_whitespace() {
> +            parts.push(temp_str.clone());
> +            temp_str.clear();
> +        } else {
> +            temp_str.push(char);
> +        }
> +    }
> +    parts.push(temp_str.clone());
> +    parts
> +}
> +
> +impl Cuesheet {
> +    pub fn process_line(&mut self, line: &str) -> Option<()> {

It seems to me that this could actually be an implementation of
the trait FromStr, and thus usage could be done though the more
idiomatic str/String::parse().

> +        let parts = parse_line(line);
> +        let keyword = parts[0].as_str();
> +        if !self.is_processing_tracks {
> +            match keyword {
> +                "FILE" => {
> +                    self.file_name = parts[1].to_owned();
> +                    self.file_type = parts[2].to_owned();
> +                }
> +                "TITLE" => self.title = parts[1].to_owned(),
> +                "PERFORMER" => self.performer = parts[1].to_owned(),
> +                "REM" => {
> +                    self.comments
> +                        .insert(parts[1].to_owned().to_lowercase(), parts[2].to_owned());
> +                }
> +                "TRACK" => self.is_processing_tracks = true,
> +                _ => {}
> +            };
> +        }
> +        if self.is_processing_tracks {
> +            if keyword == "TRACK" {
> +                let mut track = CuesheetTrack::default();
> +                track.item_position = parts[1].to_owned().parse::<u8>().ok()?;
> +                track.item_type = parts[2].to_owned();
> +                self.tracks.push(track);
> +            } else {
> +                self.tracks.last_mut()?.process_track(line);
> +            }
> +        }
> +        Some(())
> +    }
> +}
> +
> +impl CuesheetTrack {
> +    fn process_track(&mut self, line: &str) -> Option<()> {

Same as above.

> +        let parts = parse_line(line);
> +        let keyword = parts[0].as_str();
> +        match keyword {
> +            "TITLE" => self.item_title = parts[1].to_owned(),
> +            "PERFORMER" => self.item_performer = parts[1].to_owned(),
> +            "INDEX" => {
> +                let index_position = parts[1].to_owned().parse::<u8>().ok()?;
> +                if index_position == 1 {
> +                    self.item_begin_duration = parts[2].to_owned();
> +                }
> +            }
> +            _ => {}
> +        };
> +        Some(())
> +    }
> +}
> --
> 2.25.1

Regards,
--
Alexandre Janniaux
Videolabs


More information about the vlc-devel mailing list