Closed Bug 1301518 Opened 3 years ago Closed 3 years ago

Opus 255 channel mapping family not playable

Categories

(Core :: Web Audio, defect, P1)

51 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla51
Tracking Status
platform-rel --- +
firefox49 + verified
relnote-firefox --- -
firefox50 + verified
firefox51 + verified

People

(Reporter: maushundb, Assigned: padenot)

References

Details

(Whiteboard: [platform-rel-Facebook])

Attachments

(3 files)

Attached file ffprobe output
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/52.0.2743.116 Safari/537.36

Steps to reproduce:

I have two encodings for a webm video with Opus 8 channel audio: one with channel mapping family 1, and one with channel mapping family 255 (channels-7.1.webm and channels-8.webm in the attached folder respectively). Channel mapping 1 plays fine and I'm able to access the audio channels using the web audio API, while channel mapping 255 does not play. 

Repro streps:
1. Download video files from https://www.dropbox.com/sh/ffsrwfa68uvxz0u/AABR-oSV89-WakFFsdzLbXHea?dl=0
2. Drag each respective video into a new Firefox tab and see the playing vs failure

For some context: I am using the 255 channel mapping for some audio processing work using the Web Audio API and ScriptProcessorNode. In a DASH context, I'm finding that the video for the 255 channel map encoding plays, while the input buffer received by onaudioprocess in the scriptProcessorNode either: 1. Has all zero channels (FF 48.0.2) 2. Only has access to two channels (Nightly) and the rest are zero, when there should be 8. Feel free to plug these videos video into https://jsfiddle.net/se0qvha4/ and increment NUM_CHANNELS to see this behavior in the console. I have also attached a copy of the ffprobe data from each video for more reference.

Opus reference: 
https://www.opus-codec.org/docs/opusfile_api-0.4/structOpusHead.html#a338268b4264e059ae9ede890e6177304
https://wiki.xiph.org/OggOpus


Actual results:

255 channel mapping family did not play


Expected results:

255 channel mapping family should play.
Edit: https://jsfiddle.net/se0qvha4/1/ for the reference fiddle.
Motivation: 7.1 encoding treats one channel as the LFE and applies a lowpass filter, but the audio here is not 7.1, but 8 channels of full-spectrum audio.
Component: Untriaged → Audio/Video: Playback
Product: Firefox → Core
Karl, Paul, can you please have a look?  Thanks!
Component: Audio/Video: Playback → Web Audio
Flags: needinfo?(padenot)
Flags: needinfo?(karlt)
Whiteboard: [needinfo 2016/09/08 to padenot/karlt]
I haven't looked, but I suspect we reject this because there's no way to downmix mapping 255 audio. It might make sense in an audio source node though, depending on whether channel interpretations ever got implemented.
Thanks, Ehsan and Ralph.  Making this a P1, Rank:15 for now until we know more.  I just pinged Karl in irc to take a look.
Status: UNCONFIRMED → NEW
Rank: 15
Ever confirmed: true
Priority: -- → P1
Flags: needinfo?(karlt)
This modification of the testcase needs to be downloaded.  Adjust video.src.
Adjusting PLAYBACK_CHANNEL seems to produce different delays with each of the
8 channels and channels-7.1.webm, and so I guess it is picking up different
channels.

When video.src is modified to point at channels-8.webm, the Web Console
displays "Media resource file:///home/karl/tmp/1301518/channels-8.webm could
not be decoded." and channels sound silent.
I don't know why numNonZeroChannels in 8.

I think that indicates that making mapping 255 play something would first require code changes to decoding and/or HTMLMediaElement.

Reporter, can you comment on the importance of mapping 255, please?
"The number of channels of the single output equals the number of channels of
the audio referenced by the HTMLMediaElement passed in as the argument to
createMediaElementSource(), or is 1 if the HTMLMediaElement has no audio."

channels-7.1.webm seems to demonstrate that MediaElementAudioSourceNode is getting multiple channels from HTMLMediaElement.

However, I'm not clear what the appropriate channel order should be.

AudioNode.channelInterpretation applies to inputs but
MediaElementAudioSourceNode has no inputs.

Downstream nodes may set channelInterpretation to "speakers" or "discrete" to
determine how to interpret input from a MediaElementAudioSourceNode.
The default "speakers" has only "Mono (one channel), stereo (two channels), quad (four channels), and 5.1 (six channels) MUST be supported."
"discrete" would assume a channel numbering.

