New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 661861 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Dec 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug

Blocking:
issue 673392



Sign in to add a comment

CRAS: USB device should use ID_SERIAL to make stable id more robust

Project Member Reported by hychao@chromium.org, Nov 3 2016

Issue description

Currently CRAS makes the stable id from UID and VID of USB audio device, so that Chrome/webapp cannot tell between two headsets of the same brand & type.
There is usage in Kiosk app to mark specific USB hardware as defect unit and ban it persistently, so we need to find a solution for that.

udev info has two properties: ID_SERIAL and ID_SERIAL_SHORT which are supposed to be unique on different hardware unites of the same kind.

cras_udev.c is a place to extract this serial string, pack it into struct cras_alsa_card_info and pass all the way to alsa_io.

Since we've exposed the stable ID to Chrome for storing device preference for a few releases. We have to revise the stable ID while not breaking current usage. The plan is to use two properties for each node: id and id_old so that Chrome side can do something like:

if not pref.has_id(node.id) and perf.has_id(node.id_old):
  pref[node.id] = pref[node.id_old]
  pref[node.id_old] = NULL

  
 
Cc: st...@chromium.org
Components: UI>Shell>Kiosk
Labels: -Pri-3 M-57 OS-Chrome Pri-2
Owner: mojahsu@chromium.org
Status: Assigned (was: Untriaged)
Moja is very kind to take this.

Comment 3 by hychao@chromium.org, Nov 16 2016

The CRAS side change is at https://chromium-review.googlesource.com/#/c/407723/ to add new node attribute "StableDeviceIdOld".

Chrome UI should modify its code to take care backward compatibility before we merge this change. Something like: if node has StableDeviceIdOld, check preference for this key, and replace preference entry with new key: StableDeviceId.
tbarzic@ can you help with that?

Comment 4 by dgreid@chromium.org, Nov 16 2016

Should we invert the backwards compatibility?  If we make the new field StableDeviceIdNew (or something a little better) then chrome will keep working and be bisect-able across a larger range of platform versions.
I slightly prefer approach in comment 4: adding separate stable_device_id_v2 property, but it shouldn't make that much difference.

And, yes, I can help with Chrome side :)

Comment 6 by mojahsu@google.com, Nov 17 2016

Do we really need backward compatibility?
Because I saw a CL that changes the StableDeviceId too.
https://chromium-review.googlesource.com/401146
In cras_alsa_io.c it changed the parameter of hash calculate. From sizeof(name) to strlen(name).

I discussed StableDeviceIdNew with hychao@ before.
If we choose to use StableDeviceIdNew, there might be some effort when we decide to remove it. The content of the StableDeviceID will be different after we deprecated StableDeviceIdNew. StableDeviceId will keep old value during the backward compatible stage and keep new value after it.
I'll defer backward compatibility question to jennyz, since she is who owns audio stack in Chrome, but my guess is that we do need it (otherwise we lose info from Chrome preferences).

I'm fine with both StableDeviceIdNew and StableDeviceIdOld, Chrome will have to handle change from old value to new value either way, it would be only the question of when.
What I mean is we've already lose info once when this CL https://chromium-review.googlesource.com/401146 merged. But we can still keep backward compatibility this time.

I want to use StableDeviceIdOld this time for keeping the StableId consistent.

After discuss with hychao@. We decided to use StableDeviceIdNew.
It will make cras merge code easily for adding and removing the StableDeviceIdNew.

Project Member

Comment 10 by bugdroid1@chromium.org, Dec 2 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/adhd/+/c84eb15a0fbd4a6b336d11438836b644fefc2679

commit c84eb15a0fbd4a6b336d11438836b644fefc2679
Author: Moja Hsu <mojahsu@google.com>
Date: Tue Nov 08 07:25:25 2016

CRAS: dbus_control - Add serial number for USB device for stable id

Get additional sys attribute for "serial" which is serial number in
udev.
Add it into the stable_id_new calcuation and keep stable_id for backward
compitable.

BUG= chromium:661861 
TEST=Plug 2 same type USB speakers into chromebook.
  execute
  "dbus-send --system
  --type=method_call
  --print-reply
  --dest=org.chromium.cras
  /org/chromium/cras
  org.chromium.cras.Control.GetNodes"

  Check if we have the same stable_id but different stable_id_new for the 2
  speakers.

  pass make check for unit test.

Change-Id: Iefefe383fd6143dd7c28fdba3cf3f3872c2dd726
Reviewed-on: https://chromium-review.googlesource.com/407723
Commit-Ready: Hsu Wei-Cheng <mojahsu@chromium.org>
Tested-by: Hsu Wei-Cheng <mojahsu@chromium.org>
Reviewed-by: Hsinyu Chao <hychao@chromium.org>

