Chapter handling

[NCrusher74]

Talking an issue through here.

Back when we first worked on this in the spring, there was an issue I noticed with the DateFrame where, if you read in a tag in one format, then tried to switch to another format, if the version of the input tag didn't support the frame you were trying to add, it would throw an error for trying to encode a frame to a version that didn't support that frame, even though the output version did actually support the frame.

At the time, I couldn't figure out why this was happening or how to fix it, but since I've changed things up, the problem has become more apparent.

It looks like what's happening is that the identifier string remains the identifier string appropriate to the version for the tag that was initially read in, even if the output tag is a different version. In some cases, this means a 4-character identifier has changed between versions and if, say, you were converting a version 2.4 tag to a version 2.2 tag (not that I can see any reason why anyone would want to do that, but who knows, maybe someone uses an app that plays nicer with version 2.2 than version 2.4) you would end up with a situation where your frame in your output file is wonky because the identifier has four characters instead of three, and the entire frame header is ten bytes instead of six.

I had thought perhaps I would just do away with the ability to convert from one version of tag to another by getting rid of the version parameter in the write function but even then, I'm still bumping up against issues with this. For example, my NoMeta test file has a tag header that is version 2.3, even though it doesn't have any frames in the tag. Which means if I'm trying to write to test version 2.2 frames on that file, I encounter this problem.

But...it's been a hard few weeks and I'm scattered-brained. A few weeks ago, a big windstorm brought down our neighbor's tree onto our roof and punched some holes in our house. Except for some repairs in the roof and a few cracks in our drywall, we thought we had gotten fairly lucky, but it turns out it actually drove a vent pipe from the roof down into our drainage pipes, breaking them quite badly, so for three weeks, we've had raw sewage flowing into the crawlspace under our house and now the cleanup and repair job is going to be several orders of magnitude more massive than we had been prepared for and we're not even sure our house should be considered habitable at the moment.

Add that to the fact that most of the last couple of weeks, the air hasn't been breathable in our part of the world, as well as the state of the world in general, and you can see why I'd be a bit distracted.

So. yeah. Every time I try to pick through the code, my eyes start to cross and I can't figure out what I need to do to fix this. Or actually, I can, but it seems like whenever I start to try to change the version working from the end of the process (i.e. the Mp3File.write(tag:outputLocation:) function) at some point it comes into conflict with theversion` that was initialized upon reading the existuing tag from the input file. There needs to be a way to... reassign the version to the frames before they're encoded for output, so that the proper frame header data is used.

My head is starting to spin again, so I will once again be using you as my rubber duck.

To catch you up on what I've been doing...

The major change I did with this version of the library is I structured it a little more like the MP4 tagger. Instead of a FrameProtocol, I made a Frame class where I'm storing all the common-to-every-frame items, like identifier, size, flags, and content data.

