Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
531523002:
Convert channel layout for cras_input (Closed)
DescriptionConvert channel layout for cras_input
BUG=393908
TEST=None
Committed: https://crrev.com/a7ae647d8f23b07f89cb6b1e67e095d457e94337
Cr-Commit-Position: refs/heads/master@{#293438}
Patch Set 1 #Patch Set 2 : Added comment to channel conversion #
Total comments: 2
Patch Set 3 : Fix comments #
Total comments: 11
Patch Set 4 : Fix comments #Messages
Total messages: 23 (10 generated)
hychao@chromium.org changed reviewers: + grunell@chromium.org, henrika@chromium.org, tommi@chromium.org
dgreid@chromium.org changed reviewers: + dgreid@chromium.org
lgtm % nits https://codereview.chromium.org/531523002/diff/20001/media/audio/cras/cras_in... File media/audio/cras/cras_input.cc (right): https://codereview.chromium.org/531523002/diff/20001/media/audio/cras/cras_in... media/audio/cras/cras_input.cc:142: // Inivialize channel layout to all -1 to indicate that non of none https://codereview.chromium.org/531523002/diff/20001/media/audio/cras/cras_in... media/audio/cras/cras_input.cc:149: // for channels that does not present in params_.channel_layout(). s/does/are/
The CQ bit was checked by hychao@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hychao@chromium.org/531523002/40001
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
hychao@chromium.org changed reviewers: + dalecurtis@chromium.org
henrika@chromium.org changed reviewers: - henrika@chromium.org
https://codereview.chromium.org/531523002/diff/40001/media/audio/cras/cras_in... File media/audio/cras/cras_input.cc (right): https://codereview.chromium.org/531523002/diff/40001/media/audio/cras/cras_in... media/audio/cras/cras_input.cc:106: // corresponding source in Chromium defined Channels. is there a way to guarantee at compile time that this is correct? https://codereview.chromium.org/531523002/diff/40001/media/audio/cras/cras_in... media/audio/cras/cras_input.cc:145: for (size_t i = 0; i < CRAS_CH_MAX; i++) ++i use arraysize() instead of CRAS_CH_MAX here. https://codereview.chromium.org/531523002/diff/40001/media/audio/cras/cras_in... media/audio/cras/cras_input.cc:150: for (size_t i = 0; i < arraysize(kChannelMap); ++i) nit: use {} https://codereview.chromium.org/531523002/diff/40001/media/audio/cras/cras_in... media/audio/cras/cras_input.cc:153: if (cras_audio_format_set_channel_layout(audio_format, layout)) { nit: != 0 https://codereview.chromium.org/531523002/diff/40001/media/audio/cras/cras_in... media/audio/cras/cras_input.cc:154: LOG(WARNING) << "Error setting channel layout."; would PLOG make sense?
dalecurtis@chromium.org changed reviewers: - dalecurtis@chromium.org
Looks like tommi@ has this, so consider me the peanut gallery -- removing myself from reviewers. https://codereview.chromium.org/531523002/diff/40001/media/audio/cras/cras_in... File media/audio/cras/cras_input.cc (right): https://codereview.chromium.org/531523002/diff/40001/media/audio/cras/cras_in... media/audio/cras/cras_input.cc:106: // corresponding source in Chromium defined Channels. On 2014/09/03 13:28:29, tommi wrote: > is there a way to guarantee at compile time that this is correct? +1, a COMPILE_ASSERT() would be great here.
https://codereview.chromium.org/531523002/diff/40001/media/audio/cras/cras_in... File media/audio/cras/cras_input.cc (right): https://codereview.chromium.org/531523002/diff/40001/media/audio/cras/cras_in... media/audio/cras/cras_input.cc:106: // corresponding source in Chromium defined Channels. On 2014/09/03 13:28:29, tommi wrote: > is there a way to guarantee at compile time that this is correct? Added an assert here for channel map size. I can't think of a good way to assert the correctness of each mapping, other than use a big switch to map the channels. https://codereview.chromium.org/531523002/diff/40001/media/audio/cras/cras_in... media/audio/cras/cras_input.cc:145: for (size_t i = 0; i < CRAS_CH_MAX; i++) On 2014/09/03 13:28:29, tommi wrote: > ++i > > use arraysize() instead of CRAS_CH_MAX here. Done. https://codereview.chromium.org/531523002/diff/40001/media/audio/cras/cras_in... media/audio/cras/cras_input.cc:150: for (size_t i = 0; i < arraysize(kChannelMap); ++i) On 2014/09/03 13:28:29, tommi wrote: > nit: use {} Done. https://codereview.chromium.org/531523002/diff/40001/media/audio/cras/cras_in... media/audio/cras/cras_input.cc:153: if (cras_audio_format_set_channel_layout(audio_format, layout)) { On 2014/09/03 13:28:29, tommi wrote: > nit: != 0 Done. https://codereview.chromium.org/531523002/diff/40001/media/audio/cras/cras_in... media/audio/cras/cras_input.cc:154: LOG(WARNING) << "Error setting channel layout."; On 2014/09/03 13:28:29, tommi wrote: > would PLOG make sense? Change to use DLOG to be consistent with other warnings in this file.
lgtm
The CQ bit was checked by hychao@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hychao@chromium.org/531523002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was checked by hychao@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hychao@chromium.org/531523002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as 4be6477d4a9bcbd2bc76b9dd7cf6c466c3ef5ed0
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/a7ae647d8f23b07f89cb6b1e67e095d457e94337 Cr-Commit-Position: refs/heads/master@{#293438} |
