Forked on github

Sep 5, 2012 at 2:38 PM

Hi again, just a word to tell you that I forked the project on GitHub : https://github.com/renaudbedard/nvorbis

The changes I made to the library itself are small, I just profiled the code with the CLR Profiler and got rid of obvious useless dynamic allocations, since I want to use this in a realtime context.

I also made a realtime streaming test case for OpenTK/OpenAL, which is what I use it for.

Let me know if I breached a license or something!

Coordinator
Sep 6, 2012 at 1:42 PM
Edited Sep 6, 2012 at 2:02 PM

Cool...  VorbisCodebook.DecodeVQ(...) is supposed to eliminate allocations by using a closure...  Is the framework doing allocations behind my back?  I'll try profiling it again to see what is going on.

Also, I've implemented a slightly tweaked VorbisFloor.Floor1 where the post array (y in 0.4) is only allocated once per instance per concurrent thread.  It supports up to 62 post values in each packet, so it "should" work for all reasonable files.  If not, it's easily increased.

I'll have this change and a bugfix in Lib\Ogg\OggPacketReader.cs pushed to the main tree in a bit.

Coordinator
Sep 6, 2012 at 2:56 PM

D'oh!  The closures where dynamically creating object behind the scenes.  I should've known better. :(

Check out the latest commit.  It's probably still not quite as fast as your version, but I can't convince myself the codebook needs to know how to decode the residue data...

Oh, on the license: Give credit (a link to this site will work), leave the courts out of it, and try to be fair.  I'm not terribly picky beyond that (hence the Ms-PL license).

Sep 6, 2012 at 3:07 PM

Sweet, thanks for taking a look!

The 29 number came from empirical data, I ran the decoder over my files and couldn't see any more posts than 29, usually just 19. 62 should be plenty. But you were right about thread safety, didn't consider that.

I'll run your new code through the CLR Profiler to see if there's really a difference between my DecodeVQ hack and your version.

Sep 6, 2012 at 3:16 PM
Edited Sep 6, 2012 at 5:30 PM

Hmm, I'm not sure what happened, but after my pull, ReadSamples() fails at the first ReadPosts() call in Floor1.UnpackPacket().
It returns null because the packet.ReadBit() returns false and the next assignment to data.BlockSize throws a NullReferenceException.

http://i.imgur.com/dp2Hv.png

Coordinator
Sep 6, 2012 at 4:24 PM

Oops...  For now, just revert the change in VorbisFloor.Floor1.  I didn't run into the same problem you did, but there's a "warble" in the output of the new version, so I've scrapped it and will try again later.  The allocation shouldn't affect runtime performance very much (only one small allocation per packet), so I'm "ok" with it for now.  Fixing the VQ decode was a *much* bigger win.

If you want to try your hand at fixing the floor1 decode, go for it.

Sep 6, 2012 at 4:29 PM

Yep, the revert fixed it, thanks! To be honest I don't understand the code or spec enough to try and fix it myself, but I can easily live with the old version. :)

Coordinator
Sep 6, 2012 at 4:51 PM

Fair enough.  On a side note, the "warble" I was hearing was due to my playback application, *not* NVorbis.  Grrr...

I'll try putting the changes back in, but this time in a way that works right for all files... ;)

 

While I'm thinking about it, do you mind if I include cycle0.ogg in the test files?

Sep 6, 2012 at 4:53 PM

Unfortunately this was created for our game and is licensed content, so no... I can't really let it on my public dropbox too long either, I just wanted to give you a test sample.

Coordinator
Sep 6, 2012 at 5:21 PM

OK, I'll keep it out of the master repository.  I assume I need to "lose" it as soon as I'm done testing, as well?

I've pushed the optimized version of Floor1 to master.  The CLR profiler now says Ogg.ContainerReader.GatherNextPage(int) is the worst offender, which is "correct" in this design.  There are places we can improve the memory usage, but we've hit all the big stuff.

Next on my list: Seeking support...

Sep 6, 2012 at 5:30 PM

Keeping it for local testing is not something I'm too worried about, I just don't want it to spread wildly.

Just tried out the new Floor1, no problem!

Coordinator
Sep 13, 2012 at 12:39 PM

FYI, I fixed a couple other "over-allocation" issues and an ogg container handling bug.

Sep 13, 2012 at 3:59 PM

Yep, keeping up to date every couple of days, thanks for keeping at it!
It's working pretty well in-game already. 

Oct 22, 2012 at 4:27 AM

By the way, I just pushed an update to my github fork that separates the logging stuff from the OpenTK streaming classes. Still works like before but much cleaner, if you want to integrate that.

Coordinator
Oct 26, 2012 at 8:38 PM

Cool...  I'll probably pull that when I get back to doing "work" on it again.  Been on vacation until today :)