<div dir="ltr"><div dir="ltr">On Fri, Sep 11, 2020 at 6:50 AM Alexandre Janniaux <<a href="mailto:ajanni@videolabs.io">ajanni@videolabs.io</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi,<br>
<br>
Thank you for submitting this work!<br>
<br>
Here are a few review points on the Rust part.<br>
<br>
At the same time, I have a bunch of questions, some of which<br>
are also in the comments below:<br>
<br>
- how do we run the tests in the CI? Is it planned to<br>
integrate with automake?<br></blockquote><div><br></div><div>I am not sure on how to do that. I'll try to experiment and report my findings.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- do we need cbindgen to compile the module?<br></blockquote><div><br></div><div>Yes, cbindgen is needed to produce the cuesheet.h header file.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- can the cbindgen file be mutualized between plugins?<br></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- since we don't have Rust code yet, maybe the code can<br>
be formatted by software before following ten differents<br>
convention, so that anyone can use their preferred one<br>
locally?<br>
<br></blockquote><div><br></div><div>I suggest to use rustfmt for that. (<a href="https://github.com/rust-lang/rustfmt">https://github.com/rust-lang/rustfmt</a>)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
On Thu, Sep 10, 2020 at 10:17:30PM +0530, Kartik Ohri wrote:<br>
> The module is written using Rust API and serves as<br>
> a proof of concept for the same.<br>
> ---<br>
> modules/demux/Makefile.am | 26 +++<br>
> modules/demux/playlist/cuesheet.c | 179 ++++++++++++++++++<br>
> modules/demux/playlist/cuesheet/Cargo.toml | 13 ++<br>
> modules/demux/playlist/cuesheet/cbindgen.toml | 16 ++<br>
> modules/demux/playlist/cuesheet/src/<a href="http://capi.rs" rel="noreferrer" target="_blank">capi.rs</a> | 96 ++++++++++<br>
> modules/demux/playlist/cuesheet/src/<a href="http://config.rs" rel="noreferrer" target="_blank">config.rs</a> | 103 ++++++++++<br>
> modules/demux/playlist/cuesheet/src/<a href="http://lib.rs" rel="noreferrer" target="_blank">lib.rs</a> | 18 ++<br>
> modules/demux/playlist/cuesheet/src/<a href="http://parse.rs" rel="noreferrer" target="_blank">parse.rs</a> | 75 ++++++++<br>
> 8 files changed, 526 insertions(+)<br>
> create mode 100644 modules/demux/playlist/cuesheet.c<br>
> create mode 100644 modules/demux/playlist/cuesheet/Cargo.toml<br>
> create mode 100644 modules/demux/playlist/cuesheet/cbindgen.toml<br>
> create mode 100644 modules/demux/playlist/cuesheet/src/<a href="http://capi.rs" rel="noreferrer" target="_blank">capi.rs</a><br>
> create mode 100644 modules/demux/playlist/cuesheet/src/<a href="http://config.rs" rel="noreferrer" target="_blank">config.rs</a><br>
> create mode 100644 modules/demux/playlist/cuesheet/src/<a href="http://lib.rs" rel="noreferrer" target="_blank">lib.rs</a><br>
> create mode 100644 modules/demux/playlist/cuesheet/src/<a href="http://parse.rs" rel="noreferrer" target="_blank">parse.rs</a><br>
><br>
> diff --git a/modules/demux/Makefile.am b/modules/demux/Makefile.am<br>
> index 675b2d10af..31591a768b 100644<br>
> --- a/modules/demux/Makefile.am<br>
> +++ b/modules/demux/Makefile.am<br>
> @@ -1,3 +1,4 @@<br>
> +<br>
> demuxdir = $(pluginsdir)/demux<br>
> demux_LTLIBRARIES =<br>
><br>
> @@ -256,6 +257,31 @@ libplaylist_plugin_la_SOURCES = \<br>
> demux/playlist/playlist.c demux/playlist/playlist.h<br>
> demux_LTLIBRARIES += <a href="http://libplaylist_plugin.la" rel="noreferrer" target="_blank">libplaylist_plugin.la</a><br>
><br>
> +if BUILD_RUST<br>
> +CARGO_C = cargo capi install --release<br>
> +RUST_RUNTIME_LIBS = -ldl -lrt -lpthread -lgcc_s -lc -lm -lrt -lpthread -lutil<br>
> +libcuesheet_rust_plugin_la_SOURCES = \<br>
> + demux/playlist/cuesheet/src/<a href="http://capi.rs" rel="noreferrer" target="_blank">capi.rs</a> \<br>
> + demux/playlist/cuesheet/src/<a href="http://config.rs" rel="noreferrer" target="_blank">config.rs</a> \<br>
> + demux/playlist/cuesheet/src/<a href="http://lib.rs" rel="noreferrer" target="_blank">lib.rs</a> \<br>
> + demux/playlist/cuesheet/src/<a href="http://parse.rs" rel="noreferrer" target="_blank">parse.rs</a><br>
> +<br>
> +abs_src_dir = $(shell readlink -f $(srcdir))<br>
> +<br>
> +@abs_top_builddir@/modules/demux/playlist/cuesheet/cuesheet.h: $(libcuesheet_rust_plugin_la_SOURCES)<br>
> + mkdir -p demux/playlist/cuesheet && cd demux/playlist/cuesheet && \<br>
> + $(CARGO_C) --library-type=staticlib --destdir=. --includedir=.. --libdir=. \<br>
> + --manifest-path=$(abs_src_dir)/demux/playlist/cuesheet/Cargo.toml<br>
> +<br>
> +BUILT_SOURCES += @abs_top_builddir@/modules/demux/playlist/cuesheet/cuesheet.h<br>
> +CUESHEET_LIBS = @abs_top_builddir@/modules/demux/playlist/cuesheet/libcuesheet.a<br>
> +CUESHEET_LIBS += $(RUST_RUNTIME_LIBS)<br>
> +libcuesheet_plugin_la_SOURCES = demux/playlist/cuesheet.c @abs_top_builddir@/modules/demux/playlist/cuesheet/cuesheet.h<br>
> +libcuesheet_plugin_la_LIBADD = $(CUESHEET_LIBS)<br>
> +<br>
> +demux_LTLIBRARIES += <a href="http://libcuesheet_plugin.la" rel="noreferrer" target="_blank">libcuesheet_plugin.la</a><br>
> +endif<br>
> +<br>
> libts_plugin_la_SOURCES = demux/mpeg/ts.c demux/mpeg/ts.h \<br>
> demux/mpeg/ts_pid.h demux/mpeg/ts_pid_fwd.h demux/mpeg/ts_pid.c \<br>
> demux/mpeg/ts_psi.h demux/mpeg/ts_psi.c \<br>
> diff --git a/modules/demux/playlist/cuesheet.c b/modules/demux/playlist/cuesheet.c<br>
> new file mode 100644<br>
> index 0000000000..aa83e58455<br>
> --- /dev/null<br>
> +++ b/modules/demux/playlist/cuesheet.c<br>
> @@ -0,0 +1,179 @@<br>
> +/*****************************************************************************<br>
> + * cuesheet.c : cue sheet playlist import format<br>
> + *****************************************************************************<br>
> + * Copyright (C) 2020 VLC authors and VideoLAN<br>
> + *<br>
> + * Authors: Kartik Ohri <<a href="mailto:kartikohri13@gmail.com" target="_blank">kartikohri13@gmail.com</a>><br>
> + *<br>
> + * This program is free software; you can redistribute it and/or modify it<br>
> + * under the terms of the GNU Lesser General Public License as published by<br>
> + * the Free Software Foundation; either version 2.1 of the License, or<br>
> + * (at your option) any later version.<br>
> + *<br>
> + * This program is distributed in the hope that it will be useful,<br>
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of<br>
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the<br>
> + * GNU Lesser General Public License for more details.<br>
> + *<br>
> + * You should have received a copy of the GNU Lesser General Public License<br>
> + * along with this program; if not, write to the Free Software Foundation,<br>
> + * Inc., 51 Franklin Street, Fifth Floor, Boston MA 02110-1301, USA.<br>
> + *****************************************************************************/<br>
> +<br>
> +#ifdef HAVE_CONFIG_H<br>
> +# include "config.h"<br>
> +#endif<br>
> +<br>
> +#include <string.h><br>
> +<br>
> +#include <vlc_common.h><br>
> +#include <vlc_plugin.h><br>
> +#include <vlc_access.h><br>
> +#include <vlc_url.h><br>
> +<br>
> +#include "cuesheet/cuesheet.h"<br>
> +#include "playlist.h"<br>
> +<br>
> +typedef struct {<br>
> + struct CCuesheet *cuesheet;<br>
> +} cuesheet_sys_t;<br>
> +<br>
> +int Import_Cue_Sheet(vlc_object_t *);<br>
> +static int ReadDir(stream_t *, input_item_node_t *);<br>
> +static char* rust_string_to_c_string(char*);<br>
> +static char* get_cuesheet_property(CCuesheet *, const char*);<br>
> +static char* get_cuesheet_comment(CCuesheet *, const char*);<br>
> +static char* get_track_property(CCuesheet *, const char*, int);<br>
> +<br>
> +int Import_Cue_Sheet( vlc_object_t *p_this )<br>
> +{<br>
> + stream_t *p_demux = (stream_t *) p_this;<br>
> + CHECK_FILE ( p_demux );<br>
> +<br>
> + if ( !stream_IsMimeType ( p_demux->s, "application/x-cue" )<br>
> + && !stream_HasExtension( p_demux->s, ".cue" ))<br>
> + return VLC_EGENERIC;<br>
> +<br>
> + p_demux->pf_control = access_vaDirectoryControlHelper;<br>
> + p_demux->pf_readdir = ReadDir;<br>
> + return VLC_SUCCESS;<br>
> +}<br>
> +<br>
> +/**<br>
> + * free should not be called on strings allocated by rust compiler. There is no<br>
> + * way to ensure this without duplicating the string before passing it to<br>
> + * input_item_t.<br>
> + */<br>
> +static char* rust_string_to_c_string(char *psz_rust) {<br>
> + if (!psz_rust)<br>
> + return NULL;<br>
> + char *psz_c = strdup(psz_rust);<br>
> + unref_cuesheet_string(psz_rust);<br>
> + return psz_c;<br>
> +}<br>
> +<br>
> +static char* get_cuesheet_property(CCuesheet *cuesheet, const char* psz_property) {<br>
> + char* psz_val = cuesheet_get_property(cuesheet, psz_property);<br>
> + return rust_string_to_c_string(psz_val);<br>
> +}<br>
> +<br>
> +static char* get_cuesheet_comment(CCuesheet *cuesheet, const char* psz_property) {<br>
> + char* psz_val = cuesheet_get_comment_value(cuesheet, psz_property);<br>
> + return rust_string_to_c_string(psz_val);<br>
> +}<br>
> +<br>
> +static char* get_track_property(CCuesheet *cuesheet, const char* psz_property, int track_num) {<br>
> + char* psz_val = cuesheet_get_track_property(cuesheet, track_num, psz_property);<br>
> + return rust_string_to_c_string(psz_val);<br>
> +}<br>
> +<br>
> +static int ReadDir(stream_t* p_demux, input_item_node_t* p_subitems)<br>
> +{<br>
> + CCuesheet *cuesheet = cuesheet_from_demux(p_demux->s);<br>
> +<br>
> + int tracks = cuesheet_get_tracks_number(cuesheet);<br>
> + int idx = 0;<br>
> + char *psz_val = NULL;<br>
> + char *ppsz_option[2];<br>
> +<br>
> + char *psz_audio_file = get_cuesheet_property(cuesheet, "FILE_NAME");<br>
> + if (!psz_audio_file)<br>
> + goto error;<br>
> +<br>
> + char *psz_audio_file_uri = vlc_uri_encode(psz_audio_file);<br>
> + char *psz_url = vlc_uri_resolve(p_demux->psz_url, psz_audio_file_uri);<br>
> + free(psz_audio_file);<br>
> + free(psz_audio_file_uri);<br>
> +<br>
> + char *psz_album = get_cuesheet_property(cuesheet, "TITLE");<br>
> + char *psz_album_artist = get_cuesheet_property(cuesheet, "PERFORMER");<br>
> + char *psz_description = get_cuesheet_comment(cuesheet, "COMMENT");<br>
> + char *psz_genre = get_cuesheet_comment(cuesheet, "GENRE");<br>
> + char *psz_date = get_cuesheet_comment(cuesheet, "DATE");<br>
<br>
<br>
Doesn't all those strings leak?<br>
<br></blockquote><div><br></div><div>Yeah, my bad. Will fix it.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> + while ( idx < tracks )<br>
> + {<br>
> + int i_options = 0;<br>
> + input_item_t *p_item;<br>
> +<br>
> + psz_val = get_track_property(cuesheet, "TITLE", idx);<br>
> + if (psz_val)<br>
> + {<br>
> + p_item = input_item_New(psz_url, psz_val);<br>
> + p_item->i_type = ITEM_TYPE_FILE;<br>
> + }<br>
> + else<br>
> + goto error;<br>
> +<br>
> + if (psz_album)<br>
> + input_item_SetAlbum(p_item, psz_album);<br>
> +<br>
> + if (psz_album_artist)<br>
> + input_item_SetAlbumArtist(p_item, psz_album_artist);<br>
> +<br>
> + if (psz_description)<br>
> + input_item_SetDescription(p_item, psz_description);<br>
> +<br>
> + if (psz_date)<br>
> + input_item_SetDate(p_item, psz_date);<br>
> +<br>
> + if (psz_genre)<br>
> + input_item_SetGenre(p_item, psz_genre);<br>
> +<br>
> + psz_val = get_track_property(cuesheet, "PERFORMER", idx);<br>
> + if (psz_val)<br>
> + input_item_SetArtist(p_item, psz_val);<br>
> +<br>
> + int i_time = cuesheet_get_track_start(cuesheet, idx);<br>
> + if (asprintf(ppsz_option, ":start-time=%d", i_time))<br>
> + i_options++;<br>
> +<br>
> + if (idx < tracks - 1)<br>
> + {<br>
> + i_time = cuesheet_get_track_start(cuesheet, idx + 1);<br>
> + if (asprintf(ppsz_option + i_options, ":stop-time=%d", i_time))<br>
> + i_options++;<br>
> + }<br>
> +<br>
> + input_item_AddOptions(p_item, i_options, (const char **)ppsz_option, VLC_INPUT_OPTION_TRUSTED);<br>
> + input_item_node_AppendItem(p_subitems, p_item);<br>
> + input_item_Release(p_item);<br>
> + idx++;<br>
> + }<br>
> + unref_cuesheet(cuesheet);<br>
> + return VLC_SUCCESS;<br>
> +error:<br>
> + unref_cuesheet(cuesheet);<br>
> + return VLC_EGENERIC;<br>
> +}<br>
> +<br>
> +vlc_module_begin()<br>
> + add_shortcut( "playlist" )<br>
> + set_category( CAT_INPUT )<br>
> + set_subcategory( SUBCAT_INPUT_DEMUX )<br>
> + set_description ( N_("Cue Sheet importer") )<br>
> + set_capability ( "stream_filter", 320 )<br>
> + set_callback ( Import_Cue_Sheet )<br>
> +vlc_module_end()<br>
> +<br>
> +<br>
> diff --git a/modules/demux/playlist/cuesheet/Cargo.toml b/modules/demux/playlist/cuesheet/Cargo.toml<br>
> new file mode 100644<br>
> index 0000000000..597076aa13<br>
> --- /dev/null<br>
> +++ b/modules/demux/playlist/cuesheet/Cargo.toml<br>
> @@ -0,0 +1,13 @@<br>
> +[package]<br>
> +name = "cuesheet"<br>
> +version = "0.1.0"<br>
> +authors = ["Kartik Ohri <<a href="mailto:kartikohri13@gmail.com" target="_blank">kartikohri13@gmail.com</a>>"]<br>
> +edition = "2018"<br>
> +license = "LGPL-2.1-or-later"<br>
> +<br>
> +[dependencies]<br>
> +vlccore-rs = {path = "../../../../src/vlccore-rs"}<br>
> +vlccore-sys = {path = "../../../../src/vlccore-sys"}<br>
> +<br>
> +[lib]<br>
> +name = "cuesheet"<br>
> \ No newline at end of file<br>
> diff --git a/modules/demux/playlist/cuesheet/cbindgen.toml b/modules/demux/playlist/cuesheet/cbindgen.toml<br>
> new file mode 100644<br>
> index 0000000000..bc8fd14ae8<br>
> --- /dev/null<br>
> +++ b/modules/demux/playlist/cuesheet/cbindgen.toml<br>
> @@ -0,0 +1,16 @@<br>
> +header = "// SPDX-License-Identifier: LGPL-3.0-or-later"<br>
> +sys_includes = ["stddef.h", "stdint.h", "stdlib.h"]<br>
> +no_includes = true<br>
> +include_guard = "CUESHEET_H"<br>
> +tab_width = 4<br>
> +language = "C"<br>
> +documentation_style = "C"<br>
> +style = "type"<br>
> +cpp_compat = true<br>
> +<br>
> +[export]<br>
> +item_types = ["enums", "structs", "unions", "typedefs", "opaque", "functions"]<br>
> +<br>
> +[enum]<br>
> +rename_variants = "ScreamingSnakeCase"<br>
> +prefix_with_name = true<br>
> \ No newline at end of file<br>
> diff --git a/modules/demux/playlist/cuesheet/src/<a href="http://capi.rs" rel="noreferrer" target="_blank">capi.rs</a> b/modules/demux/playlist/cuesheet/src/<a href="http://capi.rs" rel="noreferrer" target="_blank">capi.rs</a><br>
> new file mode 100644<br>
> index 0000000000..b2e3e9e683<br>
> --- /dev/null<br>
> +++ b/modules/demux/playlist/cuesheet/src/<a href="http://capi.rs" rel="noreferrer" target="_blank">capi.rs</a><br>
> @@ -0,0 +1,96 @@<br>
> +use crate::config::Cuesheet;<br>
> +use std::os::raw::{c_char, c_int};<br>
> +use std::ffi::{CStr, CString};<br>
> +use std::convert::TryFrom;<br>
> +use std::io::{BufReader, BufRead};<br>
> +use std::ptr::null_mut;<br>
> +<br>
> +use vlccore_rs::stream as stream;<br>
> +use vlccore_sys::stream::stream_t;<br>
> +<br>
> +pub struct CCuesheet {<br>
> + cuesheet : Cuesheet<br>
> +}<br>
> +<br>
> +#[no_mangle]<br>
> +pub extern fn cuesheet_from_demux(s: *mut stream_t) -> *mut CCuesheet {<br>
> + let stream = stream::Stream::from(s);<br>
<br>
We usually use:<br>
<br>
let stream : stream::Stream = s.into();<br>
<br>
Mainly because the Into trait is automatically implemented<br>
from a From trait, but the reverse is not true. Most of the<br>
time, the `: stream::Stream` can also be omitted too (but<br>
probably not here).<br>
<br></blockquote><div><br></div><div>Understood, will do.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + let mut cuesheet = Cuesheet::default();<br>
> + let reader = BufReader::new(stream);<br>
> + reader.lines().for_each(|line| {<br>
> + line.map(|l| cuesheet.process_line(l.as_str()));<br>
> + });<br>
<br>
In its current form, the playlist demux will open regardless<br>
of whether the line processing failed or not and the error is<br>
dropped.<br>
<br>
It means that if a line fails to proceed, it might just be<br>
dropped silently, instead of failing to open the module. I'm<br>
not sure this is what we want here, though I haven't checked<br>
the other playlist modules.<br>
<br></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + let c_cuesheet = Box::new(CCuesheet {cuesheet});<br>
> + Box::into_raw(c_cuesheet)<br>
> +}<br>
> +<br>
> +#[no_mangle]<br>
> +pub unsafe extern fn cuesheet_get_property(c_cuesheet : *mut CCuesheet,<br>
> + line : *const c_char) -> *mut c_char {<br>
> +<br>
> + let cuesheet = & (*c_cuesheet).cuesheet;<br>
> +<br>
> + CStr::from_ptr(line).to_str().ok()<br>
> + .and_then(|line| cuesheet.get_property(line))<br>
> + .map(|property| convert_string_to_ptr(property.to_string()))<br>
> + .unwrap_or(null_mut())<br>
> +}<br>
> +<br>
> +#[no_mangle]<br>
> +pub unsafe extern fn cuesheet_get_comment_value(c_cuesheet : *mut CCuesheet,<br>
> + line : *const c_char) -> *mut c_char {<br>
> + let cuesheet = & (*c_cuesheet).cuesheet;<br>
> +<br>
> + CStr::from_ptr(line).to_str().ok()<br>
> + .and_then(|line| cuesheet.comments.get(line))<br>
> + .map(|comment| convert_string_to_ptr(comment.to_string()))<br>
> + .unwrap_or(null_mut())<br>
> +}<br>
> +<br>
> +#[no_mangle]<br>
> +pub unsafe extern fn cuesheet_get_tracks_number(c_cuesheet : *mut CCuesheet)<br>
> + -> c_int {<br>
> + (*c_cuesheet).cuesheet.tracks.len() as i32<br>
<br>
It's weird that you cast into i32 to return a c_int, neaning<br>
that it will probably error out if both are not the same.<br>
<br></blockquote><div><br></div><div>I was under the impression that the two were equivalent. IIRC, currently all rust supported platforms have 32 bit integers in C.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +}<br>
> +<br>
> +#[no_mangle]<br>
> +pub unsafe extern fn cuesheet_get_track_property(c_cuesheet : *mut CCuesheet,<br>
> + index : c_int, line : *const c_char) -> *mut c_char {<br>
> +<br>
> + let cuesheet = & (*c_cuesheet).cuesheet;<br>
> +<br>
> + usize::try_from(index).ok()<br>
> + .and_then(|index| cuesheet.get_track_at_index(index))<br>
> + .zip(CStr::from_ptr(line).to_str().ok())<br>
> + .and_then(|(track, property)| track.get_property(property))<br>
> + .map(|value| convert_string_to_ptr(value.to_string()))<br>
> + .unwrap_or(null_mut())<br>
> +}<br>
> +<br>
> +#[no_mangle]<br>
> +pub unsafe extern fn cuesheet_get_track_start(c_cuesheet: *mut CCuesheet,<br>
> + index : c_int) -> c_int {<br>
> + let cuesheet = &(*c_cuesheet).cuesheet;<br>
> +<br>
> + usize::try_from(index).ok()<br>
> + .and_then(|index| cuesheet.get_track_at_index(index))<br>
> + .map(|track| track.begin_time_in_seconds())<br>
> + .unwrap_or(-1)<br>
> +}<br>
> +<br>
> +unsafe fn convert_string_to_ptr(string: String) -> *mut c_char {<br>
> + if string.is_empty() {<br>
> + return std::ptr::null_mut();<br>
> + }<br>
> + CString::new(string).map(|ptr| ptr.into_raw()).unwrap_or(null_mut())<br>
> +}<br>
> +<br>
> +#[no_mangle]<br>
> +pub unsafe extern fn unref_cuesheet_string(text : *mut c_char) {<br>
> + let _ = CString::from_raw(text);<br>
> +}<br>
> +<br>
> +#[no_mangle]<br>
> +pub unsafe extern fn unref_cuesheet(cuesheet : *mut CCuesheet) {<br>
> + let _ = Box::from_raw(cuesheet);<br>
> +}<br>
> diff --git a/modules/demux/playlist/cuesheet/src/<a href="http://config.rs" rel="noreferrer" target="_blank">config.rs</a> b/modules/demux/playlist/cuesheet/src/<a href="http://config.rs" rel="noreferrer" target="_blank">config.rs</a><br>
> new file mode 100644<br>
> index 0000000000..118f046711<br>
> --- /dev/null<br>
> +++ b/modules/demux/playlist/cuesheet/src/<a href="http://config.rs" rel="noreferrer" target="_blank">config.rs</a><br>
> @@ -0,0 +1,103 @@<br>
> +use std::collections::HashMap;<br>
> +use std::borrow::Borrow;<br>
> +<br>
> +#[derive(Clone, Debug)]<br>
> +pub struct CuesheetTrack {<br>
> + pub item_type : String,<br>
> + pub item_position : u8,<br>
> + pub item_performer : String,<br>
> + pub item_title : String,<br>
> + pub item_begin_duration : String<br>
> +}<br>
> +<br>
> +impl Default for CuesheetTrack {<br>
> + fn default() -> Self {<br>
> + CuesheetTrack {<br>
> + item_type: "".to_owned(),<br>
> + item_position: 0,<br>
> + item_performer: "".to_string(),<br>
> + item_title: "".to_string(),<br>
> + item_begin_duration: "".to_string()<br>
> + }<br>
> + }<br>
> +}<br>
> +<br>
> +impl CuesheetTrack {<br>
> + pub fn begin_time_in_seconds(&self) -> i32 {<br>
> + let start: &str = &self.item_begin_duration;<br>
> + let mut duration_parts: Vec<&str> = start.split(":").collect();<br>
> +<br>
> + let frame_rate = 75;<br>
> + let convert_min_to_s = 60;<br>
> + let mut start_time = 0;<br>
> +<br>
> + let time = duration_parts.pop()<br>
> + .and_then(|part| part.parse::<u32>().ok())<br>
> + .map(|part| start_time = part / frame_rate)<br>
> + .and_then(|_| duration_parts.pop())<br>
> + .and_then(|part| part.parse::<u32>().ok())<br>
> + .map(|part| start_time += part)<br>
> + .and_then(|_| duration_parts.pop())<br>
> + .and_then(|part| part.parse::<u32>().ok())<br>
> + .map(|part| start_time += part * convert_min_to_s);<br>
<br>
It seems weird to mix a functional syntax with mutation from outside<br>
the input parameters. Splitting here would probably help, but it might<br>
need an intermediate private function to benefit from early return.<br>
<br></blockquote><div><br></div><div>Will fix.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> + match time {<br>
> + Some(()) => start_time as i32,<br>
> + None => -1<br>
> + }<br>
<br>
Especially given this error handling here, which feels a bit alien<br>
since the matched Option doesn't actually store the value.<br>
<br>
> + }<br>
> +<br>
> + pub fn get_property(&self, key : &str) -> Option<&str> {<br>
> + match key.to_ascii_lowercase().as_str() {<br>
> + "type" => Some(self.item_type.as_str()),<br>
> + "begin_duration" => Some(self.item_begin_duration.as_str()),<br>
> + "performer" => Some(self.item_performer.as_str()),<br>
> + "title" => Some(self.item_title.as_str()),<br>
> + _ => None<br>
> + }<br>
> + }<br>
> +}<br>
> +<br>
> +#[derive(Clone, Debug)]<br>
> +pub struct Cuesheet {<br>
> + pub file_name : String,<br>
> + pub file_type : String,<br>
> + pub tracks : Vec<CuesheetTrack>,<br>
> + pub performer : String,<br>
> + pub title : String,<br>
> + pub comments : HashMap<String, String>,<br>
> + pub is_processing_tracks : bool<br>
> +}<br>
> +<br>
> +impl Default for Cuesheet {<br>
> + fn default() -> Self {<br>
> + Cuesheet {<br>
> + file_name: "".to_string(),<br>
> + file_type: "".to_string(),<br>
> + tracks: Vec::new(),<br>
> + performer: "".to_string(),<br>
> + title: "".to_string(),<br>
> + comments: HashMap::new(),<br>
> + is_processing_tracks: false<br>
> + }<br>
> + }<br>
> +}<br>
> +<br>
> +impl Cuesheet {<br>
> + pub fn get_track_at_index(&self, index : usize) -> Option<&CuesheetTrack> {<br>
> + if index >= self.tracks.len() {<br>
> + return None;<br>
> + }<br>
> + Some(self.tracks[index].borrow())<br>
> + }<br>
> +<br>
> + pub fn get_property(&self, key : &str) -> Option<&str> {<br>
> + match key.to_ascii_lowercase().as_str() {<br>
> + "file_name" => Some(self.file_name.as_str()),<br>
> + "file_type" => Some(self.file_type.as_str()),<br>
> + "performer" => Some(self.performer.as_str()),<br>
> + "title" => Some(self.title.as_str()),<br>
> + _ => None<br>
> + }<br>
> + }<br>
> +}<br>
> \ No newline at end of file<br>
> diff --git a/modules/demux/playlist/cuesheet/src/<a href="http://lib.rs" rel="noreferrer" target="_blank">lib.rs</a> b/modules/demux/playlist/cuesheet/src/<a href="http://lib.rs" rel="noreferrer" target="_blank">lib.rs</a><br>
> new file mode 100644<br>
> index 0000000000..c768d23825<br>
> --- /dev/null<br>
> +++ b/modules/demux/playlist/cuesheet/src/<a href="http://lib.rs" rel="noreferrer" target="_blank">lib.rs</a><br>
> @@ -0,0 +1,18 @@<br>
> +mod parse;<br>
> +mod config;<br>
> +<br>
> +#[cfg(test)]<br>
> +mod tests {<br>
> + use crate::parse::parse_line_alt;<br>
> + #[test]<br>
> + fn test_parse_line_alt() {<br>
> + let parts = parse_line_alt("FILE \"Live in Berlin\" MP3");<br>
> + assert_eq!(3, parts.len());<br>
> + assert_eq!("FILE", parts[0]);<br>
> + assert_eq!("Live in Berlin", parts[1]);<br>
> + assert_eq!("MP3", parts[2]);<br>
> + }<br>
<br>
Maybe tests with duration could be added? And incorrect<br>
duration / case that should be leading to errors.<br>
<br>
It seems that the test suite is not integrated with<br>
automake too. How is it planned to execute it?<br>
<br></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +}<br>
> +<br>
> +#[cfg(cargo_c)]<br>
> +mod capi;<br>
> \ No newline at end of file<br>
> diff --git a/modules/demux/playlist/cuesheet/src/<a href="http://parse.rs" rel="noreferrer" target="_blank">parse.rs</a> b/modules/demux/playlist/cuesheet/src/<a href="http://parse.rs" rel="noreferrer" target="_blank">parse.rs</a><br>
> new file mode 100644<br>
> index 0000000000..b570638325<br>
> --- /dev/null<br>
> +++ b/modules/demux/playlist/cuesheet/src/<a href="http://parse.rs" rel="noreferrer" target="_blank">parse.rs</a><br>
> @@ -0,0 +1,75 @@<br>
> +use crate::config::*;<br>
> +<br>
> +pub fn parse_line(line: &str) -> Vec<String> {<br>
> + let characters = line.trim().chars();<br>
> + let mut parts: Vec<String> = Vec::new();<br>
> + let mut inside_quotes: bool = false;<br>
> + let mut temp_str: String = String::new();<br>
> + for char in characters {<br>
> + if char == '"' {<br>
> + inside_quotes = !inside_quotes;<br>
> + } else if inside_quotes {<br>
> + temp_str.push(char);<br>
> + } else if char.is_whitespace() {<br>
> + parts.push(temp_str.clone());<br>
> + temp_str.clear();<br>
> + } else {<br>
> + temp_str.push(char);<br>
> + }<br>
> + }<br>
> + parts.push(temp_str.clone());<br>
> + parts<br>
> +}<br>
> +<br>
> +impl Cuesheet {<br>
> + pub fn process_line(&mut self, line: &str) -> Option<()> {<br>
<br>
It seems to me that this could actually be an implementation of<br>
the trait FromStr, and thus usage could be done though the more<br>
idiomatic str/String::parse().<br>
<br></blockquote><div><br></div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + let parts = parse_line(line);<br>
> + let keyword = parts[0].as_str();<br>
> + if !self.is_processing_tracks {<br>
> + match keyword {<br>
> + "FILE" => {<br>
> + self.file_name = parts[1].to_owned();<br>
> + self.file_type = parts[2].to_owned();<br>
> + }<br>
> + "TITLE" => self.title = parts[1].to_owned(),<br>
> + "PERFORMER" => self.performer = parts[1].to_owned(),<br>
> + "REM" => {<br>
> + self.comments<br>
> + .insert(parts[1].to_owned().to_lowercase(), parts[2].to_owned());<br>
> + }<br>
> + "TRACK" => self.is_processing_tracks = true,<br>
> + _ => {}<br>
> + };<br>
> + }<br>
> + if self.is_processing_tracks {<br>
> + if keyword == "TRACK" {<br>
> + let mut track = CuesheetTrack::default();<br>
> + track.item_position = parts[1].to_owned().parse::<u8>().ok()?;<br>
> + track.item_type = parts[2].to_owned();<br>
> + self.tracks.push(track);<br>
> + } else {<br>
> + self.tracks.last_mut()?.process_track(line);<br>
> + }<br>
> + }<br>
> + Some(())<br>
> + }<br>
> +}<br>
> +<br>
> +impl CuesheetTrack {<br>
> + fn process_track(&mut self, line: &str) -> Option<()> {<br>
<br>
Same as above.<br></blockquote><div><br></div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + let parts = parse_line(line);<br>
> + let keyword = parts[0].as_str();<br>
> + match keyword {<br>
> + "TITLE" => self.item_title = parts[1].to_owned(),<br>
> + "PERFORMER" => self.item_performer = parts[1].to_owned(),<br>
> + "INDEX" => {<br>
> + let index_position = parts[1].to_owned().parse::<u8>().ok()?;<br>
> + if index_position == 1 {<br>
> + self.item_begin_duration = parts[2].to_owned();<br>
> + }<br>
> + }<br>
> + _ => {}<br>
> + };<br>
> + Some(())<br>
> + }<br>
> +}<br>
> --<br>
> 2.25.1<br>
<br>
Regards,<br>
--<br>
Alexandre Janniaux<br>
Videolabs<br>
_______________________________________________<br>
vlc-devel mailing list<br>
To unsubscribe or modify your subscription options:<br>
<a href="https://mailman.videolan.org/listinfo/vlc-devel" rel="noreferrer" target="_blank">https://mailman.videolan.org/listinfo/vlc-devel</a></blockquote></div></div>