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

Issue 753596 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Task

Blocking:
issue 807481



Sign in to add a comment

Avoid keeping the system awake for active-but-unused audio output streams

Project Member Reported by derat@chromium.org, Aug 8 2017

Issue description

As discussed in http://listnr/product/208/report/69782965302, http://doc/1pjvvAkLVoAzsu1fdfQkKmEN-KT3Z1TW7PGNzKVrdfdI/edit?ts=598a320a#heading=h.xwpy6kp8e6i8, and countless email threads and bugs, we have problems with powerd keeping the system awake due to audio streams that aren't actually playing any audible sound (there have been various causes for this).

I'm filing this to track figuring out some way to avoid counting these streams as active, since we're currently playing whack-a-mole dealing with this.

Dylan said, "We can monitor the audio levels of playing streams, but it isn't computationally cheap." over email. Is that the case for any sort of monitoring? Can we find some cheap way to just report the last time that a sample was received, for instance?
 
Most of these issues are about streams that are actively playing zeros.  So we get samples all the time, they all happen to be zero though.

To do this correctly we can either go through each sample and remember the last time we say one that != 0 or measure the power of the output and keep track of how long it has been below a given threshold.
The active-but-unused stream issue might be this one https://bugs.chromium.org/p/chromium/issues/detail?id=732450.

I don't think we should check what user plays to workaround the unused stream issue in Chrome.
Besides, it is legitimate for user to play a stream which contains only zeros. For example, user can mute audio in Youtube's UI when playing video.
Thanks!


Comment 3 by derat@chromium.org, Aug 9 2017

#2: We've had issues like this for years. Do you have an alternate proposal for how we can prevent this from happening again?

Does Chrome already have knowledge about the last time it wrote a nonzero sample to a given stream? If so, could it communicate that information to CRAS?
I don't think chrome goes through each sample currently.

I'm OK suspending a stream of zeros if the screen is off. I can't think of a case where we need to stay awake to display nothing and play nothing.

Comment 5 by derat@chromium.org, Aug 9 2017

I'm having trouble parsing this and suspect a typo: "I'm OK suspending a stream of zeros if the screen is off." :-)

Do you mean that you're okay with us suspending if the stream is just getting zeros and the screen is off? If so, do you have suggestions on how powerd can detect that case? I don't think it's able to get anything beyond the number of open streams right now.

Comment 6 by dgreid@chromium.org, Aug 10 2017

Yes that's what I meant. I'm OK with if from a user experience standpoint.  I don't have a solution in my head that doesn't involve iterating across a lot of samples. We don't have that information anywhere as far as I know and something would have to figure it out.

Comment 7 by snanda@chromium.org, Aug 10 2017

Do we use system wide mute as a hint already? My devices are generally on mute and unmuted only when I am actually listening to audio.

Comment 8 by derat@chromium.org, Aug 10 2017

We don't, although we probably could. I suspect it would help you (and me, and others on the team who use their devices in the office), but maybe not the typical user so much. :-)
I also mute often. But on systems that are more touchscreen oriented, I don't tend to mute nearly as often (a keyboard mute button is much easier and predictable than volume buttons).

Yay, anecdata!
[I wrote this earlier but forgot to submit?]

FWIW, it seems like something that doesn't need to be computed in most cases. It would only need to be computed if all other "keep awake" sources are off for long enough. And if it's really expensive, it could even use a large backoff, to minimize the cost over time (e.g., for playing music with the screen off).

Comment 11 by derat@chromium.org, Aug 10 2017

Cc: cychiang@chromium.org
I don't think that this is something we can just compute when we're considering suspending the system. CRAS can't know when the last "real" activity was unless it inspects all audio samples all the time (or am I misunderstanding what you're suggesting?).

Maybe we can sample the samples, though. Dylan/Jimmy, how often do you think we'd incorrectly identify streams as inactive and suspend the system if CRAS were to only check, say, one sample every ten seconds? powerd doesn't suspend immediately when audio stops, so even if we were unlucky and checked an empty sample that happened to fall between two songs, it feels like we'd be likely to get a nonzero sample the next time. Curious as to what you think.
Re#3: I see. If screen is on powerd will not suspend the system. Sorry I forgot that. The example of playing muted youtube is invalid.


