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

[Alexandre Janniaux]

Hi

On Fri, Sep 11, 2020 at 06:54:45PM +0530, Kartik Ohri wrote:
> > > +    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.

If it doesn't need UTF-8, maybe it can use a different String
type so as to avoid the error case? In any case, if the file
is not valid it should probably not silently drop some lines

> > > +#[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.

In cases in which they are equivalent, it's still clearer with
a cast to `c_int` when you want to return a `c_int`. ;)

> > > +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.
>

Actually you're right, no need for FromStr here then.

> > > +        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.

Indeed too, forget about my remark. :)

> > > +        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

Regards,
--
Alexandre Janniaux
Videolabs