[vlc-devel] [RFC: PATCH 2/2] stream filter: Add rust based cuesheet module
Kartik Ohri
kartikohri13 at gmail.com
Fri Sep 11 15:24:45 CEST 2020
On Fri, Sep 11, 2020 at 6:50 AM Alexandre Janniaux <ajanni at videolabs.io>
wrote:
> 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?
>
I am not sure on how to do that. I'll try to experiment and report my
findings.
> - do we need cbindgen to compile the module?
>
Yes, cbindgen is needed to produce the cuesheet.h header file.
> - can the cbindgen file be mutualized between plugins?
>
Yes, but it depends on customizations required. Like for cuesheet.h,
cbindgen is configured to include vlc_stream.h as well. Some other module
may or may not require it and so on.
> - 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?
>
>
I suggest to use rustfmt for that. (https://github.com/rust-lang/rustfmt)
> 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?
>
>
Yeah, my bad. Will fix it.
> > +
> > + 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).
>
>
Understood, will do.
> > + 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.
>
>
It was intentional. I have checked a few other modules and there is error
handling for different parts. However, in this the failure will occur only
if Rust is unable to convert the string into Utf8, this case is not
relevant to the C modules. How do you suggest to approach this ? My
intention was to continue processing and later decide while creating input
items if anything necessary is missing then handle the error there.
> > + 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.
>
>
I was under the impression that the two were equivalent. IIRC, currently
all rust supported platforms have 32 bit integers in C.
> > +}
> > +
> > +#[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.
>
>
Will fix.
> > +
> > + 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?
>
>
Actually, I hadn't planned to keep tests in the final patch. However, if we
want to keep the tests, I'll add a few more cases and try to integrate with
automake.
> > +}
> > +
> > +#[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().
>
>
I didn't use the FromStr trait because a single line does not describe the
struct completely. However, I can try to read all lines into a single
string first and then use FromStr on it.
> > + 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.
>
I can try to use FromStr here as well but it is duplicating work. In
CueSheet, we''ll obtain every line one by one and then create a new string.
Then, again in CuesheetTrack, iterate the String to break into lines.
> > + 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
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20200911/56375f3e/attachment.html>
More information about the vlc-devel
mailing list