Re#11: Do sampling sounds like a good idea. We can check the first sample in a block (e.g. 1 block = 480 frames) and see if that is 0. If this holds true for 30 seconds, then we can strongly believe this stream is inactive.


Apart from this, we can simplify the information between powerd and CRAS if powerd only asks if audio is active.
This would simplify how CRAS checks whether audio is active.

For example, CRAS can check the samples when it writes the samples to device. This happens after mixing streams so it is only once per device (usually, only one) instead of once per stream.

CRAS will keep track of number of active output device in cras_system_state. When powerd asks CRAS whether audio is active, CRAS then checks number of active output device and number of active input streams to answer that question.

Does this sound good to you ?

Thanks!
Cc: louiscollard@google.com
Polling the samples periodically is better for power.  It doesn't help our worst-case callback run time though, and with audio we are real-time and worst case is the only one that really matters.

The other option might be to better blame apps and webpages so users know what to close.  That's how android handles this.

If we need to go through samples, I think we can fit it in.  It's an incremental cost when playing to speakers as we already go through the samples to apply the EQ.
Cc: tbroch@chromium.org
c#8: do we have any UMA data that shows audio muted vs unmuted distribution?

I often see cras consuming a few percent of CPU cycles even if audio is muted. In such cases can we drop the audio stream higher up in the stack?

(to be clear, this suggestion isn't instead of the sampling etc. approaches being proposed, but in addition to it)
> I don't think that this is something we can just compute when we're considering suspending the system. CRAS can't know when the last "real" activity was unless it inspects all audio samples all the time (or am I misunderstanding what you're suggesting?).

I was only saying that if sampling is too expensive for doing "all the time" (which I thought was a previous proposal, for some definition of "all the time"?), then:
(a) powerd could only ask CRAS for sampling-based data once it's reasonably confident CRAS is the last man standing and
(b) CRAS could only start collecting this information once powerd asks for it at least once (and stop once powerd sees some other reason to stay "up")

That would mean you only add sampling costs while the screen is off and there is some active audio. It also might add more latency to the decision on when to suspend, if it takes a lot of lead time to figure out whether the audio stream is *really* silent.

All of that may be over-complex, as I don't really know the cost tradeoffs in CRAS.
Labels: -Pri-2 ReleaseBlock-Beta M-61 Pri-1

Comment 18 by derat@chromium.org, Aug 11 2017

Cc: yungleem@chromium.org
Labels: -Pri-1 -ReleaseBlock-Beta -M-61 M-62 Pri-2
This is just a discussion about how to prevent future problems. It'll be non-trivial to do (both CRAS and powerd will probably need to be changed), and it'll likely require some experimentation to balance CPU usage with accuracy. It's doesn't seem like something that should block M61 from going to the beta channel, unless there's some more context that's not included here.
Louis will be working on this. But unfortunately he does not have chromium.org account yet so I can not set the owner to him.

As Dylan suggested in #14, the cost of checking every sample is incremental since we still need to compute every sample in EQ, software volume, and stream mixing.

Let's proceed this with:

1. Add a dBus method IsAudioActive.
2. Check every sample when writing to device. This should be a good place after applying dsp and software volume: https://cs.corp.google.com/chromeos_public/src/third_party/adhd/cras/src/server/cras_iodev.c?type=cs&q=cras_scale_buffer+cras_iodev+package:%5Echromeos_public$&l=944

Then, we can compute time increment for the cost of CRAS.
If the cost does not look good, we can consider sampling, or more complicated mechanism like Brian suggested in #16.

Thanks!

Comment 20 by derat@chromium.org, Aug 14 2017

Thanks!

In addition to adding an IsAudioActive method that powerd can call at startup to get the initial state, please also emit a D-Bus signal when audio becomes active or inactive. It's fine to rate-limit it or delay it if necessary to avoid spamminess (e.g. only emit the "inactive" signal when audio has been inactive for a few seconds), but powerd needs a way to hear about changes to the state without polling.
sgtm, there is already code to calc RMS in the test client, we can probably optimize and re-use that to start.
Owner: louiscollard@chromium.org
Status: Assigned (was: Untriaged)

Comment 23 by derat@chromium.org, Jan 14 2018

