opus: Handle large comment headers

[robho]

A comment header may span multiple pages and in such situations we need to delay parsing until we have read all pages which belong to the comment header packet. The final page in the comment header packet is marked by setting "granule position" to zero.

It's common to have comment headers spanning multiple pages when album art is embedded in an opus file. A single page can contain 65025 bytes so if tags + picture needs more space then additional pages are needed.

[robho]

LMS had problems scanning many of my opus files without this fix.

I think this should fix andygrundman#7.

[mw9]

@robho
One assumption behind this change is that “intermediate” comment pages will always set a non-zero granule position.

RFC 7845 does not actually specify this, as far as I can see. It only requires that the first and last (if different) comment page(s) set a zero granule position, and is silent about the intermediate pages.

So it is “legal” for an opus file to have “intermediate” comment pages with a zero granule position. In such cases Audio::Scan would still fail to parse the file.

Thoughts ?

[robho]

RFC 7845 says:

A page that is entirely spanned by a single packet (that completes on
a subsequent page) has no granule position, and the granule position
field is set to the special value '-1' in two's complement.

So, a zero granule position for intermediate pages shouldn't exist, IMHO.

(The opus file attached to this PR has granule position == -1 in the intermediate pages. It's encoded by opusenc opus-tools 0.1.10 (using libopus 1.3.1). I haven't tested this PR with opus files encoded by something else than this encoder so it would be interesting to see how other encoders behave.)

[mw9]

So, a zero granule position for intermediate pages shouldn't exist, IMHO.

After reading the RFC again, I agree.

I had initially thought that that paragraph applied only to audio packets. In which case one would have needed to resort to examining "lacing values" in the stream to detect intermediate pages. Perfectly do-able, but rather more complicated. Your proposal is much more straightforward.

I haven't tested this PR with opus files encoded by something else than this encoder so it would be interesting to see how other encoders behave.

The only other encoder I have tried is ffmpeg. But it uses libopus, and, anyway, doesn't handle the artwork that gives rise to large comment headers. I would guess that any current encoder is likely to be using libopus (provided by Xiph.Org through https://opus-codec.org).

I think this is a worthwhile change. Scanning issues will inevitably arise if/when LMS users start to make use of ogg/opus, unless Audio::Scan is 'mended'.

Perhaps it should be offered up to https://github.com/andygrundman/Audio-Scan.

[michaelherger]

Can't say about the change itself... but would you mind adding a line to the change log (https://github.com/LMS-Community/Audio-Scan/blob/master/Changes)?

[robho]

Perhaps it should be offered up to https://github.com/andygrundman/Audio-Scan.

There's a reference to this fix in andygrundman#7 so they can take a look at this fix if/when they want to work on the issue.

---

would you mind adding a line to the change log (https://github.com/LMS-Community/Audio-Scan/blob/master/Changes)?

Done.

[michaelherger]

@robho or @mw9 - can one of you please create a PR upstream?

[robho]

@robho or @mw9 - can one of you please create a PR upstream?

Here it is: andygrundman#13