Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1224623014:
Refactor ParseArrayGeometry to use standard Chromium facilities. (Closed)
|
Created:
5 years, 5 months ago by ajm Modified:
5 years, 4 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, dzhioev+watch_chromium.org, creis+watch_chromium.org, posciak+watch_chromium.org, wjia+watch_chromium.org, nasko+codewatch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, oshima+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor ParseArrayGeometry to use standard Chromium facilities.
Now unit tested as well.
BUG=451188
TEST=A local swanky build correctly passed microphone positions to
enable beamforming.
Committed: https://crrev.com/2288183f38ae465b397b6b11200d76b417ce895a
Cr-Commit-Position: refs/heads/master@{#341865}
Patch Set 1 #Patch Set 2 : #
Total comments: 6
Patch Set 3 : Revert cmdline switch changes. #Patch Set 4 : Merge fix and clang-format. #
Total comments: 6
Patch Set 5 : Use empty(). #Patch Set 6 : Use const references. #Patch Set 7 : Fix shared library and Windows builds. #Patch Set 8 : CONTEXT_EXPORT on declaration... #
Messages
Total messages: 40 (17 generated)
ajm@chromium.org changed reviewers: + aluebs@chromium.org
Alex, please have a look before I send it out to other reviewers.
Did you get an email about the corresponding ChromeOS CL? https://chromium-review.googlesource.com/#/c/283652/3 It's my first time using gerritt, and it's not clear if it sent a mail out.
lgtm % nits https://codereview.chromium.org/1224623014/diff/20001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/1224623014/diff/20001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:100: if (position_string == "") { position_string.empty()? https://codereview.chromium.org/1224623014/diff/20001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:108: if (position_string != "") { !position_string.empty()? https://codereview.chromium.org/1224623014/diff/20001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/1224623014/diff/20001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:408: double float_token; Why does this need to be double if it is casted to float afterwards anyway?
I reverted the command-line stuff to just land the string-parsing refactor. https://codereview.chromium.org/1224623014/diff/20001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/1224623014/diff/20001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:100: if (position_string == "") { On 2015/07/07 15:40:54, aluebs-chromium wrote: > position_string.empty()? Agreed, but now reverted. https://codereview.chromium.org/1224623014/diff/20001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:108: if (position_string != "") { On 2015/07/07 15:40:54, aluebs-chromium wrote: > !position_string.empty()? Agreed, but now reverted. https://codereview.chromium.org/1224623014/diff/20001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/1224623014/diff/20001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:408: double float_token; On 2015/07/07 15:40:54, aluebs-chromium wrote: > Why does this need to be double if it is casted to float afterwards anyway? base::StringToDouble calls for a double*, and since it's a pointer, we need to give it what it wants.
The CQ bit was checked by ajm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aluebs@chromium.org Link to the patchset: https://codereview.chromium.org/1224623014/#ps60001 (title: "Merge fix and clang-format.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1224623014/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1224623014/60001
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-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.
ajm@chromium.org changed reviewers: + emircan@chromium.org
Hey, Emircan, can you committer RS this? I'm a full committer and both Alex and I are owners of the affected code, but it _additionally_ needs an lgtm from a full committer.
Sorry for my late reply, it was an unexpected CL for me :) lgtm % nits below. https://codereview.chromium.org/1224623014/diff/60001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/1224623014/diff/60001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:100: if (geometry.size() == 0) { std::vector::empty() can be used here. https://codereview.chromium.org/1224623014/diff/60001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:101: const std::string board = base::SysInfo::GetLsbReleaseBoard(); const std::string& board can avoid a copy. https://codereview.chromium.org/1224623014/diff/60001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/1224623014/diff/60001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:398: base::KEEP_WHITESPACE, base::SPLIT_WANT_NONEMPTY); const auto& tokens?
https://codereview.chromium.org/1224623014/diff/60001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/1224623014/diff/60001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:100: if (geometry.size() == 0) { On 2015/08/03 18:06:22, emircan wrote: > std::vector::empty() can be used here. Done. https://codereview.chromium.org/1224623014/diff/60001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:101: const std::string board = base::SysInfo::GetLsbReleaseBoard(); On 2015/08/03 18:06:22, emircan wrote: > const std::string& board can avoid a copy. I don't think this can avoid a copy. GetLsbReleaseBoard() returns a std::string by value. Assigning to a reference here must bind it to a temporary. Whether a copy occurs comes down to whether RVO takes place or not in the construction of that temporary. Do you agree? https://codereview.chromium.org/1224623014/diff/60001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/1224623014/diff/60001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:398: base::KEEP_WHITESPACE, base::SPLIT_WANT_NONEMPTY); On 2015/08/03 18:06:23, emircan wrote: > const auto& tokens? Same comment here. FWIW a bunch of anecdotal callsites to SplitString() I surveyed don't assign to a reference.
On 2015/08/03 18:06:23, emircan wrote: > Sorry for my late reply, it was an unexpected CL for me :) No worries! Please take a look at my comment before I land it.
On 2015/08/04 01:16:35, ajm wrote: > https://codereview.chromium.org/1224623014/diff/60001/content/renderer/media/... > content/renderer/media/media_stream_audio_processor.cc:101: const std::string > board = base::SysInfo::GetLsbReleaseBoard(); > On 2015/08/03 18:06:22, emircan wrote: > > const std::string& board can avoid a copy. > > I don't think this can avoid a copy. GetLsbReleaseBoard() returns a std::string > by value. Assigning to a reference here must bind it to a temporary. Whether a > copy occurs comes down to whether RVO takes place or not in the construction of > that temporary. > > Do you agree? You are right that compiler can&should optimize all the copies away, so no worries in both styles. So, it was a nit :) AFAIK, RVO can happen in both cases, as in returning the temporary rvalue of GetLsbReleaseBoard() to caller context. Aside from RVO, the copy I was referring to here was copy-construction of "std::string board" from the rvalue. And that can&should be optimized by the compiler as well, but why not just make sure. When "const auto& * =" is assigned to a temporary rvalue, just extends extends the lifetime of the temporary to the lifetime of the reference [0]. Also, some Chromium users have been using it as well [1]. Either way, it is just a nit :) [0] http://stackoverflow.com/questions/13318257/const-reference-to-temporary-vs-r... [1] https://code.google.com/p/chromium/codesearch#search/&q=%22const%20std::strin...
On 2015/08/04 20:12:55, emircan wrote: > On 2015/08/04 01:16:35, ajm wrote: > > > https://codereview.chromium.org/1224623014/diff/60001/content/renderer/media/... > > content/renderer/media/media_stream_audio_processor.cc:101: const std::string > > board = base::SysInfo::GetLsbReleaseBoard(); > > On 2015/08/03 18:06:22, emircan wrote: > > > const std::string& board can avoid a copy. > > > > I don't think this can avoid a copy. GetLsbReleaseBoard() returns a > std::string > > by value. Assigning to a reference here must bind it to a temporary. Whether a > > copy occurs comes down to whether RVO takes place or not in the construction > of > > that temporary. > > > > Do you agree? > > You are right that compiler can&should optimize all the copies away, so no > worries in both styles. So, it was a nit :) > > AFAIK, RVO can happen in both cases, as in returning the temporary rvalue of > GetLsbReleaseBoard() to caller context. Aside from RVO, the copy I was referring > to here was copy-construction of "std::string board" from the rvalue. And that > can&should be optimized by the compiler as well, but why not just make sure. > When "const auto& * =" is assigned to a temporary rvalue, just extends extends > the lifetime of the temporary to the lifetime of the reference [0]. Also, some > Chromium users have been using it as well [1]. Either way, it is just a nit :) > > [0] > http://stackoverflow.com/questions/13318257/const-reference-to-temporary-vs-r... > [1] > https://code.google.com/p/chromium/codesearch#search/&q=%22const%20std::strin... Thanks for the comments. IIUC, RVO is exactly causing the std::string to be constructed in |board|; there is no additional copy. I was aware that the lifetime of the temporary is extended to match that of the const reference, but thanks for pointing that out. In any case, I can't see using a const reference here causing a problem, so changed.
The CQ bit was checked by ajm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from emircan@chromium.org, aluebs@chromium.org Link to the patchset: https://codereview.chromium.org/1224623014/#ps100001 (title: "Use const references.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1224623014/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1224623014/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ajm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from emircan@chromium.org, aluebs@chromium.org Link to the patchset: https://codereview.chromium.org/1224623014/#ps120001 (title: "Fix shared library and Windows builds.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1224623014/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1224623014/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by ajm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from emircan@chromium.org, aluebs@chromium.org Link to the patchset: https://codereview.chromium.org/1224623014/#ps140001 (title: "CONTEXT_EXPORT on declaration...")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1224623014/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1224623014/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by ajm@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1224623014/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1224623014/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by ajm@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1224623014/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1224623014/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/2288183f38ae465b397b6b11200d76b417ce895a Cr-Commit-Position: refs/heads/master@{#341865} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