Owner: dgreid@chromium.org
Any updates? It doesn't look like Louis works on audio anymore.
Owner: louiscollard@chromium.org
Status: Started (was: Assigned)
Louis has posted the second patch before he moved to TPE and join us.
https://chromium-review.googlesource.com/#/c/chromiumos/third_party/adhd/+/643211/
I will get this reviewed.
FWIW, that URL gives me a 404 error.
The change is private, hence the 404. I will get it fixed up and submitted ASAP.
Labels: Hotlist-FixingTouch
Any update? I'm going to assume no, since I still can't access your CL :)

Anyway, I just experienced another "system didn't sleep due to audio activity" [1]. This time, I couldn't even pinpoint what was allegedly playing audio. I don't think I have any webpages open that even *used* to be playing audio or video.

[1] https://listnr.corp.google.com/report/84997811969
Labels: -Hotlist-FixingTouch Hotlist-Fixing-touch
Re#27
It might be related to  issue 799727 .
Recently we found that CRAS can not report a correct number of active stream.
Still investigating...
I abandoned the original change in favour of a simpler design:
https://chromium-review.googlesource.com/c/chromiumos/third_party/adhd/+/888185

Hoping to get it landed this week.
Blocking: 807481
Project Member

Comment 32 by bugdroid1@chromium.org, Feb 14 2018

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

commit 5094a7a806a203d1c84abfcda3f9e9df375c7ad4
Author: Louis Collard <louiscollard@chromium.org>
Date: Wed Feb 14 12:07:11 2018

CRAS: Add system-level audio active state.

Adds a binary flag to system state that denotes whether CRAS currently
has any streams that are active, and also playing/capturing audio that
is not empty (just zeros).

This will be used by powerd to allow the device to sleep if there are
active audio streams, but those streams are not actually producing any
sound (for example, a misbehaving ad creative).

BUG= chromium:753596 
TEST=Ran locally on eve

Change-Id: Ie56a4460a6e56bbb757d866410717b98770b97ea
Reviewed-on: https://chromium-review.googlesource.com/888185
Commit-Ready: Louis Collard <louiscollard@chromium.org>
Tested-by: Louis Collard <louiscollard@chromium.org>
Reviewed-by: Cheng-Yi Chiang <cychiang@chromium.org>

[modify] https://crrev.com/5094a7a806a203d1c84abfcda3f9e9df375c7ad4/cras/src/server/cras_system_state.h
[modify] https://crrev.com/5094a7a806a203d1c84abfcda3f9e9df375c7ad4/cras/src/server/cras_system_state.c
[modify] https://crrev.com/5094a7a806a203d1c84abfcda3f9e9df375c7ad4/cras/src/common/cras_types.h

Project Member

Comment 33 by bugdroid1@chromium.org, Feb 28 2018

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

commit c4dd97d413aa90de3542d584722f9bc073ad695b
Author: Louis Collard <louiscollard@chromium.org>
Date: Wed Feb 28 04:17:33 2018

CRAS: Add dbus interface for active status

The signals added here will be used to tell powerd when the state
changes, so that it knows whether it can put the device to sleep.

The method added to the interface will be used by powerd at startup to
query the current status.

BUG= chromium:753596 
TEST=Ran locally on eve

Change-Id: Icff42f7d71ea7bede2187a8d74682f1fad4b6178
Reviewed-on: https://chromium-review.googlesource.com/888186
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Louis Collard <louiscollard@chromium.org>
Reviewed-by: Cheng-Yi Chiang <cychiang@chromium.org>

[modify] https://crrev.com/c4dd97d413aa90de3542d584722f9bc073ad695b/cras/src/server/cras_observer.h
[modify] https://crrev.com/c4dd97d413aa90de3542d584722f9bc073ad695b/cras/src/server/cras_observer.c
[modify] https://crrev.com/c4dd97d413aa90de3542d584722f9bc073ad695b/cras/src/common/cras_observer_ops.h
[modify] https://crrev.com/c4dd97d413aa90de3542d584722f9bc073ad695b/cras/src/tests/observer_unittest.cc
[modify] https://crrev.com/c4dd97d413aa90de3542d584722f9bc073ad695b/cras/src/server/cras_dbus_control.c
[modify] https://crrev.com/c4dd97d413aa90de3542d584722f9bc073ad695b/cras/README.dbus-api

