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

Issue 625222 link

Starred by 12 users

Issue metadata

Status: Duplicate
Merged: issue 649360
Owner:
Closed: Nov 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug-Regression

Blocked on:
issue 649360



Sign in to add a comment

On every reboot seen volume slider on login screen.

Project Member Reported by abod...@chromium.org, Jul 1 2016

Issue description

ChromeOS:8530.0.0/53.0.2784.3

Please specify Cr-* of the system to which this bug/feature applies (add
the label below).

Steps To Reproduce:
(1)Reboot the device or Power On the device
(2)Observe the volume slider at login screen 


Expected Result:

Actual Result:
Seen volume slider on every reboot of the device.

How frequently does this problem reproduce? (Always, sometimes, hard to
reproduce?)
100%
What is the impact to the user, and is there a workaround? If so, what is
it?

Please provide any additional information below. Attach a screen shot or
log if possible.


 
Labels: -Type-Bug M-53 Type-Bug-Regression
Owner: jen...@chromium.org
Status: Assigned (was: Untriaged)
Possibly caused by the change to how we handle notifications with cras?
Likely! Will take a look.
Project Member

Comment 4 by sheriffbot@chromium.org, Jul 2 2016

Labels: -M-53 M-54 MovedFrom-53
Moving this nonessential bug to the next milestone.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Cc: jen...@chromium.org kavvaru@chromium.org durga.behera@chromium.org hychao@chromium.org ajha@chromium.org
 Issue 626226  has been merged into this issue.
Status: Started (was: Assigned)
This issue is caused by my change to notify the UI for volume change events based on cras signal OutputNodeVolumeChanged. During the reboot, CrasAudioHandler will intialize audio state, which sets the volume and receives cras signal for volume change signal, therefore, the volume slider bar shows up. I made a cl to add some heuristic for figuring out the volume change signal is in response to the initialization of CrasAudioHandler, if so, do not notify the UI.

However, I found out a minor issue caused by cras not sending OutputNodeVolumeChange signal to chrome if chrome sends cras command SetOutputNodeVolume with the same volume cras server has. It cause some minor UI issues as following:
1. When user changes volume, if it reaches the maximum vlue and user press volume up again, volume bar won't pop up. The same happens to minimum volume and volume down.
2. When user logs out a chrome session, CrasAudioHandler will re-initialize its internal state, and sends SetOutputNodeVolume with the same volume value, cras won't respond with OutputNodeVolumeChanged signal. Currently, CrasAudioHandler adds a heuristic to figure out the signal is lost if the first OutputNodeVolumeChanged coming with a different volume value as the volume set during the initialization. If we don't do so, user won't see the volume bar when they press the volume up/down for the first time during a new session.

haychao@, will it be possible for cras to ALWAYS send OutputNodeVolumeChanged signal to chrome whenever it receives SetOutputNodeVolume command? cras does so for mute requests.

Comment 8 by warx@chromium.org, Jul 7 2016

Cc: alemate@chromium.org warx@chromium.org
 Issue 624107  has been merged into this issue.
Project Member

Comment 9 by bugdroid1@chromium.org, Jul 8 2016

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

commit 80330de86e05ab54a5cd17b9d3942d9f65b68983
Author: jennyz <jennyz@chromium.org>
Date: Fri Jul 08 18:32:52 2016

Do not notify Volume change event to CrasAudioHandler::Observer when CrasAudioHandler change volume during initialization.

BUG= 625222 

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

[modify] https://crrev.com/80330de86e05ab54a5cd17b9d3942d9f65b68983/chromeos/audio/cras_audio_handler.cc
[modify] https://crrev.com/80330de86e05ab54a5cd17b9d3942d9f65b68983/chromeos/audio/cras_audio_handler.h

Status: Fixed (was: Started)
Issue 626832 has been merged into this issue.
Project Member

Comment 12 by bugdroid1@chromium.org, Jul 11 2016

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

commit 305b2e610ce5d023aa8cda7adee571c5da8bcc0b
Author: Hsin-Yu Chao <hychao@chromium.org>
Date: Sun Jul 10 12:50:15 2016

Revert "CRAS: iodev - Avoid set volume loop by Chrome UI"

This reverts commit b9bf4e351d33213f6bd9896b53bb1ff7fde509c0, which
was added to avoid volume changed event loop, and the root cause has
been fixed in Chrome UI.
Because Chrome UI code has been modified to display volume bar base
on CRAS volume changed event CRAS reported, we need to revert this
commit to fix UI corner case.
For example, when system volume is already at 100% max, user press
volume up key should make volume bar show up.

