1419378 - Return failure in AudioCallbackDriver::Init when out channels is 0

Status: RESOLVED FIXED

(Core :: Audio/Video: MediaStreamGraph, enhancement, P3)

Description

[Alex Chronopoulos [:achronop]]

In AudioCallbackDriver the mOutputChannels are set during during Init() method. The value is queried from MSG corresponding method which can return 0. Right now we hide the error by resetting to 1 when we get a 0 [1]. This is wrong, it would be better to return error instead of resetting the value.

[1] https://searchfox.org/mozilla-central/source/dom/media/GraphDriver.cpp#618

Comment 1

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1419378 - Fail before stream init when output number of channels is zero.

Review commit: https://reviewboard.mozilla.org/r/201602/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201602/

Comment 2

[Alex Chronopoulos [:achronop]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffa3a5f59c5f105a73e8a6fe6b0ac5e881cd2062

Comment 3

[Paul Adenot (:padenot)]

Comment on attachment 8930449 [details]
Bug 1419378 - Fail before stream init when output number of channels is zero.

https://reviewboard.mozilla.org/r/201602/#review207892

::: dom/media/GraphDriver.cpp:622
(Diff revision 1)
>    // Query and set the number of channels this AudioCallbackDriver will use.
> -  mOutputChannels = std::max<uint32_t>(1, mGraphImpl->AudioChannelCount());
> +  mOutputChannels = mGraphImpl->AudioChannelCount();
> +  if (!mOutputChannels) {
> +    LOG(LogLevel::Warning, ("Output number of channels is 0."));
> +    return false;
> +  }

How is that better? Have you tested that this changes things?

I think you should do something like: https://searchfox.org/mozilla-central/source/dom/media/GraphDriver.cpp#693

Comment 4

[Alex Chronopoulos [:achronop]]

Comment on attachment 8930449 [details]
Bug 1419378 - Fail before stream init when output number of channels is zero.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201602/diff/1-2/

Comment 5

[Alex Chronopoulos [:achronop]]

Comment on attachment 8930449 [details]
Bug 1419378 - Fail before stream init when output number of channels is zero.

https://reviewboard.mozilla.org/r/201602/#review208184

::: dom/media/GraphDriver.cpp:622
(Diff revision 1)
>    // Query and set the number of channels this AudioCallbackDriver will use.
> -  mOutputChannels = std::max<uint32_t>(1, mGraphImpl->AudioChannelCount());
> +  mOutputChannels = mGraphImpl->AudioChannelCount();
> +  if (!mOutputChannels) {
> +    LOG(LogLevel::Warning, ("Output number of channels is 0."));
> +    return false;
> +  }

It is better because we do attempt to open a device that reported zero number of channels. Right now we arbitary use mono in that case.

Added the fallback to SystemClockDriver which creates an infinite loop of retries. I am not sure how useful it is to retry with zero number of channels. Is there a better way to report that error?

Comment 6

[Paul Adenot (:padenot)]

Well, maybe a device will be plugged in?

Comment 7

[Paul Adenot (:padenot)]

Comment on attachment 8930449 [details]
Bug 1419378 - Fail before stream init when output number of channels is zero.

https://reviewboard.mozilla.org/r/201602/#review208858

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e114b48a4e78
Fail before stream init when output number of channels is zero. r=padenot

Comment 9

[Narcis Beleuzu [:NarcisB]]

Backed out for merge conflict.

https://hg.mozilla.org/integration/autoland/rev/fefa8c2175a5b969b25757473d9213a83a84b800

Comment 10

[Alex Chronopoulos [:achronop]]

Comment on attachment 8930449 [details]
Bug 1419378 - Fail before stream init when output number of channels is zero.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201602/diff/2-3/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bde36d0fa2eb
Fail before stream init when output number of channels is zero. r=padenot

Comment 12

[Alex Chronopoulos [:achronop]]

Rebased version pushed.

Comment 13

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/bde36d0fa2eb

Comment 14

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bde36d0fa2eb