Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(856)

Issue 2133293003: [Chromecast] Make ALSA rendering delay either accurate or kNoTimestamp (Closed)

Created:
4 years, 5 months ago by kmackay
Modified:
4 years, 5 months ago
CC:
chromium-reviews, alokp+watch_chromium.org, lcwu+watch_chromium.org, halliwell+watch_chromium.org, feature-media-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[Chromecast] Make ALSA rendering delay either accurate or kNoTimestamp Update the ALSA backend so that the calculated rendering delay is either accurate, or kNoTimestamp. We no longer try to guess the correct delay timestamp. Note that this requires callers of GetRenderingDelay() to correctly handle the kNoTimestamp case. BUG= internal b/30034408 Committed: https://crrev.com/dcb4ac868340a62488a6f1d2d035d1fb57587745 Cr-Commit-Position: refs/heads/master@{#405893}

Patch Set 1 #

Total comments: 3

Patch Set 2 : Fix ALSA unit tests #

Unified diffs Side-by-side diffs Delta from patch set Stats (+76 lines, -43 lines) Patch
M chromecast/media/cma/backend/alsa/alsa_wrapper.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M chromecast/media/cma/backend/alsa/alsa_wrapper.cc View 1 1 chunk +4 lines, -0 lines 0 comments Download
M chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc View 1 chunk +1 line, -9 lines 0 comments Download
M chromecast/media/cma/backend/alsa/mock_alsa_wrapper.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M chromecast/media/cma/backend/alsa/mock_alsa_wrapper.cc View 1 5 chunks +22 lines, -1 line 0 comments Download
M chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc View 1 5 chunks +16 lines, -10 lines 0 comments Download
M chromecast/media/cma/backend/alsa/stream_mixer_alsa_input_impl.cc View 3 chunks +7 lines, -3 lines 0 comments Download
M chromecast/media/cma/backend/multizone_backend_unittest.cc View 3 chunks +24 lines, -20 lines 0 comments Download

Messages

Total messages: 21 (7 generated)
kmackay
4 years, 5 months ago (2016-07-08 22:09:55 UTC) #2
jameswest
lgtm
4 years, 5 months ago (2016-07-08 22:51:46 UTC) #4
tianyuwang1
Do you know how often kNoTimestamp happens to audio output? https://codereview.chromium.org/2133293003/diff/1/chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc File chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc (right): https://codereview.chromium.org/2133293003/diff/1/chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc#newcode80 ...
4 years, 5 months ago (2016-07-08 23:00:00 UTC) #5
kmackay
On 2016/07/08 23:00:00, tianyuwang1 wrote: > Do you know how often kNoTimestamp happens to audio ...
4 years, 5 months ago (2016-07-08 23:49:37 UTC) #6
kmackay
https://codereview.chromium.org/2133293003/diff/1/chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc File chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc (right): https://codereview.chromium.org/2133293003/diff/1/chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc#newcode80 chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc:80: last_known_delay_.timestamp_microseconds = kInvalidDelayTimestamp; On 2016/07/08 23:00:00, tianyuwang1 wrote: > ...
4 years, 5 months ago (2016-07-08 23:59:10 UTC) #7
tianyuwang1
On 2016/07/08 23:59:10, kmackay wrote: > https://codereview.chromium.org/2133293003/diff/1/chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc > File chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc (right): > > https://codereview.chromium.org/2133293003/diff/1/chromecast/media/cma/backend/alsa/audio_decoder_alsa.cc#newcode80 > ...
4 years, 5 months ago (2016-07-09 00:17:23 UTC) #8
tianyuwang1
lgtm Please test this change on device to make sure we handle kNoTimestamp correctly. Otherwise ...
4 years, 5 months ago (2016-07-09 00:18:46 UTC) #9
halliwell
On 2016/07/09 00:18:46, tianyuwang1 wrote: > lgtm > > Please test this change on device ...
4 years, 5 months ago (2016-07-09 02:28:02 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2133293003/1
4 years, 5 months ago (2016-07-14 22:42:04 UTC) #12
commit-bot: I haz the power
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linux/builds/191259)
4 years, 5 months ago (2016-07-14 22:57:00 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2133293003/20001
4 years, 5 months ago (2016-07-15 22:00:38 UTC) #17
commit-bot: I haz the power
Committed patchset #2 (id:20001)
4 years, 5 months ago (2016-07-15 23:12:11 UTC) #18
commit-bot: I haz the power
CQ bit was unchecked.
4 years, 5 months ago (2016-07-15 23:12:17 UTC) #19
commit-bot: I haz the power
4 years, 5 months ago (2016-07-15 23:14:20 UTC) #21
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/dcb4ac868340a62488a6f1d2d035d1fb57587745
Cr-Commit-Position: refs/heads/master@{#405893}

Powered by Google App Engine
This is Rietveld 408576698