Project Member

Comment 34 by bugdroid1@chromium.org, Mar 1 2018

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

commit 7d6961f890b329b6df9d54660edc5a43d8f2ca4b
Author: Louis Collard <louiscollard@chromium.org>
Date: Thu Mar 01 07:54:28 2018

CRAS: Add messaging between audio and main thread for audio active state.

The audio thread will check whether active streams are currently empty,
and needs to notify the main thread of changes in state so that it can
be updated, and send a dbus signal.

BUG= chromium:753596 
TEST=Ran locally on eve

Change-Id: I94faa66a0ec0675c7f58f5a07a4176b461e4fe4a
Reviewed-on: https://chromium-review.googlesource.com/888187
Commit-Ready: Louis Collard <louiscollard@chromium.org>
Tested-by: Louis Collard <louiscollard@chromium.org>
Reviewed-by: Dylan Reid <dgreid@chromium.org>

[modify] https://crrev.com/7d6961f890b329b6df9d54660edc5a43d8f2ca4b/cras/src/server/cras_main_message.h
[modify] https://crrev.com/7d6961f890b329b6df9d54660edc5a43d8f2ca4b/cras/src/server/cras_server.c
[modify] https://crrev.com/7d6961f890b329b6df9d54660edc5a43d8f2ca4b/cras/src/Makefile.am
[add] https://crrev.com/7d6961f890b329b6df9d54660edc5a43d8f2ca4b/cras/src/server/cras_non_empty_audio_handler.h
[add] https://crrev.com/7d6961f890b329b6df9d54660edc5a43d8f2ca4b/cras/src/server/cras_non_empty_audio_handler.c

Project Member

Comment 35 by bugdroid1@chromium.org, Mar 6 2018

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

commit ab780d12ed5984ef0981d6d87ea32368ad3cfdd0
Author: Louis Collard <louiscollard@chromium.org>
Date: Tue Mar 06 13:07:29 2018

CRAS: Monitor and update audio status when necessary.

The audio thread needs to check the contents of currently active
streams to see if they contain any audio.

BUG= chromium:753596 
TEST=Ran locally on eve

Change-Id: I05402db981c881222bfc287f13c43871531cf88e
Reviewed-on: https://chromium-review.googlesource.com/888188
Commit-Ready: Louis Collard <louiscollard@chromium.org>
Tested-by: Louis Collard <louiscollard@chromium.org>
Reviewed-by: Dylan Reid <dgreid@chromium.org>

[modify] https://crrev.com/ab780d12ed5984ef0981d6d87ea32368ad3cfdd0/cras/src/tests/iodev_stub.cc
[add] https://crrev.com/ab780d12ed5984ef0981d6d87ea32368ad3cfdd0/cras/src/tests/polled_interval_checker_unittest.cc
[modify] https://crrev.com/ab780d12ed5984ef0981d6d87ea32368ad3cfdd0/cras/src/server/dev_io.h
[modify] https://crrev.com/ab780d12ed5984ef0981d6d87ea32368ad3cfdd0/cras/src/server/dev_io.c
[modify] https://crrev.com/ab780d12ed5984ef0981d6d87ea32368ad3cfdd0/cras/src/server/cras_iodev.h
[modify] https://crrev.com/ab780d12ed5984ef0981d6d87ea32368ad3cfdd0/cras/src/tests/iodev_unittest.cc
[add] https://crrev.com/ab780d12ed5984ef0981d6d87ea32368ad3cfdd0/cras/src/server/polled_interval_checker.c
[modify] https://crrev.com/ab780d12ed5984ef0981d6d87ea32368ad3cfdd0/cras/src/tests/audio_thread_unittest.cc
[modify] https://crrev.com/ab780d12ed5984ef0981d6d87ea32368ad3cfdd0/cras/src/Makefile.am
[add] https://crrev.com/ab780d12ed5984ef0981d6d87ea32368ad3cfdd0/cras/src/server/polled_interval_checker.h
[modify] https://crrev.com/ab780d12ed5984ef0981d6d87ea32368ad3cfdd0/cras/src/server/cras_iodev.c
[add] https://crrev.com/ab780d12ed5984ef0981d6d87ea32368ad3cfdd0/cras/src/tests/empty_audio_stub.cc

