748144 - Opus codec in <audio> and <video> should support multichannel audio

Status: RESOLVED FIXED

(Core :: Audio/Video, enhancement)

Description

[Ralph Giles (:rillian)]

The Ogg Opus specification requires support for multichannel (surround) audio files.

This bug is for updating the decoder from bug 675225 to handle multichannel files. This involves parsing the stream mapping table from the header, calling opus_multistream_decode instead of opus_decode and channel mapping or downmixing the result to what the audio api supports.

See https://wiki.xiph.org/OggOpus for format details.

Comment 1

[Josh Matthews [:jdm]]

Want to link to a source file this work would take place inside?

Comment 2

[Ralph Giles (:rillian)]

If you're asking about test files, I believe opusenc from https://git.xiph.org/?p=users/greg/opus-tools.git will produce them from multichannel input files, but I haven't checked.

Comment 3

[Josh Matthews [:jdm]]

Nope, I'm interested in links for people who want to do this work but have no idea where to begin.

Comment 4

[Alex Chronopoulos [:achronop]]

Hi, I am new here. Can I work on this?

Comment 5

[Timothy B. Terriberry (:derf)]

(In reply to Alexandros [:achronop] from comment #4)
> Hi, I am new here. Can I work on this?

Yes! If you need any help, feel free to join #opus on freenode or #media on irc.mozilla.org.

Comment 6

[Ralph Giles (:rillian)]

Welcome, Alexandros. Please do! I'm not going to get to it in the next week.

The first step is to copy the stream mapping table from the end of the OpusHead packet into a member variable in nsOpusState::ReadHeaders. Then use opus_multistream_decoder_create() and opus_multistream_decode[_float]() instead of opus_decode*.

Comment 7

[Alex Chronopoulos [:achronop]]

Thanks very informative! I guess you mean the nsOggReader::ReadHeaders. I let you know my progress.

Comment 8

[Ralph Giles (:rillian)]

Sorry, I was referring to nsOpusState::DecodeHeader, which is called from nsOggReader::ReadHeaders.

There next issue you might hit is that there's no multistream equivalent of opus_decode_get_nb_samples(), which we use in nsOggReader::DecodeAudio to allocate the appropriately sized pcm buffers for each packet.

I think you can use opus_packet_get_nb_frames and opus_packet_get_samples_per_frame on multistream packets and just multiply those two numbers together. You might want to double-check the code there though.

Tim's response from IRC:

rillian_lime: I just noticed there is no opus_multistream_get_nb_samples
rillian_lime: do opus_packet_get_nb_frames and opus_packet_get_samples_per_frame work on multistream packets?
derf: Yes.
derf: To be more precise, each stream can use a different frame size, but it has to also change the number of frames so that the product of those two return values is the same for all streams.
derf: So it suffices to check the first one.
derf: (the assumption that they all have the same number of total samples is validated when you actually decode)

Comment 9

[Alex Chronopoulos [:achronop]]

I guess I am looking for vorbis decoder. A problem is that I am not able to find where does the multichannel decoder live. Also I am not sure if we interesting only about channel mapping family 2 or we need to parse the optional channel mapping table from the header.

Comment 10

[Ralph Giles (:rillian)]

We're interested in adding support for channel mapping family 1. Mapping family 0 is what we support now, and the higher families don't have a defined speaker assignment for each channel, so they're only useful for specialist audio applications like multitrack mixers.

Step one for this is to replace the calls to opus_decode* with calls to opus_multistream_decode* from https://mf4.xiph.org/jenkins/view/opus/job/opus/ws/doc/html/opus__multistream_8h.html

The main difference is that the call to opus_multistream_decoder_create() takes three additional parameters: the number of coded streams, which we already parse as mStreams; the subset of streams which should be decoded as a coupled pair, which is the next byte after the one we read as mStreams, and the channel mapping table, which is the rest of the packet. So you need to read mCoupledStreams, and copy the mapping table into a buffer so you can pass those three additional parameters when creating the decoder in nsOpusState::Init.

That's the first part of the patch, and we can test that with stereo opus files using two uncoupled streams to produce two output channels. Once that works, the second part is to add downmix matricies so we can do something useful when the file has more channels than the platform supports for playback.

Does that make sense?

Comment 11

[Alex Chronopoulos [:achronop]]

I see what you say. Just to clarify for first step, I will replace totally the opus.h with opus_multistream.h

Comment 12

[Ralph Giles (:rillian)]

You'll need to include both opus.h and opus_multistream.h, but you should replace all the opus_decode calls, declared in opus.h, with opus_multistream_decode calls declared in opus_multistream.h.

Comment 13

[Alex Chronopoulos [:achronop]]

I created 2 new variables:
  int mCoupledStreams; //Number of packed coupled streams in each packet.
  const unsigned char* mMappingTable; // Channel mapping table.
I am not sure what values should be assigned to them in nsOpusState::DecodeHeader() when the channel mapping family is 0 (mChannelMapping == 0).

Another thing in case that channel mapping family is 1 at the same point of nsOpusState::DecodeHeader(), I believe I should check if the packet is greater than 20 bytes (currently is 19) since I try to read the extra part of the header. Is that correct?

Comment 14

[Timothy B. Terriberry (:derf)]

(In reply to Alexandros [:achronop] from comment #13)
> I created 2 new variables:
>   int mCoupledStreams; //Number of packed coupled streams in each packet.
>   const unsigned char* mMappingTable; // Channel mapping table.
> I am not sure what values should be assigned to them in
> nsOpusState::DecodeHeader() when the channel mapping family is 0
> (mChannelMapping == 0).

Quoting http://wiki.xiph.org/OggOpus :
* Channel count 'c'
The number of channels byte specifies the number of output channels (1...255) for this Ogg Opus stream.
...
* Two-channel stream count 'M'
...
For channel mapping family 0, this value defaults to c-1 (i.e., 0 for mono and 1 for stereo), and is not coded.
...
* Channel mapping
...
For channel mapping family 0, the first index defaults to 0, and if c==2, the second index defaults to 1. Neither index is coded.

> Another thing in case that channel mapping family is 1 at the same point of
> nsOpusState::DecodeHeader(), I believe I should check if the packet is
> greater than 20 bytes (currently is 19) since I try to read the extra part
> of the header. Is that correct?

You need to ensure there is at least one additional byte per channel. The total size of the packet for channel mapping family 1 must be 19+c bytes.

Comment 15

[Alex Chronopoulos [:achronop]]

I have problem in building. The source compile without error but I get error later in linking the new object (opus_multistream) and the process abort with "undefined reference". Should I build something else before? I copy part of the error:

-lglib-2.0   -lXt -lgthread-2.0 -lfreetype    -ldl  -lrt    
../../content/media/ogg/nsOggCodecState.o: In function `nsOpusState::Init()':
/home/achronop/MyRepos/mozilla/src/content/media/ogg/nsOggCodecState.cpp:838: undefined reference to `opus_multistream_decoder_create(int, int, int, int, unsigned char*, int*)'

Comment 16

[Timothy B. Terriberry (:derf)]

(In reply to Alexandros [:achronop] from comment #15)
> I have problem in building. The source compile without error but I get error
> later in linking the new object (opus_multistream) and the process abort
> with "undefined reference". Should I build something else before? I copy
> part of the error:

You need to add the new symbols you're using to layout/media/symbols.def.in

Comment 17

[Alex Chronopoulos [:achronop]]

I added the new definitions to the file layout/media/symbols.def.in but I still get the same error.

Comment 18

[Timothy B. Terriberry (:derf)]

(In reply to Alexandros [:achronop] from comment #17)
> I added the new definitions to the file layout/media/symbols.def.in but I
> still get the same error.

Then I don't know off-hand what the issue is. If you post a WIP patch, I may be able to help you debug further.

Comment 19

[Alex Chronopoulos [:achronop]]

Attachment: WIP patch

This is an initial version that contains some hard coded points hoping that works though.

Comment 20

[Ralph Giles (:rillian)]

OK, I'm stumped. I get a similar link error:

/home/giles/mozilla/firefox/content/media/ogg/nsOggReader.cpp:399: undefined reference to `opus_multistream_decode_float(OpusMSDecoder*, unsigned char const*, int, float*, int, int)'
/usr/bin/ld: libxul.so: hidden symbol `opus_multistream_decode_float(OpusMSDecoder*, unsigned char const*, int, float*, int, int)' isn't defined
/usr/bin/ld: final link failed: Bad value
collect2: ld returned 1 exit status
make[1]: *** [libxul.so] Error 1
make[1]: Leaving directory `/home/giles/mozilla/firefox/obj-x86_64-unknown-linux-gnu/toolkit/library'

The link line has:

../../staticlib/components/libgklayout.a (which referrs to content/media/ogg/libgkconogg_s.a which has nsOggReader.o)

followed later by:

 ../../dist/lib/libgkmedias.a
(which has media/libopus/libopus.a which has media/libopus/opus_multistream.o)

$ nm obj-x86_64-unknown-linux-gnu/media/libopus/opus_multistream.o
[...]
0000000000000000 T opus_multistream_decode
0000000000000000 T opus_multistream_decode_float
0000000000000000 t opus_multistream_decode_native
[...]

Comment 21

[Ralph Giles (:rillian)]

I thought maybe it was a function signature mis-match, since it mentions 'unsigned char const *' instead of 'const unsigned char *' as declared in the header, but adding casts hasn't resolved the error.

Comment 22

[Ralph Giles (:rillian)]

Derf pointed out we need

 #ifdef __cplusplus
 extern "C" {
   ...
 #endif

protection in media/libopus/include/opus_multistream.h.

Comment 23

[Ralph Giles (:rillian)]

So, instead of

#include "opus/opus_multistream.h"

use

extern "C" {
#include "opus/opus_multistream.h"
}

That should work around the build issues until we can get upstream fixed.

Comment 24

[Timothy B. Terriberry (:derf)]

(In reply to Ralph Giles (:rillian) from comment #23)
> That should work around the build issues until we can get upstream fixed.

Confirmed, that works for me. gmaxwell has a patch for upstream at https://git.xiph.org/?p=opus.git;h=64c2dd7d8e6b waiting to be merged by jmspeex.

Comment 25

[Ralph Giles (:rillian)]

I the meantime I'd prefer the workaround from comment #23 to than carrying the patch in our libopus tree. We can remove it when the next upstream release happens.

Comment 26

[Alex Chronopoulos [:achronop]]

Can you please provide me a short description how to test the patch. I cannot find any Opus stream in Icecast and I am not sure how to use the opus-tools.

Comment 27

[Timothy B. Terriberry (:derf)]

(In reply to Alexandros [:achronop] from comment #26)
> Can you please provide me a short description how to test the patch. I
> cannot find any Opus stream in Icecast and I am not sure how to use the
> opus-tools.

There's a stream at http://phobos.kradradio.com:8080/phobos.opus (not multichannel, but still good to test against).

You can also find a number of invalid files at https://people.xiph.org/~greg/opus_funtest/
These should help make sure we fail cleanly when given malicious inputs.

Comment 28

[Gregory Maxwell]

I threw up some quick multichannel examples at https://people.xiph.org/~greg/opus_multichannel_examples/

Comment 29

[Alex Chronopoulos [:achronop]]

(In reply to Gregory Maxwell from comment #28)
> I threw up some quick multichannel examples at
> https://people.xiph.org/~greg/opus_multichannel_examples/

What is the different of the three files? While it can play the first one the other two crash firefox.

Comment 30

[Alex Chronopoulos [:achronop]]

(In reply to Timothy B. Terriberry (:derf) from comment #27) 
> There's a stream at http://phobos.kradradio.com:8080/phobos.opus (not
> multichannel, but still good to test against).

This works fine!

> You can also find a number of invalid files at
> https://people.xiph.org/~greg/opus_funtest/
> These should help make sure we fail cleanly when given malicious inputs.

Some of them crash firefox and some of them provide a good looking message that the file is corrupted (I guess this is what we want). Can I produce real time traces or are logs available, to check it farther?

Comment 31

[Ralph Giles (:rillian)]

(In reply to Alexandros [:achronop] from comment #29)

> What is the different of the three files? While it can play the first one
> the other two crash firefox.

The first one is supposed to be a multichannel stereo file. That is, two uncoupled channels, one for right and one for left, packed as two mono streams. It will play nonsense with current firefox, and should work with your patch. However, the version you tried is incorrect, and just contains mono audio. Greg will update soon.

The second two are 6 channel files, and won't work properly until you add downmix support.

Comment 32

[Ralph Giles (:rillian)]

Greg has updated shinjuku.uncoupled.opus. The new one is a proper multistream stereo file. Getting playback to sound like 'padsp opusdec shinjuko.uncoupled.opus' should be the first goal of your patch.

The corrected version is 152385 bytes, md5sum a79ce810b7c45257f57284503257f7b7.

Comment 33

[Ralph Giles (:rillian)]

Attachment: Uncoupled stereo example

Attaching Greg's multichannel stereo test file. This is the shinjuku.uncoupled.opus mentioned above.

Note that both the left and right channels are identical in this file.

Comment 34

[Ralph Giles (:rillian)]

Note: once basic multistream decoding is working, we'll need to add a downmix matrix to convert 5.1, etc. to the stereo format our audio library supports. There are some simple matrices you can borrow in the opus-tools package.

Look for setup_downmix and read_downmix in audio_in.c: https://git.xiph.org/?p=opus-tools.git;a=blob;f=src/audio-in.c

Comment 35

[Alex Chronopoulos [:achronop]]

I tested Greg's file and it seems to work. I will move on to the next step according to the note above.

Comment 36

[Alex Chronopoulos [:achronop]]

Just to clarify, you need only the matrix for now and not any downmix logic?

Comment 37

[Ralph Giles (:rillian)]

I don't know what the distinction is. Right now firefox only supports mono and stereo playback, so if the file has more than 2 channels, you need to add functions to mix that down to 2 channels.

Comment 38

[Alex Chronopoulos [:achronop]]

Attachment: Downmix to mono

Downmix to mono for channels > 2

Comment 39

[Ralph Giles (:rillian)]

Comment on attachment 631544 [details] [diff] [review]
Downmix to mono

Generally, patch authors set feedback? if they want feedback, and the person they've asked sets + or -.

But you asked on irc. I think you need to tell the nsOggReader what number of channels you'll be decoding to, as well as passing the channel count in mAudioQueue.Push(). I hacked up your patch to output mono-as-stereo, and set mInfo.mAudioChannels = 2 in nsOggReader::ReadMetadata() and got the expected output.

Comment 40

[Ralph Giles (:rillian)]

Attachment: mono as stereo example

This is my hacked up version of the attachment 631544 [details] [diff] [review], which gives mono playback of the multistream test files.

Comment 41

[Alex Chronopoulos [:achronop]]

Thanks for the support. I am able to get the correct output now. Should I add any normalization in my patch?

Comment 42

[Ralph Giles (:rillian)]

That's great news!

I believe the normal approach with mixdowns is add the channels, not average them. So you shouldn't need to normalize, just make sure the total energy stays the same.

Comment 43

[Ralph Giles (:rillian)]

So I did a bit of research. Maybe you already know all this, but the short answer seems to be that the particular matrix you use isn't all that important, just do something reasonable. So copying the code in opusenc is still a reasonable thing to do. Hopefully by review time I'll decided whether I think it's correct or not. :)

http://ac3filter.net/wiki/Mixing_matrix suggests normalizing each row of the downmix matrix so avoid overdrive when trying to make two speakers do the work of six. On the other hand, for more average signals that will be too quiet. Probably best to get some things working and experiment.

Comment 44

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Alexandros [:achronop] from comment #29)
> (In reply to Gregory Maxwell from comment #28)
> > I threw up some quick multichannel examples at
> > https://people.xiph.org/~greg/opus_multichannel_examples/
> 
> What is the different of the three files? While it can play the first one
> the other two crash firefox.

We should never crash while loading media files, regardless of whether they're invalid or not.

So it'd be really great if you could file a bug and attach the invalid Opus files which cause the crashes, so we can make our implementation robust enough to do something sensible, rather than just crashing.

Comment 45

[Timothy B. Terriberry (:derf)]

(In reply to Chris Pearce (:cpearce) from comment #44)
> We should never crash while loading media files, regardless of whether
> they're invalid or not.

We don't. As discussed in comment 31, the crashes were in an early version of the multichannel Opus support patch. They don't crash current trunk (nor Fx 15). The crashes discussed in comment 30 were all fixed before the uplift (see bug 750714).

Comment 46

[Alex Chronopoulos [:achronop]]

I tested two different matrices, the one found in opusenc and another more simple. The second matrix provide all left channels to left and all right to right and split the rest in the middle. For both matrices I created an normalized version. I can say that after normalization the sound is significantly attenuated. After some tests I ended up with the following choices:

Sounds with channel > 8 are downmixed to a pseudo stereo with two identical channels and no normalization. Sounds with channels between 3 and 8 are downmixed with the second matrix without normalization. Of-course all of these are under debate and I am expecting your comments.

PS. Are there more opus samples to test?

Comment 47

[Ralph Giles (:rillian)]

Thanks for the summary, sorry for not getting back to you promptly.

That sounds about like what I expected. Personally, I like the oggenc 5.1 matrix better since it defuses and scales the surround channels, which I think is more accurate. There's no right answer to 'normalize' vs clip vs compressor. What do you think of a compromise, e.g. normalizing the matrix rows to nChannels/2 or nChannels/3 instead of to 1?

For > 8 channels we don't have a channel mapping, so I think we should just refuse to play such opus streams for now.

Please attach your patch so we have something concrete to look at. It would be great to get this landed soon; the deadline for new Firefox 16 features is coming up if a couple of weeks.

Comment 48

[Ralph Giles (:rillian)]

For more test files, I haven't made any new ones. Greg added my 6.opus to his collection at https://people.xiph.org/~greg/opus_testvectors/opus_multichannel_examples/

Comment 49

[Ralph Giles (:rillian)]

Oh, and if it's helpful, there are binary builds of opusenc available for macos and win32 now at https://ftp.mozilla.org/pub/mozilla.org/opus/. If you're using one of those systems it might be easier than building the tools yourself.

Comment 50

[Alex Chronopoulos [:achronop]]

Attachment: Fix for bug 748144

Since the deadline is approaching I attach the patch for review. Feel free to make comments in choices I have made, among other, and I will be happy to follow. In general I replaced the downmix matrix with the one in opusenc, added a normalize factor = 2, and abort for channels > 8.

Comment 51

[Ralph Giles (:rillian)]

Comment on attachment 631572 [details] [diff] [review]
mono as stereo example

This is obsoleted by :acronop's most recent patch.

Comment 52

[Ralph Giles (:rillian)]

Comment on attachment 638126 [details] [diff] [review]
Fix for bug 748144

Thanks for posting your updated patch. You're almost there, but it's not ready to land yet. Comments:

> -  mPrevPageGranulepos(0)
> +  mPrevPageGranulepos(0),
> +  mCoupledStreams(0)

Please initialize mCoupleStreams after mStreams, in the same order as the declaration.

> } else if (aPacket->bytes > 19) {
>         mStreams = aPacket->packet[19];
> +        mCoupledStreams = aPacket->packet[20];
> +        int i;
> +        for (i=0; i<mChannels; i++)
> +           	mMappingTable[i] = aPacket->packet[21+i];

You need to add further bounds-checks when reading mCoupledStreams (bytes > 20) and mMappingTable (bytes > 21+mChannels) and if the packet is short, return false as in the file 'else' clause.

> +      dBuffer[i*out_channels]=sampL/normalize;
> +      dBuffer[i*out_channels+1]=sampR/normalize;

I'm unhappy with this per-sample divide. This is also not how normalization works. The ac3filter article I linked in comment #43 isn't explicit about this, but if you look at their example, each row is multiplied *independently* by a factor which makes the elements of that row sum to the normalization factor (2 in our case).

Maybe the best thing to do here is to type in the normalized matrix and use it directly. If you'd rather keep it obvious what the matrix is, maybe generate the normalization once the number of channels is known, and store it in a member variable?

This also needs to work with fixed-point samples. At the very least, you should clamp so the sample range (-1.0, 1.0) for float, (-32768, +32767) for short. At best, a separate fixed-point implementation of the matrix calculation. If you don't want to do that, you could also reject streams with mChannels > 2 when using fixed-point samples in this patch and tackle that issue in another patch.

Comment 53

[Ralph Giles (:rillian)]

Comment on attachment 631544 [details] [diff] [review]
Downmix to mono

This is also obsoleted by the most recent patch.

Comment 54

[Ralph Giles (:rillian)]

As an example of what I mean by per-row normalization, each row (or column, I guess) of the 5.1 matrix line sums to 1 + 2/sqrt(2) + 1/2 + sqrt(3)/2, or about 3.7802. So to normalize to 2, multiply each element by 2.0/3.7802 for:

/*6*/{ {0.5291f,0.f}, {0.3741f,0.3741f}, {0.f.0.5291f}, {0.4582f,0.2645f}, {0.2645f,0.4582f}, {0.3741f,0.3741f}},

And you can verify the gains for the left and right output channels each sum to 2.0.

Comment 55

[Alex Chronopoulos [:achronop]]

Attachment: Patch for bug 748144 updated

Comment 56

[Alex Chronopoulos [:achronop]]

Comment on attachment 642375 [details] [diff] [review]
Patch for bug 748144 updated

I attach the updated patch according to you comments. I have modified the downmix matrix to be normalized to 2.

+        // Validate Channel Mapping Table length before copy
+        if (aPacket->bytes>20 && aPacket->bytes<22+mChannels) {
+          int i;
+          for (i=0; i<mChannels; i++)
+            mMappingTable[i] = aPacket->packet[21+i];
+        }

I added further bounds when I read the Channel Mapping Table. I have a question though. I would expect that the boundaries should be between byte 21 and 21+mChannels-1. But this did not work and after testing it with the given files I end up with the bound above.

About fixed-point samples I would prefer to deliver with a new bug if you don't mind. I can start it right after this patch is landed.

Comment 57

[Ralph Giles (:rillian)]

(In reply to Alexandros Chronopoulos [:achronop] from comment #56)

Thanks for updating the patch! Some comments:

>        } else if (aPacket->bytes > 19) {
>          mStreams = aPacket->packet[19];
> +        mCoupledStreams = aPacket->packet[20];

You need to check that the packet has at least 20 bytes before reading mCoupledStreams.


> +        // Validate Channel Mapping Table length before copy
> +        if (aPacket->bytes>20 && aPacket->bytes<22+mChannels) {
> +          int i;
> +          for (i=0; i<mChannels; i++)
> +            mMappingTable[i] = aPacket->packet[21+i];
> +        }

But you might as well just check that there are more than 21+mChannels bytes available. If the channel mapping family isn't zero, all three fields must be present.

> +  /* The rest is used if mChannelMapping != 0 */

We don't need this comment any more. The newer drafts (and your code!) are written in terms of these members having default values for mChannelMapping == 0, so they are always used.

> +      /*3*/{ {1.1716f,0}, {0.8284f,0.8284f}, {0,1.1716f}},

Now that I look at this, normalizing the smaller channel sets to 2 might not be the best idea. This will be louder than the the three-channel version, and we were trying to make it quieter. Maybe normalize to 1 up to 4 channels and 2 for 5 to 8? Whatever you think sounds reasonable in tests.

> +#else
> +    return NS_ERROR_FAILURE;
> +#endif

This is good. You should probably also disable the stream in ::DecodeHeaders though, so we don't waste time decoding a file we won't actually play.
> 
> I added further bounds when I read the Channel Mapping Table. I have a
> question though. I would expect that the boundaries should be between byte
> 21 and 21+mChannels-1. But this did not work and after testing it with the
> given files I end up with the bound above.
> 
> About fixed-point samples I would prefer to deliver with a new bug if you
> don't mind. I can start it right after this patch is landed.

Comment 58

[Timothy B. Terriberry (:derf)]

Comment on attachment 642375 [details] [diff] [review]
Patch for bug 748144 updated

Review of attachment 642375 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/ogg/nsOggReader.cpp
@@ +394,4 @@
>  nsresult nsOggReader::DecodeOpus(ogg_packet* aPacket) {
>    NS_ASSERTION(aPacket->granulepos != -1, "Must know opus granulepos!");
>  
> +  // Maximum value is 63*2880 ??

This statement is still correct, since this value is not scaled by the number of channels. So you should remove the "??".

Comment 59

[Alex Chronopoulos [:achronop]]

Attachment: Patch for bug 748144 updated 2

Comment 60

[Ralph Giles (:rillian)]

Comment on attachment 644870 [details] [diff] [review]
Patch for bug 748144 updated 2

Review of attachment 644870 [details] [diff] [review]:
-----------------------------------------------------------------

Please address the comments and submit an updated patch.

::: content/media/ogg/nsOggCodecState.cpp
@@ +873,5 @@
> +       * Left commented in since it will be re-activated soon.
> +       * mGain_Q16 = static_cast<PRInt32>(NS_MIN(65536*pow(10,0.05*gain_dB)+0.5,
> +       *                                      static_cast<double>(PR_INT32_MAX)));
> +      */
> +      return false;

I don't understand why your commented this out. This is still the correct value for the gain, which should still be applied.

What you want to do in DecodeHeader is to return false if and only if there are more than two channels *and* MOZ_SAMPLE_TYPE_FLOAT is not defined. It would be more clear to do that in a separate #ifdef, along with a comment about why it's supported to make it more obvious what to change when we do support integer mixdown.

@@ +886,3 @@
>        } else if (aPacket->bytes > 19) {
>          mStreams = aPacket->packet[19];
> +        mCoupledStreams = aPacket->packet[20];

You need to check that aPacket->bytes > 20 above before reading byte 20.

@@ +886,5 @@
>        } else if (aPacket->bytes > 19) {
>          mStreams = aPacket->packet[19];
> +        mCoupledStreams = aPacket->packet[20];
> +        // Validate Channel Mapping Table length before copy
> +        if (aPacket->bytes>20) {

You need to check there are at least 21+mChannels bytes available here.

::: content/media/ogg/nsOggCodecState.h
@@ +329,4 @@
>  #endif
>    int mChannelMapping; // Channel mapping family.
>    int mStreams;     // Number of packed streams in each packet.
> +  int mCoupledStreams; //Number of packed coupled streams in each packet.

Please add a space after the //

::: content/media/ogg/nsOggReader.cpp
@@ +394,4 @@
>  nsresult nsOggReader::DecodeOpus(ogg_packet* aPacket) {
>    NS_ASSERTION(aPacket->granulepos != -1, "Must know opus granulepos!");
>  
> +  // Maximum value is 63*2880

Please restore the period at the end of the comment as well.

Comment 61

[Ralph Giles (:rillian)]

:achronop You seem to be struggling with this, and I'd like to get it landed in the next two weeks so it can be part of Firefox 17.

Would it help if I finished the patch for you, so you can learn from that, or do you want to do more work on it yourself in the next week?

Comment 62

[Alex Chronopoulos [:achronop]]

Hi rillian, sorry for the delay, I had a busy week, I will update the patch with your comments by tomorrow. I understand your comments and I can handle them, unfortunately I did not have the time to do it.

Comment 63

[Alex Chronopoulos [:achronop]]

Attachment: Patch for bug 748144 updated

Thank you for your comments and the support. Please find attached an updated version according to your suggestions. 

Some questions:
> You need to check that aPacket->bytes > 20 above before reading byte 20.
We need to check that there are more than 20 bytes? Previews review mentioned at least 20 bytes and I checked if >19.

> You need to check there are at least 21+mChannels bytes available here.
Same logic, I check if >20+mChannels.

Comment 64

[Ralph Giles (:rillian)]

Comment on attachment 648391 [details] [diff] [review]
Patch for bug 748144 updated

Review of attachment 648391 [details] [diff] [review]:
-----------------------------------------------------------------

I believe this is correct now, but please address the nits and resubmit.

::: content/media/ogg/nsOggCodecState.cpp
@@ +884,5 @@
>          mStreams = 1;
> +        mCoupledStreams = mChannels - 1;
> +        mMappingTable[0] = 0;
> +        mMappingTable[1] = 1;
> +      } else if (aPacket->bytes > 20) {

This is correct now, but it would be shorter to combine this check with the one below. Maybe invert the check and return an error if the length is too short for the number of mapping table entries you want.

Essentially make it match the earlier check that there are at least 19 bytes in the packet before reading the always-present section. It's also less confusing if both checks use the same comparison operator.

The important thing is to always check that you're not reading past the end of the packet. Please double-check this and make sure you understand the checks yourself; this is an important issue any time you're parsing something.

@@ +895,5 @@
> +            mMappingTable[i] = aPacket->packet[21+i];
> +        }
> +        else {
> +          LOG(PR_LOG_DEBUG, ("Invalid Opus file: channel mapping %d,"
> +                             " channel mapping table shorter or greater than expected", mChannelMapping));

It can't be greater than expected. We can only know there's not enough data. If the packet is longer than what we know how to read, the spec says just to ignore it.

::: content/media/ogg/nsOggReader.cpp
@@ +545,5 @@
> +    return NS_ERROR_FAILURE;
> +#endif
> +  }
> +  else {
> +    // If channels more than 8 abort

Isn't this branch also taken when channels = 1?

Comment 65

[Alex Chronopoulos [:achronop]]

Attachment: Patch for bug 748144 updated

Thank you for your comments. Please find attached the updated patch.

> This is correct now, but it would be shorter to combine this check with the
> one below.
I combined the checks. Please tell me what you think.

> Isn't this branch also taken when channels = 1?
Good point! I missed this totally.

Comment 66

[Ralph Giles (:rillian)]

Comment on attachment 648714 [details] [diff] [review]
Patch for bug 748144 updated

Review of attachment 648714 [details] [diff] [review]:
-----------------------------------------------------------------

The nsOggCodecState.cpp part of the patch contains a large number of formatting changes. Please attach a patch with the just relevant changes. Feel free to file a separate bug if you want to change the formatting of the code.

Two other comments below.

::: content/media/ogg/nsOggCodecState.cpp
@@ +836,5 @@
>        mChannelMapping = aPacket->packet[18];
>  
>        if (mChannelMapping == 0) {
>          mStreams = 1;
> +        mCoupledStreams = mChannels - 1;

Note to anyone reading this bug later: one might worry about our not validating mChannels > 0 here, but opus_multistream_decoder_create() does validate its arguments, so this isn't a problem.

::: content/media/ogg/nsOggReader.cpp
@@ +545,5 @@
> +#else
> +      return NS_ERROR_FAILURE;
> +#endif
> +    }
> +    else {

This looks correct, thanks. However, a better style would be to just check if channels > 8 and return and error at the top of this block. Then you can avoid the indent of the whole 'channels < 9' block here.

Comment 67

[Ralph Giles (:rillian)]

(In reply to Ralph Giles (:rillian) from comment #66)

> Note to anyone reading this bug later: one might worry about our not
> validating mChannels > 0 here, but opus_multistream_decoder_create() does
> validate its arguments, so this isn't a problem.

I spoke too soon. The arguments are validated, in opus_multistream_decoder_get_size, which returns zero. opus_multistream_decoder_create passes that size to alloc. If alloc returns a valid pointer (to a zero-length buffer), the subsequent call to opus_multistream_decoder_create will segfault or scribble on the heap. This is fixed in opus upstream.

In the meantime, we should return false in nsOpusState::DecodeHeader if mChannels < 1 to guard this case.

> 
> ::: content/media/ogg/nsOggReader.cpp
> @@ +545,5 @@
> > +#else
> > +      return NS_ERROR_FAILURE;
> > +#endif
> > +    }
> > +    else {
> 
> This looks correct, thanks. However, a better style would be to just check
> if channels > 8 and return and error at the top of this block. Then you can
> avoid the indent of the whole 'channels < 9' block here.

Comment 68

[Ralph Giles (:rillian)]

Attachment: alsa-6ch-nb.opus

Small 5.1 test file I made. Unfortunately the speaker name samples are GPL, so we can't add this to the ff test suite.

Comment 69

[Alex Chronopoulos [:achronop]]

Attachment: Patch for bug 748144 updated

Please find attached the patch updated with the latest comments. Feel free to provide more. Formatting took place without my intention. File restored. The other two comments have been applied.

Comment 70

[Ralph Giles (:rillian)]

Comment on attachment 649646 [details] [diff] [review]
Patch for bug 748144 updated

Review of attachment 649646 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. Thanks!

Comment 71

[Ralph Giles (:rillian)]

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

Comment 72

[Ralph Giles (:rillian)]

Attachment: Patch updated to fix tbpl issues

I've done some minor cleanup on the patch. Carrying forward my previous r+. achronop, please review the changes and see if they're acceptable to you.

- Use PRInt32 and PRUint32 iterators. Fixes a signed/unsigned comparison warning and red linux builds in try.
- Reformatted for() statements to match local style
- Improved wording on some comments.
- Wrote a commit message.

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

Comment 73

[Ralph Giles (:rillian)]

Try push was green.

Comment 74

[Alex Chronopoulos [:achronop]]

Comment on attachment 650716 [details] [diff] [review]
Patch updated to fix tbpl issues

Review of attachment 650716 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks for supporting on try errors!

Comment 75

[Ralph Giles (:rillian)]

Comment on attachment 650716 [details] [diff] [review]
Patch updated to fix tbpl issues

Marking positive feedback.

Comment 76

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ralph Giles (:rillian) from comment #72)
> Pushed to try as https://tbpl.mozilla.org/?tree=Try&rev=05f26de09e23

<3

https://hg.mozilla.org/integration/mozilla-inbound/rev/23e85147b822

Should this have tests?

Comment 77

[Ralph Giles (:rillian)]

Thanks Ryan. It should have tests. The files attached here are sufficient to verify the new feature, but I don't think they're license compatible. I haven't had time to record a new one.

Let's go without additional multichannel tests for now. The existing open test file does exercise the common path of this code, and I've opened bug 782091 to add a proper test for the downmixing code.

Comment 78

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/23e85147b822

Comment 79

[Alex Chronopoulos [:achronop]]

> About fixed-point samples I would prefer to deliver with a new bug if you
> don't mind.

Can I work on downmixing for fixed-point opus? Should I open a bug for it?

Comment 80

[Ralph Giles (:rillian)]

(In reply to Alexandros Chronopoulos [:achronop] from comment #79)

> Can I work on downmixing for fixed-point opus?

Please do. I don't think anyone has prepared a patch so far.

> Should I open a bug for it?

Bug 790458 is already open for this.