Accept both #AUDIO and #MP3 as header tag for the audio file.

[bohning]

Since USDX supports many different audio formats (encodings + containers), the original #MP3 header tag should be superseded by the more general #AUDIO header tag. While #MP3 is not deprecated, USDX should support both tags for specifying the audio file.

[barbeque-squared]

Long review is long. I get the usecase and support it, but there's some undefined handling when both tags are present and some other edge cases which I really don't know what USDX should do with. The underlying problem is that USDX currently isn't really set up for handling even this relatively simple change, hence I propose to first address the underlying problem and then revisiting this.

[barbeque-squared]

I'm sure this will work if we define either MP3 or AUDIO, but what happens in scenarios like this:

• first AUDIO, then MP3, but AUDIO is a non-existing file
• first AUDIO, then MP3, but MP3 is a non-existing file
• first AUDIO, then MP3, but neither exists
• the above three, but MP3 is earlier in the txt file
• is any of this dependent on the existence and, if it exists, the value of a VERSION tag

what kind of behaviour should the error messages have?

in all of the above scenarios (plus if the file only defines one of them): what if we edit it in the editor and want to save it? what should it write?

Especially that last one is why I requested changes, because the proposed PR implementation will actually destroy one of the tags! (I think? I haven't actually compiled it but I'm pretty sure it does. Correct me if I'm wrong though) See below for a longer explanation. Ideally all that refactoring comes first in a new PR, and only once that's in do we come back to this one. Said refactoring doesn't need to be 100% perfect straight away, but the bulk of it should not be part of a PR that also adds new functionality.

---

It's a bit of a chicken-egg problem, but I think one thing is very clear: the current way of reading/validating/writing tags is insufficient going forward. I think it would be better to first redo the current behaviour (no AUDIO handling yet) into something like this:

• read ALL tags into some kind of dictionary, but without any validation applied
• figure out what version/tag handling logic we actually need to apply (this step wouldn't do much initially)
• actually handle the tags, do validation, etc

this does assume that there currently isn't some sneaky/hacky behaviour where the order of the tags matters.

I get that the proposed minimal change would make it somewhat work in the short term, but we're going to need that version logic at some point anyway. Might as well do it before we start adding stuff. It's possible that this also means big changes to add some extra layer of abstraction/subclassing/overriding in what is currently USong, because the rest of USDX should just be able to do "give me the audio file" and "save this" and not have to care about how it was actually represented in the txt.

---

PS I'm not actually sure if my proposed refactoring will solve every situation in the future. But at the very least it would split out the parsing and the validating, which is still an improvement.
PS2 if this entire functionality does depend on a VERSION tag, this also opens up a whole new can of worms because of the special treatment of P3 which you probably don't want at some point anymore.
PS3 if this kind of refactoring is a bit too much Pascal, I'll do it once I find some time. There's lots of places in USDX in need of it, but if this one specifically is preventing people from contributing, obviously it's going to get more priority.

[s09bQ5]

How about #VIDEO tag without #AUDIO/#MP3 tag? Having both muxed into one file is the normal case when using an official video and people either have to extract the audio from the video or mention the same file with both tags.

[bohning]

@barbeque-squared thanks for your detailed answer. I fully agree, this was a quick shot without deep-diving into the consquences, so maybe this pull request can even be deleted (since it is so small and will not stay this way anyways) and we come back to this later.

@s09bQ5 It may be the default case, but there are many exceptions to this, and some even make the opposite the default - use HQ audio files and fit the video to it (using #VIDEOGAP). How would that work for those songs if we got rid of #MP3/#AUDIO?

[s09bQ5]

I didn't want to get rid of #AUDIO/#MP3 completely. I just want to support #VIDEO without them.

[bohning]

Ah, I see. That’s a good idea IMHO.

[barbeque-squared]

I'll probably just leave it as draft or something for the time being. no intention to ever merge it, but might as well keep it open until there's a different PR addressing things properly (this is mostly to be able to find it back until that time)

I also quite like s09bQ5's idea about just using VIDEO if neither of the tags are present. I already do this for most things I download through the syncer anyway (mostly to keep a reference to the original video somewhere, but also just less files)