Thanks for your work on this, Louis! I've played around a bit with this (both with updating powerd to observe the new signal and with watching the signals manually), and I want to make sure that the behavior I see is expected.

I'm running this as root on a device to observe CRAS's D-Bus signals:

  dbus-monitor --system "type='signal',sender='org.chromium.cras'"

When I open a single tab and open a YouTube video (which starts playing immediately), I see the expected NumberOfActiveStreamsChanged and AudioOutputActiveStateChanged signals:

signal time=1520471709.466243 sender=:1.16 -> destination=(null destination) serial=91 path=/org/chromium/cras; interface=org.chromium.cras.Control; member=NumberOfActiveStreamsChanged
   int32 1
signal time=1520471709.466268 sender=:1.16 -> destination=(null destination) serial=92 path=/org/chromium/cras; interface=org.chromium.cras.Control; member=AudioOutputActiveStateChanged
   boolean true

After I pause the video, I see a NumberOfActiveStreamsChanged signal going to 0 about 10 seconds later (presumably YouTube is holding the stream open):

signal time=1520471734.534358 sender=:1.16 -> destination=(null destination) serial=95 path=/org/chromium/cras; interface=org.chromium.cras.Control; member=NumberOfActiveStreamsChanged
   int32 0

However, I don't see the AudioOutputActiveStateChanged signal reporting that output has stopped until 10 more seconds after that:

signal time=1520471744.548119 sender=:1.16 -> destination=(null destination) serial=97 path=/org/chromium/cras; interface=org.chromium.cras.Control; member=AudioOutputActiveStateChanged
   boolean false

I know that a periodic-sampling-based approach was discussed above, but it's unclear to me if that was actually used or if CRAS is looking at every sample. Periodic sampling (or a desire to avoid spamminess) may be the reason for the 10-second delay that I'm seeing between the stream getting closed and the output state changing.

I can make powerd continue to monitor the stream count and require both a nonzero count and an active output state in order to consider audio as being active, but it may be cleaner if CRAS immediately reported output as inactive when there aren't any streams open.

Thoughts?
Thanks for playing around with it!

The behaviour you're seeing is expected, though of course happy to change it if it's not suitable.

The AudioOutputActiveStateChanged signal is based only on the device activity, and does not take into account anything related to streams. Assuming a single device for simplicity, the signal will be sent when either:
- the device has been playing empty audio for 30 seconds (could be an active but empty stream, or no streams)
- the device was closed, and it played non-empty audio in the 30 seconds before it was closed 

In addition, for bonus fun, if cras detects a device is playing non-empty audio (eg YT is playing) it won't check again for 5 seconds. So if eg you mute youtube but leave it playing (so there's an active but empty stream), you should get the state change signal 30-35 seconds after you mute.

Sorry that's a bit complicated, and happy to adjust the numbers if you think that would make sense; the goal was to minimise CPU usage and avoid spammy signals, while still providing timely enough signals to save battery.

I would prefer to avoiding including a concept of streams in the calculation of the AudioOutputActiveStateChanged, just to keep the code complexity down, however I think the same effect could be achieved by reducing the length of consecutive empty audio required for cras to decide the device is idle.

I think powerd can/should just use the new AudioOutputActiveStateChanged signal, and ignore the number of stream counts; if waiting 30 seconds for the signal is not acceptable then we can adjust that. I guess I was imagining that eg the screen will remain on for at least that long after an interaction (eg pressing pause on YT), so by the time it's relevant the signal will have been sent - I have no idea about how this all works though so happy to do whatever you think is best :)

What are your thoughts? How would you like to proceed?

Project Member

Comment 38 by bugdroid1@chromium.org, Mar 27 2018

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

commit 0e225e6d6513b07bf0c285a10523592ca631301f
Author: Daniel Erat <derat@chromium.org>
Date: Tue Mar 27 14:12:46 2018

system_api: Add CRAS output-active constants.

Add constants for CRAS's IsAudioOutputActive D-Bus method
and AudioOutputActiveStateChanged signal.

BUG= chromium:753596 
TEST=built it

Change-Id: I040fe96e197b3360291502cf2abe93828fdf99ee
Reviewed-on: https://chromium-review.googlesource.com/976981
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Louis Collard <louiscollard@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Reviewed-by: Cheng-Yi Chiang <cychiang@chromium.org>

