Add some basic audio hardware/driver/format information to about:support

RESOLVED FIXED in Firefox 56

Status

()

defect
P2
normal
RESOLVED FIXED
4 years ago
8 months ago

People

(Reporter: kinetik, Assigned: chunmin)

Tracking

(Depends on 1 bug, Blocks 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(8 attachments, 10 obsolete attachments)

165.43 KB, image/png
Details
10.57 KB, patch
Details | Diff | Splinter Review
15.76 KB, patch
Details | Diff | Splinter Review
15.11 KB, patch
Details | Diff | Splinter Review
59 bytes, text/x-review-board-request
kinetik
: review+
Details
59 bytes, text/x-review-board-request
padenot
: review+
Details
59 bytes, text/x-review-board-request
Felipe
: review+
Details
59 bytes, text/x-review-board-request
Felipe
: review+
Details
To make it easier to file audio bugs, it'd be useful to add some audio-related information to about:support ala the existing graphics information.  Right now we have to ask users to run dxdiag *and* manually dig information out of the advanced settings of the sound control panel.

We don't (yet) support device selection, so we only need to query the default output device for now.

For a first pass, the following data would be useful:

- libcubeb backend being used
  available via cubeb_get_backend_id()

- Sound hardware name and driver name and version
  need a new libcubeb API to query this

- Default audio format (sample rate, bit depth, channel count, speaker mask)
  partial support in libcubeb's API to query this, would need to extend slightly

CCing Paul and Jesup in case they have input or need additional info for WebAudio/WebRTC stuff.
Flags: needinfo?(ajones)
System reported minimal latency would be useful, there are at least two headsets that return an value that is wrong, on OSX.
Flags: needinfo?(ajones)
Priority: -- → P2
Component: Audio/Video: MediaStreamGraph → Audio/Video: cubeb
alwu,
You should be interested in this bug! 
This is not a urgent bug, you can put this in your queue and come back when you are available.
Assignee: nobody → alwu
Depends on: 1288980
Could I take this bug if you're not working on this?
Flags: needinfo?(alwu)
Assignee: alwu → cchang
Flags: needinfo?(alwu)
Attachment #8876663 - Attachment description: part6: Show audio input devices on about:support → [WIP] part6: Show audio input devices on about:support
Notes:
- It's better to re-arrange all the patches
- Add driver info
Correct title for input device list
- replace "output devices" by "input devices"
Attachment #8876663 - Attachment is obsolete: true
Attachment #8876664 - Attachment is obsolete: true
Attachment #8865805 - Attachment is obsolete: true
Attachment #8865806 - Attachment is obsolete: true
Attachment #8876660 - Attachment is obsolete: true
Attachment #8876661 - Attachment is obsolete: true
Attachment #8877026 - Attachment is obsolete: true
Attachment #8877027 - Attachment is obsolete: true
Depends on: 1378299
Comment on attachment 8883506 [details]
Bug 1197045 - part1: Create an AudioDeviceInfo to expose the native device information;

https://reviewboard.mozilla.org/r/154424/#review159724

::: dom/media/AudioDeviceInfo.h:17
(Diff revision 1)
> +  NS_DECL_NSIAUDIODEVICEINFO
> +
> +  explicit AudioDeviceInfo(const nsAString& aId,
> +		                       const nsAString& aName,
> +			                     const nsAString& aGroupId,
> +			                     const nsAString& aVendor,

Indentation is off here.

::: dom/media/AudioDeviceInfo.h:33
(Diff revision 1)
> +                           uint32_t aHigestLatency);
> +
> +private:
> +  virtual ~AudioDeviceInfo() = default;
> +
> +  nsString mId;

cubeb_devid is an opaque type, you can't assume it's a string (or any displayable type) and there's no point exposing it via any UI.  The intended purpose of it is to uniquely select devices when creating streams via cubeb_stream_init.

::: dom/media/AudioDeviceInfo.h:47
(Diff revision 1)
> +  uint32_t mMaxChannels;
> +  uint32_t mDefaultRate;
> +  uint32_t mMaxRate;
> +  uint32_t mMinRate;
> +  uint32_t mLowestLatency;
> +  uint32_t mHigestLatency;

Typo: highest

But it's probably better to call this min/max latency.

::: dom/media/AudioDeviceInfo.cpp:8
(Diff revision 1)
> +NS_IMPL_ISUPPORTS(AudioDeviceInfo, nsIAudioDeviceInfo)
> +
> +AudioDeviceInfo::AudioDeviceInfo(const nsAString& aId,
> +		                             const nsAString& aName,
> +				                         const nsAString& aGroupId,
> +				                         const nsAString& aVendor,

Indentation is off here too.

::: dom/media/nsIAudioDeviceInfo.idl:14
(Diff revision 1)
> +
> +  readonly attribute DOMString groupId;
> +
> +  readonly attribute DOMString vendor;
> +
> +  // type: Unknow/Input/Output

Typo: unknown

::: dom/media/nsIAudioDeviceInfo.idl:20
(Diff revision 1)
> +  const unsigned short TYPE_UNKNOWN = 0;
> +  const unsigned short TYPE_INPUT   = 1;
> +  const unsigned short TYPE_OUTPUT  = 2;
> +  readonly attribute unsigned short type;
> +
> +  // state: disabled/Enabled/Unplugged

Inconsistent capitalization.  Upper case d in disabled?  And make the other comments match capitalizations style too.

::: dom/media/nsIAudioDeviceInfo.idl:42
(Diff revision 1)
> +  const unsigned short FMT_F32LE = 0x1000;
> +  const unsigned short FMT_F32BE = 0x2000;
> +  readonly attribute unsigned short supportedFormat;
> +  readonly attribute unsigned short defaultFormat;
> +
> +  // Max number of channels: [1, 255]

Where does the [1, 255] originate from?
Attachment #8883506 - Flags: review?(kinetik) → review-
Comment on attachment 8883506 [details]
Bug 1197045 - part1: Create an AudioDeviceInfo to expose the native device information;

https://reviewboard.mozilla.org/r/154424/#review159730

Thanks for working on this.  Generally looks good, but r- for a few small issues.

Also, from the original bug comment, one item I feel is very important for supporting the audio code, and which is painful to ask users to find via other methods (e.g. looking in Device Manager, running dxdiag.exe):
    "- Sound hardware name and driver name and version
       need a new libcubeb API to query this"

If this part isn't going to be handled by this patch series/in this bug, please file a new bug for this so that it stays on the radar.
Comment on attachment 8883506 [details]
Bug 1197045 - part1: Create an AudioDeviceInfo to expose the native device information;

https://reviewboard.mozilla.org/r/154424/#review159724

> cubeb_devid is an opaque type, you can't assume it's a string (or any displayable type) and there's no point exposing it via any UI.  The intended purpose of it is to uniquely select devices when creating streams via cubeb_stream_init.

OK, I'll remove the id.

> Typo: highest
> 
> But it's probably better to call this min/max latency.

Thanks!

> Where does the [1, 255] originate from?

The limit of the channel count is 256 in cubeb_mixer:
http://searchfox.org/mozilla-central/rev/5e1e8d2f244bd8c210a578ff1f65c3b720efe34e/media/libcubeb/src/cubeb_mixer.h#31
See Also: → 1378633
Blocks: 1378634
Comment on attachment 8883506 [details]
Bug 1197045 - part1: Create an AudioDeviceInfo to expose the native device information;

https://reviewboard.mozilla.org/r/154424/#review160092
Attachment #8883506 - Flags: review?(kinetik) → review+
Comment on attachment 8883508 [details]
Bug 1197045 - part3: Add audio devices information to about:support;

https://reviewboard.mozilla.org/r/154428/#review160800

Not to call any favorites :P, but I believe the graphics section will be more often used than the media section, so I'd be prefer if you added it after the graphics one.
Attachment #8883508 - Flags: review?(felipc) → review+
Comment on attachment 8883507 [details]
Bug 1197045 - part2: Expose audio channels, layout, sample-rate, and native device information via DOMWindowUtils;

https://reviewboard.mozilla.org/r/154426/#review159620

r+ with the comments addressed.

::: dom/media/CubebUtils.cpp:540
(Diff revision 1)
> +}
> +
> +uint16_t ConvertCubebType(cubeb_device_type aType)
> +{
> +  // Store the mapping table to global space instead of stack space
> +  // so it won't be created and destroyed on every call.

Please use a normal variable here. Putting `static` here means that this is always allocated, and uses memory all the time. We don't really care about the time complexity here.

::: dom/media/CubebUtils.cpp:553
(Diff revision 1)
> +
> +uint16_t ConvertCubebState(cubeb_device_state aState)
> +{
> +  // Store the mapping table to global space instead of stack space
> +  // so it won't be created and destroyed on every call.
> +  static uint16_t map[] = {

Same.
Attachment #8883507 - Flags: review?(padenot) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/57842092a282
part1: Create an AudioDeviceInfo to expose the native device information; r=kinetik
https://hg.mozilla.org/integration/autoland/rev/feea60a002a7
part2: Expose audio channels, layout, sample-rate, and native device information via DOMWindowUtils; r=padenot
https://hg.mozilla.org/integration/autoland/rev/78b66a4c6b7c
part3: Add audio devices information to about:support; r=Felipe
Keywords: checkin-needed
Already upload a patch to fix it.
Flags: needinfo?(cchang)
Comment on attachment 8887817 [details]
Bug 1197045 - part4: Test case;

https://reviewboard.mozilla.org/r/158730/#review164882
Attachment #8887817 - Flags: review?(felipc) → review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a4ea1d5fcb4e
part1: Create an AudioDeviceInfo to expose the native device information; r=kinetik
https://hg.mozilla.org/integration/autoland/rev/cf756c62b0a6
part2: Expose audio channels, layout, sample-rate, and native device information via DOMWindowUtils; r=padenot
https://hg.mozilla.org/integration/autoland/rev/fd48cc71efef
part3: Add audio devices information to about:support; r=Felipe
https://hg.mozilla.org/integration/autoland/rev/3af0f387a8f2
part4: Test case; r=Felipe
Keywords: checkin-needed
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1711c8455215
part1: Create an AudioDeviceInfo to expose the native device information; r=kinetik
https://hg.mozilla.org/integration/autoland/rev/118833b05c41
part2: Expose audio channels, layout, sample-rate, and native device information via DOMWindowUtils; r=padenot
https://hg.mozilla.org/integration/autoland/rev/062fb0855b3c
part3: Add audio devices information to about:support; r=Felipe
https://hg.mozilla.org/integration/autoland/rev/8ab4d7eefbf6
part4: Test case; r=Felipe
Keywords: checkin-needed
Depends on: 1384944
Depends on: 1400390
Depends on: 1505483
You need to log in before you can comment on or make changes to this bug.