Tag parsing

[NCrusher74]

Question...

I'm trying to wrap my head around the various Ints, UInts, endians, bit-shifting and whatnot, and not having much luck with it. To try to familiarize myself with how they work, I'm seeing how many of the various byte and string adapters from ID3TagEditor I can turn into extensions of String, Data, etc.

This is a UInt32 extension I found a few days ago when I first started looking at how to handle the [UInt8] <-> UInt32 conversion I was looking at needing:

    var byteArrayLittleEndian: [UInt8] {
        return [
            UInt8((self & 0xFF000000) >> 24),
            UInt8((self & 0x00FF0000) >> 16),
            UInt8((self & 0x0000FF00) >> 8),
            UInt8(self & 0x000000FF)
        ]
    }

This is the UInt32ToByteArrayAdapterUsingUnsafePointer.adapt method from ID3TagEditor, which I've turned into an extension for UInt32.

    var byteArrayWithUnsafePointer: [UInt8] {
        var currentUInt32 = self
        let bytes = withUnsafePointer(to: &currentUInt32) {
            $0.withMemoryRebound(to: UInt8.self, capacity: MemoryLayout<UInt32>.size) {
                Array(UnsafeBufferPointer(start: $0, count: MemoryLayout<UInt32>.size))
            }
        }
        return bytes.reversed()
    }

What is the difference? Is there a difference? And how do they differ from this extension of Data (part of the same set of extensions I found with the first example):

    var uint32: UInt32 {
        get {
            let i32array = self.withUnsafeBytes { $0.load(as: UInt32.self) }
            return i32array
        }
    }

[NCrusher74]

Okay, this is why I need to do this. ID3TagEditor just goes about things in a too complicated way. Does every single thing need its own protocol?

Anyway. I'm trying to simplify ID3FrameContentSizeParser, which is as follows:

    func parse(mp3: NSData, framePosition: Int, version: ID3Version) -> Int {
        var frameSize: UInt32 = getFrameSizeFrom(mp3: mp3, framePosition: framePosition, version: version)
        frameSize = decodeIfIsASynchsafeInteger(frameSize: frameSize, for: version)
        return Int(frameSize)
    }

    private func getFrameSizeFrom(mp3: NSData, framePosition: Int, version: ID3Version) -> UInt32 {
        let frameSizePosition = framePosition + id3FrameConfiguration.sizeOffsetFor(version: version)
        var frameSize: UInt32 = 0
        mp3.getBytes(&frameSize, range: NSMakeRange(frameSizePosition, 4))
        frameSize = frameSize.bigEndian & id3FrameConfiguration.sizeMaskFor(version: version)
        return frameSize
    }

    private func decodeIfIsASynchsafeInteger(frameSize: UInt32, for version: ID3Version) -> UInt32 {
        var newFrameSize = frameSize
        if version == .version4 {
            newFrameSize = synchsafeIntegerDecoder.decode(integer: frameSize)
        }
        return newFrameSize
    }

I've turned the SynchSafeIntegerDecoder into an extension of UInt32:

    var synchSafeDecode: UInt32 {
        var decodedInteger: UInt32 = 0
        var mask: UInt32 = 0x7F000000;

        while (mask != 0) {
            decodedInteger = decodedInteger >> 1;
            decodedInteger = decodedInteger | self & mask;
            mask >>= 8;
        }

        return decodedInteger;
    }

And then I simplified those three methods to parse the frame content size into a single method, changing the NSMakeRange portion the way we discussed the other day:

    internal func contentSize(framePosition: Int) throws -> Int {
        let frameSizePosition = framePosition + sizeOffset
        var frameSize: UInt32 = 0
        let mp3Data = try Data(contentsOf: self.mp3File.location)
        let frameData = mp3Data[frameSizePosition..<frameSizePosition+Int(frameSize)]
        frameSize = frameSize.bigEndian & sizeMask
        if self.version == .version24 {
            frameSize = frameSize.synchSafeDecode
        }
        return Int(frameSize)
    }

When it's all said and done, however, frameData isn't being used anywhere. Since the same can't be said about mp3.getBytes(&frameSize, range: NSMakeRange(frameSizePosition, 4)) clearly I've missed something, but I'm not familiar enough with how the NSMakeRange piece of things works to understand how to adapt it to Swift.

What am I missing?

[SDGGiesbrecht]

What is the difference? Is there a difference?

A UInt32 in actual bits as physically present in memory is this:

[01010101 10101011 01101010 10011101]

(Each group of eight bits is one byte.)

