Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2902823005:
Added logging of average power levels to AudioOutputController. (Closed)
|
Created:
3 years, 7 months ago by ossu-chromium Modified:
3 years, 6 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, xjz+watch_chromium.org, mfoltz+watch_chromium.org, miu+watch_chromium.org, Henrik Grunell Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded logging of average power levels to AudioOutputController.
BUG=chromium:714119
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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/2902823005
Cr-Commit-Position: refs/heads/master@{#476259}
Committed: https://chromium.googlesource.com/chromium/src/+/05a9ef01217b8c47c69f5d3960d63cfab1c673b0
Patch Set 1 #
Total comments: 4
Patch Set 2 : Added logging on stream close, otherwise short streams would log no average power. #Patch Set 3 : Rebase #
Depends on Patchset: Messages
Total messages: 30 (21 generated)
Description was changed from ========== Added logging of average power levels to AudioOutputController. BUG=chromium:714119 ========== to ========== Added logging of average power levels to AudioOutputController. BUG=chromium:714119 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 ==========
The CQ bit was checked by ossu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ossu@chromium.org changed reviewers: + henrika@chromium.org
This is a draft of how I'd log the output level similarly to what we're doing on the input side. Since you wrote that bit, and also changed it from using AudioPowerMonitor, I'd like your input on this. Early testing looks pretty good, but I'm having trouble with my local chrome build, so I'll have to confirm more after the long weekend.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks like useful stuff to me. Hence lgtm. However, I am not an owner in this area. Sorry.
Description was changed from ========== Added logging of average power levels to AudioOutputController. BUG=chromium:714119 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 ========== to ========== Added logging of average power levels to AudioOutputController. BUG=chromium:714119 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 ==========
ossu@chromium.org changed reviewers: + liberato@chromium.org
+liberato for owner review here too. This CL adds some more logging to AudioOutputController, to bring it on par with AudioInputController to help us troubleshoot audio issues.
lgtm thanks -fl https://codereview.chromium.org/2902823005/diff/1/media/audio/audio_output_co... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/2902823005/diff/1/media/audio/audio_output_co... media/audio/audio_output_controller.cc:173: last_audio_level_log_time_ = base::TimeTicks::Now(); i think that this will be default-constructed to 0, which should work fine for the check @303.
Added an extra logging call in StopStream, so that we get some info from short (<15s) streams as well. https://codereview.chromium.org/2902823005/diff/1/media/audio/audio_output_co... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/2902823005/diff/1/media/audio/audio_output_co... media/audio/audio_output_controller.cc:173: last_audio_level_log_time_ = base::TimeTicks::Now(); On 2017/05/30 17:00:53, liberato wrote: > i think that this will be default-constructed to 0, which should work fine for > the check @303. Acknowledged. https://codereview.chromium.org/2902823005/diff/1/media/audio/audio_output_co... media/audio/audio_output_controller.cc:173: last_audio_level_log_time_ = base::TimeTicks::Now(); On 2017/05/30 17:00:53, liberato wrote: > i think that this will be default-constructed to 0, which should work fine for > the check @303. It needs to be reset to something in DoPlay, and Now() seems as good a value as any, to me.
The CQ bit was checked by ossu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by ossu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2902823005/diff/1/media/audio/audio_output_co... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/2902823005/diff/1/media/audio/audio_output_co... media/audio/audio_output_controller.cc:173: last_audio_level_log_time_ = base::TimeTicks::Now(); On 2017/05/31 11:44:40, ossu-chromium wrote: > On 2017/05/30 17:00:53, liberato wrote: > > i think that this will be default-constructed to 0, which should work fine for > > the check @303. > > It needs to be reset to something in DoPlay, and Now() seems as good a value as > any, to me. ah -- i mis-read the diff. yeah, you're right. anyway, one can drop the {} for single-line if, here and elsewhere.
+cc grunell
The CQ bit was checked by ossu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrika@chromium.org Link to the patchset: https://codereview.chromium.org/2902823005/#ps60001 (title: "Rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496320255782910,
"parent_rev": "3fc1f818eaab417801a21886ce9c293701b10486", "commit_rev":
"05a9ef01217b8c47c69f5d3960d63cfab1c673b0"}
Message was sent while issue was closed.
Description was changed from ========== Added logging of average power levels to AudioOutputController. BUG=chromium:714119 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 ========== to ========== Added logging of average power levels to AudioOutputController. BUG=chromium:714119 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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/2902823005 Cr-Commit-Position: refs/heads/master@{#476259} Committed: https://chromium.googlesource.com/chromium/src/+/05a9ef01217b8c47c69f5d3960d6... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/05a9ef01217b8c47c69f5d3960d6... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
