Closed Bug 790459 Opened 12 years ago Closed 11 years ago

Use surround downmix code in the Vorbis decoder

Categories

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

Product:
Core
Component:
Audio/Video
Platform:
x86_64
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla25

People

(Reporter: rillian, Assigned: achronop)

References

Details

(Whiteboard: [mentor=rillian] [lang=c++])

Attachments

(3 files, 4 obsolete files)

test files
2.48 MB, application/x-xz
Details
Fix for DownmixToSereo ready for check in (part 1)
9.92 KB, patch
rillian
: review+
Details | Diff | Splinter Review
Patch to correction broken mochitest test_framebuffer.html (part 2)
1.12 KB, patch
rillian
: review+
Details | Diff | Splinter Review 
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.
Whiteboard: [mentor=rillian] [lang=c++]
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?
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).
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.
hello rillian,

     Can you assign this bug to me.
Assignee: nobody → mohamedasheik
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.
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla14
How's this going? Have any code I can look at?
Target Milestone: mozilla14 → mozilla19
Are you still interested in working on this? the deadline for Firefox 19 is coming up very soon.
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.
Brian, please go ahead and take the bug.

vKoder, do you have any code you can share?
Ok I'll jump into this one, I have my copy of the source tree, digging in to find the location of the bug
I found the area in question, have a deadline at work so I'll be working on this over the weekend.
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.
Assignee: mohamedasheik → brianwredfern
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.
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.
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
  }
Attached file test files — 
Here are some test files you can use to verify correct downmixing.
(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.
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.
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;
}
(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.
(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.
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.
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.
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.
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.
I got this comment:
--- Comment #22 from Alexandros Chronopoulos [:achronop] <achronop@gmail.com> 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.
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.
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.
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.
Sorry I've been having to spend time reading an intro book on C++ to wrap my head around methods and pointers.
I'll be posting another try at my method within a couple of days
Thanks for the update!
Are you still working on this Brian, or should we return it to the pool of available bugs?
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.
In this case can I jump in to that? I am pretty familiar with this part of code ...
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.
Assignee: brianwredfern → achronop
Target Milestone: mozilla19 → ---
Attached patch Initial effort (obsolete) — — Splinter Review 
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!
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!
Attachment #741952 - Attachment is obsolete: true
Attachment #759478 - Flags: review?(giles)
Hooray! Thanks for persevering. Sorry I was no help tracking down the issue.
Blocks: 706327
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;
}
Attachment #759478 - Flags: review?(giles) → review+
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!
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.
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!
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.
The patch updated with the latest comments. Please note that it has not be tested on mobile platform yet.
Attachment #759478 - Attachment is obsolete: true
Attachment #765624 - Flags: review?(giles)
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.
Attachment #765634 - Flags: review?(giles)
Tested on Android armv6 and it works fine for multichannel vorbis and opus.
Win7 failed mochitests run green in a new try of the same patch
https://tbpl.mozilla.org/?tree=Try&rev=bd24865b385f
Yeah, I don't think those are your fault.
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.
Attachment #765634 - Flags: review?(giles) → review+
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.
Attachment #765624 - Flags: review?(giles) → review+
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.
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 ...
Attachment #765624 - Attachment is obsolete: true
Attachment #774198 - Flags: review?(giles)
This is the patch to correct broken mochitest with rillian's suggestion. Purposed commit message is included in the patch.
Thanks!
Attachment #765634 - Attachment is obsolete: true
Attachment #774210 - Flags: review?(giles)
Attachment #774198 - Flags: review?(giles) → review+
Attachment #774210 - Flags: review?(giles) → review+
Looks good, thanks!
Oops, bug number in the commit message is wrong. That should have been 790459 not 790559. Sorry for missing that in the review.
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.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Oops, this is my fault! Sorry for that! If there is anything I can do to correct it please let me know ...
It's too late to correct now. We both just need to be more careful about that in the future.
Depends on: 894801
Blocks: 911482
Blocks: 952326
No longer blocks: 952326
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: