Add support for WAVE files

[postlund]

No tests so far, need good example files.

Fixes #49

[sampsyo]

Cool; if that's as easy as it truly is, then that's awesome! You're right that some tests would be nice; then we can ship this with the next version.

[postlund]

I added some tests (converted files from one of the other file formats with ffmpeg) and came to the conclusion that it's not that easy. It works fine as long as metadata is attached as ID3, but I ended up with metadata in RIFF info tags which mutagen doesn't support. Currently trying to navigate the mutagen code base and will make an attempt to implement support for that.

[postlund]

So, made an initial attempt and pushed a PR here: quodlibet/mutagen#538. The development pace seems to be rather low right now, so let's see where it goes.

Technically, I would argue that this PR is still "finished" in the sense that it reads metadata via the interface provided by mutagen. The fact that mutagen doesn't support metadata in all forms out there is unfortunate of course, but not really a blocker. What is a blocker however is that I need a test file with metadata that mutagen supports for tests, since we don't mock that interface here. I have a small script in pyatv (here) to generate dummy audio files (that I used to test AirPlay streaming) which I can extend with missing fields to generate compatible files.

Question now is: which way do you want me to go? I can generate the test file with ffmpeg, but that requires the PR I made to be merged and a new release of mutagen to be made. Alternatively I can generate files that are compatible with mutagen today, that however means we won't support the format I add support for in the PR until later.

[sampsyo]

Aha, thanks for exploring and for explaining the current situation on the Mutagen side!

My opinion is that it's worth adding support to MediaFile now for the metadata style that Mutagen already supports (if I understand correctly, that's ID3 tags, as opposed to RIFF/INFO chunks, a la quodlibet/mutagen#207). That's a clean win for a specific use case that we can support already, and there's no harm in generating test files using Mutagen to support exactly that use case. We can then come back around and support the RIFF/INFO version later when that support appears in Mutagen too.

[postlund]

I would say that we are on the same page then 😄 What is currently supported by mutagen is good enough for me right now, better support can come later. It's only a matter of which version of mutagen that is installed. I will prepare test files by tonight and see if I can push an update. One problem is that only a few metadata fields are supported as RIFF/INFO is really Exif, which lacks lots of the types that ID3 has. So it's tricky to make the tests pass as they are kinda written for all fields to exist. But I'll see what I can come up with.

[postlund]

I've pushed an updated with some test data now, please have look! 👀

[postlund]

Ok, so the action for python 2.7 fails because WAVE support was added after mutagen dropped support for python 2. If you still want to support python 2 in mediafile I guess we can make a hack not support WAVE in that case, but I would strongly suggest to drop python 2 support.

[sampsyo]

Nice; this all looks pretty good so far! One small comment on the test code: it's not immediately obvious to me why the test class needs to override the methods test_write_counters_without_total, test_delete_year, and test_delete_packed_total. Maybe some docstrings in the code could help explain to future travelers why they're there?

You're right about dropping Python 2 support. I have opened #51 to that effect. Any chance you could give that a quick code review? If everything's in order, we can merge that immediately.

[postlund]

Nice; this all looks pretty good so far! One small comment on the test code: it's not immediately obvious to me why the test class needs to override the methods test_write_counters_without_total, test_delete_year, and test_delete_packed_total. Maybe some docstrings in the code could help explain to future travelers why they're there?

They depend on fields that aren't present in WAVE, like disc for instance, so I had to remove that. I can add some comments about that.

You're right about dropping Python 2 support. I have opened #51 to that effect. Any chance you could give that a quick code review? If everything's in order, we can merge that immediately.

I'll take a look!

[postlund]

I added some comments regarding the test cases I had to override as well as rebased on master, so I believe we are all set now.

[sampsyo]

Fantastic; thank you!! This looks really great. Nice work on this! ✨

[postlund]

Perfect! I'll eagerly be waiting for a new release so I can make switch in pyatv. Do you have any plans for when the next one is due?

[sampsyo]

I've released v0.8.0!
https://pypi.org/project/mediafile/

[postlund]

Awesome, thank you! 👍