Setting DecodedTime Throws ArgumentOutOfRange Exception

Nov 17, 2014 at 8:44 PM
Whenever I attempt to set the DecodedTime of a VorbisReader to anything but zero, it throws an ArgumentOutOfRange exception. Seemingly, this happens at random, as some computers are affected and others are not.

However, if I catch the exception and immediately attempt to set the DecodedTime to the exact same value, it works just fine. The ogg file begins playback from the requested time as expected.

Poking around in the NVorbis code, if I change line 405 in OggPacketReader.cs from:
while (packet != null && packet.PageSequenceNumber == lastPacketInPage.PageSequenceNumber);
...to something like:
while (packet != null && packet.PageSequenceNumber >= 0);
...then seeking works perfectly well.

But although it solves my problem, this strikes me as more of a hack than a proper fix. I'm not particularly familiar with the structure of .ogg files, but I assume the FindPacket method in OggStreamDecoder.cs or the packetGranuleCountCallback callback in VorbisReaderDecoder.cs is not calculating the right page to look in for some reason.
Coordinator
Nov 17, 2014 at 9:12 PM
My immediate thought is "race condition", but I'm not certain that tells the whole story. By the time FindPacketInPage gets called, the "correct" packet should be in the linked list and "find-able".

BTW, I believe your fix is incorrect. It'll always return the 2nd last packet on the correct page, which is (obviously) incorrect in most cases. In any case, the change is relatively benign since the seek logic is smart enough to update the indicated position in the decoder correctly even on a mis-seek.

Lemme see that exception and we'll go from there.
Nov 17, 2014 at 10:35 PM
Yeah, I figured it wasn't a permanent fix. When FindPacketInPage is called by FindPacket, the linked list is in place as far as I can tell, but looking at the requested positions, it seems like it's searching too far ahead? I'm honestly not sure.

The actual exception is thrown on line 871, because FindPacket returns null. Here is the section in question:
if (granulePos > 0)
{
  packet = _packetProvider.FindPacket(granulePos, GetPacketLength);
  if (packet == null) throw new ArgumentOutOfRangeException("granulePos");
}
I wouldn't doubt that some sort of race condition is agitating the problem. The problem not showing up hardly at all on some computers (like my old system), and ordering the thread to sleep for an arbitrary long time (such as a full second) before calling FindPacket eliminates the problem, if only about a quarter of the time. Setting the DecodedTime value after the song is finished to go back to the loop point never seems to throw the exception either.

Still, I'm not sure a race condition the root cause.

Oddly enough, what seems to work 100% of the time is this:
if (granulePos > 0)
{
  packet = _packetProvider.FindPacket(granulePos, GetPacketLength);
  packet = _packetProvider.FindPacket(granulePos, GetPacketLength);
  if (packet == null) throw new ArgumentOutOfRangeException("granulePos");
}
First time returns null, but the second time returns the correct result.
Coordinator
Nov 17, 2014 at 10:38 PM
Interesting!

Another question: Are you seeking forward or backward when FindPacket returns null? I'm going to guess "forward". :)
Nov 17, 2014 at 10:46 PM
When the exception is thrown, I'm seeking forward. It's right after the VorbisReader object has been created. Doing it before or after reading samples changes nothing.

Using DecodedTime to loop back never seem to throw the exception. In fact, looking back at my code I just realized I never even bothered to wrap the looping code in a try/catch block.
Coordinator
Nov 17, 2014 at 11:05 PM
OK, sounds like it isn't loading pages like it should... I'll have to read the code to see where it's not working right. Might need to look at it in the morning since I'm a bit fried this evening... ;)
Nov 18, 2014 at 12:16 AM
Edited Nov 18, 2014 at 1:10 AM
Well, when you do get the chance to look at it, I may have found the problem.

OggPacketReader.cs method GetLastPacketInPage will occasionally return a packet that is, in fact, in the next page.

GetLastPacketInPage, after reaching the last packet, checks if the packet following it (Packet.Next) has been set. If it's null and Packet.IsContinued is true, the method eventually calls ContainerReader.GatherNextPage which, somewhere down the line, calls OggPacket.MergeWith. OggPacket.MergeWith has the following section:
// per the spec, a partial packet goes with the next page's granulepos.  we'll go ahead and assign it to the next page as well
PageGranulePosition = continuation.PageGranulePosition;
PageSequenceNumber = continuation.PageSequenceNumber;
And so the "last packet" that GetLastPacketInPage has found gets its page number changed, tripping up FindPacketInPage. This would also explain why it works the second time around; at that point, the correct page value has been assigned to the packets.

No clue the correct way to go about making this work, though.

EDIT: Actually, now that I think about it...
Packet GetLastPacketInPage(Packet packet)
{
    if (packet != null)
    {
        var pageSeqNumber = packet.PageSequenceNumber;

        while (packet.Next != null && packet.Next.PageSequenceNumber == pageSeqNumber)
        {
            packet = packet.Next;
        }

        while (packet != null && packet.IsContinued)
        {
            // gotta go grab the next page
            if (_eosFound)
            {
                packet = null;
            }
            else
            {
                _container.GatherNextPage(_streamSerial);
                if (_eosFound)
                {
                    packet = null;
                }

                // Check if the page sequence number changed.
                while (packet != null && packet.PageSequenceNumber > pageSeqNumber)
                {
                    packet = packet.Prev;
                }
            }
        }
    }
    return packet;
}
Seems to be working now.
Coordinator
Nov 18, 2014 at 1:16 AM
Well I feel sheepish...

In GetLastPacketInPage(Packet),
while (packet != null && packet.IsContinued)
{
    // code removed for brevity
}
should be
if (packet != null && packet.IsContinued)
{
    // move to the *actual* last packet of the page... If .Prev is null, something is wrong and we can't seek anyway
    packet = packet.Prev;
}
This returns the correct packet, plus it delays loading the next page until actually needed.

Try it out and let me know. I'll test it and check in the change once I get my dev environment back up and running (new machine).
Coordinator
Nov 18, 2014 at 1:19 AM
Just noticed your updated version. That'll work too, though I prefer my version. :)
Nov 18, 2014 at 1:28 AM
Yeah, I prefer your version too. :P

Either way, it seems to be working just fine. Seeks correctly every time, forwards or backwards. Thanks a lot!
Coordinator
Nov 18, 2014 at 1:33 AM
Awesome! I'll try to get that into the source tree tomorrow and possibly do a release.

Thanks for helping me troubleshoot it.