Proper way to setup Popularimeter?

[UnknownSuperficialNight]

So for insertion I've done

match tag.tag_type() {
    // Check if the tag is an ID3 tag
    TagType::Id3v2 => {
        // Parse the rating value
        let rating_value: u8 = match value.parse() {
            Ok(val) => val,
            Err(_) => {
                return Err(LoftyError::new(ErrorKind::Id3v2(Id3v2Error::new(Id3v2ErrorKind::BadFrame("POPM".to_string(), "Input was not a valid rating value"))))); // Adjust with a meaningful error message
            }
        };

        // Create a PopularimeterFrame
        let popularimeter = PopularimeterFrame::new(String::new(), rating_value, 0).as_bytes()?;

        // Insert the new PopularimeterFrame into the tag
        tag.insert(TagItem::new(ItemKey::Popularimeter, ItemValue::Binary(popularimeter)));
    }
    _ => {
        tag.insert_text(ItemKey::Popularimeter, value.clone());
    }
}

and this W.I.P for reading

metadata.rating = match tag.tag_type() {
    // Check if the tag is an ID3 tag
    TagType::Id3v2 => {
        // Attempt to retrieve the binary data for the Popularimeter frame
        let binary_data = tag.get_binary(&ItemKey::Popularimeter, true);

        // Check if binary_data is Some and has sufficient length
        if let Some(data) = binary_data {
            // Create a cursor to read from the binary data
            let mut reader = Cursor::new(data);

            // Parse the binary data into a PopularimeterFrame
            match PopularimeterFrame::parse(&mut reader, FrameFlags::default()) {
                Ok(popularimeter) => Some(popularimeter.rating),
                Err(e) => {
                    println!("Error parsing PopularimeterFrame: {:?}", e); // Print the error
                    None // Return None on parse error
                }
            }
        } else {
            // Print an error if the binary data retrieval fails
            println!("Error: Popularimeter frame not found in the tag.");
            None // Return None if the frame is not found
        }
    }
    _ => tag.get_string(&ItemKey::Popularimeter).and_then(|v| v.parse::<u8>().ok()),
};

however, so far reading the Popularimeter gets me an error: Error: Popularimeter frame not found in the tag.

even though the tag is there if I use external editors popularimeter = ":10:0"

So what is the correct way to get this working?

[Serial-ATA]

So the docs about the POPM frames are actually outdated. They haven't been converted into TagItems since 0.20 (see #401).

The issue is some formats just use a single numeric star rating, others use an email + star rating, then there's ID3v2 which uses a star rating, email, and a play counter. I guess I could encode them in a special string format like email|rating|play count and then parse the information back out when converting/writing them. Then add Tag::popularimeter() to handle the parsing for callers.

So I'm imagining:

// Rating scales are also different between formats, so
// they'll need to be unified in some way.
//
// Unfortunately, that *probably* means disallowing fractional ratings, since
// as far as I know, only some formats support them.
enum StarRating {
    Zero,
    One,
    Two,
    Three,
    Four,
    Five,
}

impl TryFrom<char> for StarRating {
    type Error = ();

    fn try_from(value: char) -> Result<Self, Self::Error> {
        match value {
            '0' => Ok(Self::Zero),
            '1' => Ok(Self::One),
            '2' => Ok(Self::Two),
            '3' => Ok(Self::Three),
            '4' => Ok(Self::Four),
            '5' => Ok(Self::Five),
            _ => Err(()),
        }
    }
}

impl Display for StarRating {
    fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), Error> {
        write!(
            f,
            "{}",
            match value {
                Self::Zero => '0',
                Self::One => '1',
                Self::Two => '2',
                Self::Three => '3',
                Self::Four => '4',
                Self::Five => '5',
            }
        )
    }
}

// Generic popularimeter, almost identical to `PopularimeterFrame`, since
// it's the most feature packed implementation
struct Popularimeter {
    email: Option<String>,
    rating: StarRating,
    /// NOTE: Only used in ID3v2, ...
    counter: u64,
}

