invalid chapter "length" parameter in json output

[itzexor]

Hello,

I have an ogg opus file which is part of a split-file audiobook. The first file contains embedded chapters which can be loaded by audiobookshelf. ABS uses tone for metadata analysis, and is getting some invalid chapter data past the first chapter. I decided to test tone standalone and see how the metadata was coming through. For some reason, this tool seems to generate invalid data for the "length" property for all except the first chapter. The embedded metadata is the standard vorbis chapters comment format, with no durations specified. Additionally, the chapters list total duration far exceeds the playback time of the file, so that could be having some fun interactions.

tone dump & opusinfo output: https://gist.github.com/itzexor/29cd09be45ea986900fc8e6d7b2904d5

edit: it's clear from the gist, but i forgot to mention that this is with json output. i didn't actually see a duration in the default dump

[sandreas]

@itzexor Thanks for reporting. I'm pretty sure, that I would need the original files to reproduce this. Can I assume that these files cannot be shared publicly?

If so, I would invite you to a private github repository within 24 hours after your response, where we can share a one click hoster link to the files (e.g. mega.nz), so that I can reproduce the problem on my system. Would that be ok?

[itzexor]

Here's a version of the file made using the same exact process, except I piped in a 30s wave file with no sound. The duration is a bit different but the metadata should be exact. The issue seems to show up the same.

test_file_no_audio.zip

[sandreas]

Ok, I think I got it. The problem seems to be that the chapters are read "as is" instead of also checking if they can be valid.

A file of 10 seconds cannot have chapters starting at > 10 seconds in my opinion, but I have to check the specification if overflows are allowed here.

My current approach is to write an additional FixInvalidChapters method, where the validation checks and corrections are performed, but I would like to talk to @Zeugma440 about this... maybe this is not the best idea to do :-)

private static void FixInvalidChapters(MetadataTrack track)
    {
        var newChapters = new List<ChapterInfo>();
        var totalLengthMs = (uint)track.TotalDuration.TotalMilliseconds;
        for (var i = 0; i < track.Chapters.Count; i++)
        {
            var current = track.Chapters.ElementAt(i);
            if (current.StartTime >= track.TotalDuration.TotalMilliseconds)
            {
                break;
            }

            if (current.EndTime <= current.StartTime)
            {
                if (i < track.Chapters.Count - 1)
                {
                    var next = track.Chapters.ElementAt(i + 1);
                    current.EndTime = next.StartTime;
                }
                else if(totalLengthMs > 0)
                {
                    current.EndTime = totalLengthMs;
                }
            }
            newChapters.Add(current);
        }

        track.Chapters = newChapters;

    }

[itzexor]

So your intention is to stip the metadata then? This is one file of 78. The chapter metadata is valid for the track set as a whole, and the track is only used as part of that complete track set. If the metadata is stripped then the chapter metadata as a whole is unnecessary and the track would be more efficient without chapters at all. Of course then the book has no chapter metadata.. Where else would I put it? There are no playlists or other files involved. This works as is with the exception of the chapter lengths, except the first, all being invalid.

IMO Tone shouldn't be generating additional metadata, and should just present it as is. There is no duration/length aspect to vorbis chapters.

Edit: Additionally, any chapter metadata in files 2-78 would be ignored by ABS. Only the first file is checked for chapters and cover.

[sandreas]

So your intention is to stip the metadata then?

Not exactly. The question is, if metadata can be valid, that does not fit into the file.

Where else would I put it?

Good question... I would say, that metadata of a file has to be "valid", so out of range chapters are kind of fishy. Although I must say that your approach to put the whole chapter set into the first file to trick ABS into showing them correctly is not totally wrong. I think I have to discuss this with an expert and do some research :-)

IMO Tone shouldn't be generating additional metadata, and should just present it as is. There is no duration/length aspect to vorbis chapters.

Yeah this is absolutely correct. Metadata that is not there should not come out of nowhere in the dump command.

There is no duration/length aspect to vorbis chapters.

I did not know that. Maybe I have to do some corrections, that length is not a mandatory thing to have and that files can have chapter specs, that may not fit into the file.