As with atoms in the MP4 tagger, all the specific frame types are now subclasses of the Frame class. I've made a few things that were version-dependent properties of the Version enum, as well, such as the four-character idString for each identifier:

   func idString(_ id: KnownIdentifier) -> String? {
        switch id {
            case .album:
                switch self {
                    case .v2_2: return "TAL"
                    case .v2_3, .v2_4: return "TALB"
                }
           // etc etc

And instead of having FrameKey as its own enum that is 95% a straight-up duplicate of the KnownIdentifier enum, instead the frameKey is a string property of the FrameIdentifier/KnownIdentifier enums:

    func frameKey(_ additionalID: Any?) -> String {
        switch self {
            case .attachedPicture:
                if let uInt8 = additionalID as? UInt8 {
                    if let type = ImageType(rawValue: uInt8) {
                        return self.rawValue + ": \(type.pictureDescription)"
                    } else {
                        fatalError("Unable to determine unique frame ID for image frame")                    }
                } else if let description = additionalID as? String {
                    return self.rawValue + ": \(description)"
                } else {
                    return self.rawValue
                }
            case .chapter:
                if let startTime = additionalID as? Int {
                    return self.rawValue + ": \(startTime)"
                } else {
                    return self.rawValue
                }
            // etc
            default: return self.rawValue

I'm not entirely sure about this change yet. I like having the frameKey as a String because it makes it a little more portable but...idk. Either way, it doesn't really matter to what I'm trying to work out here, just a note in case you were wondering.

Since Frame is now a class, I got rid of the Frame enum, and instead put the task of assigning a particular frame to a particular frame subclass as a function of the FrameIdentifier enum.

So my order of operations, as it were, goes something like this:

Step 1: Initialize Mp3File with the audio file location and initialize a data property for the file data.

Step 2: Run the file data through the Tag initializer, which is pretty much the same as it was before, with one significant exception:

version is a stored property in Tag. I'm...drawing a blank right now on why that was necessary, but I think it had something to do with making it logic simpler in the accessors in cases where a particular frame needs different handling depending on the version. It seemed expedient at the time, and this may or may not be contributing to the lack of flexibility with switching versions. I don't think so, however, because I noticed an issue with version-switching months ago.

So we parse out the version in the tag initializer, and use that as a parameter for my frame-header parsing function, which is one of those extensions I told you about.

(Tag initializer)

        while !remainder.isEmpty {
            // pass the frames data over to `Data.Subsequence` exension for parsing
            if let frame = try remainder.extractAndParseToFrame(version) {
                // get the frame key
                let frameKey = frame.frameKey
                // add frame to dictionary
                frames[frameKey] = frame
            }
        }
        self.frames = frames

Step 3: ( extension)

    mutating func extractAndParseToFrame(_ version: Version) throws -> Frame? {
        // extract the frame header data and use that to initialize the specific parser
        let identifier = try self.extractAndDecodeFrameID(version)
        let size = self.extractAndCalculateFrameSize(version)
        let flags = self.extractFlags(version)
        let payload = self.extractFirst(size)
        return try identifier.parse(version: version,
                                    size: size,
                                    flags: flags,
                                    payload: payload)
    }

Step 4: (FrameIdentifier)

    func parse(version: Version,
               size: Int,
               flags: Data,
               payload: Data) throws -> Frame {
        switch self.parseAs {
            case .string, .boolean, .integer, .url:
                return try StringFrame(identifier: self,
                                       version: version,
                                       size: size,
                                       flags: flags,
                                       payload: payload)
            case .date:
                return try DateFrame(identifier: self,
                                     version: version,
                                     size: size,
                                     flags: flags,
                                     payload: payload)
            // etc
    }

Step 6: Parse the specific frame. (I'll use the DateFrame as an example, since that's one frame where I know this version issue becomes apparent.)

    var timeStamp: Date?
    init(identifier: FrameIdentifier,
         version: Version,
         size: Int,
         flags: Data,
         payload: Data
    ) throws {
        var data = payload
        let encoding = try data.extractEncoding()
        let dateString = try data.extractAndDecodeString(encoding: encoding)

        // assumes frame contents are spec-compliant, 4-characters, DDMM string
        if identifier == .known(.date) {
            // assumes frame contents are spec-compliant, 4-characters, HHmm string
            self.timeStamp = try dateString.dateFromDDMMString()
        } else if identifier == .known(.time) {
            self.timeStamp = try dateString.timefromHHMMString()
        } else if identifier == .known(.year) ||
                    // versions 2.2 and 2.3 should only have a year for this frame
                    (identifier == .known(.originalReleaseTime) &&
                        (version == .v2_2 || version == .v2_3)) {
            self.timeStamp = try dateString.yearFromYYYYString()
        } else {
            let formatter = ISO8601DateFormatter()
            formatter.formatOptions = [.withInternetDateTime]
            formatter.timeZone = TimeZone(secondsFromGMT: 0) ?? .current
            self.timeStamp = formatter.date(from: dateString)
        }

        super.init(identifier: identifier,
                   version: version,
                   size: size,
                   flags: flags)
    }

Step 7: Initialize the Frame class, which looks like this:

class Frame {
    var identifier: FrameIdentifier
    var version: Version
    var size: Int
    var flags: Data

    init(identifier: FrameIdentifier,
         version: Version,
         size: Int,
         flags: Data) {
        self.identifier = identifier
        self.version = version
        self.size = size
        self.flags = flags
    }

    var frameKey: String {
        switch self.identifier {
            case .known(.attachedPicture), 
                    .known(.chapter), 
                    .known(.comments), 
                    .known(.unsynchronizedLyrics), 
                    .known(.userDefinedText), 
                    .known(.userDefinedWebpage):
                fatalError("Override from frame subclass is required")
            default: return self.identifier.frameKey(nil)
        }
    }

    @available(OSX 10.12, *)
    var contentData: Data {
        fatalError("Override from frame subclass is required")
    }

    @available(OSX 10.12, *)
    var encode: Data {
        // Prints warning if a frame is non-standard or not generally supported for a particular version
        if let warning = identifier.warnings(version: self.version) {
            print(warning)
        }
        var data = Data()
        data.append(self.identifierData)
        data.append(self.encodedFrameContentSize)
        data.append(self.version.defaultFlags)
        data.append(self.contentData)
        return data
    }
}

Step 8: Call Frame.encode from the function in Tag where we assemble all the frames and append them to the new tag header data.

    var tagWithHeader: Data {
        var framesData = Data()
        for (_, frame) in self.frames {
            framesData.append(frame.encode)
        }
        var tagData = self.version.versionBytes
        tagData.append(self.defaultFlag)
        tagData.append(framesData.count.uInt32.encodingSynchsafe().beData)
        tagData.append(framesData)
        return tagData
    }

Step 9: Call tagWithHeader from the file-builder function:

    private func buildNewFile(tag: Tag) -> Data {
        var data = self.data
        let tagSizeData = data.subdata(in: tag.tagSizeDataRange)

        let size = (tagSizeData as NSData)
            .bytes.assumingMemoryBound(to: UInt32.self).pointee.bigEndian
        let tagSize = size.decodingSynchsafe().toInt

        let tagDataRange = data.startIndex ..< tag.tagHeaderLength + tagSize

        let tagData = tag.tagWithHeader
        data.replaceSubrange(tagDataRange, with: tagData)
        return data
    }

Step 10: Use the freshly built file in the write function:

    public func write(tag: Tag,
                      outputLocation: URL) throws {
        let data = buildNewFile(tag: tag)
        try data.write(to: outputLocation)
    }

So, as of Frame.encode, which is called from Step 8, the version being considered is still the version from the source tag, rather than the output tag. I can work around this by simply editing the version when working with the tag instance (maybe that's why I decided to make that a stored property of Tag?)

        var tag = tagNoMeta
        tag.version = .v2_2

outputs a properly formatted frame for version 2.2:

     43 4f 4d // three byte identifier
     0 0 1b // three byte size
    // no flags - six bytes total header size.

Except...no. Just tested that. It will properly output frames that are being altered or added, but existing, untouched frames are still passed through with the old version data.

So, alternatively, working backward from the final step, I'd be looking at something like this:

Step 10: (This is how it used to be, but it never worked properly because, while it changed the tag header data, it didn't change how the frame headers were encoded.)

    public func write(tag: Tag,
                                 version: Version,
                                 outputLocation: URL) throws {
        let data = buildNewFile(tag: tag, version: version)
        try data.write(to: outputLocation)
    }

Step 9: (Note to self: need to work logic in for handling situations where the file has no existing tag, i.e. we create one from scratch and insert it at the beginning. Right now, I believe a file without a tag will throw an error.)

    private func buildNewFile(tag: Tag, version: Version) -> Data {
//
        let tagData = tag.tagWithHeader(version: version)
        data.replaceSubrange(tagDataRange, with: tagData)
        return data
    }

Step 8:

    func tagWithHeader(version: Version) -> Data {
        var framesData = Data()
        for (_, frame) in self.frames {
            framesData.append(frame.encode(version: version))
        }
 //
        return tagData
    }

Step 7:

    func encode(version: Version) -> Data {
        // Prints warning if a frame is non-standard or not generally supported for a particular version
        if let warning = identifier.warnings(version: self.version) {
            print(warning)
        }
        var data = Data()
        // the identifier data varies depending on version
        data.append(self.identifierData(version: version))
        // the size bytes are different depending on version
        data.append(self.encodedFrameContentSize(version: version))
        // version is already considered here, just need to use the parameter version instead of self.version
        data.append(version.defaultFlags)
        data.append(self.contentData(version: version))
        return data
    }

However... I would also need to change contentData to a function, instead of a computed property, because it, too, would need to take into account the version. Some frames are handled differently internally, depending on version, not just in the header data.

Right now, contentData is established using the version of the input tag, not the output.

So it looks like what I need to do is recalculate all the frame header data that gets added in Frame.encode with the new version, and that includes encoding <FrameSubclass>.contentData in the individual frame types.

I think where I'm stumbling is actually the frame-building initializer (as opposed to the frame-parsing initializer).

That has a version parameter, so if tag.version is edited, and then the tag.<accessor> is used, and the tag.<accessor> initializes the frame using the frame-building initializer when it's set, then the frame will get the new version.

But any frame that isn't accessed using one of the accessors still has the version it was initialized with. So it's not just a matter of encoding. I have to actually RE-initialize all the frames with the new version. I think?

Okay, yeah. I think that's what I need to do. Change tagWithHeader to this:

    func tagWithHeader(version: Version) throws -> Data {
        var framesData = Data()
        for (_, frame) in self.frames {
            let identifier = frame.identifier
            let flags = version.defaultFlags
            let payload = frame.contentData
            let size = payload.count
            let newFrame = try identifier.parse(version: version,
                                                size: size,
                                                flags: flags,
                                                payload: payload)
            framesData.append(newFrame.encode)
        }
        var tagData = self.version.versionBytes
        tagData.append(self.defaultFlag)
        tagData.append(framesData.count.uInt32.encodingSynchsafe().beData)
        tagData.append(framesData)
        return tagData
    }

Upon testing, this APPEARS to work. The frames that are passed through without alteration are being changed to have the proper header data. It feels scary and iffy though. I'm not at all confident this is the right way to go. Maybe it's just the fact that I'm using the parsing initializer to re-initialize in the final encoding process?

Or maybe I just need an "attagirl" after the month I've had. Or a stiff scotch? That would be good too.

[NCrusher74]

Ugh. nevermind. I just saw what I was missing. All I have to do is this:

    func tagWithHeader(version: Version) throws -> Data {
        var framesData = Data()
        for (_, frame) in self.frames {
            frame.version = version // <- ugh this is so obvious how did I miss this?
            framesData.append(frame.encode)
        }
        var tagData = self.version.versionBytes
        tagData.append(self.defaultFlag)
        tagData.append(framesData.count.uInt32.encodingSynchsafe().beData)
        tagData.append(framesData)
        return tagData
    }

[NCrusher74]

Question:

I'm reworking my date handling to (hopefully) give my parsing the best chance to come up with a valid date if the date isn't in ISO8601-compliant format. Would something like this work?

    private var dateFormatters: [DateFormatter] {
        var formatters = [DateFormatter]()
        let formatter = DateFormatter()
        formatter.dateStyle = .full
        formatters.append(formatter)
        formatter.dateStyle = .long
        formatters.append(formatter)
        formatter.dateStyle = .medium
        formatters.append(formatter)
        formatter.dateStyle = .short
        formatters.append(formatter)

        formatter.locale = Locale(identifier: "en_US_POSIX")
        formatter.dateFormat = "yyyy-MM-ddTHH:mm"
        formatters.append(formatter)
        formatter.dateFormat = "yyyy-MM-ddTHH"
        formatters.append(formatter)
        formatter.dateFormat = "yyyy-MM-dd"
        formatters.append(formatter)
        formatter.dateFormat = "MM-dd-yyyy"
        formatters.append(formatter)
        formatter.dateFormat = "yyyy-MM"
        formatters.append(formatter)
        formatter.dateFormat = "yyyy"
        formatters.append(formatter)
        return formatters
    }

// then when parsing
        if let date = isoFormatter.date(from: self) {
            return date
        } else {
            for format in dateFormatters {
                if let date = format.date(from: self) {
                    return date
                } else {
                    return nil
                }
            }; return nil
        }
}

[SDGGiesbrecht]

DateFormatter is a class, so you are making all those changes on the same one, and the array just contains many references to the same thing. I’m also pretty sure the format overrides the style anyway. (The style is how it decides which format to ask the system for.)

You’d have to adjust it to something like this:

private var formats: [String] {
  return ["yyyy-MM-ddTHH:mm", "yyyy-MM-ddTHH", /* ... */]
}
private let dateFormatters: [DateFormatter] = formats.map { format in
  let formatter = Locale(identifier: "en_US_POSIX")
  formatter.dateFormat = format
  return formatter
}

[NCrusher74]

Ah okay, I hadn't been sure about that part. Good to know. Thank you!