BUG= chromium:625222 
TEST=Adjust volume to 100%, wait for volume bar disappear, press
volume up key again to see volume bar show up again.

Change-Id: I4960dfe6255a601ef18d02daef25b6ab111f78ea
Reviewed-on: https://chromium-review.googlesource.com/359161
Commit-Ready: Hsinyu Chao <hychao@chromium.org>
Tested-by: Hsinyu Chao <hychao@chromium.org>
Reviewed-by: Cheng-Yi Chiang <cychiang@chromium.org>

[modify] https://crrev.com/305b2e610ce5d023aa8cda7adee571c5da8bcc0b/cras/src/server/cras_iodev.c

Labels: -Pri-2 -M-54 -MovedFrom-53 M-53 Pri-1
Status: Assigned (was: Fixed)
Would this be safe to merge to 53? We're seeing reports of the problem there.
Status: Started (was: Assigned)
After hychao's cl in comment #12, it exposes another issue during power on process, I have fixed the issue in the pending cl:
https://codereview.chromium.org/2146313005/

Regarding #13, we will need to port back both chrome and platform changes to R53.
Project Member

Comment 16 by bugdroid1@chromium.org, Jul 18 2016

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

commit e7818501bc53a0f764221189ad2fcf9c7c101fac
Author: jennyz <jennyz@chromium.org>
Date: Mon Jul 18 16:51:04 2016

Count the SetOutputNodeVolume requests sent during the audio initialization and track them with the OutputNodeVolumeChanged signals responding from cras, do not pop up the volume bar for such signals.

BUG= 625222 

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

[modify] https://crrev.com/e7818501bc53a0f764221189ad2fcf9c7c101fac/chromeos/audio/cras_audio_handler.cc
[modify] https://crrev.com/e7818501bc53a0f764221189ad2fcf9c7c101fac/chromeos/audio/cras_audio_handler.h

Status: Fixed (was: Started)
 Issue 634940  has been merged into this issue.
Any word on when this release will be pushed? Still happening: 
Version 53.0.2785.55 beta (64-bit)
Platform 8530.49.0 (Official Build) beta-channel samus
ARC Version 3127873
Firmware Google_Samus.6300.174.0
Cc: keta...@chromium.org
Labels: Merge-Request-53

Comment 21 by dimu@chromium.org, Aug 12 2016

Labels: -Merge-Request-53 Merge-Approved-53 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M53 (branch: 2785)
Status: Started (was: Fixed)
Seems the CRAS change #12 doesn't need merge to M53 because the CL got reverted wasn't landed there.
Jenny: could you help do M53 merge for the CL in #16?
Project Member

Comment 23 by sheriffbot@chromium.org, Aug 15 2016

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Sorry, I was OOO for vacation last week. Will merge today.
Owner: hychao@chromium.org
Looks like hychao's cl in cras side (comment #12) is not in R53. I will need that change to be merged back to R53 first, then I can merge my chrome side cl. 

hychao, please merge #12 cl in R53, then assign the issue back to me, so that I can merge chrome cl after.
Project Member

Comment 26 by bugdroid1@chromium.org, Aug 16 2016

Labels: merge-merged-release-R53-8530.B
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/adhd/+/ff8c9b52d413a00041f539b4d7760c61ac0259d2

commit ff8c9b52d413a00041f539b4d7760c61ac0259d2
Author: Hsin-Yu Chao <hychao@chromium.org>
Date: Sun Jul 10 12:50:15 2016

Revert "CRAS: iodev - Avoid set volume loop by Chrome UI"

This reverts commit b9bf4e351d33213f6bd9896b53bb1ff7fde509c0, which
was added to avoid volume changed event loop, and the root cause has
been fixed in Chrome UI.
Because Chrome UI code has been modified to display volume bar base
on CRAS volume changed event CRAS reported, we need to revert this
commit to fix UI corner case.
For example, when system volume is already at 100% max, user press
volume up key should make volume bar show up.

BUG= chromium:625222 
TEST=Adjust volume to 100%, wait for volume bar disappear, press
volume up key again to see volume bar show up again.

