Can't parse opus file with TRACKNUMBER equal to "<current>/<total>"

[RocketRide9]

Reproducer

use std::fs::File;

use lofty::config::ParseOptions;
use lofty::file::AudioFile;
use lofty::ogg::{self};
use lofty::tag::Accessor;

fn main() {
    let wrong_opus = "./wrong.opus";
    
    let mut file_content = File::open(wrong_opus).unwrap();
    let opus_file = ogg::OpusFile::read_from(&mut file_content, ParseOptions::new()).unwrap();
    let tag = opus_file.vorbis_comments().track().unwrap();
    
    println!("{tag}");
}

Summary

Previously reported here: https://codeberg.org/edestcroix/Recordbox/issues/72

I did a little investigation and it seems to me that this library attempts tot parse "1/7" as u32 here: https://github.com/Serial-ATA/lofty-rs/blob/main/lofty/src/ogg/tag.rs#L336

Frankly, I don't know well how tags works, but this file was produced by converting mp3 using ffmpeg and apps like Tagger, Rhythmbox and Ear Tag can read this tag properly.

Expected behavior

No response

Assets

opus file with "1/7": https://codeberg.org/attachments/cd2f08e1-a57a-47e6-9769-27820a4e75a6

this and another file inside hex editor:

[Serial-ATA]

Hello!

Yeah, this was reported recently and someone created a workaround for x/y and vinyl track numbers:

#431 (comment)

TRACKNUMBER=x/y technically isn't valid, but there's no reason to not support it. I just don't have any time to work on Lofty at the moment. I'll get around to this when I can.

[RocketRide9]

BTW I wrote a program to recursively change tags in all opus files to make them more friendly towards lofty. Based on #431 (comment) . Run inside target directory without any terminal arguments.

EDIT: DO NOT use it. It seem to break some files

Details

use std::fs::OpenOptions;

use lofty::config::{ParseOptions, WriteOptions};
use lofty::file::AudioFile;
use lofty::ogg::{self};
use lofty::tag::{Accessor, MergeTag, SplitTag, Tag};
use lofty::tag::ItemKey::*;
use regex::Regex;
use walkdir::WalkDir;

pub(crate) fn fix_track_numbering(tags: &mut Tag) -> bool {
    if tags.track().is_some() {
        return true;
    }
    if replace_total_track_numbering(tags).is_ok() {
        return true;
    }
    // if replace_vinyl_track_numbering(tags).is_ok() {
    //     return true;
    // }
    false
}

/* 
    fn replace_vinyl_track_numbering(tags: &mut Tag) -> Result<(), Error> {
        let track = tags.get_string(&TrackNumber).ok_or_else(|| {
            error(
                "replace vinyl track numbering",
                "No track number string".to_owned(),
            )
        })?;
        let (disc_number, track_number) = get_numeric_from_vinyl_format(track).ok_or_else(|| {
            error(
                "replace vinyl track numbering",
                "Not vinyl format".to_owned(),
            )
        })?;
        trace!(
            "Replacing vinyl track ({track}) with numeric: track {track_number}, disc {disc_number}"
        );
        tags.set_disk(disc_number);
        tags.set_track(track_number);
        Ok(())
    }
*/

fn replace_total_track_numbering(tags: &mut Tag) -> Result<(), ()> {
    if let Some(track) = tags.get_string(&TrackNumber) {
        if let Some((track_number, track_total)) = get_numeric_from_total_format(track) {
            tags.set_track(track_number);
            tags.set_track_total(track_total);
            return Ok(());
        }
    }
    return Err(());
}

/*
    pub(crate) fn get_numeric_from_vinyl_format(input: &str) -> Option<(u32, u32)> {
        let re = Regex::new(r"^([A-Z])(\d+)$").ok()?;
        let captures = re.captures(input)?;
        let disc_letter = captures.get(1)?.as_str().chars().next()?;
        let track_number: u32 = captures.get(2)?.as_str().parse().ok()?;
        let disc_number = letter_to_number(disc_letter)?;
        Some((disc_number, track_number))
    }
*/

pub(crate) fn get_numeric_from_total_format(input: &str) -> Option<(u32, u32)> {
    let re = Regex::new(r"^(\d+)/(\d+)$").ok()?;
    let captures = re.captures(input)?;
    let track_number: u32 = captures.get(1)?.as_str().parse().ok()?;
    let track_total: u32 = captures.get(2)?.as_str().parse().ok()?;
    Some((track_number, track_total))
}

fn main() {
    let current_dir = std::env::current_dir().expect("Failed to get current directory");

    // assert_eq!(current_dir.file_name(), Some(OsStr::new("Music")));
    
    for entry in WalkDir::new(current_dir) {
        match entry {
            Ok(file) => {
                if file.path().extension().and_then(|s| s.to_str()) == Some("opus") {
                    println!("{}", file.path().display());
                    let mut file = OpenOptions::new().read(true).write(true).open(file.path()).unwrap();
                    let mut opus_file = ogg::OpusFile::read_from(&mut file, ParseOptions::new()).unwrap();
                    
                    let comments = opus_file.vorbis_comments().clone();
                    let (remainder, mut tag) = comments.split_tag();
                    fix_track_numbering(&mut tag);
                    
                    let new_comments = remainder.merge_tag(tag);
                    opus_file.set_vorbis_comments(new_comments);
                    opus_file.save_to(&mut file, WriteOptions::default()).unwrap();
                }
            }
            Err(e) => eprintln!("Error reading entry: {}", e),
        }
    }
}

[RocketRide9]

The code that I send above breaks one of my files. I found minimal reproducer, which doesn't even edit tags:

use std::fs::OpenOptions;

use lofty::config::{ParseOptions, WriteOptions};
use lofty::file::AudioFile;
use lofty::ogg::{self};
use lofty::tag::{Accessor, MergeTag, SplitTag, Tag};
use lofty::tag::ItemKey::*;

fn main() {
    let path = "break_this.opus";
    let mut file = OpenOptions::new().read(true).write(true).open(path).unwrap();
    let mut opus_file = ogg::OpusFile::read_from(&mut file, ParseOptions::new()).unwrap();
    
    let comments = opus_file.vorbis_comments().clone();
    let (remainder, tag) = comments.split_tag();
    
    let new_comments = remainder.merge_tag(tag);
    opus_file.set_vorbis_comments(new_comments);
    opus_file.save_to(&mut file, WriteOptions::default()).unwrap();
}

File: break_this.zip

@Serial-ATA, could you check if I'm doing something wrong here, when you will have free time?

EDIT: and by "breaks" I mean other programs can't recognize tags in this file and running this program again show this error:

called `Result::unwrap()` on an `Err` value: FileDecoding(Vorbis: "File missing magic signature")
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[Serial-ATA]

Thanks, do you have the original file? break_this.opus is the already corrupted one

[RocketRide9]

here:
break_this.zip

[Serial-ATA]

Looks like I introduced a bug in #475. The size of the tag in this file is 256 bytes. When you write the tag, the size is being written as 0. Should be a simple fix, I'll look into it when I have time.

[Serial-ATA]

@RocketRide9 Actually, looks like I got things backwards. Were you using the git version? It looks like #475 fixed the corruption.

[RocketRide9]

@Serial-ATA I was using latest from crates io. Should be 0.21.1

[Serial-ATA]

I just published 0.22.0 a few hours ago, should be good now

[Serial-ATA]

Replaced by #499