[modify] https://crrev.com/0e225e6d6513b07bf0c285a10523592ca631301f/dbus/service_constants.h

Comment 39 by derat@chromium.org, Mar 31 2018

Owner: derat@chromium.org
Thanks for the details (and sorry for my slow reply). The current CRAS behavior seems reasonable; in the worst case powerd can just continue to observe stream counts as well. I'm doing some refactoring in powerd to simplify its D-Bus code that'll make the changes here a bit cleaner.
Project Member

Comment 40 by bugdroid1@chromium.org, Apr 3 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/1348ba0454a446e38711e2ccbb7880bddc68d24f

commit 1348ba0454a446e38711e2ccbb7880bddc68d24f
Author: Daniel Erat <derat@chromium.org>
Date: Tue Apr 03 03:13:00 2018

power: Watch for D-Bus name owner changes in DBusWrapper.

Make system::DBusWrapper watch for dbus-daemon's
NameOwnerChanged signals so that the too-big Daemon class
doesn't need to do it itself.

With this change, Daemon is still notifying other classes
about service restarts, but followup changes will move more
of this logic directly into the destination classes.

BUG= chromium:753596 
TEST=unit tests pass; also deployed to a device and verified
     that "D-Bus org.chromium.______ ownership changed"
     messages are still written to
     /var/log/power_manager/powerd.LATEST

Change-Id: I13058e2504165db1a47bea4d566eacb00316ee32
Reviewed-on: https://chromium-review.googlesource.com/989214
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Eric Caruso <ejcaruso@chromium.org>

[modify] https://crrev.com/1348ba0454a446e38711e2ccbb7880bddc68d24f/power_manager/powerd/system/dbus_wrapper_stub.h
[modify] https://crrev.com/1348ba0454a446e38711e2ccbb7880bddc68d24f/power_manager/powerd/system/dbus_wrapper.cc
[modify] https://crrev.com/1348ba0454a446e38711e2ccbb7880bddc68d24f/power_manager/powerd/system/dbus_wrapper.h
[modify] https://crrev.com/1348ba0454a446e38711e2ccbb7880bddc68d24f/power_manager/powerd/system/dbus_wrapper_stub.cc
[modify] https://crrev.com/1348ba0454a446e38711e2ccbb7880bddc68d24f/power_manager/powerd/daemon.cc
[modify] https://crrev.com/1348ba0454a446e38711e2ccbb7880bddc68d24f/power_manager/powerd/daemon.h
[modify] https://crrev.com/1348ba0454a446e38711e2ccbb7880bddc68d24f/power_manager/powerd/daemon_unittest.cc

Project Member

Comment 41 by bugdroid1@chromium.org, Apr 5 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/1451637548baabc8079a99d86485ace637142948

commit 1451637548baabc8079a99d86485ace637142948
Author: Daniel Erat <derat@chromium.org>
Date: Thu Apr 05 01:52:02 2018

power: Move CRAS D-Bus code into AudioClient.

Move code for watching for CRAS restarts and registering for
its D-Bus signals out of the Daemon class and into
AudioClient, the class that actually consumes this
information.

Also update DBusWatcherStub to support notifying observers
about name ownership changes and handling outgoing D-Bus
method calls from powerd, and add unit tests to exercise
AudioClient's D-Bus code.

BUG= chromium:753596 , chromium:828151 
TEST=unit tests pass; also watched
     /var/log/power_manager/powerd.LATEST and verified that
     audio activity start/stop and headphone plug/unplug
     events are reported

Change-Id: If315bf85cf1646893d6be85ca9adfc7c031ef973
Reviewed-on: https://chromium-review.googlesource.com/991197
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Eric Caruso <ejcaruso@chromium.org>