Change-Id: I4960dfe6255a601ef18d02daef25b6ab111f78ea
Reviewed-on: https://chromium-review.googlesource.com/359161
Commit-Ready: Hsinyu Chao <hychao@chromium.org>
Tested-by: Hsinyu Chao <hychao@chromium.org>
Reviewed-by: Cheng-Yi Chiang <cychiang@chromium.org>
(cherry picked from commit 305b2e610ce5d023aa8cda7adee571c5da8bcc0b)
Reviewed-on: https://chromium-review.googlesource.com/371000
Reviewed-by: Hsinyu Chao <hychao@chromium.org>
Commit-Queue: Hsinyu Chao <hychao@chromium.org>

[modify] https://crrev.com/ff8c9b52d413a00041f539b4d7760c61ac0259d2/cras/src/server/cras_iodev.c

Owner: jen...@chromium.org
Oops my bad in #comment 22.
Done https://chromium-review.googlesource.com/#/c/371000/1 merged to R53.

hychao, do you know which build has #26? I will need to try out with my cl merging. 
Since M53-8530.59.0
Project Member

Comment 30 by bugdroid1@chromium.org, Aug 17 2016

Labels: -merge-approved-53 merge-merged-2785
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/24448767738300854e3df5ff45e9720f36d0262c

commit 24448767738300854e3df5ff45e9720f36d0262c
Author: Jenny Zhang <jennyz@chromium.org>
Date: Wed Aug 17 17:45:53 2016

Do not notify Volume change event to CrasAudioHandler::Observer when CrasAudioHandler change volume during initialization.

BUG= 625222 
TBR=hychao@chromium.org

Review URL: https://codereview.chromium.org/2256773002 .

Review-Url: https://codereview.chromium.org/2127033004
Cr-Original-Commit-Position: refs/heads/master@{#404448}
Cr-Commit-Position: refs/branch-heads/2785@{#639}
Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382}

[modify] https://crrev.com/24448767738300854e3df5ff45e9720f36d0262c/chromeos/audio/cras_audio_handler.cc
[modify] https://crrev.com/24448767738300854e3df5ff45e9720f36d0262c/chromeos/audio/cras_audio_handler.h

Project Member

Comment 31 by bugdroid1@chromium.org, Aug 17 2016

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

commit f615009fa3d171ebc5700b8928e349f83f861841
Author: Jenny Zhang <jennyz@chromium.org>
Date: Wed Aug 17 18:02:54 2016

Count the SetOutputNodeVolume requests sent during the audio initialization and track them with the OutputNodeVolumeChanged signals responding from cras, do not pop up the volume bar for such signals.

BUG= 625222 
TBR=hychao@chromium.org

Review URL: https://codereview.chromium.org/2250963003 .

Review-Url: https://codereview.chromium.org/2146313005
Cr-Original-Commit-Position: refs/heads/master@{#406014}
Cr-Commit-Position: refs/branch-heads/2785@{#642}
Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382}

[modify] https://crrev.com/f615009fa3d171ebc5700b8928e349f83f861841/chromeos/audio/cras_audio_handler.cc
[modify] https://crrev.com/f615009fa3d171ebc5700b8928e349f83f861841/chromeos/audio/cras_audio_handler.h

The merging is done. But it looks like they are not picked up in the latest R53 build yet.
Version 53.0.2785.70 beta (64-bit)
Platform 8530.62.0 (Official Build) beta-channel chell
Firmware Google_Chell.7820.121.0

Still happening after today's beta update
The cls are not in 53.0.2785.70. they are picked up in 53.0.2785.72. Please test 8530.63.0 and beyond.
Status: Fixed (was: Started)
Status: Verified (was: Fixed)
Verified on ChromeOS:8530.63.0/chrome:53.0.2785.72
HAHAHAHA well good to know it's verified now...any status? 
It will be fixed for you in the next beta push 
 Issue 640832  has been merged into this issue.

Comment 40 Deleted

Labels: M-55
Status: Assigned (was: Verified)
I'm seeing this again on samus.  Did it regress?

55.0.2878.0 (Official Build) dev (64-bit)
8861.0.0 (Official Build) dev-channel samus

(It only happens sometimes, by the way. I saw it on my second-to-last reboot, which was after an update was applied. It did not see it on my most recent reboot.)

Blockedon: 649360
The workaround we added in chromeos is a heuristic kind of solution, it is not completely bullet-proof. We are discussing to implement a more reliable fix with the help of cras API changes. See detailed discussion in crbug.com/649360.

Comment 44 by derat@chromium.org, Nov 17 2016

Mergedinto: 649360
Status: Duplicate (was: Assigned)

Sign in to add a comment