impl Display for Popularimeter {
    fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), Error> {
        write!(f, "{}|{}|{}", self.email.unwrap_or(""), self.rating, self.counter)
    }
}

impl Popularimeter {
    // won't accept partial inputs, to encourage users to properly encode them
    pub fn parse(input: &str) -> Option<Self> {
        let mut parts = input.splitn(3, '|');
        let Some(email) = parts.next() else {
            return None;
        };
        let Some(rating) = parts
            .next()
            .and_then(|rating_str| rating_str.chars().next())
            .and_then(|rating| StarRating::try_from(rating).ok())
        else {
            return None;
        };
        let Some(counter) = parts.next().and_then(|counter| counter.parse().ok()) else {
            return None;
        };

        Some(Self {
            email,
            rating,
            counter,
        })
    }
}

Conversion

So if you read an ID3v2 tag with a POPM frame:

PopularimeterFrame {
    email: "[email protected]",
    rating: 128,
    counter: 50,
}

It gets converted into a Popularimeter:

let popularimeter = Popularimeter {
    email: "[email protected]",
    rating: StarRating::Three,
    counter: 50,
};

Then into a string TagItem:

TagItem::new(ItemKey::Popularimeter, ItemValue::Text(popularimeter.to_string()));

Usage

Then the docs on ItemKey::Popularimeter would have to point users to Tag::popularimeter(), which would handle parsing for them:

impl Tag {
    pub fn popularimeter(&self) -> Option<Popularimeter> {
        self.get_string(ItemKey::Popularimeter).and_then(Popularimeter::parse)
    }
}

And then encourage insertion like:

tag.insert_text(ItemKey::Popularimeter, Popularimeter {
    email: "[email protected]",
    rating: StarRating::Three,
    counter: 50,
}.to_string());

Rather than manual construction of the encoded string.

Let me know if that flow makes sense to you. Sorry for the code dump, just jotting down the first thing that comes to mind.

[obust]

But it looks like the ID3v2 spec expect a binary frame with null terminated string email.
https://id3.org/id3v2.3.0

So if we want to be compatible with other software then it should be stored as binary and not text right ?

I have similarly tried to write POPM like this:

    #[test]
    fn test_popm() {
        let (_temp_dir, test_file) = setup_test_file();

        let path = Path::new(&test_file); // some test mp3 file

        let mut tagged_file = read_from_path(path)
            .map_err(|e| format!("Failed to read file: {}", e)).unwrap();

        // Get or create a tag
        let tag = match tagged_file.primary_tag_mut() {
            Some(primary_tag) => primary_tag,
            None => {
                if let Some(first_tag) = tagged_file.first_tag_mut() {
                    first_tag
                } else {
                    // Create a new tag for the file type
                    tagged_file.insert_tag(lofty::tag::Tag::new(tagged_file.primary_tag_type()));
                    tagged_file.primary_tag_mut().unwrap()
                }
            }
        };

        // Add POPM item
        let popm1 = PopularimeterFrame::new("[email protected]".to_string(), 51,0);
        tag.insert(TagItem::new(ItemKey::Popularimeter, ItemValue::Binary(popm1.as_bytes().unwrap())));

        // Read POPM item
        let mut data = tag.get_binary(&ItemKey::Popularimeter, false).unwrap();
        let popm2 = PopularimeterFrame::parse(&mut data, FrameFlags::default()).unwrap();
        assert_eq!(popm2.rating, popm1.rating);

        // Save to file
        tagged_file.save_to_path(path, WriteOptions::default())
            .map_err(|e| format!("Failed to save metadata: {}", e)).expect("Failed to save metadata");


        let tagged_file = read_from_path(path)
            .map_err(|e| format!("Failed to read file: {}", e)).unwrap();

        let tag = tagged_file.primary_tag().or_else(|| tagged_file.first_tag()).unwrap();

        // Read POPM item from file
        if let Some(data) = tag.get_binary(&ItemKey::Popularimeter, false) {
            let popm3 = PopularimeterFrame::parse(&mut Cursor::new(data), FrameFlags::default()).unwrap();
            assert_eq!(popm3.rating, popm1.rating);
        } else {
            panic!("Popularimeter frame not found");
        }

    }