[modify] https://crrev.com/c84eb15a0fbd4a6b336d11438836b644fefc2679/cras/src/server/cras_a2dp_iodev.c
[modify] https://crrev.com/c84eb15a0fbd4a6b336d11438836b644fefc2679/cras/src/server/cras_iodev_list.c
[modify] https://crrev.com/c84eb15a0fbd4a6b336d11438836b644fefc2679/cras/src/server/cras_dbus_control.c
[modify] https://crrev.com/c84eb15a0fbd4a6b336d11438836b644fefc2679/cras/src/common/cras_types.h
[modify] https://crrev.com/c84eb15a0fbd4a6b336d11438836b644fefc2679/cras/src/tests/alsa_io_unittest.cc
[modify] https://crrev.com/c84eb15a0fbd4a6b336d11438836b644fefc2679/cras/src/tests/alsa_card_unittest.cc
[modify] https://crrev.com/c84eb15a0fbd4a6b336d11438836b644fefc2679/cras/src/server/cras_udev.c
[modify] https://crrev.com/c84eb15a0fbd4a6b336d11438836b644fefc2679/cras/src/server/cras_iodev.h
[modify] https://crrev.com/c84eb15a0fbd4a6b336d11438836b644fefc2679/cras/src/server/cras_alsa_io.h
[modify] https://crrev.com/c84eb15a0fbd4a6b336d11438836b644fefc2679/cras/src/server/cras_loopback_iodev.c
[modify] https://crrev.com/c84eb15a0fbd4a6b336d11438836b644fefc2679/cras/src/server/cras_alsa_card.c
[modify] https://crrev.com/c84eb15a0fbd4a6b336d11438836b644fefc2679/cras/src/common/cras_iodev_info.h
[modify] https://crrev.com/c84eb15a0fbd4a6b336d11438836b644fefc2679/cras/src/server/cras_bt_io.c
[modify] https://crrev.com/c84eb15a0fbd4a6b336d11438836b644fefc2679/cras/src/server/cras_hfp_iodev.c
[modify] https://crrev.com/c84eb15a0fbd4a6b336d11438836b644fefc2679/cras/src/server/cras_alsa_io.c
[modify] https://crrev.com/c84eb15a0fbd4a6b336d11438836b644fefc2679/cras/README.dbus-api

Project Member

Comment 11 by bugdroid1@chromium.org, Dec 7 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/system_api/+/4489d28f1ec210c840d4f4827f8210ab27141f2d

commit 4489d28f1ec210c840d4f4827f8210ab27141f2d
Author: Toni Barzic <tbarzic@google.com>
Date: Wed Nov 16 23:46:54 2016

system_api: Update dbus service constants for cras nod properties

Adds const for new property exposed by cras node struct:
StableDeviceIdOld - this will expose soon to be deprecated stable
audio device ID value.

BUG= chromium:661861 
TEST=None

Change-Id: I90715e6669b7520c2d68979ab2c1c301854b3dbf
Reviewed-on: https://chromium-review.googlesource.com/411987
Commit-Ready: Toni Barzic <tbarzic@chromium.org>
Tested-by: Toni Barzic <tbarzic@chromium.org>
Reviewed-by: Hsinyu Chao <hychao@chromium.org>

[modify] https://crrev.com/4489d28f1ec210c840d4f4827f8210ab27141f2d/dbus/service_constants.h

Project Member

Comment 12 by bugdroid1@chromium.org, Dec 9 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ff0f2ca2e95f208655869178c32cf666c8f6e478

commit ff0f2ca2e95f208655869178c32cf666c8f6e478
Author: tbarzic <tbarzic@chromium.org>
Date: Fri Dec 09 02:33:16 2016

Roll cros_system_api to pick up new cras constant

Rolling cros_system_api up to:
https://chromium.googlesource.com/chromiumos/platform/system_api/+/4489d28f1ec210c840d4f4827f8210ab27141f2d

BUG= 661861 ,  659732 

Review-Url: https://codereview.chromium.org/2556203002
Cr-Commit-Position: refs/heads/master@{#437439}

[modify] https://crrev.com/ff0f2ca2e95f208655869178c32cf666c8f6e478/DEPS

Blocking: 673392
Owner: tbarzic@chromium.org
The code in cras is ready and merged.
Feel free to reuse or close this issue.
Project Member

Comment 15 by bugdroid1@chromium.org, Dec 17 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3ab01a28f2ccd9abc62a3833d72959205ddce98b

commit 3ab01a28f2ccd9abc62a3833d72959205ddce98b
Author: tbarzic <tbarzic@chromium.org>
Date: Sat Dec 17 03:12:53 2016

Handle audio node stable device ID change

Chrome side change for updating the way stable device ID is
calculated for USB audio device nodes in order to make them more
usefull when distinguishing among USB audio devices with same
USB vendor and product IDs.

Cras will start providing new property - StableDeviceIdOld that
will contain old, now deprecated stable device ID - this will be
used to migrate audio device prefs to new stable ID (to provide
some backward compatibility). This property is expected to eventually
go away.

This does not expose new value to audio extension API, but cl will
follow - for now, keep providing old ID version through API.

Huge line diff is mostly due to refactoring test to account for
new stable device ID version :)

