790459 - Use surround downmix code in the Vorbis decoder

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P3)

Description

[Ralph Giles (:rillian)]

Bugs 748144 added code to downmix surround audio to stereo to nsOggReader::DecodeOpus.

Vorbis files use the same set of channel mappings. This bug is about moving the downmix code to a separate function and using it from nsOggReader::DecodeVorbis as well.

Comment 1

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

Can you outline the work that needs to be done here?

Is it as simple as refactoring out the code in nsOggReader.cpp that begins with the comment "More than 2 decoded channels must be downmixed to stereo" to a new function that can be shared by DecodeVorbis?

Comment 2

[vKoder]

Hi Ralph Giles,

           This is Asheik, newly joined the firefox contribution team.I would like to work on this bug,I have good background in vecoders (worked on LAME,mp3,wav,ogg and WAV).

Comment 3

[Ralph Giles (:rillian)]

Hi vKoder. Thanks for taking a look at the bug!

I think Chris Pearce's suggestion is fine. Since Opus and Vorbis use the same channel definitions, we can resolve this just by moving the downmix code out of nsOggReader::DecodeOpus into a separate function called by both DecodeOpus and DecodeVorbis.

You'll also need to add code to nsVorbisState::DecodeHeader to reject streams with unhandled numbers of channels, and construct some test files to verify everything works.

Comment 4

[vKoder]

hello rillian,

     Can you assign this bug to me.

Comment 5

[Ralph Giles (:rillian)]

Thanks Anand.

vKoder, don't be shy about assigning a bug to yourself if you've started working on it. That's fine to do if it's assigned to 'nobody'. If it's already assigned to another person, you should ask them first, of course.

Comment 6

[Ralph Giles (:rillian)]

How's this going? Have any code I can look at?

Comment 7

[Ralph Giles (:rillian)]

Are you still interested in working on this? the deadline for Firefox 19 is coming up very soon.

Comment 8

[Brian W. Redfern]

Even though this one is currently assigned I can take a stab at this one unless the original person handling it has code for it now.

Comment 9

[Ralph Giles (:rillian)]

Brian, please go ahead and take the bug.

vKoder, do you have any code you can share?

Comment 10

[Brian W. Redfern]

Ok I'll jump into this one, I have my copy of the source tree, digging in to find the location of the bug

Comment 11

[Brian W. Redfern]

I found the area in question, have a deadline at work so I'll be working on this over the weekend.

Comment 12

[Brian W. Redfern]

Not sure how to take the bug over within bugzilla but I'm working on the refactoring, will need some pointers on setting up my tests as this is my first step into mozilla development, I've gone back over the comment and am working through the refactoring today.

Comment 13

[Ralph Giles (:rillian)]

How far did you get this weekend? Note that the filenames and classes have mostly lost their 'ns' prefixes since comments #1 and #3, but the basic approach is fine.

Feel free to post your work-in-progress whenever you get to a stopping point so we can see how it's going.

Comment 14

[Brian W. Redfern]

I'll need some help setting up a test for my changes a little later today, or just pointers to which tests to look at in order to write my own for this.

Comment 15

[Brian W. Redfern]

Basically I'm working this into a re-usable funtion: 