Perhaps the meaning of MediaElementAudioSourceNode.channelInterpretation could
be specified to describe how to interpret media element channels, but that may
be somewhat difficult to implement, being downstream of the source.
Assignee: nobody → padenot
Flags: needinfo?(padenot)
Comment on attachment 8789803 [details]
Bug 1301518 - Support mapping family 255 when playing an Opus file, for use with the Web Audio API.

https://reviewboard.mozilla.org/r/77894/#review76292

::: dom/media/ogg/OpusParser.cpp:90
(Diff revision 1)
>        mStreams = 1;
>        mCoupledStreams = mChannels - 1;
>        mMappingTable[0] = 0;
>        mMappingTable[1] = 1;
> -    } else if (mChannelMapping == 1) {
> -      // Currently only up to 8 channels are defined for mapping family 1
> +    } else if (mChannelMapping == 1 || mChannelMapping == 255) {
> +      // Currently only up to 8 channels are defined for mapping family 1 and

this isn't strictly true for mapping 255.
maybe just a note that we currently only support up to 8 channels for 255

::: dom/media/ogg/OpusParser.cpp:101
(Diff revision 1)
>        }
>        if (aLength>static_cast<unsigned>(20+mChannels)) {
>          mStreams = aData[19];
>          mCoupledStreams = aData[20];
>          int i;
> +        MOZ_ASSERT(mChannels < 255);

the test line 92 errors if we have more than 8 channels.

this assert serves no purpose
Attachment #8789803 - Flags: review?(jyavenard) → review+
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/mozilla-inbound/rev/59d15db9bc38
Support mapping family 255 when playing an Opus file, for use with the Web Audio API. r=jya
> Reporter, can you comment on the importance of mapping 255, please?

As Hans mentioned, 7.1 encoding treats one channel as the LFE and applies a lowpass filter, but the audio here is not 7.1, but 8 channels of full-spectrum audio. Thus mapping 255 has been chosen to bet fit our needs for full spectrum multichannel audio. I can ping Hans and have him elaborate on the details.
Comment on attachment 8789803 [details]
Bug 1301518 - Support mapping family 255 when playing an Opus file, for use with the Web Audio API.

Approval Request Comment
[Feature/regressing bug #]: This is something Firefox never supported, apparently

[User impact if declined]: Some opus files can't be played on Firefox, they can be played on Chrome

[Describe test coverage new/current, TreeHerder]: No test added, this is going to be dealt with in bug 1301703

[Risks and why]: Low risk, trivial patch, prevent rejecting valid Opus files

[String/UUID change made/needed]: none
Attachment #8789803 - Flags: approval-mozilla-aurora?
Thanks for the prompt attention Paul, and everyone. I can give a little more context.

Ralph hit the nail on the head and I'd like to draw attention to that distinction: there's a difference between playing an audio file, and decoding audio. Channel mappings without a well-defined way to downmix to stereo don't make a lot of sense for playback, but our application does additional post-decoding processing before playing back the resulting stereo audio.

The problem with 7.1 is simply that lowpass filter on the LFE channel. The Opus spec doesn't leave a way to get 8 channels for mapping family 1 without the LFE optimizations, because mapping family 1 has an implicit mapping from channel count to surround sound layouts. Hence mapping family 255.

Karl brought up the concern about what the channel mapping would be. The spec requires that the channel mapping be explicit when mapping family is 255, and your code already handles reading that explicit map (https://hg.mozilla.org/integration/mozilla-inbound/file/59d15db9bc38/dom/media/ogg/OpusParser.cpp#l102). We are using [0,1,2,3,4,5,6,7], i.e. the channels are not remapped they're just in the order we want them.

We also have a need to handle 10-channel audio, and potentially in the future we and others might need even more channels. I notice this diff still caps it at 8. Would you consider increasing the size allocated to mMappingTable, to allow for up to the maximum 255 channels allowed by the spec?

Thanks!
Hans, I have filed bug 1301703 [0], right after pushing this patch, so that we support more channels with mapping family 255 (which makes sense indeed). I've CC-ed you on it so you're notified of our progress.

There is a big more work that will need to happen, because for now, the rest of the pipeline assumes a number of channel less or equal to 8. This is something we want to do, and is probably not very complex, but requires some thinking.

That said, and considering a number of channel <= 8, let us know if everything works for you: not having your custom rendering code, we can just check that something is playing, not much more.

In any case, this patch will be included in tomorrows Firefox Nightly [1], and we'll try to uplift it so that it reach release earlier.

[0]: https://bugzilla.mozilla.org/show_bug.cgi?id=1301703
[1]: https://nightly.mozilla.org/
platform-rel: --- → ?
Whiteboard: [needinfo 2016/09/08 to padenot/karlt] → [needinfo 2016/09/08 to padenot/karlt][platform-rel-Facebook]
Thanks Paul, that's great. Brandon will test out the <= 8 channel code.
Whiteboard: [needinfo 2016/09/08 to padenot/karlt][platform-rel-Facebook] → [platform-rel-Facebook]
After talking with Maire, sounds like we will want to keep an eye on this and discuss further for the 50 timeframe and possibly mid-49. Tracking.
(In reply to hans from comment #13)
> Thanks for the prompt attention Paul, and everyone. I can give a little more
> context.
> 
> Ralph hit the nail on the head and I'd like to draw attention to that
> distinction: there's a difference between playing an audio file, and
> decoding audio. Channel mappings without a well-defined way to downmix to
> stereo don't make a lot of sense for playback, but our application does
> additional post-decoding processing before playing back the resulting stereo
> audio.

At this stage, we don't have a code path to distinguish playback from decoding only.
We have to have this channel number restrictions as with a carefully crafted file, we've had security vulnerability

Once we can take a different path when all we care is decoding audio, the restriction will be removed.
https://hg.mozilla.org/mozilla-central/rev/59d15db9bc38
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Hello there, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(maushundb)
Comment on attachment 8789803 [details]
Bug 1301518 - Support mapping family 255 when playing an Opus file, for use with the Web Audio API.

Maire mentioned that this was verified by Facebook engg, Aurora50+
Attachment #8789803 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8789803 [details]
Bug 1301518 - Support mapping family 255 when playing an Opus file, for use with the Web Audio API.

Please see Comment 12 for uplift details from padenot. tl;dr - trivial, low risk patch to prevent Firefox from rejecting valid Opus files, which play in Chrome.  This patch is needed for a new feature to work on one major site that will likely launch its feature before Firefox 50 ships.
Attachment #8789803 - Flags: approval-mozilla-release?
Yep verified using the provided test examples.
Hmm so given the test files, this fix is working fine. But on DASH it seems to still be broken. Is there any reason ya'll can think of that this wouldn't work with a DASH implementation? Don't wanna open up a new bug if this is an issue with our end.
Jean-Yves, any idea about this one ?
Flags: needinfo?(jyavenard)
It shouldn't make a difference. 

However, dash is for playback. And according to the plans; we want to disable playback of such file as the channel layout has no meaning in this context. 

We will only allow those files in the context of webaudio
Flags: needinfo?(jyavenard)
We are planning a dot release at this point, not today but next week. So let's take this as a ridealong for 49.0.2.
Comment on attachment 8789803 [details]
Bug 1301518 - Support mapping family 255 when playing an Opus file, for use with the Web Audio API.

Let's take this for 49.0.2 now that we are planning it. Sounds like preliminary testing went well, the risk is low (limited to the audio for this particular kind of file).
Attachment #8789803 - Flags: approval-mozilla-release? → approval-mozilla-release+
I've managed to reproduce this bug on an affected build (49.0.1-build3,
20160922113459) using Windows 10 x64 with the video samples from Comment 0.

This is verified fixed on:

  - 49.0.2-build1 (20161018030522)
  - 50.0b8-build1 (20161017130949)
  - 51.0a2 (2016-10-18)
  - 52.0a1 (2016-10-18)

using Windows 10 x64, Mac OS X 10.11.6 and Ubuntu 14.04 x86.
In the release notes of 51 with "Added support for Spatial Audio for 360 Videos on Facebook  with Opus 255 Channel Mapping" as wording.
(In reply to Gerry Chang [:gchang] from comment #32)
> In the release notes of 51 with "Added support for Spatial Audio for 360
> Videos on Facebook  with Opus 255 Channel Mapping" as wording.

Hi Gerry, although we did resolve the blocker issues for this Facebook feature, I think this bug is something we should not call out in the release notes since I believe there are some additional issues that Facebook is working through at their end with DASH (video-related).  I did make this same comment in a shared google doc on the 51 release notes.  Thanks
Flags: needinfo?(gchang)
Maire,
Yes, I've removed this one in release note. Thanks for reminder.
Flags: needinfo?(gchang)
Yes it's not necessary to mention Facebook. However my apologies that we seem to have failed to communicate earlier that everything is fixed on both ends and we now serve spatial audio to Firefox.
(v51 and up)
Glad to hear that Hans. Let us know if we can be of any help in the future !
Flags: needinfo?(maushundb)
See Also: → 1378077
Duplicate of this bug: 1301703
You need to log in before you can comment on or make changes to this bug.