Closed Bug 865244 Opened 7 years ago Closed 7 years ago

Implement AudioDestinationNode.maxChannelCount

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: ehsan, Assigned: padenot)

References

Details

Attachments

(6 files, 6 obsolete files)

18.10 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
1.94 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
1.76 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
1.38 KB, patch
Details | Diff | Splinter Review
6.54 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
2.50 KB, patch
padenot
: review+
Details | Diff | Splinter Review
Do we have a cross platform way of getting the number of channels that the audio hardware that cubeb talks to supports?
Flags: needinfo?(paul)
We don't. I can make you one, I guess.
Assignee: nobody → paul
Status: NEW → ASSIGNED
Flags: needinfo?(paul)
(In reply to comment #1)
> We don't. I can make you one, I guess.

That would be nice!  :-)
This adds a new function to all the backends, to get the maximum number of channels that the current hardware/software configuration supports.
Attachment #755295 - Flags: review?(kinetik)
Comment on attachment 755295 [details] [diff] [review]
add a function to cubeb to get the max number of channels

Alex, could you have a look at the sndio part? I haven't got around to setup a BSD vm to test.
Attachment #755295 - Flags: review?(alex)
I implemented my proposal [1], instead of the current spec, since there was agreement from Chris Rogers.

[1] http://lists.w3.org/Archives/Public/public-audio/2013AprJun/0437.html
Attachment #755298 - Flags: review?(ehsan)
Attachment #755299 - Flags: review?(ehsan)
Comment on attachment 755296 [details] [diff] [review]
Expose the new cubeb function in the AudioStream as a static method.

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

Why are you using int here?  We're going to need the unsigned value when we use this, so the conversion from unsigned to signed and then back to unsigned seems unnecessary.
Comment on attachment 755298 [details] [diff] [review]
Implement the maxChannelCount attribute on top of the other patches.

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

