961044 - vorbis static codebook length tables take up too much space

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Nathan Froyd [:froydnj]]

The _vq_lengthlist__* arrays in lib/books/* are all defined as arrays of long, which means they take up not quite 2.5MB on an x86-64 build and half of that on a 32-bit build.

AFAICS, though, there's no need for them to be arrays of long; arrays of uint8_t/unsigned char would work just fine, as none of the entries are ever large (scanning through the code suggests entries are limited to values <= 32).  The lists also don't appear to be used in any sort of vectorized assembly code, suggesting they can be changed freely.

Reducing them to arrays of uint8_t/unsigned char would be a huge space win.

I'm not familiar enough with vorbis to know whether this is a duplicate of bug 937455; omitting them sounds different than shrinking them, but if we can do both, that'd be great!

Comment 1

[Timothy B. Terriberry (:derf)]

Monty, can you take a look?

Comment 2

[Nicholas Nethercote [inactive]]

Nice find!

Comment 3

[Nicholas Nethercote [inactive]]

Thinking some more... vorbis isn't that common(?), so most of the time this won't affect physical memory consumption, but it will reduce virtual memory consumption, which is good for Win32.

Comment 4

[Ralph Giles (:rillian)]

Changing the base time to save footprint is indeed different from trimming the tables themselves. This is a lot easier though!

Comment 5

[Monty Montgomery (:xiphmont)]

:derf: Will look yes.

BTW, we should only have encoder tables for modes we actually use.  They're split into a few ranges of sampling rates (8, 16, 44+ IIRC) and channel sets (mono, stereo, surround).  If any of those can be eliminated as unused, that reclaims space too.

Comment 6

[Monty Montgomery (:xiphmont)]

OK, looked it over and looked at the code history.  There is no practical reason the lengthlist needs to be long; uint8 is more than sufficient.  There will need to be only minor code work to make it so-- testing will be a bigger deal than the actual modifications.

The long lengthlists appear stem from a time when the lengthlists were just not very big.  When things moved from tesselated to lattice codebooks (and lengthlists became larger), they weren't changed to take less space.

I don't see any external considerations either; shall I make this change upstream?  One reason I'm offering is because there _must_ be something I'm missing, this just seems too obvious.

Comment 7

[Monty Montgomery (:xiphmont)]

(In reply to Ralph Giles (:rillian) from comment #4)
> Changing the base time to save footprint is indeed different from trimming
> the tables themselves. This is a lot easier though!

We could do both, both look quite easy really.

Comment 8

[Ralph Giles (:rillian)]

(In reply to Monty Montgomery (:xiphmont) from comment #6)

> I don't see any external considerations either; shall I make this change
> upstream?

Yes, please.

Comment 9

[Monty Montgomery (:xiphmont)]

I have landed the lengthlist long -> char conversion upstream.  I used 'char' and not another type to follow the preexisting convention elsewhere in the codebook code.

I'll cut a new libvorbis dot release from the current code today.

Comment 10

[Ralph Giles (:rillian)]

Attachment: Upstream fix

Attaching patch for monty's upstream fix. Rather than carry this, I'll prepare a patch moving us to the new 1.3.4 release.

Comment 11

[Ralph Giles (:rillian)]

Attachment: Update libvorbis to 1.3.4

Comment 12

[Ralph Giles (:rillian)]

Pushed to try https://tbpl.mozilla.org/?tree=Try&rev=a45e6e7d64eb

Comment 13

[Ralph Giles (:rillian)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6aefaebc145d

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6aefaebc145d

Comment 15

[Nathan Froyd [:froydnj]]

Nominating for 1.3T based on library footprint reduction (somewhere around a megabyte).

Comment 16

[Joe Cheng [:jcheng] (please needinfo)]

can you confirm is this memory saving in RAM or storage size? Thanks

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Joe, from what I understand this is mainly storage size.  I recommended Nathan nom these since I believe we are short on system partition space on the tarako.

Comment 18

[Nathan Froyd [:froydnj]]

I don't have anything to add to what Ben already said.

Comment 19

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/aada028a135e