// More than 2 decoded channels must be downmixed to stereo.
  if (channels > 2) {
    // Opus doesn't provide a channel mapping for more than 8 channels,
    // so we can't downmix more than that.
    if (channels > 8)
      return NS_ERROR_FAILURE;

#ifdef MOZ_SAMPLE_TYPE_FLOAT32
    uint32_t out_channels;
    out_channels = 2;

    // dBuffer stores the downmixed sample data.
    nsAutoArrayPtr<AudioDataValue> dBuffer(new AudioDataValue[frames * out_channels]);
    // Downmix matrix for channels up to 8, normalized to 2.0.
    static const float dmatrix[6][8][2]= {
        /*3*/{ {0.5858f,0}, {0.4142f,0.4142f}, {0,0.5858f}},
        /*4*/{ {0.4226f,0}, {0,0.4226f}, {0.366f,0.2114f}, {0.2114f,0.366f}},
        /*5*/{ {0.651f,0}, {0.46f,0.46f}, {0,0.651f}, {0.5636f,0.3254f}, {0.3254f,0.5636f}},
        /*6*/{ {0.529f,0}, {0.3741f,0.3741f}, {0,0.529f}, {0.4582f,0.2645f}, {0.2645f,0.4582f}, {0.3741f,0.3741f}},
        /*7*/{ {0.4553f,0}, {0.322f,0.322f}, {0,0.4553f}, {0.3943f,0.2277f}, {0.2277f,0.3943f}, {0.2788f,0.2788f}, {0.322f,0.322f}},
        /*8*/{ {0.3886f,0}, {0.2748f,0.2748f}, {0,0.3886f}, {0.3366f,0.1943f}, {0.1943f,0.3366f}, {0.3366f,0.1943f}, {0.1943f,0.3366f}, {0.2748f,0.2748f}},
    };
    for (int32_t i = 0; i < frames; i++) {
      float sampL = 0.0;
      float sampR = 0.0;
      for (uint32_t j = 0; j < channels; j++) {
        sampL+=buffer[i*channels+j]*dmatrix[channels-3][j][0];
        sampR+=buffer[i*channels+j]*dmatrix[channels-3][j][1];
      }
      dBuffer[i*out_channels]=sampL;
      dBuffer[i*out_channels+1]=sampR;
    }
    channels = out_channels;
    buffer = dBuffer;
#else
  return NS_ERROR_FAILURE;
#endif
  }

Comment 16

[Ralph Giles (:rillian)]

Attachment: test files

Here are some test files you can use to verify correct downmixing.

Comment 17

[Ralph Giles (:rillian)]

