Metadata

[NCrusher74]

Question:

I just realized I made a pretty big oversight with SwiftTaggerID3 in that I have no version of listMetadata() in there. I have the frames: [FrameKey: Frame] variable in Tag, but that isn't really human-readable. What I really need is [(FrameKey: Any)]

So I'm trying to implement that, like so:

    public var metadata: [(frameKey: FrameKey, value: Any)] {
        var metadata = [(FrameKey, Any)]()
        for frame in self.frames {
            let frameKey = frame.key
            switch frameKey {
                case .album:
                    if let value = self.album { // where self.album is the get/set accessor already implemented
                        let entry = (frameKey, value)
                        metadata.append(entry)
                }
                //
            }
        }
    }

However, I'm not sure exactly how to handle the frames that use language as one of the parameters:

                case .comments(description: let description):
                    if let value = self.localizedGetter(for: .comments(description: description), language: <#T##ISO6392Codes?#>, description: description) {
                        let entry = (frameKey, value)
                        metadata.append(entry)
                }

Ideally, this should return any item with description regardless of the language, but I'm pretty sure just passing nil for that parameter would only return comments where the language is nil, right?

So what I really need is an anyLanguage option, presumably in my ISO6392Codes enum? I'm just... not sure how I'd go about implementing that.

Or would this actually work? (I'm not to the point where I can test it yet)

                case .comments(description: let description):
                    for language in ISO6392Codes.allCases {
                        if let value = self.localizedGetter(for: .comments(description: description), language: language, description: description) {
                            let entry = (frameKey, value)
                            metadata.append(entry)
                        }
                }

[NCrusher74]

Actually, I think my real issue with those types of frames is I need to be able to list them regardless of description. Hrm.

Is there a way to use a wildcard character for description in this?

    internal func get(for frameKey: FrameKey,
                      language: ISO6392Codes?,
                      description: String?) -> String? { }

though honestly that still leaves me with the frameKey parameter, which requires a non-optional description to satisfy. Ugh. Now I see why I didn't implement this before.

So what I really need is to be able to pass some sort of wildcard or whatever here: for frameKey: .userDefinedText(description: String)

Either that, or FrameKey itself conforms to Hashable. Is there some way to get all the frame keys that contain userDefined or comment or lyrics or whatever?

[NCrusher74]

I have to run out the door to work, but I think?... I'm getting closer? I just still need a way to sidestep providing a specific description in these.

        let keys = frameKeys.filter({$0.hashValue == 13 || // comments
            $0.hashValue == 70 || // lyrics
            $0.hashValue == 71 || // user text
            $0.hashValue == 72 ||
        }) // user webpage

        for key in keys {
            for language in ISO6392Codes.allCases {
            // comment and lyrics
            if let value = get(for: key, language: language, description: <#T##String?#>) {
                let entry = (frameKey, value)
                metadata.append(entry)
                }
            } else {
                // user defined text and user defined webpage
                if let value = get(for: key, description: <#T##String?#>) {
                    let entry = (frameKey, value)
                    metadata.append(entry)
                }
            }
        }

[NCrusher74]

Okay, I think I found an easier way.

I add this to Tag: public static var listMetadata = [(frameKey: FrameKey, value: Any)]()

And then each time I initialize a frame (after the properties are initialized):

        Tag.listMetadata.removeAll(where: {$0.frameKey == self.frameKey})
        Tag.listMetadata.append((self.frameKey, self.contentString))  // or whatever that frame's content property is

Limited testing shows that the remove/append seems to be working to update the array when I change the content values after parsing the file upon reading it in, so I think this should work?

[SDGGiesbrecht]

I have the frames: [FrameKey: Frame] variable in Tag, but that isn't really human-readable. What I really need is [(FrameKey: Any)]

What advantage does the second have over the former?

Maybe I misunderstand something, but if it’s really necessary, can’t you trivially transform the first into the second like this?

let start = [FrameKey: Frame] = [/* ... */]

var converted: [(FrameKey, Any)] = []
for (key, value) in start {
  converted.append((key, value))
}

print("Done: \(converted)")

[NCrusher74]

I wouldn't call it an advantage, per se, it's just that I want to have a means to dump all the metadata in human-readable form if necessary. So someone using the library can just print(listMetadata) and see all the metadata that file contains. Mostly just to provide an overview, not necessarily for manipulating the data.

The problem is that Frame isn't the value I'm after. Frame is an enum that initializes the particular structure needed to parse the contents of a frame. The value I need is whatever content comes out of that parsing, whether it's a string or a part of Total tuple or array or what have you.

For the most part, getting to that content is simple, because I've already got accessors in place to get and set that content. But for the items that require something like a descriptionString parameter to differentiate between potentially multiple comment or userDefinedText frames (for which I use a subscript accessor instead of a computed property), the accessors don't help because I don't have that descriptionString. So I needed to find a way to get at that content, regardless of the descriptionString.

So, making listMetadata a static var that all the various frame initializers add that frame's frameKey and content properties to seems to be the best option, because right there in the initializer, I've got a frameKey property that I don't need to provide a differentiating descriptionString for (because that's already been parsed out and used to initialize the frameKey property) and I've got the frame's content as a human-readable property.

[SDGGiesbrecht]

So someone using the library can just print(listMetadata) and see all the metadata that file contains.

Would conforming any ungainly types to CustomStringConvertible clean things up so that print(tag.frames) produces something concise and legible?

[NCrusher74]

I was actually thinking about that earlier. I did add customStringConvertible conformance to the frames at the time I was working on SwiftTaggerID3, but I didn't really understand what I was doing, so it's sort of still a mess. I will look through those and see if I can clean them up.

[NCrusher74]

Question about versioning (I think?):

As I work on this, I'm finding a lot of places where I need to go back and make little tweaks to the dependencies. I've created branches for this so I'm not committing those tweaks directly to master until I've tested them out, and I'm using that branch for the dependency in my package manifest, but I think I'd probably be better off figuring out versioning.

I've read the Github documentation about releases and I sort of get lost about what I should consider a major or minor revision, or even where I should start my version numbering at this point.

How would you recommend I approach this?

[NCrusher74]

Ugh, I swear the PresetOptionsFrame in SwiftTaggerID3 is never going to let me move on.

I'm having trouble in situations where the accessor is a tuple, because I have to make the accessor's return value optional (in able to be able to remove the frame/atom that accessor represents when the accessor is set to nil) but doing so is causing issues like the one I encountered last week with the Mp4 disc/TotalDiscs tuple where it was failing because of the optional. I've tried to make it as waterproof as I can, but I'm still encountering that with various tuples.

So here is what I have:

These are my "mid-level" get and set functions that get used by all the accessors for this particular frame:

    internal func get(forPresetOptionFrame frameKey: FrameKey) -> [String]? {
        if let frame = self.frames[frameKey],
            case .presetOptionsFrame(let presetOptionsFrame) = frame {
            return presetOptionsFrame.genreMediaOrFileInfo
        } else {
            return nil
        }
    }

    internal mutating func set(_ layout: FrameLayoutIdentifier,
                               _ frameKey: FrameKey,
                               infoArray: [String]) {
        let frame = PresetOptionsFrame(
            layout, genreMediaOrFileInfo: infoArray)
        self.frames[frameKey] = .presetOptionsFrame(frame)
    }

These serve as an intermediate step between the tuple that the public accessor presents, and the array that is the initialized property of the frame.

