Byte skipping between ID3 and MP3 blocks

[localthomas]

As discussed in #28, junk bytes between initial metadata blocks and following MP3 frames are skipped with this implementation change.

The new function search_for_frame_sync uses a reader to search for the first appearence of frame sync bits. This function is then used for probing the file type (Probe::guess_inner) and reading MP3 data (mp3::read::read_from). All three additions are checked by three new tests. The tests for the new function and the Probe are in the same module as the implementation, while the mp3::read_from test is separted into the tests and uses an example *.mp3 file.

Please comment on the positioning of the tests and the changes on the two existing functions.

---

I am a bit confused by the Probe::guess_inner function:

• Why does it return Result<Option<FileType>>?
• What is the logical difference between the return values Ok(None) and Err(LoftyError::UnknownFormat)?
• Why is the return value Err(LoftyError::UnknownFormat) from FileType::from_buffer_inner transfomed into Ok(None), but internal errors such as buf_len < 3 use Err(LoftyError::UnknownFormat)?
• Would a return type of Result<FileType> make more sense?

[Serial-ATA]

Thanks for working on this!

The locations of the tests are fine, could you rename the asset to something like junk.mp3 just to make its purpose a little clearer. Those a.* names came from awhile ago, I'll have to update those eventually.

Why does it return Result<Option>?

Since the only error that can occur is std::io::Error:

lofty-rs/src/probe.rs

Lines 138 to 142 in c88cad0

	/// # Errors
	///
	/// All errors that occur within this function are [`std::io::Error`].
	/// If an error does occur, there is likely an issue with the provided
	/// reader, and the entire `Probe` should be discarded.

We just use Option::or to set the file type rather than error on no type being found.

What is the logical difference between the return values Ok(None) and Err(LoftyError::UnknownFormat)?

Ok(None) means there were no io errors, but we also couldn't determine the format.

Why is the return value Err(LoftyError::UnknownFormat) from FileType::from_buffer_inner transfomed into Ok(None), but internal errors such as buf_len < 3 use Err(LoftyError::UnknownFormat)?

That's actually a mistake, the length check shouldn't be there at all. I forgot to remove those when I chose to only return io::Error. Good catch. Could you remove the instances of Err(LoftyError::*) and just return Ok(None) in their place?

[Serial-ATA]

Looking good! Just some minor things.

[localthomas]

I have updated the pull request according to the code review and also fixed the clippy errors (very good to hve that in CI).

---

Would it be useful to also change the type definition of Probe::guess_file_type to std::io::Result to make the comment about "All errors that occur within this function are std::io::Error." irrelevant?
It would change the public API, so this might not be desirable to do in this pull request.

[Serial-ATA]

Yeah, I'll save that change for the next major version. This can be included in the next minor release.

Also, could you revert that clippy lint for the negative multiplication? I think that was a lot more readable. You can just throw an #[allow(clippy::neg_multiply)] in there.

[Serial-ATA]

Thanks!