Fix a bug in webrtc::ByteReader

Fix a bug in webrtc::ByteReader

The specializations for 4-byte reading did not return correct
values. This has to do with the order of casting and shifting. Also
adding a test to expose the bug (and verify the other byte sizes).

Committed: https://crrev.com/7a83951b274115df896bdfae895b61bac48b3018
Cr-Commit-Position: refs/heads/master@{#11364}

[hlundin-webrtc, 2016-01-22 13:55:50]

Patchset #1 (id:1) has been deleted

[hlundin-webrtc, 2016-01-22 13:56:39]

henrik.lundin@webrtc.org changed reviewers:
+ sprang@webrtc.org

[hlundin-webrtc, 2016-01-22 13:56:39]

Språng, please, take a look.

Thanks!

[sprang_webrtc, 2016-01-22 14:10:54]

Interesting, could you explain in detail what went wrong? I'm surprised this
current tests didn't catch this.
Shouldn't the same bug happen in the uint16_t case?

I'm leaning even more towards removing the 2, 4, 8 byte specializations now...

[hlundin-webrtc, 2016-01-22 14:25:22]

On 2016/01/22 14:10:54, språng wrote:
> Interesting, could you explain in detail what went wrong? I'm surprised this
> current tests didn't catch this.
> Shouldn't the same bug happen in the uint16_t case?
> 
> I'm leaning even more towards removing the 2, 4, 8 byte specializations now...

This is what happens:

[----------] 2 tests from ByteIo
[ RUN      ] ByteIo.SanityCheckFixedByteArrayUnsignedReadBigEndian
../../webrtc/modules/rtp_rtcp/source/byte_io_unittest.cc:218: Failure
Value of: value
  Actual: 18446744073708428748
Expected: static_cast<uint64_t>(0xFFEEDDCC)
Which is: 4293844428
[  FAILED  ] ByteIo.SanityCheckFixedByteArrayUnsignedReadBigEndian (0 ms)
[ RUN      ] ByteIo.SanityCheckFixedByteArrayUnsignedReadLittleEndian
../../webrtc/modules/rtp_rtcp/source/byte_io_unittest.cc:236: Failure
Value of: value
  Actual: 18446744072851681023
Expected: static_cast<uint64_t>(0xCCDDEEFF)
Which is: 3437096703
[  FAILED  ] ByteIo.SanityCheckFixedByteArrayUnsignedReadLittleEndian (0 ms)
[----------] 2 tests from ByteIo (0 ms total)

Note in particular that the function returns rather big values from parsing 4
bytes... :)

[sprang_webrtc, 2016-01-22 15:45:39]

On 2016/01/22 14:25:22, hlundin-webrtc wrote:
> On 2016/01/22 14:10:54, språng wrote:
> > Interesting, could you explain in detail what went wrong? I'm surprised this
> > current tests didn't catch this.
> > Shouldn't the same bug happen in the uint16_t case?
> > 
> > I'm leaning even more towards removing the 2, 4, 8 byte specializations
now...
> 
> This is what happens:
> 
> [----------] 2 tests from ByteIo
> [ RUN      ] ByteIo.SanityCheckFixedByteArrayUnsignedReadBigEndian
> ../../webrtc/modules/rtp_rtcp/source/byte_io_unittest.cc:218: Failure
> Value of: value
>   Actual: 18446744073708428748
> Expected: static_cast<uint64_t>(0xFFEEDDCC)
> Which is: 4293844428
> [  FAILED  ] ByteIo.SanityCheckFixedByteArrayUnsignedReadBigEndian (0 ms)
> [ RUN      ] ByteIo.SanityCheckFixedByteArrayUnsignedReadLittleEndian
> ../../webrtc/modules/rtp_rtcp/source/byte_io_unittest.cc:236: Failure
> Value of: value
>   Actual: 18446744072851681023
> Expected: static_cast<uint64_t>(0xCCDDEEFF)
> Which is: 3437096703
> [  FAILED  ] ByteIo.SanityCheckFixedByteArrayUnsignedReadLittleEndian (0 ms)
> [----------] 2 tests from ByteIo (0 ms total)
> 
> Note in particular that the function returns rather big values from parsing 4
> bytes... :)

Gotcha. Which is FFFFFFFFCCDDEEFF, so it get's sign extended..
I think I'll take another pass at these functions next week.
lgtm

[hlundin-webrtc, 2016-01-25 06:48:10]

The CQ bit was checked by henrik.lundin@webrtc.org

[commit-bot: I haz the power, 2016-01-25 06:48:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1615653011/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1615653011/20001

[commit-bot: I haz the power, 2016-01-25 07:47:55]

Committed patchset #1 (id:20001)

[commit-bot: I haz the power, 2016-01-25 07:48:05]

Description was changed from

==========
Fix a bug in webrtc::ByteReader

The specializations for 4-byte reading did not return correct
values. This has to do with the order of casting and shifting. Also
adding a test to expose the bug (and verify the other byte sizes).
==========

to

==========
Fix a bug in webrtc::ByteReader

The specializations for 4-byte reading did not return correct
values. This has to do with the order of casting and shifting. Also
adding a test to expose the bug (and verify the other byte sizes).

Committed: https://crrev.com/7a83951b274115df896bdfae895b61bac48b3018
Cr-Commit-Position: refs/heads/master@{#11364}
==========

[commit-bot: I haz the power, 2016-01-25 07:48:06]

Patchset 1 (id:??) landed as
https://crrev.com/7a83951b274115df896bdfae895b61bac48b3018
Cr-Commit-Position: refs/heads/master@{#11364}