And this is the accessor:

    public var genre: (presetGenre: GenreType?, customGenre: String?)? {
        get {
            // if the array exists and isn't empty
            if let frameArray = get(forPresetOptionFrame: .genre) {
                var presetType: GenreType = .none
                var customString: String = ""
                var tuple: (presetGenre: GenreType?, customGenre: String?) = (nil, nil)
                // for each item in the array...
                for item in frameArray {
                    // parse stuff out of the array that may or may not overwrite the variables above
                }
                if presetType != .none {
                    tuple.presetGenre = presetType
                }
                if customString != "" {
                    tuple.customGenre = customString
                }
                if tuple != (nil, nil) {
                    return tuple
                } else {
                    return nil
                }
            } else {
                return nil
            }
        }
        set {
            if let new = newValue, new != (nil, nil) {
                var frameArray = [String]()
                if let genre = new.presetGenre, genre != .none {
                    frameArray.append(genre.rawValue)
                }
                if let custom = new.customGenre, custom != "" {
                    frameArray.append(custom)
                }
                if !frameArray.isEmpty {
                    set(.known(.genre), .genre, infoArray: frameArray)
                }
            } else {
                self.frames[.genre] = nil
            }
        }
    }

This results in this test failing:

        var tag = Tag()

        tag.genre?.presetGenre = .Blues
        tag.genre?.customGenre = "Blues Refinement"

        let output = try tempDirectory().appendingPathComponent("test.mp3")
        XCTAssertNoThrow(try TestFile.noMeta.mp3File()?.write(tagVersion: .v2_4, using: tag, writingTo: output))

        let fileWritten = try Mp3File(location: output)
        let tagWritten = try Tag(readFrom: fileWritten)

        XCTAssertEqual(tagWritten.genre?.presetGenre, .Blues)
        XCTAssertEqual(tagWritten.genre?.customGenre, "Blues Refinement")

It works fine when writing to a tag where this frame already exists, but when trying to create the frame where it doesn't already exist, I get nil for those last two checks.

How can I get around this situation? Like I said, I need to have these accessors return optional values so they can be set to nil and the frame/atom removed from the metadata, but I've tried changing up how I've written this several times and I can't quite make it work.

[NCrusher74]

Edit: Scratch that last question, I finally found a way.

[SDGGiesbrecht]

Extensive versioning advice is here.

In short:

• Use the form 0.x.y until the package seems “complete”, then switch the first number to 1 and use the form x.y.z (where x starts as 1).
• If a release includes a breaking change, meaning a usage case would need to be adjusted too to match, bump the x number.
• Otherwise, if the release includes a new feature, meaning there are new things available to use, bump the y number.
• Otherwise, the release only includes bug fixes, meaning the available interface didn’t change and there is no meaningful difference to users beyond that it is “more reliable” or “faster”, then bump the z number. (Or the y number if there is not z because are still in 0.x.y mode.)

(I’ll come back and look at your other comment later. I have to go.)

[SDGGiesbrecht]

There is (or was?) nothing wrong with the implementation. Both here and earlier when there was a similar problem, it was the call site that contained the error, in that it was asking to modify the value only if it already exists, which is the semantic intention of an optional chained assignment.

[NCrusher74]

True, kind of got those issues mixed up.

Ultimately, what I needed was an implementation where, if the tuple doesn't already exist, it would proceed as though it does exist and is just (nil, nil) (or whatever is needed for that particular frame) without any of that being visible or requiring additional effort at the call site (because I'm trying to keep the call site as clean and straightforward as possible and let the convoluted stuff happen further under the hood.)