(In reply to Brian W. Redfern from comment #15)
> Basically I'm working this into a re-usable funtion: 

That's the right idea. Note you may want to build your patch on top of achronop's from bug 790458, which adds a fixed-point version of the same code.

Comment 18

[Brian W. Redfern]

I'm taking a look at bug 790458, that's funny I thought I needed to write some kind of unit tests, thanks for  your help with the files, right now my system at home is only setup for stereo.

Comment 19

[Brian W. Redfern]

Might not be completely right but this is what I have so far

uint32_t OggReader::DownmixChannels(uint32_t channels) {
  // More than 2 decoded channels must be downmixed to stereo.
  if (channels > 2) {
    // Opus doesn't provide a channel mapping for more than 8 channels,
    // so we can't downmix more than that.
    if (channels > 8)
      return NS_ERROR_FAILURE;
    uint32_t out_channels;
    out_channels = 2;

    // dBuffer stores the downmixed sample data.
    nsAutoArrayPtr<AudioDataValue> dBuffer(new AudioDataValue[frames * out_channels]);
    // Downmix matrix for channels up to 8, normalized to 2.0.
    static const int16_t dmatrix[6][8][2]= {
        /*3*/{ {0.5858f,0}, {0.4142f,0.4142f}, {0,0.5858f}},
        /*4*/{ {0.4226f,0}, {0,0.4226f}, {0.366f,0.2114f}, {0.2114f,0.366f}},
        /*5*/{ {0.651f,0}, {0.46f,0.46f}, {0,0.651f}, {0.5636f,0.3254f}, {0.3254f,0.5636f}},
        /*6*/{ {0.529f,0}, {0.3741f,0.3741f}, {0,0.529f}, {0.4582f,0.2645f}, {0.2645f,0.4582f}, {0.3741f,0.3741f}},
        /*7*/{ {0.4553f,0}, {0.322f,0.322f}, {0,0.4553f}, {0.3943f,0.2277f}, {0.2277f,0.3943f}, {0.2788f,0.2788f}, {0.322f,0.322f}},
        /*8*/{ {0.3886f,0}, {0.2748f,0.2748f}, {0,0.3886f}, {0.3366f,0.1943f}, {0.1943f,0.3366f}, {0.3366f,0.1943f}, {0.1943f,0.3366f}, {0.2748f,0.2748f}},
    };
     for (int32_t i = 0; i < frames; i++) {
      int32_t sampL = 0;
      int32_t sampR = 0;
      for (uint32_t j = 0; j < channels; j++) {
        sampL+=buffer[i*channels+j]*dmatrix[channels-3][j][0];
        sampR+=buffer[i*channels+j]*dmatrix[channels-3][j][1];
      }
      sampL = (sampL + 32768)>>16;
      dBuffer[i*out_channels]   = static_cast<AudioDataValue>(MOZ_CLIP_TO_15(sampL));
      sampR = (sampR + 32768)>>16;
      dBuffer[i*out_channels+1] = static_cast<AudioDataValue>(MOZ_CLIP_TO_15(sampR));
    }
    return channels;
}

Comment 20

[Ralph Giles (:rillian)]

(In reply to Brian W. Redfern from comment #18)
> I'm taking a look at bug 790458, that's funny I thought I needed to write
> some kind of unit tests,

We'll need to add some multichannel tests (possibly these, although they're quite large) to the mochitest suite, but it's ok to focus on the functional patch first.

> thanks for  your help with the files, right now my
> system at home is only setup for stereo.

That's ok, Firefox only supports stereo output anyway. That's the point of this bug: to downmix surround files to stereo so we can play them back instead of silently failing.

Comment 21

[Ralph Giles (:rillian)]

(In reply to Brian W. Redfern from comment #19)
> Might not be completely right but this is what I have so far

Great! thanks for sharing. Some quick comments:

>   if (channels > 2) {

You probably want to turn this into 'if (channels <= 2)' so you can return early and not have to nest the rest of the function.

>     if (channels > 8)
>       return NS_ERROR_FAILURE;

I guess this is negative, so could be shared with the channel count, but you should probably return just an nsresult if the method can fail.

>       dBuffer[i*out_channels+1] =
> static_cast<AudioDataValue>(MOZ_CLIP_TO_15(sampR));
>     }
>     return channels;

You can't just return the number of channels here. You need a way to update both the sample buffer and the channel count in the caller's context. So for example pass inbuffer, outbuffer, and a pointer to the channel count as arguments, or make this a method on AudioData so you can call .Downmix(2) before pushing it onto mAudioQueue.

Comment 22

[Brian W. Redfern]

Yeah I'm taking a look at the notes in the header file as well, I'm going to try to make it a method of AudioData.

Comment 23

[Brian W. Redfern]

Still working on this, having to go back and refresh on my c++, trying to move from c# to c++ so bear with me, but I'm determined to figure out how this works.

Comment 24

[Ralph Giles (:rillian)]

Good luck then. :) Just let me know if you have any questions. You can also try the #introduction channel on irc.mozilla.org if you need help with basics.

Comment 25

[Brian W. Redfern]

I'm putting in some more work on it tonight, I think I'm getting the idea of how this works now so I'll take another shot and see if I can't get things to compile and test tonight.

Comment 26

[Brian W. Redfern]

I got this comment:
--- Comment #22 from Alexandros Chronopoulos [:achronop] <[e-mail]> 2012-12-14 09:59:48 PST ---
After several combinations I end up with normalization to 2 for 3,4 and 4 for
5-8. This doe not cause any clipping on louder sections testing with the file
above. That patch lies in attachment 682150 [details] [diff] [review] uploaded with Comment 15. As I
mentioned there, the difference between the stereo and multichannel samples
remains obvious.

So I'll work on integrating this code to hopefully slay this bug this weekend.

Comment 27

[Ralph Giles (:rillian)]

Hi Brian. Are you still working on this? Note that now that bug 790458 has landed you'll need to copy the new downmix code supporting both float and integer audio sample types.

Comment 28

[Brian W. Redfern]

I should be getting my machine back today had to send my laptop for repairs and was without a machine for almost three weeks but I'll be back on the bug today.

Comment 29

[Brian W. Redfern]

Ok my machine is back sorry lost a couple of weeks saving for a replacement hard disk, but I'm back on the bug now.

Comment 30

[Brian W. Redfern]

Sorry I've been having to spend time reading an intro book on C++ to wrap my head around methods and pointers.

Comment 31

[Brian W. Redfern]

I'll be posting another try at my method within a couple of days

Comment 32

[Ralph Giles (:rillian)]

Thanks for the update!

Comment 33

[Ralph Giles (:rillian)]

Are you still working on this Brian, or should we return it to the pool of available bugs?

Comment 34

[Brian W. Redfern]

Yeah my c++ skills are too rudimentary right now to be effective in this area, I'm better off letting go of this bug and then taking a look at javascript related tasks I can help out on.

Comment 35

[Alex Chronopoulos [:achronop]]

In this case can I jump in to that? I am pretty familiar with this part of code ...

Comment 36

[Ralph Giles (:rillian)]

Ok, thanks Brian. JS isn't my area, but best of luck finding something more rewarding to work on.

Achronop, thanks for offering to take over. This should be a straightforward refactoring of the code you've already worked on.

Comment 37

[Alex Chronopoulos [:achronop]]

Attachment: Initial effort

This is an initial effort. It compiles and runs correctly for OPUS. For Vorbis more investigation is needed since it produce terrible distortion. I do not set it for review since it is not finished yet. Any comment is welcome! Thanks!

Comment 38

[Alex Chronopoulos [:achronop]]

Attachment: DownmixToStereo for Opus and Vorbis

Finally I got the issue and now it works for both Opus and Vorbis. The error was a slight modification in the ReadMetadata for Vorbis to read the correct number of channels. The following days I will build for Mobile to test it there as well. In the meantime I set it for review since it looks quite close to what we want.

Thanks!

Comment 39

[Ralph Giles (:rillian)]

Hooray! Thanks for persevering. Sorry I was no help tracking down the issue.

Comment 40

[Ralph Giles (:rillian)]

Comment on attachment 759478 [details] [diff] [review]
DownmixToStereo for Opus and Vorbis

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

Sorry for taking so long to get back to this. The patch itself looks fine. Please see one style nit below.

You also need to update content/media/test/test_framebuffer.html to expect the downmixed output as well (mozChannels 2 instead of 6, shorter decode buffers). Please do that and verify that the other media mochitests run correctly. You can either update this patch, or probably better, attach a second one fixing the test. Either way we need to address that before this can land.

R+ with these issues addressed.

There are also some problems with the test files in the first attachment. the LFE channel on the 5.1 test is garbled, and I think the channel order is incorrect on some of them; for example totem and firefox+you patch downmix the surround channels on 6.1 test to the wrong locations on my system.

::: content/media/ogg/OggReader.cpp
@@ +407,5 @@
> +    // More than 2 decoded channels must be downmixed to stereo.
> +    if (channels > 2) {
> +      // No channel mapping for more than 8 channels.
> +      if (channels > 8)
> +        return NS_ERROR_FAILURE;

To match local style, please use { } around the body of all if statements, even if they are single line. I suggest:

if (channels > 8) {
  // No channel mapping for more than 8 channels.
  return NS_ERROR_FAILURE;
}

Comment 41

[Alex Chronopoulos [:achronop]]

I am thinking to move the function in AudioData class. I believe it is more appropriate place and it will be available to every audio stream. Please let me know what you think. Thanks!

Comment 42

[Ralph Giles (:rillian)]

That also sounds reasonable. At the rate this patch is going you might want to do that in a separate bug though. :)

Note that longer term we want to move this to the output layer (libcubeb) so it can provide multichannel support to all of firefox, and either downmix to stereo internally, or pass through to the hardware when it supports surround on that particular platform.

Comment 43

[Alex Chronopoulos [:achronop]]

In this case patch is ready to be delivered. The only thing missing is the mobile testing. I see that bug874132 is almost ready so I will test soon.

I made the update to content/media/test/test_framebuffer.html and now it runs green. I will upload it right after in a separate patch. I ran the whole bunch of tests under content/media/test/ and they are green too. Let me know if I should run more.

Can I request level 1 access to push it to try by myself?
Thanks!

Comment 44

[Ralph Giles (:rillian)]

I'll vouch for Level 1 commit access for you. Please follow the instructions at https://www.mozilla.org/hacking/committer/ and cc me on the bug.

In the meantime I'm happy to push things to try. Just update the bug and ask.

Comment 45

[Alex Chronopoulos [:achronop]]

Attachment: Fix for Bug 790459: DownmixToStereo function for Opus and Vorbis

The patch updated with the latest comments. Please note that it has not be tested on mobile platform yet.

Comment 46

[Alex Chronopoulos [:achronop]]

Attachment: Correct the mochitest content/media/test/test_framebuffer.html

This patch needs to be delivered with the fix for that bug. It provides a correction to the mochitest content/media/test/test_framebuffer.html. The test breaks because multichannel audio media are downmixed to stereo thus expected channels cannot be more than 2.

Comment 47

[Alex Chronopoulos [:achronop]]

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

Comment 48

[Alex Chronopoulos [:achronop]]

Tested on Android armv6 and it works fine for multichannel vorbis and opus.

Comment 49

[Alex Chronopoulos [:achronop]]

Win7 failed mochitests run green in a new try of the same patch
https://tbpl.mozilla.org/?tree=Try&rev=bd24865b385f

Comment 50

[Ralph Giles (:rillian)]

Yeah, I don't think those are your fault.

Comment 51

[Ralph Giles (:rillian)]

Comment on attachment 765634 [details] [diff] [review]
Correct the mochitest content/media/test/test_framebuffer.html

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

::: content/media/test/test_framebuffer.html
@@ +23,2 @@
>  var testFileSampleRate = 48000;
> +var testFileFrameBufferLength = 2 * 1024;

Should probably make this testFileChannelCount * 1024.

Comment 52

[Ralph Giles (:rillian)]

Comment on attachment 765624 [details] [diff] [review]
Fix for Bug 790459: DownmixToStereo function for Opus and Vorbis

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

Thanks. Sorry for taking to long to get back to this.

Comment 53

[Ralph Giles (:rillian)]

Please write commit messages for each of your patches. If you want to land this as two separate patches it's also conventional to put 'Bug 790459 - Part 1: ...' and 'Bug 790459 - Part 2: ...' in the first line so it's clear that they go together and which one is which in the bug.

See https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Commit_message_restrictions or other recent commits for guidelines.

Comment 54

[Alex Chronopoulos [:achronop]]

Attachment: Fix for DownmixToSereo ready for check in (part 1)

This is the same patch, r+ previously, ready for check in. Purposed commit message is included. I am not sure if I should ask for review for the new upload. I did just in case ...

Comment 55

[Alex Chronopoulos [:achronop]]

Attachment: Patch to correction broken mochitest test_framebuffer.html (part 2)

This is the patch to correct broken mochitest with rillian's suggestion. Purposed commit message is included in the patch.
Thanks!

Comment 56

[Ralph Giles (:rillian)]

Looks good, thanks!

Comment 57

[Ralph Giles (:rillian)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/440da67cf398
https://hg.mozilla.org/integration/mozilla-inbound/rev/e080b87c18a5

Comment 58

[Ralph Giles (:rillian)]

Oops, bug number in the commit message is wrong. That should have been 790459 not 790559. Sorry for missing that in the review.

Comment 59

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/440da67cf398
https://hg.mozilla.org/mozilla-central/rev/e080b87c18a5

This was landed with bug 790559 in the commit message. Please verify bug numbers before pushing.

Comment 60

[Alex Chronopoulos [:achronop]]

Oops, this is my fault! Sorry for that! If there is anything I can do to correct it please let me know ...

Comment 61

[Ralph Giles (:rillian)]

It's too late to correct now. We both just need to be more careful about that in the future.