937455 - shrink vorbis encoder tables

Status: NEW

(Core :: Audio/Video: Recording, defect, P5)

Description

[Ralph Giles (:rillian)]

Monty pointed out the full encoder mode tables used by vorbisenc are quite large. It would be nice to omit some of them if we don't need the full flexibility.

Comment 1

[Ralph Giles (:rillian)]

I don't understand the requirements properly here. What numbers of channels and samplerates do we need to support for the MediaEncoder api?

Comment 2

[Ralph Giles (:rillian)]

Looks like we can get by with only Mono/Stereo. Not sure about sample rates.

Comment 3

[Monty Montgomery (:xiphmont)]

I can make a patch against vorbis upstream to remove the surround tables and fall back to pre-surround pairwise channel coupling for > 2 channels.  That means surround encoding will still work, it just won't be optimized.

Comment 4

[Ralph Giles (:rillian)]

I'd rather it just failed. More obvious than mysteriously degraded quality.

The recording API rejects non-stereo streams currently, so there's nothing we need to avoid breaking.

Comment 5

[Shelly Lin [:shelly]]

I think the recording API accepts any number of streams, it is the MediaEncoder who checks for acceptance of channels. The vorbis encoder in Bug 883749 rejects channels <= 0 || channels > 8, though, it would be an easy change to restrict it to max channels of two.

Comment 6

[Ralph Giles (:rillian)]

Right, thanks. Do you know if we can usefully restrict supported sample rates for the MediaEncoder interface.

Comment 7

[Shelly Lin [:shelly]]

Hmm...for now the only way to restrict supported sample rates is to copy what we did in OpusTrackEncoder. 

Maybe the MediaEncoder can export some methods of restricting supported channels and sample rates, so that it would be easier to set up and clearer to tell the restrictions per different codecs?

Comment 8

[Monty Montgomery (:xiphmont)]

Just so I don't accidentally drop the action-ball here, I'm waiting for a clear decision on the best path forward before doing anything; all of the suggestions made thusfar are equally doable on the Vorbis library side.

Comment 9

[Ralph Giles (:rillian)]

Ok. I think we should patch our in-tree libvorbis to remove (or just not build) the 5.1 tables and be satisfied with that. Monty, can you make it so?

Comment 10

[Monty Montgomery (:xiphmont)]

yes. Should I patch against master or upstream?

Comment 11

[Ralph Giles (:rillian)]

You should write a patch, apply it to firefox, add a copy to media/libvorbis and change update.sh to apply it after copying upstream files. In this way we maintain variance against upstream.

If you think a --disable-surround configure option is useful upstream, that would be ok too, but it sounds like more work.

Comment 12

[Monty Montgomery (:xiphmont)]

I would be worried about build of standard libvorbis that either mysteriously can't encode surround, or do a bad job of it.  For the existing installbase, this would probably violate principle of least surprise, especially when it would be hard for a user to figure it out.

Comment 13

[Ralph Giles (:rillian)]

Yep. Hence the ff-only patch.