::: content/media/webaudio/AudioDestinationNode.cpp
@@ +196,5 @@
>  
> +unsigned long
> +AudioDestinationNode::MaxChannelCount() const
> +{
> +  return AudioStream::MaxNumberOfChannels();

This does the wrong thing for offline destination nodes.

::: content/media/webaudio/AudioDestinationNode.h
@@ +34,5 @@
>    {
>      return 0;
>    }
>  
> +  unsigned long MaxChannelCount() const;

uint32_t.

::: dom/webidl/AudioDestinationNode.webidl
@@ +17,1 @@
>      //attribute unsigned long numberOfChannels;

While you're here, can you please remove numberOfChannels?  It has long been removed from the spec, and I forgot to remove it here.
Attachment #755298 - Flags: review?(ehsan) → review-
Comment on attachment 755299 [details] [diff] [review]
Add a test for the maxChannelCount attribute.

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

::: content/media/webaudio/test/Makefile.in
@@ +27,5 @@
>    test_analyserNode.html \
>    test_AudioBuffer.html \
>    test_AudioContext.html \
>    test_AudioListener.html \
> +  test_AudioDestinationNode.html \

Nit: I'd call this test_maxChannelCount.html...

::: content/media/webaudio/test/test_AudioDestinationNode.html
@@ +12,5 @@
> +SimpleTest.waitForExplicitFinish();
> +addLoadEvent(function() {
> +  SpecialPowers.setBoolPref("media.webaudio.enabled", true);
> +  var ac = new AudioContext();
> +  ok(ac.destination.maxChannelCount != undefined, "We can query the maximum number of channels");

Hmm, I'm not sure what you wanted to do here, but in JS, comparing anything with undefined returns false, AFAIK, so this check always returns true, no matter what.  :-)

I think a better way to test this is to test whether maxChannelCount > 0.  Also, you need to test with OfflineAudioContext as well.  You can construct a couple of offline contexts with a different channel count and make sure that the correct number is reflected in maxChannelCount.
Attachment #755299 - Flags: review?(ehsan) → review-
Comment on attachment 755295 [details] [diff] [review]
add a function to cubeb to get the max number of channels

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

::: media/libcubeb/src/cubeb_pulse.c
@@ +111,3 @@
>      ctx->error = 1;
> +  } else if (state == PA_CONTEXT_READY){
> +    WRAP(pa_context_get_server_info)(c, server_info_callback, ctx);

Can we call this from cubeb_init after the context is ready, rather than here?
Attachment #755295 - Flags: review?(kinetik) → review+
Attachment #755296 - Flags: review?(kinetik) → review+
Hi,

By default, sndio doesn't limit the number of channels. It routes stream channels that wouldn't be present on the hardware to existing ones (ex. a 8-channel file can be played on a 4-speaker set, and all channels of 8 channels will sound on the "appropriate" speakers). Similarly a stereo file will sound on all speakers of a 4-speaker set (each channel is routed to a pair of speakers). The "routing" is somewhat user-configurable; it's global, allowing the user to change the routing for all programs.

If this is OK for cubeb, I'd suggest something like:

static int
sndio_get_max_channel_count(cubeb * ctx, uint32_t * max_channels)
{
  assert(ctx && max_channels);
  *max_channels = 8;
  return CUBEB_OK;
}

FWIW, by default, the channels count returned by sio_getcap() doesn't correspond to number of channels present on the hardware; it's a hard-coded value similar to the above.
Attached patch patch (obsolete) — Splinter Review
I found that the pulseaudio implementation was a bit racy: if one called the
function right after the context init, and because the API to get the sink info
is async, it is possible that `sink_info_callback` was not called yet. In
pratice, this happended and the test went orange.

This patch (to apply on top of the other) fixes it by waiting until we have the
info to return. Theoretically, this can block the main thread for a short (1 or
2 ms) period of time the first time it is called.
Attachment #755916 - Flags: review?(kinetik)
(removed unwanted changes in the test).
Attachment #755917 - Flags: review?(kinetik)
Attachment #755916 - Attachment is obsolete: true
Attachment #755916 - Flags: review?(kinetik)
This fixes the issues, and limits the number of channels to 32 for the
OfflineAudioContext.

From what I see in other bugs, you might already have the part that throws if
the channel count is invalid.
forgot to qref.
Attachment #755925 - Flags: review?(ehsan)
Attachment #755924 - Attachment is obsolete: true
Attachment #755298 - Attachment is obsolete: true
Attachment #755917 - Attachment description: patch → Fix a race in the pulseaudio implementation.
Attachment #755927 - Flags: review?(ehsan)
Attachment #755299 - Attachment is obsolete: true
Comment on attachment 755927 [details] [diff] [review]
Test for AudioContext.destination.maxChannelCount. r=

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

::: content/media/webaudio/test/test_maxChannelCount.html
@@ +16,5 @@
> +  var ac = new AudioContext();
> +  ok(ac.destination.maxChannelCount > 0, "We can query the maximum number of channels");
> +
> +  var oac = new OfflineAudioContext(2, 1024, 48000);
> +  ok(oac.destination.maxChannelCount, 32, "OfflineAudioContext has 32 max channels.");

As discussed on IRC, this should be 2.  Also please add another similar test which passes another value for the number of channels to the OfflineAudioContext ctor, such as 6 or whatever.
Attachment #755927 - Flags: review?(ehsan) → review+
Comment on attachment 755925 [details] [diff] [review]
Implement AudioContext.destination.maxChannelCount. r=

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

::: content/media/webaudio/AudioContext.cpp
@@ +115,5 @@
>  
> +  if (!aNumberOfChannels || aNumberOfChannels > WebAudioUtils::MaxChannelCount) {
> +    aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);
> +    return nullptr;
> +  }

Bug 877231 which is on inbound now should make this unnecessary.

@@ +386,5 @@
>  
> +uint32_t
> +AudioContext::MaxChannelCount()
> +{
> +  return mIsOffline ? WebAudioUtils::MaxChannelCount : AudioStream::MaxNumberOfChannels();

As discussed on irc, for offline audio contexts, we need to return the number of channels passed to the ctor.  Currently we only "keep track" of that value in the length of OfflineDestinationNodeEngine::mInputChannels, but it's not kosher to touch the engine's guts from the main thread, so you can just add a new member to AudioContext I guess...

::: content/media/webaudio/AudioContext.h
@@ +195,5 @@
>    void UnregisterPannerNode(PannerNode* aNode);
>    void UnregisterScriptProcessorNode(ScriptProcessorNode* aNode);
>    void UpdatePannerSource();
>  
> +  uint32_t MaxChannelCount();

Nit: Make this const please.

::: content/media/webaudio/AudioDestinationNode.h
@@ +34,5 @@
>    {
>      return 0;
>    }
>  
> +  uint32_t MaxChannelCount() const;

You should also make sure that setting destination.channelCount to something greater than destination.maxChannelCount throws, and test that in part 4.
Attachment #755925 - Flags: review?(ehsan) → review-
Attachment #755925 - Attachment is obsolete: true
Added another OfflineAudioContext with 6 channels, plus check that an exception
is thrown when setting a channelCount higher than the initial channel count on
an OfflineAudioContext.

Carrying forward r+.
Attachment #755927 - Attachment is obsolete: true
Attachment #756048 - Flags: review+
Comment on attachment 756046 [details] [diff] [review]
Implement AudioContext.destination.maxChannelCount. r=

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

Nice!
Attachment #756046 - Flags: review?(ehsan) → review+
Attachment #755917 - Flags: review?(kinetik) → review+
Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.
Component: Video/Audio → Web Audio
Fix another bustage on another android:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6053305ba2b3
Attachment #755295 - Flags: review?(alex)
Depends on: 928547
You need to log in before you can comment on or make changes to this bug.