Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
599123002:
More efficient BitReader::SkipBits for large numbers of bits. (Closed)
DescriptionMore efficient BitReader::SkipBits for large numbers of bits.
BUG=376450
Committed: https://crrev.com/9158797a8bf4fc013cdf009b7c6f4aa28dab3bd6
Cr-Commit-Position: refs/heads/master@{#296956}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Address CR from patch set #1. #Patch Set 3 : Increase unit test coverage. #
Total comments: 23
Patch Set 4 : Fix remarks on the unit test. #Patch Set 5 : Minor adjustement in the unit test. #
Messages
Total messages: 20 (2 generated)
damienv@chromium.org changed reviewers: + dalecurtis@chromium.org, scherkus@chromium.org, wolenetz@chromium.org
PTAL. Thanks !
lgtm % nit. https://codereview.chromium.org/599123002/diff/1/media/base/bit_reader_core.cc File media/base/bit_reader_core.cc (right): https://codereview.chromium.org/599123002/diff/1/media/base/bit_reader_core.c... media/base/bit_reader_core.cc:84: int window_size = const since you're doing so above.
(Also any extra unittests you can add are always appreciated, the ones we have provide some coverage for what you're doing here though).
lgtm
lgtm
I'll add one more unit test to exercise various SkipBits scenario (small number of bits, medium and large). https://codereview.chromium.org/599123002/diff/1/media/base/bit_reader_core.cc File media/base/bit_reader_core.cc (right): https://codereview.chromium.org/599123002/diff/1/media/base/bit_reader_core.c... media/base/bit_reader_core.cc:84: int window_size = On 2014/09/24 18:35:43, DaleCurtis wrote: > const since you're doing so above. Done.
Please take a look at the updated unit test. Thanks.
https://codereview.chromium.org/599123002/diff/40001/media/base/bit_reader_un... File media/base/bit_reader_unittest.cc (right): https://codereview.chromium.org/599123002/diff/40001/media/base/bit_reader_un... media/base/bit_reader_unittest.cc:75: uint8 buffer[256]; uint8 buffer[256](); will zero initialize IIRC. Might be = {0} instead. https://codereview.chromium.org/599123002/diff/40001/media/base/bit_reader_un... media/base/bit_reader_unittest.cc:76: for (size_t k = 0; k < arraysize(buffer); k++) I think you can just use sizeof() here, no? Otherwise you need ARRAYSIZE_UNSAFE since this is a local variable. https://codereview.chromium.org/599123002/diff/40001/media/base/bit_reader_un... media/base/bit_reader_unittest.cc:79: // The test alternates between ReadBits and SkipBits. Add a comment about how the values were chosen (arbitrarily is fine). https://codereview.chromium.org/599123002/diff/40001/media/base/bit_reader_un... media/base/bit_reader_unittest.cc:82: size_t pattern_read_skip[][2] = { const https://codereview.chromium.org/599123002/diff/40001/media/base/bit_reader_un... media/base/bit_reader_unittest.cc:93: for (size_t k = 0; k < arraysize(pattern_read_skip); k++) { Ditto on arraysize. https://codereview.chromium.org/599123002/diff/40001/media/base/bit_reader_un... media/base/bit_reader_unittest.cc:93: for (size_t k = 0; k < arraysize(pattern_read_skip); k++) { ++k instead all over this test.
https://codereview.chromium.org/599123002/diff/40001/media/base/bit_reader_un... File media/base/bit_reader_unittest.cc (right): https://codereview.chromium.org/599123002/diff/40001/media/base/bit_reader_un... media/base/bit_reader_unittest.cc:83: { 5, 17 }, nit: suggest adding (tiny) read+skip within sequences of < 8 bits, aligned within and across byte boundaries. https://codereview.chromium.org/599123002/diff/40001/media/base/bit_reader_un... media/base/bit_reader_unittest.cc:94: SetBit(buffer, arraysize(buffer), pos); nit: skip the setbits if the read is 0? https://codereview.chromium.org/599123002/diff/40001/media/base/bit_reader_un... media/base/bit_reader_unittest.cc:100: BitReader bit_reader(buffer, sizeof(buffer)); nit: confirm how many bits should be available? https://codereview.chromium.org/599123002/diff/40001/media/base/bit_reader_un... media/base/bit_reader_unittest.cc:104: EXPECT_EQ(value, 1 + (1 << (pattern_read_skip[k][0] - 1))); nit: this will fail for read_skip pattern with read size 1. Suggest using |
https://codereview.chromium.org/599123002/diff/40001/media/base/bit_reader_un... File media/base/bit_reader_unittest.cc (right): https://codereview.chromium.org/599123002/diff/40001/media/base/bit_reader_un... media/base/bit_reader_unittest.cc:75: uint8 buffer[256]; On 2014/09/25 01:25:53, DaleCurtis wrote: > uint8 buffer[256](); will zero initialize IIRC. Might be = {0} instead. Done. https://codereview.chromium.org/599123002/diff/40001/media/base/bit_reader_un... media/base/bit_reader_unittest.cc:76: for (size_t k = 0; k < arraysize(buffer); k++) On 2014/09/25 01:25:53, DaleCurtis wrote: > I think you can just use sizeof() here, no? Otherwise you need ARRAYSIZE_UNSAFE > since this is a local variable. I don't think I have to use ARRAYSIZE_UNSAFE. Here the type uint8 is not an anonymous type. Here is a copy of the doc: // ARRAYSIZE_UNSAFE performs essentially the same calculation as arraysize, // but can be used on anonymous types or types defined inside // functions. Did I miss sth ? https://codereview.chromium.org/599123002/diff/40001/media/base/bit_reader_un... media/base/bit_reader_unittest.cc:79: // The test alternates between ReadBits and SkipBits. On 2014/09/25 01:25:53, DaleCurtis wrote: > Add a comment about how the values were chosen (arbitrarily is fine). Done. https://codereview.chromium.org/599123002/diff/40001/media/base/bit_reader_un... media/base/bit_reader_unittest.cc:82: size_t pattern_read_skip[][2] = { On 2014/09/25 01:25:53, DaleCurtis wrote: > const Done. https://codereview.chromium.org/599123002/diff/40001/media/base/bit_reader_un... media/base/bit_reader_unittest.cc:93: for (size_t k = 0; k < arraysize(pattern_read_skip); k++) { On 2014/09/25 01:25:53, DaleCurtis wrote: > ++k instead all over this test. I am not sure there is a convention on primitive types (I think both are allowed). I see either case in Chrome media code. I am fine with either one. https://codereview.chromium.org/599123002/diff/40001/media/base/bit_reader_un... media/base/bit_reader_unittest.cc:93: for (size_t k = 0; k < arraysize(pattern_read_skip); k++) { On 2014/09/25 01:25:53, DaleCurtis wrote: > Ditto on arraysize. Done.
https://codereview.chromium.org/599123002/diff/40001/media/base/bit_reader_un... File media/base/bit_reader_unittest.cc (right): https://codereview.chromium.org/599123002/diff/40001/media/base/bit_reader_un... media/base/bit_reader_unittest.cc:76: for (size_t k = 0; k < arraysize(buffer); k++) On 2014/09/25 01:45:51, damienv1 wrote: > On 2014/09/25 01:25:53, DaleCurtis wrote: > > I think you can just use sizeof() here, no? Otherwise you need > ARRAYSIZE_UNSAFE > > since this is a local variable. > > I don't think I have to use ARRAYSIZE_UNSAFE. Here the type uint8 is not an > anonymous type. > Here is a copy of the doc: > // ARRAYSIZE_UNSAFE performs essentially the same calculation as arraysize, > // but can be used on anonymous types or types defined inside > // functions. > > Did I miss sth ? Ah, you're right. arraysize() is fine, my mistake. https://codereview.chromium.org/599123002/diff/40001/media/base/bit_reader_un... media/base/bit_reader_unittest.cc:93: for (size_t k = 0; k < arraysize(pattern_read_skip); k++) { On 2014/09/25 01:45:51, damienv1 wrote: > On 2014/09/25 01:25:53, DaleCurtis wrote: > > ++k instead all over this test. > > I am not sure there is a convention on primitive types (I think both are > allowed). I see either case in Chrome media code. I am fine with either one. We definitely prefer ++ for all for(). It is in the style guide too: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Preincrement_...
https://codereview.chromium.org/599123002/diff/40001/media/base/bit_reader_un... File media/base/bit_reader_unittest.cc (right): https://codereview.chromium.org/599123002/diff/40001/media/base/bit_reader_un... media/base/bit_reader_unittest.cc:83: { 5, 17 }, On 2014/09/25 01:33:56, wolenetz wrote: > nit: suggest adding (tiny) read+skip within sequences of < 8 bits, aligned > within and across byte boundaries. Done. https://codereview.chromium.org/599123002/diff/40001/media/base/bit_reader_un... media/base/bit_reader_unittest.cc:94: SetBit(buffer, arraysize(buffer), pos); On 2014/09/25 01:33:56, wolenetz wrote: > nit: skip the setbits if the read is 0? Done. https://codereview.chromium.org/599123002/diff/40001/media/base/bit_reader_un... media/base/bit_reader_unittest.cc:100: BitReader bit_reader(buffer, sizeof(buffer)); On 2014/09/25 01:33:56, wolenetz wrote: > nit: confirm how many bits should be available? Done. https://codereview.chromium.org/599123002/diff/40001/media/base/bit_reader_un... media/base/bit_reader_unittest.cc:104: EXPECT_EQ(value, 1 + (1 << (pattern_read_skip[k][0] - 1))); On 2014/09/25 01:33:56, wolenetz wrote: > nit: this will fail for read_skip pattern with read size 1. Suggest using | Good catch. Thanks. Done.
https://codereview.chromium.org/599123002/diff/40001/media/base/bit_reader_un... File media/base/bit_reader_unittest.cc (right): https://codereview.chromium.org/599123002/diff/40001/media/base/bit_reader_un... media/base/bit_reader_unittest.cc:93: for (size_t k = 0; k < arraysize(pattern_read_skip); k++) { On 2014/09/25 01:50:15, DaleCurtis wrote: > On 2014/09/25 01:45:51, damienv1 wrote: > > On 2014/09/25 01:25:53, DaleCurtis wrote: > > > ++k instead all over this test. > > > > I am not sure there is a convention on primitive types (I think both are > > allowed). I see either case in Chrome media code. I am fine with either one. > > We definitely prefer ++ for all for(). It is in the style guide too: > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Preincrement_... From this style guide: "For simple scalar (non-object) values there is no reason to prefer one form and we allow either. For iterators and other template types, use pre-increment."
lgtm
lgtm
The CQ bit was checked by damienv@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/599123002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as 7c4b26e13c2185463f3c939c8dd1d9dbab22a29f
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/9158797a8bf4fc013cdf009b7c6f4aa28dab3bd6 Cr-Commit-Position: refs/heads/master@{#296956} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
