Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1578983003:
[Chromecast] Store ALSA Mixer test data as uint32_t. (Closed)
|
Created:
4 years, 11 months ago by slan Modified:
4 years, 11 months ago CC:
chromium-reviews, gunsch+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] Store ALSA Mixer test data as uint32_t.
::media::AudioBus relies on 16-byte alignment for any float data it
wraps, which is a very brittle assumption for a float array declared
statically in a testing source file. Store this data instead as 32-bit
integers, copying these into memory that ::media::AudioBus allocates.
This change also replaces some magic numbers with appropriately-named
constants.
Bug: b/26445504
BUG=
Committed: https://crrev.com/24b25293b834fd58a0b41b498b03a773eb032784
Cr-Commit-Position: refs/heads/master@{#368933}
Patch Set 1 #Patch Set 2 : Correct typo. #
Total comments: 13
Patch Set 3 : Comments addressed. #
Total comments: 2
Patch Set 4 : Comment update, sizeof => size_t #
Messages
Total messages: 18 (4 generated)
slan@chromium.org changed reviewers: + cleichner@chromium.org, halliwell@chromium.org, kmackay@chromium.org
These tests started failing on pepperoni when these got upstreamed last week - not sure why we weren't seeing this error before. +halliwell@ for OWNERS stamp.
https://codereview.chromium.org/1578983003/diff/20001/chromecast/media/cma/ba... File chromecast/media/cma/backend/alsa/stream_mixer_alsa_unittest.cc (right): https://codereview.chromium.org/1578983003/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/stream_mixer_alsa_unittest.cc:30: const int kBytesPerSample = sizeof(int32_t); size_t https://codereview.chromium.org/1578983003/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/stream_mixer_alsa_unittest.cc:500: // samples instead of floats to avoid alignment issues. By mixing these two Mind clarifying the nature of the issues? https://codereview.chromium.org/1578983003/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/stream_mixer_alsa_unittest.cc:507: -0x7FFFFFFF, -0x7FFFFFFF, This isn't 2^31, is the range meant to be inclusive? https://codereview.chromium.org/1578983003/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/stream_mixer_alsa_unittest.cc:522: -0x7FFFFFFF, -0x7FFFFFFF, Ditto.
https://codereview.chromium.org/1578983003/diff/20001/chromecast/media/cma/ba... File chromecast/media/cma/backend/alsa/stream_mixer_alsa_unittest.cc (right): https://codereview.chromium.org/1578983003/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/stream_mixer_alsa_unittest.cc:499: // Create edge case data for the inputs. These are respresented as raw nit: represented https://codereview.chromium.org/1578983003/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/stream_mixer_alsa_unittest.cc:507: -0x7FFFFFFF, -0x7FFFFFFF, On 2016/01/12 17:12:32, cleichner wrote: > This isn't 2^31, is the range meant to be inclusive? +1, should be -0x80000000 (or std::numeric_limits<int32_t>::min(), which might be better).
https://codereview.chromium.org/1578983003/diff/20001/chromecast/media/cma/ba... File chromecast/media/cma/backend/alsa/stream_mixer_alsa_unittest.cc (right): https://codereview.chromium.org/1578983003/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/stream_mixer_alsa_unittest.cc:30: const int kBytesPerSample = sizeof(int32_t); On 2016/01/12 17:12:32, cleichner wrote: > size_t Are you proposing sizeof(size_t)? The datatype is explicitly int32_t. https://codereview.chromium.org/1578983003/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/stream_mixer_alsa_unittest.cc:499: // Create edge case data for the inputs. These are respresented as raw On 2016/01/12 17:19:28, kmackay wrote: > nit: represented Done. https://codereview.chromium.org/1578983003/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/stream_mixer_alsa_unittest.cc:500: // samples instead of floats to avoid alignment issues. By mixing these two On 2016/01/12 17:12:32, cleichner wrote: > Mind clarifying the nature of the issues? Clarified. https://codereview.chromium.org/1578983003/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/stream_mixer_alsa_unittest.cc:507: -0x7FFFFFFF, -0x7FFFFFFF, On 2016/01/12 17:19:28, kmackay wrote: > On 2016/01/12 17:12:32, cleichner wrote: > > This isn't 2^31, is the range meant to be inclusive? > > +1, should be -0x80000000 (or std::numeric_limits<int32_t>::min(), which might > be better). Ah, darn. My mistake. Done. https://codereview.chromium.org/1578983003/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/stream_mixer_alsa_unittest.cc:522: -0x7FFFFFFF, -0x7FFFFFFF, On 2016/01/12 17:12:32, cleichner wrote: > Ditto. Done.
https://codereview.chromium.org/1578983003/diff/20001/chromecast/media/cma/ba... File chromecast/media/cma/backend/alsa/stream_mixer_alsa_unittest.cc (right): https://codereview.chromium.org/1578983003/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/stream_mixer_alsa_unittest.cc:30: const int kBytesPerSample = sizeof(int32_t); On 2016/01/12 17:45:07, slan wrote: > On 2016/01/12 17:12:32, cleichner wrote: > > size_t > > Are you proposing sizeof(size_t)? The datatype is explicitly int32_t. const int kBytesPerSample = sizeof(int32_t); should be const size_t kBytesPerSample = sizeof(int32_t); sizeof creates size_t constants
https://codereview.chromium.org/1578983003/diff/40001/chromecast/media/cma/ba... File chromecast/media/cma/backend/alsa/stream_mixer_alsa_unittest.cc (right): https://codereview.chromium.org/1578983003/diff/40001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/stream_mixer_alsa_unittest.cc:504: // every combination of {-(2^31), 0, 2^31-1} is tested. Note that this test seriously nitty nit: You have two notes in a row.
lgtm
Done, thanks for picking up the small stuff. https://codereview.chromium.org/1578983003/diff/20001/chromecast/media/cma/ba... File chromecast/media/cma/backend/alsa/stream_mixer_alsa_unittest.cc (right): https://codereview.chromium.org/1578983003/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/stream_mixer_alsa_unittest.cc:30: const int kBytesPerSample = sizeof(int32_t); On 2016/01/12 18:13:54, cleichner wrote: > On 2016/01/12 17:45:07, slan wrote: > > On 2016/01/12 17:12:32, cleichner wrote: > > > size_t > > > > Are you proposing sizeof(size_t)? The datatype is explicitly int32_t. > > const int kBytesPerSample = sizeof(int32_t); should be > const size_t kBytesPerSample = sizeof(int32_t); > > sizeof creates size_t constants Ah, of course.... Done. https://codereview.chromium.org/1578983003/diff/40001/chromecast/media/cma/ba... File chromecast/media/cma/backend/alsa/stream_mixer_alsa_unittest.cc (right): https://codereview.chromium.org/1578983003/diff/40001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/stream_mixer_alsa_unittest.cc:504: // every combination of {-(2^31), 0, 2^31-1} is tested. Note that this test On 2016/01/12 18:15:27, cleichner wrote: > seriously nitty nit: You have two notes in a row. Done.
lgtm
lgtm
On 2016/01/12 18:35:32, cleichner wrote: > lgtm lgtm
The CQ bit was checked by slan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kmackay@chromium.org Link to the patchset: https://codereview.chromium.org/1578983003/#ps60001 (title: "Comment update, sizeof => size_t")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1578983003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1578983003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [Chromecast] Store ALSA Mixer test data as uint32_t. ::media::AudioBus relies on 16-byte alignment for any float data it wraps, which is a very brittle assumption for a float array declared statically in a testing source file. Store this data instead as 32-bit integers, copying these into memory that ::media::AudioBus allocates. This change also replaces some magic numbers with appropriately-named constants. Bug: b/26445504 BUG= ========== to ========== [Chromecast] Store ALSA Mixer test data as uint32_t. ::media::AudioBus relies on 16-byte alignment for any float data it wraps, which is a very brittle assumption for a float array declared statically in a testing source file. Store this data instead as 32-bit integers, copying these into memory that ::media::AudioBus allocates. This change also replaces some magic numbers with appropriately-named constants. Bug: b/26445504 BUG= Committed: https://crrev.com/24b25293b834fd58a0b41b498b03a773eb032784 Cr-Commit-Position: refs/heads/master@{#368933} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/24b25293b834fd58a0b41b498b03a773eb032784 Cr-Commit-Position: refs/heads/master@{#368933} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