I think you are right, the show metadata as specified should be the way to go - and maybe later a flag --try-to-fix-invalid-chapters to perform the above method for totally unintended chapter misconceptions and bogus files.

I'll get back to you when I discussed this with @Zeugma440. Thank you for your effort to clear things up.

[itzexor]

I will concede that this is definitely non-standard, and honestly it's not my preferred approach. My initial efforts were focused on a monolithic opus file with the chapters embedded as usual. I ran into issues with firefox and vlc playing 20+ hour long opus files, and so went this route.

Thanks for your time looking into this.

PS: You mentioned a specification earlier. I'm not sure any formal specification actually exists. This is the only thing I could really find when looking into it. https://wiki.xiph.org/Chapter_Extension

[itzexor]

So I've been following the discussion on the other issue as well, and in the meantime trying to find more information on how this situation should be and is handled out there...

Usage guidelines or specification wise, I couldn't find really anything else about this.

Dumping tools wise: ffprobe $ ffprobe [file] has a similar behavior as tone/atl in that it outputs more information that it inputs. In ffprobe, the format is displayed like:

  Duration: 00:11:37.07, start: 0.000000, bitrate: 42 kb/s
  Chapters:
    Chapter #0:0: start 0.000000, end 697.063000
      Metadata:
        title           : Chapter 1
    Chapter #0:1: start 697.063000, end 697.070000
      Metadata:
        title           : Chapter 2
    Chapter #0:2: start 1930.878000, end 1930.878000
      Metadata:
        title           : Chapter 3

I don't know that I'd call this an invalid output on its own, since as @zeugma440 suggested, some chapters could be some kind of tag or cue mark rather than a full fledged chapter. Either way it: allows output exceeding the file duration, adds generated end time information, outputs without any warnings or notification that there's an issue with chapter metadata.

$ opusinfo [file] and $ mediainfo [file] both display the list like:

00:00:00.000                             : Chapter 1
00:11:37.063                             : Chapter 2
00:32:10.878                             : Chapter 3
00:45:23.096                             : Chapter 4

This is my personal expectation since that's more or less what's actually in the file.

I can also understand the aspect of a unified output between file formats. In cases where the chapters describe only the file they are contained within, which I'd imagine is the majority, it's pretty easy to generate additional valid metadata that should more or less fill in the gaps and output the same format for differing chaptering-related parameter support between containers. This only applies to the start/end paradigm for files with chapters exceeding the runtime, since it could convert the chapters to start=end "tags" (if we are accepting this to be a valid usage form), and still leave the data there as ffprobe does.

Ultimately I guess it just comes down to what do you want your tool to do @sandreas? If you'd like to have as unified json output as possible with a length property always there, then I would suggest you need to drop the invalid chapters entirely for the output, because at that point it's not ever going to be valid. Alternatively, maybe output 0 length, which would be the start=end equivalent. If you don't mind the length aspect not being there (which currently it does not generate for the first chapter in my file), then I would suggest dropping them from invalid cases and leaving them in valid cases.

I believe the lack of length on chapter 1 may be some unrelated bug? I can't figure why a length would be generated and left there for an out-of-bounds chapter, but not the first chapter. Especially when considering the first is the only one we can actually determine the length of (or at least make it start to end-of-file).

Dumping aside and bringing in the processing aspect: I ran a file through ffmpeg with the following command: ffmpeg -i [input] -metadata year=2023 -c copy [output]. Interestingly, this didn't modify or remark on the comments at all as far as I can tell...but it did do other things to the file that I can't quite explain. The metadata is all there, it just displays in different ways afterwards. Ordering and such is my best guess. The most interesting part of this, though, is that it causes the chaptering code in tone to generate an exception!

At this point with so many different things to consider, I feel like I may have put more work on you than I intended. I thought this was going to be a simple case of make it don't output "length" when it can't be determined. In the end if you feel like my usage case is not valid, or at least not within what you're wanting to support, that's totally fine with me. I will find a way to make it work. Audiobookshelf should support importing chapters by multiple files someday, and other than that I can use the filenames to transport the chapter titles. This is my last choice though due to filename restrictions, and chapters containing restricted characters often. I'd rather not lose data during that "transport", but it wouldn't be the end of the world.