[modify] https://crrev.com/1451637548baabc8079a99d86485ace637142948/power_manager/powerd/system/dbus_wrapper_stub.h
[modify] https://crrev.com/1451637548baabc8079a99d86485ace637142948/power_manager/powerd/system/audio_client.cc
[modify] https://crrev.com/1451637548baabc8079a99d86485ace637142948/power_manager/power_manager.gyp
[modify] https://crrev.com/1451637548baabc8079a99d86485ace637142948/power_manager/powerd/system/audio_client_interface.h
[modify] https://crrev.com/1451637548baabc8079a99d86485ace637142948/power_manager/powerd/system/dbus_wrapper_stub.cc
[modify] https://crrev.com/1451637548baabc8079a99d86485ace637142948/power_manager/powerd/daemon.cc
[modify] https://crrev.com/1451637548baabc8079a99d86485ace637142948/power_manager/powerd/system/input_watcher_unittest.cc
[add] https://crrev.com/1451637548baabc8079a99d86485ace637142948/power_manager/powerd/system/audio_client_unittest.cc
[modify] https://crrev.com/1451637548baabc8079a99d86485ace637142948/power_manager/powerd/system/audio_client.h
[modify] https://crrev.com/1451637548baabc8079a99d86485ace637142948/power_manager/powerd/daemon.h
[modify] https://crrev.com/1451637548baabc8079a99d86485ace637142948/power_manager/powerd/daemon_unittest.cc
[modify] https://crrev.com/1451637548baabc8079a99d86485ace637142948/power_manager/powerd/system/audio_client_stub.cc
[modify] https://crrev.com/1451637548baabc8079a99d86485ace637142948/power_manager/powerd/system/audio_client_stub.h

The last powerd change for this is out for review at https://crrev.com/c/1000513.
Project Member

Comment 43 by bugdroid1@chromium.org, Apr 7 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/6ae036323b3655d57e88723b9a637e16483a20c1

commit 6ae036323b3655d57e88723b9a637e16483a20c1
Author: Daniel Erat <derat@chromium.org>
Date: Sat Apr 07 02:50:53 2018

power: Call CRAS D-Bus methods asynchronously.

Update powerd's AudioClient class to call CRAS's GetNodes
and GetNumberOfActiveOutputStreams D-Bus methods
asynchronously rather than synchronously. I don't think
there's any reason to block on these; it was just easier to
do given the poor state of Chrome OS D-Bus libraries when
this code was originally written.

Also simplify D-Bus signal registration.

BUG= chromium:753596 
TEST=updated unit tests pass

Change-Id: I34844c5183b1f8dca013e16fe49a3797108e05b6
Reviewed-on: https://chromium-review.googlesource.com/999116
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Eric Caruso <ejcaruso@chromium.org>

[modify] https://crrev.com/6ae036323b3655d57e88723b9a637e16483a20c1/power_manager/powerd/system/audio_client.h
[modify] https://crrev.com/6ae036323b3655d57e88723b9a637e16483a20c1/power_manager/powerd/system/audio_client_unittest.cc
[modify] https://crrev.com/6ae036323b3655d57e88723b9a637e16483a20c1/power_manager/powerd/system/audio_client.cc
[modify] https://crrev.com/6ae036323b3655d57e88723b9a637e16483a20c1/power_manager/powerd/system/dbus_wrapper_stub.cc

Project Member

Comment 44 by bugdroid1@chromium.org, Apr 7 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/771d1ae5afac32eb9e91ec5996afdcd07b079843

commit 771d1ae5afac32eb9e91ec5996afdcd07b079843
Author: Daniel Erat <derat@chromium.org>
Date: Sat Apr 07 05:00:22 2018

power: Track whether audio output is active or not.

In addition to following the number of open output streams
reported by CRAS, make powerd keep track of whether there
are any streams with nonzero data getting sent to them.

BUG= chromium:753596 
TEST=updated unit tests; also verified that when i create
     an <audio> element, make it start playing, and then set
     its volume to 0, powerd reports that audio activity
     stops about 30 seconds later

Change-Id: If466b75321371f2dbf8a6a3c08d9db75b0189496
Reviewed-on: https://chromium-review.googlesource.com/1000513
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/771d1ae5afac32eb9e91ec5996afdcd07b079843/power_manager/powerd/system/audio_client.h
[modify] https://crrev.com/771d1ae5afac32eb9e91ec5996afdcd07b079843/power_manager/powerd/system/audio_client_unittest.cc
[modify] https://crrev.com/771d1ae5afac32eb9e91ec5996afdcd07b079843/power_manager/powerd/system/audio_client.cc

Status: Fixed (was: Started)

Sign in to add a comment