Enforce non-negative BitstreamBuffer id

Enforce non-negative BitstreamBuffer id

There is some video decode accelerator code that assumes that a negative
BitstreamBuffer id is never given, and uses it to mark error conditions.

* Check for non-negative id when receving over IPC.
* Make sure we don't send negative ids from our video decode pipeline (by
  handling wrapping)
* Update PPAPI interface to document the fact that negative ids should not
  be used.

BUG=chromium:168293
TEST=local build, run on snow


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

[sheu, 2013-01-10 22:43:23]

PTAL.

[Ami GONE FROM CHROMIUM, 2013-01-10 23:13:56]

lgtm

https://chromiumcodereview.appspot.com/11826064/diff/1/content/common/gpu/med...
File content/common/gpu/media/video_decode_accelerator_unittest.cc (right):

https://chromiumcodereview.appspot.com/11826064/diff/1/content/common/gpu/med...
content/common/gpu/media/video_decode_accelerator_unittest.cc:607:
next_bitstream_buffer_id_ = (next_bitstream_buffer_id_ + 1) & 0x7FFFFFFF;
This is kind of silly (since this is a unittest with controlled inputs) but to
be pedantic: your change relies on wrap-around overflow behavior (n_b_b_i_ is an
int) which is not what the standard gives you.
You could make n_b_b_i_ int64 (like GVD) or uint32 to make this snippet correct.

[scherkus (not reviewing), 2013-01-10 23:18:49]

[removing myself in lieu of fischman]

[sheu, 2013-01-10 23:34:48]

https://chromiumcodereview.appspot.com/11826064/diff/1/content/common/gpu/med...
File content/common/gpu/media/video_decode_accelerator_unittest.cc (right):

https://chromiumcodereview.appspot.com/11826064/diff/1/content/common/gpu/med...
content/common/gpu/media/video_decode_accelerator_unittest.cc:607:
next_bitstream_buffer_id_ = (next_bitstream_buffer_id_ + 1) & 0x7FFFFFFF;
On 2013/01/10 23:13:56, Ami Fischman wrote:
> This is kind of silly (since this is a unittest with controlled inputs) but to
> be pedantic: your change relies on wrap-around overflow behavior (n_b_b_i_ is
an
> int) which is not what the standard gives you.
> You could make n_b_b_i_ int64 (like GVD) or uint32 to make this snippet
correct.

Fine.  :-)

[Ami GONE FROM CHROMIUM, 2013-01-10 23:41:31]

https://chromiumcodereview.appspot.com/11826064/diff/3002/content/common/gpu/...
File content/common/gpu/media/video_decode_accelerator_unittest.cc (right):

https://chromiumcodereview.appspot.com/11826064/diff/3002/content/common/gpu/...
content/common/gpu/media/video_decode_accelerator_unittest.cc:607:
next_bitstream_buffer_id_ = (next_bitstream_buffer_id_ + 1) & 0x3FFFFFFF;
lolololol

https://chromiumcodereview.appspot.com/11826064/diff/3002/media/filters/gpu_v...
File media/filters/gpu_video_decoder.cc (right):

https://chromiumcodereview.appspot.com/11826064/diff/3002/media/filters/gpu_v...
media/filters/gpu_video_decoder.cc:288: next_bitstream_buffer_id_ =
(next_bitstream_buffer_id_ + 1) & 0x3FFFFFFF;
Weird enough to deserve a comment.  Note that here (unlike the test) n_b_b_i_ is
an int64, so the previous code was actually fine.  If you want to keep this
3FFFFFFF then probably best to make n_b_b_i_ int32 here, too.

[sheu, 2013-01-11 00:18:24]

PTAL

[Ami GONE FROM CHROMIUM, 2013-01-11 00:37:33]

still lgtm

[sheu, 2013-01-12 01:15:25]

Looking to get an OWNERS lgtm, thanks.

[sheu, 2013-01-12 01:17:02]

On 2013/01/12 01:15:25, sheu wrote:
> Looking to get an OWNERS lgtm, thanks.

Specifically: brettw@ or yzshen@

[brettw, 2013-01-14 05:47:20]

Please ask somebody specific. I will always assume somebody else will do it if
it's unclear.

[yzshen1, 2013-01-15 04:30:04]

.*ppapi.* LGTM

[commit-bot: I haz the power, 2013-01-15 04:33:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/11826064/11001

[commit-bot: I haz the power, 2013-01-15 07:06:35]

Change committed as 176816