But it fails on panic!("Popularimeter frame not found")

Do you know why the POPM is not persisted to file ?

[Serial-ATA]

But it looks like the ID3v2 spec expect a binary frame with null terminated string email.

What I'm suggesting wouldn't effect the final encoding. That's handled here:

lofty-rs/lofty/src/id3/v2/items/popularimeter.rs

Line 124 in 2619847

content.extend(encode_text(&self.email, TextEncoding::Latin1, true));

This is just a generic format so you can safely pass popularimeter tags around.

Do you know why the POPM is not persisted to file ?

See #401, ItemKey::Popularimeter hasn't had a mapping for ID3v2 since 0.20. The docs are just out of date.

[obust]

Hmm, I am not sure I follow.

I realize that PopularimeterFrame.as_bytes() has the right encoding. Which is why I am trying to use that in the code snippet I shared.

Your suggestion seems to bypass the PopularimeterFrame:

tag.insert_text(ItemKey::Popularimeter, Popularimeter {
    email: "[email protected]",
    rating: StarRating::Three,
    counter: 50,
}.to_string());

this is equivalent to:

tag.insert_text(ItemKey::Popularimeter, "[email protected]|3|50".to_string())

and to read it back:

let pop: Popularimeter = self.get_string(ItemKey::Popularimeter).and_then(Popularimeter::parse)

That would not save it in the expected ID3v2 binary encoding. Or am I missing something ?

See #401, ItemKey::Popularimeter hasn't had a mapping for ID3v2 since 0.20. The docs are just out of date.

So this seems to be the main issue. In the meantime, is this the recommended alternative ? In which case, how do you read it back from file ?

Also as you mentioned there is no standard on how to save ratings. For example traktor uses a 0–255 rating. This makes the suggested StarRating rather arbitrary. I think we want a solution as general as possible and push the specialization on the user.

[Serial-ATA]

That would not save it in the expected ID3v2 binary encoding. Or am I missing something ?

Tag doesn't do any encoding itself, it defers to the concrete tag types at write time. For example, in the case of ID3v2, Tag::save_to() will call this:

lofty-rs/lofty/src/id3/v2/tag/conversion.rs

Line 420 in 2619847

pub(crate) fn tag_frames(tag: &Tag) -> impl Iterator<Item = Frame<'_>> {

Which converts the TagItems into Frames, which can then be passed into the same encoding functions used by Id3v2Tag::save_to().

So this seems to be the main issue. In the meantime, is #401 (reply in thread) the recommended alternative ?

Yeah, using Id3v2Tag directly will allow you to use PopularimeterFrame.

In which case, how do you read it back from file ?

If you're using a concrete file type like MpegFile, then you get direct access to its Id3v2Tag.

If you absolutely only want to use Tag, you could also do this, though it's pretty inefficient:

let tag: Tag = /* ... */
let mut popm = None;

if tag.tag_type() == TagType::Id3v2 {
    let mut id3v2: Id3v2Tag = tag.into();
    
    if let Some(Frame::Popularimeter(frame)) = id3v2.get(&FrameId::Valid("POPM".into())) {
        popm = Some(frame.clone());
    }

    tag = id3v2.into();
}

That works because internally, when you call lofty::read_from{_path}(), it will parse an Id3v2Tag then convert it into a Tag. Any frames that can't be converted will be retained in the original Id3v2Tag and stored immutably inside the Tag, so its information isn't lost between conversions.

Also as you mentioned there is no standard on how to save ratings. For example traktor uses a 0–255 rating. This makes the suggested StarRating rather arbitrary. I think we want a solution as general as possible and push the specialization on the user.

Unfortunately, there's no standard for ratings. AFAICT, ID3v2 is the only format that supports fractional ratings, but even then it's rarely supported, and when it is, it's often done with arbitrary ranges.

That's why I think it should just be whole number ratings that will just map to whatever is the most popular mapping in ID3v2. Then users needing fractional support are pushed to implement it themselves using Id3v2Tag directly.