The way I worked around it was by adding another intermediate layer. So instead of

    public var genre: (presetGenre: GenreType?, customGenre: String?)?  {
        get {//}
        set {//}
    }

What I have is

    public var genre: String? {
        get {
            if let string =  _genre.customGenre {
                return string
            } else {
                return nil
            }
        }
        set {
            if let new = newValue, new != "" {
                _genre.customGenre = new
            } else {
                _genre.customGenre = nil
            }
        }

    internal var _genre: (presetGenre: GenreType?, customGenre: String?)  {
        get {
            // return the tuple even if it's (nil, nil)
        }
        set {
            if newValue != (nil, nil) {
                // call the `set` function
            } else {
                // remove the frame from the frames dictionary
            }
        }
    }

I might see about adding a struct as a sub-structure of Tag that contains the new public accessors so that the call is still tag.whatever.genre or tag.whatever.presetGenre or what have you, so that there's some context to indicate "this genre variable is one potential piece of a larger metadata item that may contain other properties".

I haven't fully completed testing with this new way, but I think I'm on the right path with it?

[SDGGiesbrecht]

The general strategy of translating to and from an optional string should work. Unless you’ve elided it though, your genre property is completely ignoring presetGenre. Is that intentional?

[NCrusher74]

No, I just left that part out for brevity. It's in my code, I just didn't include it in the comment above.

[NCrusher74]

Question:

Back when I was trying to make things work with AVFoundation, we had a brief discussion about what I should do with the discNumber and trackNumber atoms, because the data contained in them was in a slightly weird format:

// AVFoundation
// discNumber (1 of 2)
0 0 0 1 0 2

// trackNumber (7 of 8)
0 0 0 7 0 8 0 0

Now that I'm parsing this atom myself, it's gotten a little more convoluted because I've discovered other apps handle it differently:

// Fission
     0 0 0 1 0 2 0 0
     0 0 0 7 0 8 0 0

// and I think I remember seeing one that returned 0 0 0 1 0 0 0 2 one or both of them

Atomic Parsley writes it the same way as AVFoundation but describes these atoms as being in uint8 format. But then it describes pretty much any atom that isn't a string as uint8. It also describes the zero bytes between as padding:

Because I can't find any documentation on what the "official" specification for these atoms, I can't really be sure how I'm going to come across it. Two 8-bit integers with no padding between? I have no way of knowing.

Here is how mp4v2 handles it, but what it's doing I can't say for sure.

Tags::fetchDisk( const CodeItemMap& cim, MP4TagDisk& cpp, const MP4TagDisk*& c )
{    
    cpp.index = 0;
    cpp.total = 0;
    c = NULL;

    CodeItemMap::const_iterator f = cim.find( CODE_DISK );
    if( f == cim.end() || 0 == f->second->dataList.size )
        return;

    MP4ItmfData& data = f->second->dataList.elements[0];

    if( NULL == data.value )
        return;

    cpp.index = (uint16_t(data.value[2]) <<  8)
              | (uint16_t(data.value[3])      );

    cpp.total = (uint16_t(data.value[4]) <<  8)
              | (uint16_t(data.value[5])      );

    c = &cpp;
}

I can tell it's handling them as 16-bit values, so I'm going to err on the side of assuming whatever I come across will be 16-bit, though it may actually just be 8-bit with padding.

So here is how I was handling it:

    public func get(_ ID: MetadataIDArrayInt) throws -> [Int] {
        let metadata = try listMetadata()
        if let arrayMeta = metadata.first(where: {$0.name == ID.rawValue}) {
            var arrayData = arrayMeta.data

            // handle as an array
            // if it's more than 6 bytes or more, treat as firstInt = int32, lastInt = int16
            if arrayData.count >= 6 {
                let firstInt = arrayData.extractToInt(.uInt32)
                let lastInt = arrayData.extractToInt(.uInt16)
                return [firstInt, lastInt]
            } else if arrayData.count >= 4 {
                // check if it's one int32 or two int16
                // if the first two bytes are zero, assume it's one int32
                let rangeToCheck = 0 ..< 2
                let dataToCheck = arrayData.subdata(in: rangeToCheck) // this is where the error below happens
                if dataToCheck.toInt_16 == 0 {
                    let singleInt = arrayData.toInt_32
                    return [singleInt]
                    // otherwise, assume it's two int16
                } else {
                    let firstInt = arrayData.extractToInt(.uInt16)
                    let lastInt = arrayData.extractToInt(.uInt16)
                    return [firstInt, lastInt]
                }
                // if it's less than four bytes, assume it's one int16 value
            } else if arrayData.count >= 2 {
                let singleInt = arrayData.toInt_16
                return [singleInt]
            } else {
                return []
            }

        } else {
            return []
        }
    }

This worked with all my tests, but now I'm testing removing the metadata and I'm getting a very unhelpful Thread 1: EXC_BAD_INSTRUCTION (code=EXC_I386_INVOP, subcode=0x0) error.

This is the test I'm running:

        tag.trackNumber.track = nil
        tag.trackNumber.totalTracks = nil
        tag.discNumber.disc = nil
        tag.discNumber.totalDiscs = nil

        XCTAssertNil(output.trackNumber)
        XCTAssertNil(output.discNumber)

It looks like the error is occurring when trying to read the file back in after outputting it, because there is an output file, but the track and disc atoms aren't being removed as they should be:

     0 0 0 1c // track atom data
     74 72 6b 6e
     0 0 0 14
     64 61 74 61
     0 0 0 0
     0 0 0 0
     0 0 0 8 // <- I assume this is the 8 of (7/8)

     0 0 0 1c
     64 69 73 6b
     0 0 0 14
     64 61 74 61
     0 0 0 0
     0 0 0 0
     0 0 0 2 // <- 2 of (1/2)

Since these atoms exist in the output file still, it's apparent I'm doing something wrong with setting the atom to nil in my accessor:

    public var discNumber: (disc: Int?, totalDiscs: Int?) {
        set {
            if newValue != (nil, nil) {
                do {
                    var array = [Int]()
                    if let disc = newValue.disc {
                        array.append(disc)
                    }
                    if let total = newValue.totalDiscs {
                        array.append(total)
                    }
                    try parser.set(.discNumber, arrayValue: array)
                } catch {
                    print("WARNING: Unable to set metadata atom \(AtomIdentifier.discNumber.rawValue)")
                }
            } else {
                do {
                    try parser.set(.discNumber, arrayValue: nil)
                } catch {
                    print("WARNING: Unable to remove metadata atom \(AtomIdentifier.discNumber.rawValue)")
                }
            }
        }
    }

// which calls parser.set(_:arrayValue:)
    public func set(_ ID: MetadataIDArrayInt,
                    arrayValue: [Int]?) throws {
        if let array = arrayValue {
            let metaAtom = try MetadataAtom(name: ID.rawValue,
                                            arrayValue: array)
            try set(metadataAtom: metaAtom)
        } else {
            try remove(ID)
        }
    }

// which calls:
    private func remove(_ ID: MetadataIDArrayInt) throws {
        if let metadataAtoms = self.moov?.udta?.meta?.ilst.children as? [MetadataAtom] {
            self.moov?.udta?.meta?.ilst.children = metadataAtoms.filter({$0.name != ID.rawValue})
        }
    }

I don't see how that is going wrong. It's not removing the values from the atom, it's removing the atom at the source, so I don't see how I'm ending up with only the trackTotal or discTotal value left.

Unless...

do I need to do it in this order?

        tag.trackNumber.totalTracks = nil
        tag.trackNumber.track = nil
        tag.discNumber.totalDiscs = nil
        tag.discNumber.disc = nil

Because otherwise, the total value is getting moved up to the first item in the array when the tuple gets converted to an array? Or...

Ugh. I'm not sure how it would be best to handle these atoms.

Okay, that was the overview. Walking through it a little more precisely line by line:

The test (as originally written), sets trackNumber to nil. This calls the set portion of the accessor:

        set {
            if newValue != (nil, nil) {
                do {
                    var array = [Int]()
                    if let track = newValue.track {
                        array.append(track)
                    }
                    if let total = newValue.totalTracks {
                        array.append(total)
                    }
                    try parser.set(.trackNumber, arrayValue: array)
                } catch {
                    print("WARNING: Unable to set metadata atom \(AtomIdentifier.trackNumber.rawValue)")
                }
            } else {
                do {
                    try parser.set(.trackNumber, arrayValue: nil)
                } catch {
                    print("WARNING: Unable to remove metadata atom \(AtomIdentifier.trackNumber.rawValue)")
                }
            }
        }

Which...yes, only appends the total value, because the track value is nil. Thus, we have an array of a single value (total) when calling parser.set:

    public func set(_ ID: MetadataIDArrayInt,
                    arrayValue: [Int]?) throws {
        if let array = arrayValue {
            let metaAtom = try MetadataAtom(name: ID.rawValue,
                                            arrayValue: array)
            try set(metadataAtom: metaAtom)
        } else {
            try remove(ID)
        }
    }

Which, since there IS an array there (though it's an array with a single value) initializes a MetadataAtom with the array, and then calls the set function to add that atom to the ilst atom's children:

    private func set(metadataAtom: MetadataAtom) throws {
        // ilst atom already exists
        if let ilst = self.moov?.udta?.meta?.ilst {
            var ilstChildren = ilst.children
            if metadataAtom.identifier == "----" {
                ilstChildren.append(metadataAtom)
            } else {
                var filteredChildren = ilstChildren.filter({$0.identifier != metadataAtom.identifier})
                filteredChildren.append(metadataAtom)
                ilstChildren = filteredChildren
            }
            // update the children array
            self.moov?.udta?.meta?.ilst.children = ilstChildren
// snip
              }
            }
        }
    }

So. The ilst children array is set, and that's the end of things until we write. Which means we move on to our next command, which is to remove the totalTracks:

        tag.trackNumber.totalTracks = nil

This invokes the get portion of the accessor to get the current (track:totalTracks) tuple.

        get {
            var tuple: (track: Int?, totalTracks: Int?) = (nil, nil)
            do {
                let array = try parser.get(.trackNumber)

                if let total = array.last, array.count > 1 {
                    tuple.totalTracks = total
                }

                if let track = array.first {
                    tuple.track = track
                }

            } catch {
                print("WARNING: Unable to retrieve metadata atom \(AtomIdentifier.trackNumber.rawValue)")
            }
            return tuple
        }

Which...yeah, okay, there it is. Since array.count isn't greater than 1, it handles the total value like it's the track value and thus doesn't do anything since I'm calling to set totalTracks to nil.

The question then becomes...how do I handle this atom to sidestep these sorts of problems? I can't guarantee that whoever uses the library is going to set totalTracks to nil before they set tracks to nil. They could, of course, use tag.trackNumber = (nil, nil). But that's messier than I'd like to be.

Can you see a better way to handle this?

[SDGGiesbrecht]

Here is how mp4v2 handles it, but what it's doing I can't say for sure.

It is reading bytes 3 and 4 as a UInt16 numerator and bytes 5 and 6 as a UInt16 denominator. It is ignoring bytes 1, 2, 7 and 8.

This worked with all my tests, but now I'm testing removing the metadata and I'm getting a very unhelpful Thread 1: EXC_BAD_INSTRUCTION (code=EXC_I386_INVOP, subcode=0x0) error.

That most commonly indicates the attempted use of an invalid index of an Array or similar collection. The debugger ought to stop on the precise line where the access is attempted.

[NCrusher74]

Yeah, that's what puzzled me. It was stopping at this line:

let dataToCheck = arrayData.subdata(in: rangeToCheck)

of this portion of get(_ID:) for intArray handling:

            } else if arrayData.count >= 4 {
                // check if it's one int32 or two int16
                // if the first two bytes are zero, assume it's one int32
                let rangeToCheck = 0 ..< 2
                let dataToCheck = arrayData.subdata(in: rangeToCheck) // this is where the error below happens
                if dataToCheck.toInt_16 == 0 {
                    let singleInt = arrayData.toInt_32
                    return [singleInt]
                    // otherwise, assume it's two int16
                } else {
                    let firstInt = arrayData.extractToInt(.uInt16)
                    let lastInt = arrayData.extractToInt(.uInt16)
                    return [firstInt, lastInt]
                }

Which doesn't make sense because even if if the totalTracks value did get moved forward in the array so that it was in the where we'd expect to find the track value, there were still 4 bytes there, so arrayData.count >= 4 is true, and since there are four bytes, rangeToCheck = 0 ..< 2 shouldn't be a problem in this line: let dataToCheck = arrayData.subdata(in: rangeToCheck) right?

[NCrusher74]

Can you tell if mp4v2 is handling (index, total) as a tuple, or an array, or...?

I've been handling this atom as an array and then converting things to a tuple on the user end, but maybe I should just get rid of the array handling altogether and deal with it as a tuple from the get-go instead? As in tuple = (part: (bytes3...4), total: (bytes5...6)) essentially?

[SDGGiesbrecht]

It was stopping at this line:

let dataToCheck = arrayData.subdata(in: rangeToCheck)

Then try printing arrayData.startIndex and arrayData.endIndex on the line before.

Can you tell if mp4v2 is handling (index, total) as a tuple, or an array, or...?

It appears to be a custom structure equivalent to this:

struct Disk {
  var index: Int
  var total: Int
}

[NCrusher74]

It appears to be a custom structure equivalent to this:

struct Disk {
  var index: Int
  var total: Int
}

Which isn't effectively much different to treating it as Disk = (index: Int, total: Int), I don't think? Except that I'm approaching it more as (index: Int?, total: Int?) Which I suppose maybe they're doing also, this this part:

    if( NULL == data.value )
        return;

(I can't do the print of the error because I'm in the middle of tweaking some things now, but if it persists afterward, I will.)

[NCrusher74]

Okay, switching things around to handling these atoms as tuples from the get-go seems to work because it removes the possibility of making mistakes with array indices.

Clearly I still don't quite understand how data indices and extraction and closed ranges work, because at first I tried this:

            if tupleData.count >= 6 {
                tupleData.dropFirst(2)
                let part = tupleData[3...4].toInt_16 // out of bound error
                let total = tupleData[5...6].toInt_16
                return (part, total)
            } else if tupleData.count >= 4 {
                tupleData.dropFirst(2)
                let part = tupleData.extractToInt(.uInt16)
                return (part, nil)
            } else {
                return nil
            }

But dropping the first two bytes and then using a variation on the extractFirst(_k:) extension to Data.Subsequence, which extracts and performs the conversion to Int all at once, works.

            if tupleData.count >= 6 {
                tupleData.dropFirst(2)
                let part = tupleData.extractToInt(.uInt16)
                let total = tupleData.extractToInt(.uInt16)
                return (part, total)
            } else if tupleData.count >= 4 {
                tupleData.dropFirst(2)
                let part = tupleData.extractToInt(.uInt16)
                return (part, nil)
            } else {
                return nil
            }

[SDGGiesbrecht]

dropFirst() isn’t removeFirst(). The former returns a copy without the first element; the latter modifies the collection and returns the removed element. So your calls to tupleData.dropFirst(2) are doing nothing, because you aren’t doing anything with the copy they produce.

---

Your excerpt doesn’t show what type tupleData is. Unless the specific type guarantees otherwise, you cannot assume that a collection starts at 0.

let array = [1, 1, 2, 3, 5, 8, 13, 21]
let filtered = array.filter { $0 % 2 == 0 } // The even numbers.
for index in filtered.indices {
  print(index) // 2 (pointing at 2), 5 (pointing at 8)
}

The proper thing to do is let the collection decide the right index for a particular offset.

let secondIndex = filtered.index(filtered.startIndex, offsetBy: 1)
print(secondIndex) // 5 (pointing at 8)

It is possible that the extract methods are doing this properly, as opposed to tupleData[3...4], which may not be valid if the real six indices are something like 1057, 1058, 1059, 1060, 1061 and 1062 after things have been filtered off the front.

[NCrusher74]

Because there was so much repetitive code in the various atoms (lots of converting data to int and back) I started handling a LOT of this as convenience extensions to Data, Data.Subsequence, Int and the uInt Types.

Just to be sure I'm doing the right thing, I'll go through what each of my extensions does. And what we're dealing with from the outset.

tupleData is the data property of the DataAtom atom type., (so named because I couldn't name the atom Data, for obvious reasons.)

The data property of DataAtom is what's left after everything else in the atom has been parsed out:

class DataAtom: Atom {
    var dataType: DataType
    private var locale: Data
    var data: Data

    /// Initialize a `data` atom for parsing from the root structure
    override init(id: String, size: Int, payload: Data) throws {
        var data = payload
        let typeInt = data.extractToInt(.uInt32)
        if let type = DataType(rawValue: typeInt) {
            self.dataType = type
        } else {
            fatalError("\(Mp4File.Error.UnsupportedMetadataFormat)")
        }
        self.locale = data.extractFirst(4)
        self.data = data
        try super.init(id: id,
                       size: size,
                       payload: payload)
    }

Pretty much everything parsed out of the file up until this point is parsed by extraction, so we SHOULD always be starting at zero as our Data.Index when parsing the payload of an atom. In fact, I actually combined the parse function with the extractFirst(_k:) function in my Data.Subsequence extension to remove repeating all that code in every atom that has child atoms:

    /// Extracts 8, 4, 2, or 1 bytes of data and converts to an integer
    internal mutating func extractToInt(_ bits: Bits) -> Int {
        var k = Int()
        switch bits {
            case .uInt8: k = 1
            case .uInt16: k = 2
            case .uInt32: k = 4
            case .uInt64: k = 8
        }
        let extraction = self.extractFirst(k)
        switch bits {
            case .uInt8: return extraction.toInt_8
            case .uInt16: return extraction.toInt_16
            case .uInt32: return extraction.toInt_32
            case .uInt64: return extraction.toInt_64
        }
    }

    /// Extracts 4 bytes and converts to a `ISO-8859-1`-encoded string
    ///
    /// The second four bytes of an atom are always it's identifier
    internal mutating func extractAtomID() -> String? {
        let extraction = self.extractFirst(4)
        let string = String(data: extraction, encoding: .isoLatin1)
        return string
    }

    /// Extracts 4 bytes and converts to integer (assumes big endian)
    ///
    /// The first four bytes of an atom are always its size
    internal mutating func extractAtomSize() -> Int {
        return self.extractToInt(.uInt32)
    }

    /// Parse the content from an MP4 atom
    internal mutating func extractAndParseToAtom() throws -> Atom? {
        let preliminarySize = self.extractAtomSize()
        guard let idString = self.extractAtomID() else { return nil }

        var size = Int()
        var payload = Data()

        if preliminarySize == 0 {
            payload = self
            size = self.count + 8
        } else if preliminarySize == 1 {
            // if used, the extendedSizeData comes after the idData
            size = self.extractToInt(.uInt64)
            payload = self.extractFirst(size - 16)
        } else {
            size = preliminarySize
            payload = self.extractFirst(size - 8)
        }

        if let id = AtomIdentifier(rawValue: idString) {
            switch id {
                case .data: return try DataAtom(id: idString,
                                                size: size,
                                                payload: payload)
                // all the atoms initialized on a case by case basis
            }

So when parsing the data atom out of its parent atom (trkn or disk in this case), we've got four bytes parsed out for the size (by extraction), four bytes parsed out for the identifier string (by extraction), and then everything after that is payload. Which means our starting index with payload (since we've extracted everything else) should be 0, right?

Since payload is immutable, we store the data as var data = payload and then begin slicing off it. Even if our starting index with payload weren't 0, our starting index for data would be?)

The first four bytes of data are the dataType property, extracted and converted to a 32-bit int.

The next four bytes are extracted for the locale property (which we basically ignore because no metadata handling library I've come across yet seems to use this. We extract it and forget about it.)

The rest is the data property, and how we read that data depends on what the dataType property is. For these particular atoms, there IS no dataType, the bytes are always 0 0 0 0 which is:

public enum DataType: Int {
    case reserved = 0
// etc
}

So no help there. Again, this is the same for every metadata library I've come across. They don't specific a dataType, so we're left to guess at how the data should be handled. All I know is that for trkn, the data is usually 0 0 0 X 0 Y 0 0 and for disk the data is usually 0 0 0 X 0 Y (but may also include two trailing zeroes.)

Other extensions that may be relevant:

// Data extensions
// similar functions and variables are in place for 8, 16, and 64 bit ints
    /// Converts 4 bytes of uInt32 Big Endian data to an integer value
    var toInt_32: Int {
        return Int(uInt32BigEndian)
    }

    private var uInt32BigEndian: UInt32 {
        return UInt32(parsing: self, .bigEndian)
    }

    mutating func extractToUInt32BE() -> UInt32 {
        let extraction = self.extractFirst(4)
        return extraction.uInt32BigEndian
    }

On the flipside, this is how I'm converting my integers back to data:

    /// Converts integer to big endian data appropriate for the chosen bits
    internal func beData(_ bits: Bits) -> Data {
        switch bits {
            case .uInt64: return self.uInt64BEData
            case .uInt32: return self.uInt32BEData
            case .uInt16: return self.uInt16BEData
            case .uInt8: return self.uInt8BEData
        }
    }

    private var uInt32: UInt32 {
        return UInt32(truncatingIfNeeded: self)
    }

    private var uInt32BEData: Data {
        return uInt32.beData
    }

extension UInt32 {
    /// Converts the UInt32 to big endian data
    var beData: Data {
        var int = self.bigEndian
        return Data(bytes: &int, count: MemoryLayout<UInt32>.size)
    }
}

So far this has all been working, but it would probably be best to make sure I'm not bungling something that will bite me later.

[NCrusher74]

Oh, and this is my initializer for the DataAtom atom, when initializing from one of these tuples:

    /// Initialize a `data` atom with integer array data for building a metadata list
    init(tupleValue: (index: Int?, total: Int?)) throws {
        self.dataType = .reserved
        self.locale = Data(repeating: 0x00, count: 4)

        var data = Data()
        if let index = tupleValue.index {
            data.append(index.beData(.uInt16))
        }
        if let total = tupleValue.total {
            data.append(total.beData(.uInt16))
        }
        self.data = data

        let dataTypeInt = dataType.rawValue
        var payload = Data()
        payload.append(dataTypeInt.beData(.uInt32))
        payload.append(self.locale)
        payload.append(self.data)

        let size = payload.count + 8
        try super.init(id: "data",
                       size: size,
                       payload: payload)
    }

It's called by a similar initializer for the MetadataAtom type, since all these atoms have the same structure of a DataAtom contained within a MetadataAtom:

    // MARK: - Tuple Initializer
    /// Initialize a metadata atom with `name` from integer array content
    init(name: String, tupleValue: (Int?, Int?)) throws {
        var children = [Atom]()
        self.name = name

        let dataAtom = try DataAtom(tupleValue: tupleValue)
        self.dataType = dataAtom.dataType
        self.data = dataAtom.data
        children.append(dataAtom)

        var payload = Data()
        for child in children {
            payload.append(child.encode())
        }

        let size = payload.count + 8
        try super.init(id: name,
                       size: size,
                       children: children)
    }

That's the initializer called by my tupleMetadata set function:

    /// Set an integer array metadata tag
    public func set(_ ID: MetadataIDTuple,
                    tupleValue: (Int?, Int?)?) throws {
        if let tuple = tupleValue {
            let metaAtom = try MetadataAtom(name: ID.rawValue,
                                            tupleValue: tuple)
            try set(metadataAtom: metaAtom)
        } else {
            try remove(ID)
        }
    }    

[SDGGiesbrecht]

If I recall correctly, Data is its own SubSequence, which means if you’ve previously used the extract methods, then startIndex probably isn’t 0. Have you actually printed startIndex to find out what it is?

Or have you tried switching this...

let part = tupleData[3...4].toInt_16

...to this...

let index3 = tupleData.index(tupleData.startIndex, offsetBy: 3)
let index4 = tupleData.index(tupleData.startIndex, offsetBy: 4)
let part = tupleData[index3 ... index4].toInt_16

...?

[NCrusher74]

Hm, okay, wow, turns out the startIndex is, indeed, 83456 on the last test I ran.

Also, I'm writing the trkn and disk atoms correctly, according to every app I try looking at the file with. I'm just not parsing it correctly yet.

Hrmph.

Okay, here's my test (working in the SwiftMp4MetadataParser dependency at present.):

    func testTupleWriting() throws {
        let source = try Mp4File(location: sampleNoMeta)
        try source.set(.discNumber, tupleValue: (2, 3))
        try source.set(.trackNumber, tupleValue: (7, 8))

        //        let outputUrl = try tempDirectory().appendingPathComponent("test.m4a")
        let outputUrl = try localDirectory(fileName: "pot", fileExtension: "m4a")
        try source.write(outputLocation: outputUrl)

        let output = try Mp4File(location: outputUrl)
        XCTAssertEqual(try output.get(.discNumber).part, 2) // XCTAssertEqual failed: ("Optional(0)") is not equal to ("Optional(2)")
        XCTAssertEqual(try output.get(.discNumber).total, 3) // XCTAssertEqual failed: ("Optional(2)") is not equal to ("Optional(3)")
        XCTAssertEqual(try output.get(.trackNumber).part, 7) // XCTAssertEqual failed: ("Optional(0)") is not equal to ("Optional(7)")
        XCTAssertEqual(try output.get(.trackNumber).part, 8) // XCTAssertEqual failed: ("Optional(0)") is not equal to ("Optional(8)")
    }

Like I said, looking at the output file in other apps confirms that the atoms are there and readable. So it's MY parsing that is the problem at this point. Apparently I'm reading the part value as the total value for the discNumber atom, and idek what I'm reading for the second one.

Here's the data my parsing is working with:

     0 0 0 1e // size
     64 69 73 6b // disk atom
     0 0 0 16 // size
     64 61 74 61 // data subatom
     0 0 0 0
     0 0 0 0
     0 0 0 2 0 3 // <- part and total data

     0 0 0 1e // size
     74 72 6b 6e // trkn atom
     0 0 0 16 // size
     64 61 74 61 // data subatom
     0 0 0 0
     0 0 0 0
     0 0 0 7 0 8 // <- part and total data

Here's the get(_ ID:) function that returns the tuple, which I've not changed to include your suggested code:

    public func get(_ ID: MetadataIDTuple) throws -> (part: Int?, total: Int?) {
        let metadata = try listMetadata()
        if let metadataAtom = metadata.first(where: {$0.name == ID.rawValue}) {
            var tupleData = metadataAtom.data
            print(tupleData.hexadecimal()) // 0 0 0 2 0 3  and 0 0 0 7 0 8, respectively
            if tupleData.count >= 6 {
                let index3 = tupleData.index(tupleData.startIndex, offsetBy: 3)
                let index4 = tupleData.index(tupleData.startIndex, offsetBy: 4)
                let part = tupleData[index3 ... index4].toInt_16

                let index5 = tupleData.index(tupleData.startIndex, offsetBy: 5)
                let index6 = tupleData.index(tupleData.startIndex, offsetBy: 6) // out of range error
                let total = tupleData[index5 ... index6].toInt_16
                return (part, total)
            } else if tupleData.count >= 4 {
                // haven't altered this yet
                _ = tupleData.dropFirst(2)
                let part = tupleData.extractToInt(.uInt16)
                return (part, nil)
            } else {
                return (nil, nil)
            }
        } else {
            return (nil, nil)
        }
    }

As you can see, I get an out of range error on the last one. So now I'm trying it this way:

    public func get(_ ID: MetadataIDTuple) throws -> (part: Int?, total: Int?) {
        let metadata = try listMetadata()
        if let metadataAtom = metadata.first(where: {$0.name == ID.rawValue}) {
            let tupleData = metadataAtom.data
//            print(tupleData.hexadecimal())
            if tupleData.count >= 6 {
                let partIn = tupleData.startIndex + 2
                let partOut = partIn + 2
                let partRange = partIn ..< partOut
                let partData = tupleData.subdata(in: partRange)
                // print(partData.hexadecimal())
                let part = partData.toInt_16
                // print(part)

                let totalIn = partOut
                let totalOut = totalIn + 2
                let totalRange = totalIn ..< totalOut
                let totalData = tupleData.subdata(in: totalRange)
                // print(totalData.hexadecimal())
                let total = totalData.toInt_16
                // print(total)

                return (part, total)
            } else if tupleData.count >= 4 {
                let partIn = tupleData.startIndex + 2
                let partOut = partIn + 2
                let partRange = partIn ..< partOut
                let partData = tupleData.subdata(in: partRange)
                print(partData.hexadecimal())
                let part = partData.toInt_16
                print(part)
                return (part, nil)
            } else {
                return (nil, nil)
            }
        } else {
            return (nil, nil)
        }
    }

That has the four test items passing. I'm not sure how it's effectively different to what you recommended, but I guess I'll go with it?

[SDGGiesbrecht]

Sorry, I neglected to actually check the offset numbers. The first byte is offset 0, so byte 3 is at offset 2. That is why they are all pointing one byte further than expected, and why the last one is overflowing.

[NCrusher74]

Ah okay, that makes sense, and is something I should have thought to check as well.

That last remaining issue I have with this field (please let it be the last!) is that I still have to set discNumber.totalDiscs or trackNumber.totalTracks to nil BEFORE I set discNumber.discs or trackNumber.track to nil, (or I have to set the entire tuple to (nil, nil)). Otherwise, the offset of the second value shifts and the removal doesn't work properly.

Is this something I should even worry about? Should I just put a note in the README about what is required there and call it good, or is there a way to handle this so the API is cleaner?

[SDGGiesbrecht]

Does discNumber.discs = nil remove the bytes? Such that the total slides over to be the new discs number? That would be a problem. When setting to nil, if both are nil, you can probably just remove the atom instead right? If you need to support a nil numerator but a non‐nil denominator, then you probably have to use a placeholder 0 instead just so that the denominator can stay in the right place.

[NCrusher74]

Yeah, the atom does get removed when both values are nil, but the process of setting them to nil involves, if you don't set the tuple to nil, means you're setting one to nil after the other, which yeah, causes the latter value to shift to a new offset. I will try to track down any places where the numerator gets set to nil without the denominator and make sure there's a placeholder there.

I sincerely doubt anyone would deliberately use just the denominator without the numerator, but yeah there is that moment when setting one to nil before the other that causes an issue.

[NCrusher74]

Silly question as I work on implementing the credits list stuff:

What is the proper syntax for a situation like this, where you want to see if a value is one unwrapped optional or another?

            if let credit = Enum(rawValue: string) ||
                if let credit = Enum.involvementFromAtomID(string) {
                //
                }
            }

[NCrusher74]

Correction: I guess what I actually meant to ask is if there's a way to do it without if-else.

[SDGGiesbrecht]

Assuming credit will have the same type either way...

if let credit = Enum(rawValue: string) ?? Enum.involvementFromAtomID(string) {
  //
}

[NCrusher74]

Yep, that's what I was after. It hadn't occurred to me to use the second attempt like I normally would a fallback. Thank you!

[NCrusher74]

Question:

I think the way I'm encoding the string from the elng (extended language atom) is causing my audio not to work.

The documentation describes it like this:

A NULL-terminated C string containing an RFC 4646 (BCP 47) compliant language tag string in ASCII encoding, such as "en-US", "fr-FR", or "zh-CN".

I was using this, but I not sure it's the correct approach:

    var encodedUtf8WithNullTerminator: Data {
        var data = Data()
        data.append(contentsOf: self.utf8)
        data.append(Data(repeating: 0x00, count: 1))
        // UTF‐8 is a superset of ASCII.
        return data
    }

How should I handle encoding this string?

[SDGGiesbrecht]

data.append(Data(repeating: 0x00, count: 1))

I’m surprised that compiles. data.append(0x00) should have sufficed.

Otherwise what you have does exactly what the documentation described, as long as the string really does only contain ASCII characters.

[NCrusher74]

The string should only contain ascii characters. The enum in question is 1500 lines long, so I haven't checked every single item on it, but the test in question is just using "en_US" as the language.

I realized I wasn't adding the null terminator when encoding the atom contents, but that didn't fix the problem of the atom breaking the audio. This is what I'm seeing when I print the bytes:

     0 0 0 12 // size
     65 6c 6e 67 // "elng"
     0 0 0 0 // version and flags
     65 6e 5f 55 53 0 // "en_US" + null

The reason I had asked was mostly because of the C-string specification. I'm not really sure how to work with C-strings or if I should be doing anything different there.

[NCrusher74]

Okay, it looks like the problem isn't actually the elng atom at all, but rather the mdhd atom, whose language property changes to keep it aligned with the elng atom, if it exists.

The problem is in how I create a new mdhd atom.

The timeScale and duration properties of most other header (atoms that end with hd in their identifier, and are typically required atoms of their parent; mvhd for a moov parent atom, tkhd for a trak parent atom, etc; mdhd is the header atom for the mdia atom)... anyway, most other header atoms have a timeScale property of something like "1000", and a duration property of "5016", which would mean the media is 5016 milliseconds long, or 5.016 seconds, when you divide the duration by the timescale.

When I write a file with Subler and check out the mdhd atom, however, the timeScale is "44100" and the duration is "221184". Which is still 5.015 seconds when you divide duration by timescale, however, if you're at all familiar with audio file encoding, "44100" is obviously the audio sampling rate, 44.1 kHz.

There is absolutely NOTHING in the Apple documentation about this atom to indicate this would be the case. It describes the atom's timeScale and duration properties in the same terms as are used for other header atoms. headdesk Also, this only applies to an mdhd grandchild of the SOUND trak atom. For a chapter trak atom, the timeScale and duration properties are still "1000" and "5015" double headdesk

So, obviously I can't go building this atom using the same sort of timeScale and duration calculations I use for other header atoms. But I don't want to mess with trying to calculate these properties because I don't know from whence they derive. However, since I'm not re-encoding or re-sampling the audio, I think I can get away with reading a file in and and storing these properties for use if I have to rebuild the atom upon writing it out again. Which, since mdhd is a required child of mdia, which is a required child of trak, I WILL have to do whenever I create a chapter track.

[NCrusher74]

Ooh nevermind, it's a lot easier than that. Turns out I was setting a new mdhd atom when setting an elng atom, regardless of what sort of trak atom the parent was. All I had to do was add in a condition for just updating the mdhd atom's language property without changing anything else when the parent trak atom was a sound track.

[NCrusher74]

Question about how you'd recommend handling something:

I'm getting to the last few metadata accessors I need to implement, which are the ones where mp4 and id3 handle things so differently that there's no really tidy way to bridge them, even by spoofing an item that exists in one format but not the other by creating a freeform atom/user defined test frame for the format where that item doesn't exist.

For example, ID3's language frame has a property that is an array of ISO-639-2 codes. MP4 doesn't actually have language as a straight-up metadata property at all-- by which I mean that the language data, if it exists, doesn't live with the media's metadata in the udta.meta.ilst atom. It does have an optionalextended language atom elsewhere in the file's media-handling atoms, but even then, the property is a single ICULocaleCode string (like "en_US").

I debated on whether or not I should create a different accessor for each way, but that's quite messy. Then again, I'm not sure the way I ultimately landed on is much cleaner:

    public var language: (id3Languages: [ISO6392Codes]?, mp4Language: ICULocaleCode?) {
        get {
            switch library {
                case .mp4:
                    if let language = mp4Tag.language {
                        if let code = ICULocaleCode(rawValue: language.rawValue) {
                            return (nil, code)
                        } else {
                            return (nil, nil)
                        }
                    } else {
                        return (nil, nil)
                }
                case .id3:
                    if let languages = id3Tag.languages {
                        var array = [ISO6392Codes]()
                        for language in languages {
                            if let code = ISO6392Codes(rawValue: language.rawValue) {
                                array.append(code)
                            }
                        }
                        return (array, nil)
                    } else {
                        return (nil, nil)
                }
            }
        }
    }

I can probably do something similar with the predefined genres, which have vastly different enums for each format.

MediaType is even clumsier, however.

For Mp4, this is the stik (the media type atom` enum:

public enum Stik: Int {
    case movieOld = 0
    case music = 1
    case audiobook = 2
    case whackedBookmark = 5
    case musicVideo = 6
    case movie = 9
    case tvShow = 10
    case booklet = 11
    case ringtone = 14
    case podcast = 21
    case iTunesU = 23
    case undefined = 255
}

Whereas, if you remember my struggles from working on the ID3 side of things with the PresetOptionsFrame, this is what I'm looking at for ID3:

public var mediaType: (mediaType: MediaType?, mediaTypeRefinement: MediaTypeRefinements?, additionalInformation: String?)

I CAN do it the same way I did with languages, sort of:

public var mediaType: (mp4StikValue: Stik, id3MediaTypeValue: (mediaType: MediaType?, mediaTypeRefinement: MediaTypeRefinements?, additionalInformation: String?))

But...wow is that ugly.

How would you handle this sort of situation?

Also, while I'm on the subject, I'm running into a lot of situations where I'm dealing with multiple copies of the same enum. The InvolvedPersonCredits and MusicianAndPerformerCredits enums are present both in SwiftTaggerID3 and SwiftTaggerMP4 AND in SwiftTagger. Same enum, just different copies, because otherwise I end up with ridiculously long types like SwiftTaggerMP4.MusicianAndPerformerCredits.

But then, of course, I've got to convert from one to the other, because SwiftTagger.MusicianAndPerformerCredits is not the same as SwiftTaggerMP4.MusicianAndPerformerCredits (even though they totally are) so I've got to initialize one using the rawValue of the other. Again, it's messy.

Could I just...idk...create a type alias for the enums in SwiftTagger that refers to the enum in the dependency instead of having a whole new copy of that enum in SwiftTagger that I have to use instead?

[NCrusher74]

Um...am I missing something?

XCTAssertEqual failed: ("Optional(["Performer 1", " Performer 2"])") is not equal to ("Optional(["Performer 1", "Performer 2"])")

[SDGGiesbrecht]

the ones where mp4 and id3 handle things so differently that there's no really tidy way to bridge them, even by spoofing an item that exists in one format but not the other by creating a freeform atom/user defined test frame for the format where that item doesn't exist.

[...]

How would you handle this sort of situation?

I’m not sure. One idea that comes to mind is to arrange the library as a class or protocol hierarchy, so that the two file types can share where it makes sense, but don’t have to where it doesn’t.

class AudioFile { /* shared interface. */ }
class MP4: AudioFile { /* unique interface */ }
class ID3: AudioFile { /* unique interface */ }

---

Could I just...idk...create a type alias for the enums in SwiftTagger that refers to the enum in the dependency instead of having a whole new copy of that enum in SwiftTagger that I have to use instead?

Sounds reasonable.

---

Um...am I missing something?

XCTAssertEqual failed: ("Optional(["Performer 1", " Performer 2"])") is not equal to ("Optional(["Performer 1", "Performer 2"])")

There is an extra leading space in the first " Performer 2".

[NCrusher74]

There is an extra leading space in the first " Performer 2".

Ah I couldn't see that because of the way the text was wrapped. Thank you.

I figured out what if I didn't put the enums in SwiftTagger, and if they only exist in one of the dependencies, then there's no ambiguity when I use the enum as a type. I did use typealias to shorten some of the enum names, but there's actually very little overlap once I moved all the handling for metadata items that exist in one format but not the other out of the dependencies and entirely into SwiftTagger.

I’m not sure. One idea that comes to mind is to arrange the library as a class or protocol hierarchy, so that the two file types can share where it makes sense, but don’t have to where it doesn’t.

At this point, I'm down to the stuff where they don't actually share anything, except a vaguely similar premise.

I don't think I've actually explained what I'm doing here, so just so we're on the same page...

The way I structured things with SwiftTaggerID3 and SwiftTaggerMP4 is that those two modules can stand entirely alone. If you only want to tag/chapter MP4 files but not MP3, you'd just use SwiftTaggerMP4 by itself.

All SwiftTagger is, essentially, is a collection of accessors that enables us to use the same accessors regardless of whether we're working with an mp4 or mp3 file.

So what I have, for each "archtype" of metadata I'm handling (metadata that contains a string value, or int, or whatever), is some internal functions like this:

    func get(_ stringMetadataID: MetadataID_String) -> String? {
        switch library {
            case .mp4:
                switch stringMetadataID {
                    case .acknowledgment: return mp4Tag.acknowledgment
                    case .album: return mp4Tag.album
                    // etc
                }
            case .id3:
                switch stringMetadataID {
                    case .acknowledgment: return id3Tag["Acknowledgment"]
                    case .album: return id3Tag.album
                    // etc
                }
        }
    }

    mutating func set(_ stringMetadataID: MetadataID_String, stringValue: String?) {
        if let string = stringValue {
            switch library {
                case .mp4:
                    switch stringMetadataID {
                        case .acknowledgment:
                            self.mp4Tag.acknowledgment = string
                        case .album:
                            self.mp4Tag.album = string
                         // etc
                }
                case .id3:
                    switch stringMetadataID {
                        case .acknowledgment:
                            self.id3Tag["Acknowledgment"] = string
                        case .album:
                            self.id3Tag.album = string
                         // etc
                }
            }
        } else {
            switch library {
                case .mp4:
                    switch stringMetadataID {
                        case .acknowledgment:
                            self.mp4Tag.acknowledgment = nil
                        case .album:
                            self.mp4Tag.album = nil
                        // etc
                }
                case .id3:
                    switch stringMetadataID {
                        case .acknowledgment:
                            self.id3Tag["Acknowledgment"] = nil
                        case .album:
                            self.id3Tag.album = nil
                        // etc
                }
            }
        }
    }

As you can see, in the case of Acknowledgment, that metadata item doesn't actually exist for ID3, so I create a user-defined text frame for it instead. I do the same thing for MP4 when the reverse is true.

That enables me not to have to put a lot of repetitive code in each individual accessor:

    public var acknowledgment: String? {
        get {
            if let value = self.get(.acknowledgment) {
                return value
            } else {
                return nil
            }
        }
        set {
            set(.acknowledgment, stringValue: newValue)
        }
    }

But anyway, for these last few items, there's no way to bridge them effectively into a single accessor because while the name (like "media type") may be similar they actually contain far different types of data.

For MP4, media type (the stik atom)describes the content of the media, like whether it's an audiobook or a movie or a ringtone or whatever. For ID3, media type describes the kind of media the content comes from:

public enum MediaType: String, CaseIterable {    
    /// Compact Disc
    case compactDisc = "CD"
    /// Laserdisc
    case laserdisc = "LD"
    /// Minidisc
    case minidisc = "MD"
    /// DAT
    case dat = "DAT"
    /// Turntable records
    case turntableRecord = "TT"
    /// DVD
    case dvd = "DVD"
    /// Normal Cassette
    case normalCassette = "MC"
    /// Reel
    case reel = "REE"
    case none = "none"

And then mediaTypeRefinement gives more detail, if available:

    case analogTransfer
    /// Other Analog Media/Wax cylinder
    case waxCylinder
    /// Other Analog Media/8-track tape cassette
    case eightTrack
    /// Turntable Records/33.33 rpm
    case rpm33
    /// Turntable Records/45 rpm
    case rpm45
// etc

So really, even creating an accessor that combines them both is sort of pointless, I guess? Unless I were to create a freeform atom to imitate ID3's mediaType, but...would it really serve any purpose? Is it necessary information for anyone getting metadata from an MP4 file (honestly I question whether it's necessary information for MP3 files, but have to make allowances for MP3 dating back to a time when media formats were very in flux as everything went digital.)

The MP4 stik atom is a lot more useful, and I can see spoofing that as a user text frame for MP3, but... Maybe I should just leave it the ID3 "media type" value in SwiftTaggerID3 and not implement it in SwiftTagger at all? The part of me that prefers to take a completist approach to handling things hates that idea, but I also hate the idea of implementing a metadata tag in SwiftTagger that isn't used by both ID3 and MP4 tagging systems. So I'm not sure what I should do there.

[NCrusher74]

Question:

The issue with that error where the space didn't get removed from the beginning of a string is actually the result of a rather convoluted situation where a lot of times I'm handling atoms and frames as though they hold arrays, when the data actually being stored as a string in the atom or frame.

I've created two convenience extensions for this specific bit of fake-outery:

extension String {
    /// Convenience Extension. Divides string in components separated by `"; "`
    var toArray: [String] {
        return self.components(separatedBy: "; ")
    }
}

extension Array where Element == String {
    /// Convenience extension. Joins array into string with components separated by `"; "`
    var toString: String {
        return self.joined(separator: "; ")
    }
}

I have the space after the semicolon because it makes the string cleaner to read.

Where I ran into the problem was somewhere in the following situation:

    public var performanceCreditsList: [ PerformanceCredits : [String]] {
        set {
            switch library {
                case .id3:
//snip
                case .mp4:
                    if !newValue.isEmpty {
                        for (key, value) in newValue {
                            let valueString = value.toString
                            self.addPerformanceCredit(key, person: valueString)
                        }
                    } else {
                        clearPerformanceCreditList()
                }
            }
        }
    }

In ID3, the performanceCreditsList stores the credits in a [EnumType: [String]] dictionary, if you'll recall. MP4 doesn't actually have a similar atom, but it DOES have a performer atom (which is meant to hold a string value, and I'm sure most apps look for string data there.)

So what I'm doing with these accessors in SwiftTagger is getting and setting them directly in ID3, but in MP4, I check the role/key, and if an atom already exists for that role (like artist or narrator), I convert the array to a string and store it in that atom. However, with performer it's a little different because that is one of those atoms that I'm handling as an array on the front end (even though it's still a string atom on the back end.)

But the addPerformanceCredit call in the accessor above is only meant to add one credit at a time, so it has a string parameter for person, which requires me to convert the newValue.value array to a string, and then convert it back when checking it against the existing contents of the performer atom to see if there's any duplication.

    public mutating func addPerformanceCredit(_ role: PerformanceCredits, person: String) {
        switch library {
            case .mp4:
                switch role {
                    case .artist: // snip
                    case .narrator: // snip
                    case .soloist: // snip
                    default:
                         if let existingArray = self.performers {
                            let newArray = checkAndMergeCreditArrays(existingCredit: existingArray, 
                                                                                                     newCredit: person.toArray)
                            self.performers = newArray
                        } else {
                            self.performers = person.toArray
                    }
            }
            case .id3:
                id3Tag.addPerformanceCredit(role: role, person: person)
        }
    }

This calls checkAndMergeCreditArrays which is one of a pair of functions I created for checking for duplicates when adding new values to a string or array:

    private func checkAndMergeCreditStrings(existingCredit: String, newCredit: String) -> String {
        let newCreditArray = newCredit.toArray
        var existingCreditArray = existingCredit.toArray
        for credit in newCreditArray {
            if !existingCreditArray.contains(credit) {
                existingCreditArray.append(credit)
            }
        }
        let newString = existingCreditArray.toString
        return newString
    }

    private func checkAndMergeCreditArrays(existingCredit: [String], newCredit: [String]) -> [String] {
        var newArray = existingCredit
        for credit in newCredit {
            if !newArray.contains(credit) {
                newArray.append(credit)
            }
        }
        return newArray
    }

Items that I snipped out above, like artist, get checked comparing string to string, because they're handled as strings on the front end. performer gets handled as an array on the front end, so it gets handled a little differently.

My question is... am I taking the long way around here? Is there a cleaner way to deal with all this? Both handling of strings as arrays and vice versa, and the duplicate checking?

[SDGGiesbrecht]

No, I can’t think of any improvements off of the top of my head.