Smarter algorithm for extending 8-bit frame IDs to 32-bit frame IDs.

Smarter algorithm for extending 8-bit frame IDs to 32-bit frame IDs.

The previous algorithm was limited to jumps of ~64, the new algorithm
can handle jumps of ~127.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290023

[Alpha Left Google, 2014-08-12 01:07:15]

https://codereview.chromium.org/458313003/diff/1/media/cast/net/cast_transpor...
File media/cast/net/cast_transport_defines.h (right):

https://codereview.chromium.org/458313003/diff/1/media/cast/net/cast_transpor...
media/cast/net/cast_transport_defines.h:73: if (ret - largest_frame_id_seen_ >
127) {
This code has some funny behavior for the following case.

Suppose:
largest_frame_id_seen_ is 0
over_the_wire_frame_id is 128

ret = 128; // largest_frame_id_seen_ & 0xFFFFFF00 is still 0.

Then ret - 0 > 127 is true. ret becomes -128.
Then ret - 0 < -127 is true. ret becomes 128.

Is this expected?

https://codereview.chromium.org/458313003/diff/1/media/cast/net/cast_transpor...
media/cast/net/cast_transport_defines.h:79: if (ret - largest_frame_id_seen_ >
0LL) {
Why 0LL?

[hubbe, 2014-08-13 17:37:03]

Comments addressed and test added.

PS: The existing test was not actually running - fixed that too.

https://codereview.chromium.org/458313003/diff/1/media/cast/net/cast_transpor...
File media/cast/net/cast_transport_defines.h (right):

https://codereview.chromium.org/458313003/diff/1/media/cast/net/cast_transpor...
media/cast/net/cast_transport_defines.h:73: if (ret - largest_frame_id_seen_ >
127) {
On 2014/08/12 01:07:15, Alpha wrote:
> This code has some funny behavior for the following case.
> 
> Suppose:
> largest_frame_id_seen_ is 0
> over_the_wire_frame_id is 128
> 
> ret = 128; // largest_frame_id_seen_ & 0xFFFFFF00 is still 0.
> 
> Then ret - 0 > 127 is true. ret becomes -128.
> Then ret - 0 < -127 is true. ret becomes 128.
> 
> Is this expected?

Fixed.

https://codereview.chromium.org/458313003/diff/1/media/cast/net/cast_transpor...
media/cast/net/cast_transport_defines.h:79: if (ret - largest_frame_id_seen_ >
0LL) {
On 2014/08/12 01:07:15, Alpha wrote:
> Why 0LL?

Leftover from when I was using int64s, removed.

[miu, 2014-08-13 18:38:58]

https://codereview.chromium.org/458313003/diff/60001/media/cast/net/cast_tran...
File media/cast/net/cast_transport_defines.h (right):

https://codereview.chromium.org/458313003/diff/60001/media/cast/net/cast_tran...
media/cast/net/cast_transport_defines.h:76: if (ret - largest_frame_id_seen_ <
-128) {
s/if/else if/

https://codereview.chromium.org/458313003/diff/60001/media/cast/net/cast_tran...
media/cast/net/cast_transport_defines.h:79: if (ret - largest_frame_id_seen_ >
0) {
I think this is wrong.  Shouldn't it be:

  if (static_cast<uint32>(ret) - static_cast<uint32>(largest_frame_id_seen_) >
0) {
    ...

https://codereview.chromium.org/458313003/diff/60001/media/cast/net/frame_id_...
File media/cast/net/frame_id_wrap_helper_test.cc (right):

https://codereview.chromium.org/458313003/diff/60001/media/cast/net/frame_id_...
media/cast/net/frame_id_wrap_helper_test.cc:51: srand(0);
For the normal, consecutive case, consider adding:

  for (uint32 frame_id = 0; frame_id < 100000; ++frame_id) {
    EXPECT_EQ(...);
  }

and also test wrap-around case:

  for (uint32 frame_id = 0xfffff000; frame_id < 0x00000fff; ++frame_id) {
    EXPECT_EQ(...);
  }

[Alpha Left Google, 2014-08-13 18:47:25]

https://codereview.chromium.org/458313003/diff/60001/media/cast/net/cast_tran...
File media/cast/net/cast_transport_defines.h (right):

https://codereview.chromium.org/458313003/diff/60001/media/cast/net/cast_tran...
media/cast/net/cast_transport_defines.h:69: :
largest_frame_id_seen_(kStartFrameId) {}
Please add another ctor for testing edge cases.

https://codereview.chromium.org/458313003/diff/60001/media/cast/net/cast_tran...
media/cast/net/cast_transport_defines.h:76: if (ret - largest_frame_id_seen_ <
-128) {
On 2014/08/13 18:38:57, miu wrote:
> s/if/else if/

I think so too.

[hubbe, 2014-08-13 21:18:29]

ID wrap-arounds should now be properly tested.

https://codereview.chromium.org/458313003/diff/60001/media/cast/net/cast_tran...
File media/cast/net/cast_transport_defines.h (right):

https://codereview.chromium.org/458313003/diff/60001/media/cast/net/cast_tran...
media/cast/net/cast_transport_defines.h:69: :
largest_frame_id_seen_(kStartFrameId) {}
On 2014/08/13 18:47:24, Alpha wrote:
> Please add another ctor for testing edge cases.

Added setter instead.

[miu, 2014-08-13 22:03:51]

lgtm

[hubbe, 2014-08-13 22:44:36]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2014-08-13 22:47:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hubbe@chromium.org/458313003/140001

[commit-bot: I haz the power, 2014-08-14 08:28:18]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-14 09:00:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-14 09:00:03]

Try jobs failed on following builders:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Alpha Left Google, 2014-08-14 21:46:54]

LGTM as well.

[hubbe, 2014-08-15 20:42:39]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2014-08-15 20:43:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hubbe@chromium.org/458313003/200001

[commit-bot: I haz the power, 2014-08-15 21:54:22]

Committed patchset #11 (200001) as 290023