Parametrizes cras audio client unit tests by the version od
stable device ID provided by audio device - to ensure it keeps
working as expcted with both stable device ID version (does not
test for mixing devices with different stable device ID versions,
that is never expected to happen).

Added audio devices pref handler for migrating audio device records
when audio device stable device ID changes.

BUG= 661861 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2510093003
Cr-Commit-Position: refs/heads/master@{#439304}

[modify] https://crrev.com/3ab01a28f2ccd9abc62a3833d72959205ddce98b/chromeos/audio/audio_device.cc
[modify] https://crrev.com/3ab01a28f2ccd9abc62a3833d72959205ddce98b/chromeos/audio/audio_device.h
[modify] https://crrev.com/3ab01a28f2ccd9abc62a3833d72959205ddce98b/chromeos/audio/audio_devices_pref_handler_impl.cc
[modify] https://crrev.com/3ab01a28f2ccd9abc62a3833d72959205ddce98b/chromeos/audio/audio_devices_pref_handler_impl.h
[modify] https://crrev.com/3ab01a28f2ccd9abc62a3833d72959205ddce98b/chromeos/audio/audio_devices_pref_handler_impl_unittest.cc
[modify] https://crrev.com/3ab01a28f2ccd9abc62a3833d72959205ddce98b/chromeos/audio/cras_audio_handler.cc
[modify] https://crrev.com/3ab01a28f2ccd9abc62a3833d72959205ddce98b/chromeos/audio/cras_audio_handler_unittest.cc
[modify] https://crrev.com/3ab01a28f2ccd9abc62a3833d72959205ddce98b/chromeos/dbus/audio_node.cc
[modify] https://crrev.com/3ab01a28f2ccd9abc62a3833d72959205ddce98b/chromeos/dbus/audio_node.h
[modify] https://crrev.com/3ab01a28f2ccd9abc62a3833d72959205ddce98b/chromeos/dbus/cras_audio_client.cc
[modify] https://crrev.com/3ab01a28f2ccd9abc62a3833d72959205ddce98b/chromeos/dbus/cras_audio_client_unittest.cc
[modify] https://crrev.com/3ab01a28f2ccd9abc62a3833d72959205ddce98b/chromeos/dbus/fake_cras_audio_client.cc
[modify] https://crrev.com/3ab01a28f2ccd9abc62a3833d72959205ddce98b/extensions/browser/api/audio/audio_apitest.cc
[modify] https://crrev.com/3ab01a28f2ccd9abc62a3833d72959205ddce98b/extensions/browser/api/audio/audio_service_chromeos.cc
[modify] https://crrev.com/3ab01a28f2ccd9abc62a3833d72959205ddce98b/media/audio/audio_manager_unittest.cc

Status: Fixed (was: Assigned)
Project Member

Comment 17 by bugdroid1@chromium.org, Jan 18 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4e3e93214f404cd6f3da984000a447d8284cf828

commit 4e3e93214f404cd6f3da984000a447d8284cf828
Author: tbarzic <tbarzic@chromium.org>
Date: Wed Jan 18 01:18:23 2017

Update audio api to use v2 stable device ID

Algorithm used to calculate stable audio device ID has been updated
in cras to better distinguish instances of identical USB devcies.
This exposes the new stable device ID to audio API.
The existing stableDeviceId does not to be currently used, so it
should be OK just to overwrite it.

BUG= 661861 

Review-Url: https://codereview.chromium.org/2585413002
Cr-Commit-Position: refs/heads/master@{#444221}

[modify] https://crrev.com/4e3e93214f404cd6f3da984000a447d8284cf828/extensions/browser/api/audio/audio_apitest.cc
[modify] https://crrev.com/4e3e93214f404cd6f3da984000a447d8284cf828/extensions/browser/api/audio/audio_service_chromeos.cc
[modify] https://crrev.com/4e3e93214f404cd6f3da984000a447d8284cf828/extensions/test/data/api_test/audio/add_nodes/background.js
[modify] https://crrev.com/4e3e93214f404cd6f3da984000a447d8284cf828/extensions/test/data/api_test/audio/remove_nodes/background.js

Cc: r...@chromium.org
Cc: -st...@chromium.org
Labels: VerifyIn-61
Status: Verified (was: Fixed)
Verified in Chrome OS 9801.0.0, 62.0.3174.0. 

Sign in to add a comment