[sandreas]

Ultimately I guess it just comes down to what do you want your tool to do @sandreas?

As already said, I want my tool to be as accurate as possible while retaining user friendliness and flexibility. That basically means:

IF there is a bunch of chapters in the file, show exactly these chapters, even if it is invalid information - oh, and maybe show that this could contain invalid information, if not using json as output, that can be further processed.

At this point with so many different things to consider, I feel like I may have put more work on you than I intended.

Don't feel bad about this. You've put MUCH effort into this and improving my tool is one of my favourite hobbies, so I have to thank you for this investigation.

I will also talk to @advplyr, whether his chapter handling code is robust enough to omit the length property. Probably I will just omit the length property, if it is not set and ignore the fact, that the total length of the file is shorter than the chapter endings in the dump command.

However the tag command is more tricky... I have to experiment with what happens when I use chapters for a file that exceed it's duration, probably I will not allow this without providing a flag (like --ignore-duration-for-chapters or something like this).

Anyway thanks for providing all this information. Have a nice day.

[itzexor]

I believe that exception may be an issue in the atl vorbis parser and have reported it there. https://github.com/Zeugma440/atldotnet/issues/180

[sandreas]

I believe that exception may be an issue in the atl vorbis parser and have reported it there.

Yeah that's what I would assume :-)

[itzexor]

I will also talk to @advplyr, whether his chapter handling code is robust enough to omit the length property. Probably I will just omit the length property, if it is not set and ignore the fact, that the total length of the file is shorter than the chapter endings in the dump command.

I can't speak for @advplyr or how his code works, but I can say that with the output tone is giving now, omitting the "length" property entirely on the first chapter, that chapter does get imported properly.

from the gist in the first comment:

    "chapters": [
      {
        "start": 0,
        "title": "Opening Credits"
      },
      {
        "start": 14303,
        "length": 4294952993,
        "title": "Chapter 1: A Question You Don’t Yet Know to Ask"
      },

is auto imported like this:

My assumption is that it does support lack of "length" property, at least to some degree.

[sandreas]

My assumption is that it does support lack of "length" property, at least to some degree.

Great, I think that is the way to go then. Thanks again.

[sandreas]

Fixed - length is now omitted, if chapter.EndTime < chapter.StartTime.

Release target is tone 1.1.3, hopefully soon.

[itzexor]

So I have tested the new version with the file in question, everything looks okay except the first chapter now:

    "chapters": [
      {
        "start": 0,
        "length": 0,
        "title": "Opening Credits"
      },
      {
        "start": 14303,
        "title": "Chapter 1: A Question You Don’t Yet Know to Ask"
      },
      {
        "start": 1522253,
        "title": "Chapter 2: Looking Forward"
      },

[sandreas]

So I have tested the new version with the file in question, everything looks okay except the first chapter now:

Thank you for this feedback. Yeah this is a bit tricky... I cannot differentiate between chapters with a length of 0 and chapters with NO length, when chapter start is equal to zero. Maybe it is better to omit length when chapter.End == 0 instead of chapter.EndTime < chapter.StartTime ... I'll change that immediately.

[sandreas]

So since I already had to do a re-release yesterday I improved this and did a re-release again ( I normally would never do that, but since it was not fixed correctly and the re-release already had to take place yesterday, I think this is acceptable to fix your issue completely).

Another problem is, that id3v2 chapters can have startOffset and endOffset values instead of startTime and endTime, if useOffset==true. I tried to clarify this in a discussion, maybe this requires an additional improvement. So I'll leave this issue open since this is clarified.

Thanks for your support on this.

[itzexor]

Thanks for the continued help. Sorry for how troublesome it has turned out to be! The latest release now outputs only information contained within the file, and no additional metadata.

I will understand completely if the best solution in the end turns out to be to remove this metadata. Since the file length does not match, it cannot conform to the standard metadata output... but in the interim I think this is the best alternate solution. To only put out data that can be directly observed or accurately calculated. :+1:

[sandreas]

Closed due to no further changes needed atm. May be reopened if issue happens again.