Serial-ATA / lofty-rs

Audio metadata library
Apache License 2.0
179 stars 34 forks source link

Adding picture corrupts file #350

Closed probablykasper closed 2 months ago

probablykasper commented 4 months ago

Reproducer

use lofty::{Picture, Probe, TagExt, TaggedFileExt};
use std::fs::{self, File};
use std::io::{BufReader, Cursor};

fn main() {
  let song_path = "song.opus";
  let bugged_song_path = "bugged.opus";
  let image_path = "3000.jpg";

  fs::copy(song_path, bugged_song_path).unwrap();

  // Open
  let probe = probe_open(bugged_song_path);
  let mut tagged_file = probe.read().unwrap();
  let tag = tagged_file.tag_mut(lofty::TagType::VorbisComments).unwrap();

  // Add picture and save
  tag.set_picture(0, get_picture(image_path));
  tag.save_to_path(bugged_song_path).unwrap();

  // Open again
  let probe = probe_open(bugged_song_path);
  if let Err(e) = probe.read() {
    eprintln!("ERROR: {:#?}", e.kind());
  }
}

fn probe_open(path: &str) -> Probe<BufReader<File>> {
  let parse_options = lofty::ParseOptions::new()
    .read_properties(false)
    .parsing_mode(lofty::ParsingMode::Strict);
  let probe = lofty::Probe::open(path)
    .unwrap()
    .options(parse_options.clone());
  probe
}

fn get_picture(image_path: &str) -> Picture {
  let new_bytes = fs::read(image_path).unwrap();
  let mut reader = Cursor::new(new_bytes);
  let picture = lofty::Picture::from_reader(&mut reader).unwrap();
  picture
}

Summary

When I add this JPG to this Opus file, it seems to corrupt the image. After saving and reading the file again, I get this error from probe.read():

called `Result::unwrap()` on an `Err` value: NotAPicture

UnexpectedEof

I've also been seeing this error:

OggPage(Io(Error { kind: UnexpectedEof, message: "failed to fill whole buffer" }))

This error is what I originally ran into and tried to reproduce, but instead I got to the NotAPicture error. It seems like the file got destroyed in this case, turning into a few hundred kb. Not sure how the two errors relate though.

Assets

files.zip

Serial-ATA commented 4 months ago

Hm, it's not immediately obvious what's causing this.

I tried adding a similar sized (file size) image and it worked, as did another 3000x3000 image. For some reason though, this image causes a size mismatch which leads the parser to continue a few bytes into the next packet. The file is otherwise valid and parses just fine in Relaxed/BestAttempt mode.

UnexpectedEof

I've also been seeing this error:

OggPage(Io(Error { kind: UnexpectedEof, message: "failed to fill whole buffer" }))

This is what I'd expect to happen in the event of a size mismatch. Strange that it just works all the way up until it comes time to do the base64 decode.

I'll have to look more into this another time, was hoping this would be an obvious quick fix. :smile:

probablykasper commented 4 months ago

Good to know the issues probably have the same root cause.

I tried adding the JPG to a different Opus file, and that worked too

Serial-ATA commented 4 months ago

I tried adding the JPG to a different Opus file, and that worked too

Another thing, if you write only the picture and no other tags, it's perfectly fine.

probablykasper commented 4 months ago

What about if the old tag is deleted from the file before the new one is written?

Serial-ATA commented 4 months ago

Removing the tag doesn't seem to make a difference, it's something with this combination of items.

What's interesting is if I remove any one of the text fields (title, artist, album) the issue goes away. If I take away the track number though, it makes no difference.

Serial-ATA commented 2 months ago

Finally managed to fix this. Still not really sure what the issue was, but it was fixed after just rewriting the packet pagination. Guess with such a large packet something was just slowly sliding out of sync.

probablykasper commented 2 months ago

Niiiiiiiiice, it seems to work great! You're awesome