This line of code pulls out the first byte:

UInt8((self & 0xFF000000) >> 24)

It does it like this:

[01010101 10101011 01101010 10011101]
& (binary “and”; only bits that are “true” in both will be kept in the result)
[11111111 00000000 00000000 00000000] (in hex: FF 00 00 00)
=
[01010101 00000000 00000000 00000000]
(We’ve essentially kept the first byte as‐is, but obliterated the rest.)

>> 24 (slide everything 24 bits (i.e. 3 bytes) to the right
=
[00000000 00000000 00000000 01010101]

UInt8() (convert it to an 8‐byte value)
=
[01010101]
(We’ve essentially dropped the first three empty bytes.)

The other similar lines do the same thing with the other tree bytes to put them in their own separate UInt8s. Put them back in a row again as an array and you have this:

[01010101] [10101011] [01101010] [10011101]

That is the exact same bitstream we started with, except it’s composed of four separate small pieces, instead of one big piece. It should be lightning fast because those are all direct machine instructions.

The other way of doing things is to point at that section of memory, do a Jedi hand wave, and tell the compiler it is really [UInt8] instead of UInt32. That is what withUnsafe is about. It might have been marginally faster by effectively doing nothing at runtime, except that afterward it still needs to duplicate the temporary array in order to reverse it, so it ends up with wasteful allocations that probably far out‐weight the skipped machine instructions. It is also dangerous because the compiler cannot tell if what you are doing is valid or not.

In my opinion, the first extension is the better one.

[SDGGiesbrecht]

Does every single thing need its own protocol?

No. Upstream did that so they could swap the real implementation out for a fake implementation (a “mock”) during testing. In my opinion that just (a) made the tests nonsensical and hard to reason about, and (b) slowed everything down by forcing dynamic dispatch for every single method call.

[NCrusher74]

In my opinion, the first extension is the better one.

Okay, so they're basically the same, just the first is going about things in a more direct and bullet-proof manner, if I'm understanding you correctly?

So, if I were using it in code, (someUInt32).byteArrayLittleEndian would basically accomplish the same thing as (someUInt32).byteArrayWithUnsafePointer`, but would be a more solid choice?

[NCrusher74]

Does every single thing need its own protocol?

No. Upstream did that so they could swap the real implementation out for a fake implementation (a “mock”) during testing. In my opinion that just (a) made the tests nonsensical and hard to reason about, and (b) slowed everything down by forcing dynamic dispatch for every single method call.

I sort of suspect upstream was maybe doing ID3TagEditor as a learning project, and they'd just mastered protocol usage.

Because I know each time I have a "eureka!" moment with a new concept, I have a tendency to try to use that new concept everywhere for a while.

[SDGGiesbrecht]

When it's all said and done, however, frameData isn't being used anywhere.

var frameSize: UInt32 = 0
// ...
let frameData = mp3Data[frameSizePosition..<frameSizePosition+Int(frameSize)]

That pulls a zero‐length slice of data from the start of the size declaration and then does nothing with it.

Maybe those two lines should be changed to this instead:

let lengthOfSizeDeclaration = 4 // ? Or whatever it actually is.
let sizeDataRange = frameSizePosition ..< frameSizePosition + lengthOfSizeDeclaration
guard sizeDataRange.upperBound <= mp3Data.endIndex else {
  // If the data is corrupt and doesn’t even have room for a size declaration,
  // describe the size of whatever is actually there instead.
  return mp3Data.distance(from: frameSizePosition, to: mp3Data.endIndex)
}
let frameSizeData = mp3Data[sizeDataRange]
let frameSize = UInt32(parsing: frameSizeData)
}

It would probably also be good to change the framePosition parameter to a Data.Index or [UInt8].Index instead of an Int, whichever you plan on using.

[SDGGiesbrecht]

Okay, so they're basically the same, just the first is going about things in a more direct and bullet-proof manner, if I'm understanding you correctly?

Yes.

[NCrusher74]

Okay, I need to figure out which one is the LengthOfSizeDeclaration:

Here is what ID3TagEditor has in FrameConfiguration:

    private let headerSizesInBytes: [ID3Version : Int] = [
        .version2 : 6,
        .version3 : 10,
        .version4 : 10
    ]
    private let sizeOffsetInBytes: [ID3Version : Int] = [
        .version2 : 2,
        .version3 : 4,
        .version4 : 4
    ]
    private let sizeMask: [ID3Version : UInt32] = [
        .version2 : 0x00FFFFFF,
        .version3 : 0xFFFFFFFF,
        .version4 : 0xFFFFFFFF
    ]
    private let identifierSizeInBytes: [ID3Version : Int] = [
        .version2 : 3,
        .version3 : 4,
        .version4 : 4
    ]

which, in what I'm calling FrameProperties, I've simplified to:

    /// the version-dependent size of the frame header, in bytes
    internal var frameHeaderSize: Int {
        switch self.version {
            case .version22: return 6
            case .version23, .version24: return 10
        }
    }

    /// the version-dependent byte offset for frame size
    internal var sizeOffset: Int {
        switch self.version {
            case .version22: return 2
            case .version23, .version24: return 4
        }
    }

    /// I have no idea what this is for
    internal var sizeMask: UInt32 {
        switch self.version {
            case .version22: return 0x00FFFFFF
            case .version23, .version24: return 0xFFFFFFFF
        }
    }

    /// the version-dependent byte-count of the id3identifier
    internal var identifierSize: Int {
        switch self.version {
            case .version22: return 3
            case .version23, .version24: return 4
        }
    }

The spec has this to say for version2.2:

   The three character frame identifier is followed by a three byte size
   descriptor, making a total header size of six bytes in every frame.
   The size is calculated as framesize excluding frame identifier and
   size descriptor (frame size - 6).

And this for version 2.4:

     Frame ID      $xx xx xx xx  (four characters)
     Size      4 * %0xxxxxxx
     Flags         $xx xx

So...the LengthOfSizeDeclaration would be 3/4 depending on version. It doesn't appear to be required to be UInt32 in version 2.2 or version 2.3, but in version 2.4 it is, probably because of the implementation of the SynchSafe encode/decode stuff.

I don't think I actually have that listed in my FrameProperties yet.

[NCrusher74]

Different question:

I've got a bunch of what should probably be computed properties that are being handled as functions because try Data(contentsOf: mp3File.location) throws.

Is there somewhere I can handle the potentially thrown error from the outset so that I can use mp3Data in computed properties?

I tried adding this to Mp3File:

    internal func data() -> Data? {
        do {
            return try Data(contentsOf: self.location)
        } catch {
            Mp3File.Error.CannotReadFile
        }; return nil
    }

but I get a warning that Result of call to function returning 'Mp3File.Error' is unused.

What is the best way to handle this?

[SDGGiesbrecht]

So... the LengthOfSizeDeclaration would be 3/4 depending on version.

Yes.

It doesn't appear to be required to be UInt32 in version 2.2 or version 2.3, but in version 2.4 it is.

Well 4 bytes is a UInt32, 3 bytes is a... UInt24, which Swift doesn’t provide.

Presumably that is what the mask was for. A “mask” is a nickname for a value where the bits you care about are set to 1, like [11111111 00000000 00000000 00000000] in the example I traced for you. 0x00FFFFFF ([00000000 11111111 11111111 11111111]) would obliterate the first byte but keep the last three. I guess for version 2.2, they read in a UInt32 from one byte early so that it included the last byte of the identifier, but then used the mask to eradicate that first byte that had nothing to do with the size.

[SDGGiesbrecht]

I've got a bunch of what should probably be computed properties that are being handled as functions because try Data(contentsOf: mp3File.location) throws.

You don’t actually want to load it repeatedly. That would be immensely wasteful. Load it only once and have all the other methods accept the Data as a parameter. Or put the methods on Data itself, so that some of them can become computed properties if they have no other parameters. Or have the Data be a stored property of a type, and that type’s methods can access self.data whenever they want, without needing to pass it through parameters.

[NCrusher74]

I've got a bunch of what should probably be computed properties that are being handled as functions because try Data(contentsOf: mp3File.location) throws.

You don’t actually want to load it repeatedly. That would be immensely wasteful. Load it only once and have all the other methods accept the Data as a parameter. Or put the methods on Data itself, so that some of them can become computed properties if they have no other parameters. Or have the Data be a stored property of a type, and that type’s methods can access self.data whenever they want.

I think that's what I'm trying to do.

I've got my Mp3File type, which is basically just our AudioFile type (that is the most useful thing you've ever taught me) and I'm using that as the initializer for most of my other types (except for those that require ID3Version instead, or in addition to.)

So, I have:

struct Mp3File {

    let location: URL

    init(location: URL) {
        self.location = location
    }

    internal var data: Data? {
        do {
            return try Data(contentsOf: self.location)
        } catch {
            Mp3File.Error.CannotReadFile
        }; return nil
    }

}

(eventually this will also have my read and write functions)

And I'm trying to make data a property of it, so all my other types using Mp3File as an initializer can use self.mp3File.data in their computed properties.

But like I said, my data property is giving me a warning (not an error) that the error handling I've got there isn't being used. Since I try to treat warnings as though they were errors whenever possible, I would like to make sure that's ok?

[NCrusher74]

Well 4 bytes is a UInt32, 3 bytes is a... UInt24, which Swift doesn’t provide.

Presumably that is what the mask was for. A “mask” is a nickname for a value where the bits you care about are set to 1, like [11111111 00000000 00000000 00000000] in the example I traced for you. 0x00FFFFFF ([00000000 11111111 11111111 11111111]) would obliterate the first byte but keep the last three. I guess for version 2.2, they read in a UInt32 from one byte early so that it included the last byte of the identifier, but then used the mask to eradicate that first byte that had nothing to do with the size.

So, if I wanted to make a frameSizeDeclaration computed property (in a type that already uses version as an initializer) would I just do this?

    internal var sizeMask: UInt32 {
        switch self.version {
            case .version22: return 0x00FFFFFF
            case .version23, .version24: return 0xFFFFFFFF
        }
    }

    internal var frameSizeDeclaration: UInt32 {
        let frameSize: UInt32 = 0
        return frameSize + sizeMask
    }

Or am I missing a conversion back to Int for the number of bytes?

[SDGGiesbrecht]

But like I said, my data property is giving me a warning (not an error) that the error handling I've got there isn't being used.

That warning might be stale. I don’t see why the code you posted would cause that.

But you probably want that property to be stored, and to load it once in the initializer (like the location property). The way you have it, it will load the file each time you query for the property.

[NCrusher74]

Okay will do!

[NCrusher74]

I don't think I'm quite writing this properly?

    init(location: URL, data: Data) throws {
        self.location = location
        if try Data(contentsOf: self.location) == nil {
            throw Mp3File.Error.CannotReadFile
        } else {
            self.data = try Data(contentsOf: self.location)
        }
    }

But I have a warning that the comparison to nil always returns false, and I don't want to check for nil anyway, I just want to handle any error.

[NCrusher74]

I think I got it worked out.

     public let location: URL
    internal var data: Data

    public init(location: URL) throws {
        self.location = location
        do {
            self.data = try Data(contentsOf: self.location)
        } catch {
            throw Mp3File.Error.CannotReadFile
        }
    }

I hope.

[SDGGiesbrecht]

So, if I wanted to make a frameSizeDeclaration computed property (in a type that already uses version as an initializer) would I just do this?

Hmm... This is how I would do it:

extension Version {
  // Frame component sizes.
  var identifierLength: Int {
    switch self {
    case .v2_2:
      return 3
    case .v2_3, .v2_4:
      return 4
    }
  }
  var sizeDeclarationLength: Int {
    switch self {
      case .v2_2:
        return 3
      case .v2_3, .v2_4:
        return 4
    }
  }
  var flagsLength: Int {
    switch self {
      case .v2_2:
        return 0
      case .v2_3, .v2_4:
        return 2
    }
  }
  // ...

  // Frame component offsets:
  var identifierOffset: Int {
    return 0
  }
  var sizeDeclarationOffset: Int {
    return identifierOffset  + identifierLength
  }
  var flagsOffset: Int {
    return sizeDeclarationOffset + sizeDeclarationLength
  }
}

enum Endianness {
  case littleEndian
  case bigEndian
}

extension UnsignedInteger {
  private init<D>(parsingLittleEndian data: D)
    where D: Collection, D.Element == UInt8 {
      assert(
        data.count <= MemoryLayout<Self>.size,
        "Data is too large to be expressed with \(Self.self)."
      )

      self = data.lazy.enumerated()
        .reduce(into: 0 as Self) { underConstruction, entry in
          let byte = Self(entry.element)
          let repositioned = byte << (entry.offset * 8)
          underConstruction |= repositioned
      }
  }
  private func encodingLittleEndian() -> Data {
    let byteMask: Self = 0xFF
    var data = Data()
    for offset in 0 ..< MemoryLayout<Self>.size {
      let byte = self & (byteMask << (offset * 8))
      let repositioned = byte >> (offset * 8)
      data.append(UInt8(repositioned))
    }
    return data
  }

  init<D>(parsing data: D, _ endianness: Endianness)
    where D: Collection, D.Element == UInt8 {

      switch endianness {
      case .bigEndian:
        self.init(parsingLittleEndian: data.reversed())
      case .littleEndian:
        self.init(parsingLittleEndian: data)
      }
  }
  func encoding(endianness: Endianness) -> Data {
    switch endianness {
    case .littleEndian:
      return encodingLittleEndian()
    case .bigEndian:
      return Data(encodingLittleEndian().reversed())
    }
  }

  func encodingSynchsafe() -> Self {
    let byteMask: Self = 0b0111_1111
    var encoded: Self = 0
    for offset in 0 ..< MemoryLayout<Self>.size {
      // Pull out seven bits at a time.
      let relevantBits = self & (byteMask << (offset * 7))
      // Shift them to make gaps.
      let shifted = relevantBits << offset * 1
      encoded |= shifted
    }
    return encoded
  }
  func decodingSynchsafe() -> Self {
    let byteMask: Self = 0b0111_1111
    var decoded: Self = 0
    for offset in 0 ..< MemoryLayout<Self>.size {
      // Pull out each byte without the leading bit.
      let relevantBits = self & (byteMask << (offset * 8))
      // Shift them to close the gaps.
      let shifted = relevantBits >> offset * 1
      decoded |= shifted
    }
    return decoded
  }
}

// You can put this method wherever
// and swap whatever you want between parameters and type properties.
func declaredSize(
  file: Data,
  frameStart: Data.Index,
  version: Version
  ) -> Int {

  let sizeDataStart = frameStart + version.sizeDeclarationOffset
  let sizeDataRange = sizeDataStart ..< sizeDataStart + version.sizeDeclarationLength
  guard sizeDataRange.upperBound <= file.endIndex else {
    // If the data is corrupt and doesn’t even have room for a size declaration,
    // it also doesn’t have room for content,
    // and the header isn’t considered part of the size.
    return 0
  }
  let frameSizeData = file[sizeDataRange]
  let raw = UInt32(parsing: frameSizeData, .bigEndian)
  switch version {
  case .v2_2, .v2_3:
    return Int(raw)
  case .v2_4:
    return Int(raw.decodingSynchsafe())
  }
}

[SDGGiesbrecht]

I think I got it worked out.

Yes, that last initializer is logically correct. (But the compiler might not like the third self. If so, you can just remove it. self.location and location are guaranteed to be the same at that point.)

[NCrusher74]

Wow, I was so far off the mark there. But that's going to make my FrameProperties a lot neater.

The other thing I'd like to simplify are StringEncoding, UTF16StringToByteAdapter and ISO88591StringToByteAdapter (all found here.)

I've managed to consolidate the StringEncodingDetector and StringEncodingConverter from ID3TagEditor into functions in StringEncoding (well, I'm still working on the detector function) and I'm convinced there must be a better way to handle the StringToBytes adapters, but I haven't found it yet.

[NCrusher74]

I'm heading to work now, but thank you for all your help! Have a wonderful Easter weekend!

[SDGGiesbrecht]

The other thing I'd like to simplify are StringEncoding, UTF16StringToByteAdapter and ISO88591StringToByteAdapter (all found here.)

• StringEncoding’s exists(in:) and convert(version:) were just hacks because upstream had no way to throw.
• While you need to be able to read in every encoding, you don’t really have a reason to support writing out more than one encoding. You really don’t need either adapter.

extractPrefixAsStringUntilNullTermination(_:) and String(ascii:) will already do all the work of reading. The following is sufficient to handle writing. It will always produce UTF‐16.

extension StringEncoding {
  var sizeOfTermination: Int {
    switch self {
    case .isoLatin1, .utf8:
      return 1
    case .utf16WithBOM, .utf16BigEndian:
      return 2
    }
  }
}

extension StringEncoding {
  // Because every string can be losslessly encoded this way,
  // and because it is supported by all ID3 versions.
  static let preferred = utf16WithBOM
}

extension String {

  func encoded(withNullTermination: Bool) -> Data {
    let encoding = StringEncoding.preferred
    guard var result = data(using: encoding.standardLibraryEncoding) else {
      // This will never happen unless “preferred” is changed to something besides Unicode.
      fatalError("\(encoding) cannot encode “\(self)”.")
    }
    let null = Data(repeating: 0x00, count: encoding.sizeOfTermination)
    result.append(contentsOf: null)
    return result
  }

  func encodedASCII() -> Data {
    // UTF‐8 is a superset of ASCII.
    return Data(utf8)
  }
}

Make sure all code that writes out an encoding identifier specifies StringEncoding.preferred.

I used preferred everywhere instead of utf16WithBOM directly, because it will make updates easier if a hypothetical version 2.5 comes out and changes what the most compatible encoding is.

[NCrusher74]

Thank you! I assume I do still need the detect method from StringEncoding? Which is just a simple matter of the encoding byte being one of the rawValues in the StringEncoding enum?

[NCrusher74]

Okay. I think I have everything in place as far as the parsing guts are concerned.

Now I need to put together the... steps...for going from parsed data to an actual frame, and back again.

I am trying to handle as much of this as enums as possible. I don't want a different file for every single frame the way ID3TagEditor has, and I'm certain that's not necessary.

I think I'm going to need a couple protocols, but protocols are something I'm not all that familiar with because I just haven't had to use them yet in anything I've done, and the couple of times I've poked at them, I don't think I've been able to quite figure it out.

So, I have my FrameName enum, which is the common handle that is going to be used to refer to a frame in any given situation.

It's got a String rawValue, which in most situations will serve as the FrameKey, but in situations such as the user text and comment frames, where more than one frame of that type is allowed, the FrameKey will be the description string from the frame instead.

FrameName is initialized by the identifier (the version-dependent 3-4 byte identifier string) and in my FrameProperties type, I have this:

    internal func identifier(
        from frameData: inout Data.SubSequence,
        version: Version,
        frameInfo: FrameInformation
    ) -> FrameName {
        let identifier = frameData.extractFirst(version.identifierLength)
        assert(
            String(ascii: identifier) == frameInfo.id3Identifier(version: version),
            "Mismatched frame name: \(String(ascii: identifier)) ≠ \(String(describing: frameInfo.id3Identifier))"
        )
        return FrameName(identifier: String(ascii: identifier))
    }

So, when parsing frames, I parse out the identifier and use it to initialize the FrameName.

Then I have my FrameInformation type, which is initialized by FrameName.

This type sorts out how frames are handled. It has boolean checks to see if a frame is a type with a description string, language string, or any other sort of situation where the FrameKey won't be FrameName(rawValue: String). It assigns a FrameKey to a frame based on that.

FrameInformation also has a frameType computed property (which corresponds to a FrameType enum) and this determines how each frame's content is going to be presented on the API side, such as taking a string with two integers in it and presenting those integers as discNumber.part and discNumber.totalDiscs or whatever. So we've got stringFrame, stringArrayFrame, intArrayFrame and so forth.

There is also a parserType computed property (and corresponding enum) which sorts of which kind of parsing approach will be taken with a particular frame. Basically, what you find in ID3TagEditor's FrameContentParsingOperation types.

So now I need to figure out how to bridge all this together and I'm scratching my head a little at that. I'm modeling most of my frame-specific parsing types after the FrameParser you created for ID3TagCreator, so this would be my StringFrameParser type:

internal struct StringFrameParser: FrameParser {

    let frameName: FrameName

    func parse(frame: Data, version: Version,
               frameInfo: FrameInformation,
               completed: (FrameName, FrameData) throws -> ()) {
        var parsing = frame[...]
        extractHeader(from: &parsing, version: version, frameInfo: frameInfo)

        let encoding = try extractEncoding(from: &parsing)

        let parsed = extractContentString(from: &parsing, encoding: encoding)
        let constructed = StringFrame(contentString: parsed)
        completed(frameName, constructed)
    }
}

I'm just not sure what my FrameData (or whatever I end up calling it) type that it returns should be. Presumably, I think, it would be a protocol to which all my content-type specific frames would conform?

I've got FrameData and TagData types that are basically empty right now except for the initializer, so I would have a type to use as the return from my frame and tag parsing. I'm just not sure what those types should do.

As far as your FrameParser that I'm using, was making FrameParser a protocol and then adding the functions as an extension done to be more compliant with the way ID3TagEditor handled things, or should I leave that as-is? I figured all my content-specific parsers would conforms to it, but if there is something I should change about it because it's not necessary outside of ID3TagEditor, better I should find out now.

[NCrusher74]

I think...maybe I've already got my bridge in place, don't I? It's FrameInformation. I need to make that a protocol, and make all it's methods and variables an extension, the way you did with FrameParser. All my parsers would conform to it, and all my frame creators would use it as a parameter to retrieve the information for the frame...right?

[SDGGiesbrecht]

Okay, looking at it afresh and pretending I had never seen the way the other library did it, I would create these things:

1. An enum called FrameLayoutIdentifer. This will be the isolated little bit you will load first on its own so that you can decided how to interpret the data of that frame. It will have no raw value, because it will simultaneously represent the three and four letter codes used by different versions of the specification, but it will need an initializer to create one from each kind of code and a property to produce each kind of code. Known cases will be basic: case author, case comment. But one will need an associated value to prevent stomping frames the library hasn’t been taught how to handle: case unknown(name: String). That way you can still read in and write out a frame you know nothing about to keep it intact while you adjust the ones you do understand.

2. An enum called FrameKey. This will be how you identify whether two frames are allowed to coexist. Two frames will not be allowed to have the same FrameKey. If they do, they need to be otherwise merged into a single frame before they can be placed in the same tag. Some cases will be basic: case author. Others will have additional discriminators: case comment(language: String). The unknown case will be: case unknown(uuid: UUID), so that no unknown cases will ever clash with each other.

3. An enum called Frame. This will be the type passed around as an actual frame. It will have several cases that vary according to the layout case string(StringFrame) (such as author), case localizedString(LocalizedStringFrame) (such as comment), case unknown(UnknownFrame).

4. A protocol called FrameProtocol. This will declare all the methods shared by the various frame layout types. That includes encoding and decoding from data, as well as getting and setting the name, the size, the flags, the key, etc.

5. Each of the individual layout types. StringFrame, LocalizedStringFrame, UnkownFrame, etc. These will each conform to FrameProtocol, doing so according to their various actual data layouts.

6. A Tag type. This type will hold all the frames as a dictionary ([FrameKey: Frame]). It will be responsible for the global encoding and decoding. (It will load a frame’s header to get its size and FrameLayoutIdentifer, then use that to hand off the data to the correct frame layout type for its own specific decoding.)

[NCrusher74]

LOL okay the answer wasn't as simple as I thought. I was trying to avoid overthinking things the way I usually do but I went too far down and was trying to be too basic.

[NCrusher74]

Okay, looking at it afresh and pretending I had never seen the way the other library did it, I would create these things:

1. An `enum` called `FrameLayoutIdentifer`. This will be the isolated little bit you will load first on its own so that you can decided how to interpret the data of that frame. It will have no raw value, because it will simultaneously represent the three and four letter codes used by different versions of the specification, but it will need an initializer to create one from each kind of code and a property to produce each kind of code. Known cases will be basic: `case author`, `case comment`. But one will need an associated value to prevent stomping frames the library hasn’t been taught how to handle: `case unknown(name: String)`. That way you can still read in and write out a frame you know nothing about to keep it intact while you adjust the ones you do understand.

Okay, I think this is basically what I'm doing with FrameName, but I can change what it's called. I just need to move the property associating the FrameName with the ID3 identifier from FrameInformation to FrameLayoutIdentifier.

I think maybe I might also want a type alias like FrameName or FrameID just to make it a little more...user friendly?

2. An `enum` called `FrameKey`. This will be how you identify whether two frames are allowed to coexist. Two frames will not be allowed to have the same `FrameKey`. If they do, they need to be otherwise merged into a single frame before they can be placed in the same tag. Some cases will be basic: `case author`. Others will have additional discriminators: `case comment(language: String)`. The unknown case will be: `case unknown(uuid: UUID)`, so that no unknown cases will ever clash with each other.

Okay. Right now I just have this method on FrameInformation to derive the FrameKey from the FrameName rawValue, except in special situations:

    // Checks if the frame as a Description field
    private var hasDescription: Bool {
        switch self.frameName {
            case .comments,
                 .unsynchronizedLyrics,
                 .userDefinedText,
                 .userDefinedWebpage:
                return true
            default: return false
        }
    }

    /** the key used to refer to a particular frame. Usually this is the rawValue of the `FrameName`, but in cases where a frame may be duplicated, will derive from information contained in the frame, such as a description field */
    public var frameKey: String {
        if self.hasDescription /* && count for that ID3 Frame return > 1 */ {
            return "\(self.frameName.rawValue): description" // placeholder
        } else if self.hasLanguage /* && count for that ID3 Frame return > 1 */ {
            return "Language += 1" // placeholder
        } else {
            return self.frameName.rawValue
        }
    }

But I can change that to an enum instead.

3. An `enum` called `Frame`. This will be the type passed around as an actual frame. It will have several cases that vary according to the layout `case string(StringFrame)` (such as author), `case localizedString(LocalizedStringFrame)` (such as comment), `case unknown(UnknownFrame)`.

Okay. I think this is basically what I was shooting for with FrameType but I don't think I knew that you could put parameter on a case like that. That is going to make my life so much easier.

4. A `protocol` called `FrameProtocol`. This will declare all the methods shared by the various frame layout types. That includes encoding and decoding from data, as well as getting and setting the name, the size, the flags, the key, etc.

Yeah, this is what I was missing to bridge it all together.

5. Each of the individual layout types. `StringFrame`, `LocalizedStringFrame`, `UnkownFrame`, etc. These will each conform to `FrameProtocol`, doing so according to their various actual data layouts.

Okay, this is what I was shooting for with my various ParserTypes, I think. I can tweak that.

6. A `Tag` type. This type will hold all the frames as a dictionary (`[FrameKey: Frame]`). It will be responsible for the global encoding and decoding. (It will load a frame’s header to get its size and `FrameLayoutIdentifer`, then use that to hand off the data to the correct frame layout type for its own specific decoding.)

Got it. I think that is what TagData was meant to be, and then I just...forgot what I was going for there. Will do.

[SDGGiesbrecht]

Okay, I think this is basically what I'm doing with FrameName, but I can change what it's called.

You can call it whatever you want. I tentatively chose the longer name, because I didn’t want it confused with FrameKey that clients will actually see and use. This type is only internal and simply identifies the layout structure of a particular frame’s data. If “name” is what the specification calls it, you can opt for that name instead.

Right now I just have this method on FrameInformation to derive the FrameKey from the FrameName rawValue

What you have already suffices as a unique String that can serve as a dictionary key. The advantage an enumeration has over what you’ve done is that the calling code querying the dictionary doesn’t have to learn the conventions of the string representation:

let frames: [String: Frame] = // ...
let theOneIWant = frames["COMM, language: eng"] // ✓
let oops = frames["COMM language: eng"] // ✗ (forgot the comma)

Compare that to the compiler’s ability to make sure you get this right (and even help you find it with code completion):

let frames: [FrameKey: Frame] = // ...
let theOneIWant = frames[.comment(language: "eng")]

You were already starting to find the right direction on your own. I’m just helping you get there faster. 😉

[NCrusher74]

Which is awesome and so very tremendously appreciated! Seriously, you're amazing. Thank you.

Good point about it not mattering what the internally used stuff is called. I keep forgetting that not everyone is going to see all of this.

[NCrusher74]

question: is there a way to include a type alias for a frame as listed in the FrameKey enum?

For instance, the Arranger frame (as listed in the enum) is often known by some people/apps as Remixer or Interpreted By or whatever, but it's all meant to go to the TPE4 frame.

[SDGGiesbrecht]

I use this whenever I have two names for the same case:

case arranger
static var remixer: FrameKey { return .arranger }
static var interpreter: FrameKey { return .arranger }

It works in most places. For example, these both work:

let frame = dictionary[.arranger]
let frame = dictionary[.remixer]

But there are others places where the aliases don’t work as well as the original:

switch value {
case .arranger:
  break
} // ✓ Compiler knows the switch is exhaustive.

switch value {
case .remixer:
  break
default: // ← Needed because the compiler cannot tell we tried everything.
  break
}

[NCrusher74]

Hmm, okay. I guess I need to figure out if I'm going to be using the FrameKey enum as a switch anywhere else before I commit to that.

[SDGGiesbrecht]

The existence of the alias won’t interfere with switches that don’t reference it. There is no harm in having it available. It just won’t be able to be used everywhere the case itself can.

[NCrusher74]

Excellent. I would like to handle these frames more along the lines of the way that TagLib does, eventually.

    /// Maps to the `Arranger` frame. If another `Arranger` frame is already present, the frame will be created as a `UserDefinedText` frame with the description, "Remixer"
    static var remixer: FrameKey { return .arranger }

    /// Maps to the `Arranger` frame. If another `Arranger` frame is already present, the frame will be created as a `UserDefinedText` frame with the description, "Interpreted By"
    static var interpretedBy: FrameKey { return .arranger }

    /// Maps to the `contentGroup` frame. If another `contentGroup` frame is already present, the frame will be created as a `Grouping` frame. If another `Grouping` frame is present, the frame will be created as a `UserDefinedText` frame with the description, "Work"

    /// Maps to the `Subtitle` frame, which is often used for description text. If another `Subtitle` frame is already present, the frame will be created as a `UserDefinedText` frame with the description, "Description"
    static var description: FrameKey { return .subtitle }

I'll probably add to the list as I go. Those are just the ones I can think of off the top of my head.