Reorganize media::VideoCapture* types

Reorganize media::VideoCapture* types

The purpose of this CL is to clean up the distinction between
VideoCaptureFormat (which identifies the captured type of a frame),
VideoCaptureParams (which identifies the requested format of a capture),
and VideoCaptureCapability (which identifies the capture capabilities of a
device).

Notably:
* VideoCaptureFormat::frame_size_type ->
  VideoCaptureParams::allow_resolution_change, as variable resolution
  capability is a per-session, not a per-frame property.
* VideoCaptureCapability::color -> VideoCaptureFormat::pixel_format, as frame
  color format is a per-frame property.
  * As VideoCaptureParams holds a VideoCaptureFormat member, capture requests
    are able now to request a particular capture color format.

BUG=269312
TEST=local build, run unittests, chrome on CrOS snow, desktop Linux

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

[sheu, 2013-11-18 23:36:04]

jiayl@, nick@: PTAL.  You should probably just look at
media/video/capture/video_capture_types.h and comment on that first; all the
rest of this massive changelist is that fallout from that, and may change based
on comments.

[ncarter (slow), 2013-11-19 00:14:23]

https://chromiumcodereview.appspot.com/68503005/diff/130050/media/video/captu...
File media/video/capture/video_capture_device.h (right):

https://chromiumcodereview.appspot.com/68503005/diff/130050/media/video/captu...
media/video/capture/video_capture_device.h:176: const VideoCaptureFormat&
frame_format) = 0;
In my mind, this was one boundary where passing a pixel format was necessary
(and that's why I thought Capability instead of Format made sense here
historically). You've addressed this by adding a pixel format to Format, and
switching away from Capabilities. That will allow Capabilities to evolve to
support the needs of GetDeviceSupportedFormats, which as far as I can tell is
the last use of VideoCaptureCapability -- e.g. it could evolve to support min
and max.

However, the addition of a pixel format field in VideoCaptureFormat means that
now there will be pixel formats in contexts where previously things were assumed
to be I420.

Here at the (Device -> OnIncomingCapturedFrame) boundary we have a much larger
range of supported formats than we will ever want to support at the IPC (Browser
-> Renderer) boundary. In my mind the two acceptable formats for the IPC
boundary are: shared-memory I420, and gpu-resident ARGB. And yet both spots are
tied to use the same type, VideoCaptureFormat.

In other words, by adding a pixel_format to Format, you wind up introducing a
parameter that will be ignored in a whole bunch of contexts. I think this is
appropriate only if you expect that the pixel_format parameter will eventually
come to be meaningful in all or most the following contexts:

1. VideoCaptureCapabilities returned by GetDeviceSupportedFormats()
2. VideoCaptureParams requested by the renderer when starting a stream.
3. VideoCaptureFormats returned by the browser upon delivery of a frame.

I'm skeptical that we want to involve pixel formats in cases [1] and [2]. Today
no camera needs outside advice in determining which capture format should be
used -- it simply will capture in the most efficiently obtained format, fully
knowing that the downstream stack is going to want I420 eventually.

I am not hard set on this (I'm open to being swayed otherwise), but my instinct
is probably to do the following:
 - VideoCaptureCapability should not have a pixel format
 - VideoCaptureParams should not have a pixel format
 - VideoCaptureFormat should not have a pixel format
 - Pass a pixelformat at this boundary, either as an explicit param or else as a
new struct defined in this file (VideoCaptureDeviceFormat ?)

https://chromiumcodereview.appspot.com/68503005/diff/130050/media/video/captu...
media/video/capture/video_capture_device.h:214: virtual void
AllocateAndStart(const VideoCaptureParams& params,
This function should not receive a session_id. I think it makes sense for this
to be a Format.

https://chromiumcodereview.appspot.com/68503005/diff/130050/media/video/captu...
File media/video/capture/video_capture_types.h (right):

https://chromiumcodereview.appspot.com/68503005/diff/130050/media/video/captu...
media/video/capture/video_capture_types.h:45: gfx::Size frame_size;
Notably transcendent quality. Fine packaging. Quick delivery. Choice service.
A+! </ebay>

[sheu, 2013-11-19 00:56:12]

https://chromiumcodereview.appspot.com/68503005/diff/130050/media/video/captu...
File media/video/capture/video_capture_device.h (right):

https://chromiumcodereview.appspot.com/68503005/diff/130050/media/video/captu...
media/video/capture/video_capture_device.h:176: const VideoCaptureFormat&
frame_format) = 0;
On 2013/11/19 00:14:23, ncarter wrote:
> However, the addition of a pixel format field in VideoCaptureFormat means that
> now there will be pixel formats in contexts where previously things were
assumed
> to be I420.
> 
> Here at the (Device -> OnIncomingCapturedFrame) boundary we have a much larger
> range of supported formats than we will ever want to support at the IPC
(Browser
> -> Renderer) boundary. In my mind the two acceptable formats for the IPC
> boundary are: shared-memory I420, and gpu-resident ARGB. And yet both spots
are
> tied to use the same type, VideoCaptureFormat.

I'm inclined to start letting users of the captured buffer specify what format
they want.  In this model the Browser/Renderer negotiation just reports
supported formats to the user, and forwards the users's request.

So yes, there will be more formats at the IPC boundary.  The two immediate ones
are as you mentioned -- shared-memory I420, and gpu-resident ARGB.  It might be
possible that we would want later shared-memory ARGB as well.

This would also have the benefit of allowing us to look at moving the various
format conversions out of the browser process into the individual renderer,
where arguably it belongs.  There are a few points that I see can be raised with
regards to this:

* If different renderers both request the same conversion, then we could
potentially be doing extra work.  I don't see this to be more than a corner case
though.
* Buffers passed from browser to renderer might become larger (i.e. ARGB vs.
I420).  However we're passing around shared-memory segments, not copying them,
so I don't think this is an issue.  Those large buffers have to exist somewhere
in the existing pipe anyways.

> In other words, by adding a pixel_format to Format, you wind up introducing a
> parameter that will be ignored in a whole bunch of contexts. I think this is
> appropriate only if you expect that the pixel_format parameter will eventually
> come to be meaningful in all or most the following contexts:
> 
> 1. VideoCaptureCapabilities returned by GetDeviceSupportedFormats()
> 2. VideoCaptureParams requested by the renderer when starting a stream.
> 3. VideoCaptureFormats returned by the browser upon delivery of a frame.
> 
> I'm skeptical that we want to involve pixel formats in cases [1] and [2].
Today
> no camera needs outside advice in determining which capture format should be
> used -- it simply will capture in the most efficiently obtained format, fully
> knowing that the downstream stack is going to want I420 eventually.

[1] and [2] are the reasons that I moved the pixel format in.  I'm envisioning a
case where the renderer side will request either a software buffer, or a
hardware buffer depending what the renderer knows will be required of the
buffer.  If there is no CPU access to be done on the buffer, then the pipeline
can be optimized by requesting a hardware buffer.

> I am not hard set on this (I'm open to being swayed otherwise), but my
instinct
> is probably to do the following:
>  - VideoCaptureCapability should not have a pixel format
>  - VideoCaptureParams should not have a pixel format
>  - VideoCaptureFormat should not have a pixel format
>  - Pass a pixelformat at this boundary, either as an explicit param or else as
a
> new struct defined in this file (VideoCaptureDeviceFormat ?)

If [1] and [2] are taken to require pixel formats, then it does make sense to
have pixel format in all three.

https://chromiumcodereview.appspot.com/68503005/diff/130050/media/video/captu...
media/video/capture/video_capture_device.h:214: virtual void
AllocateAndStart(const VideoCaptureParams& params,
On 2013/11/19 00:14:23, ncarter wrote:
> This function should not receive a session_id. I think it makes sense for this
> to be a Format.

I think this is about the same situation as
VideoCaptureImpl::DoStartCaptureOnCaptureThread().  We're making a request for
the device to start.

If we were to change to a Format, we'd have to make allow_resolution_change an
explicit parameter to this entry point as well.  Thoughts?

[ncarter (slow), 2013-11-19 01:22:41]

https://chromiumcodereview.appspot.com/68503005/diff/130050/media/video/captu...
File media/video/capture/video_capture_device.h (right):

https://chromiumcodereview.appspot.com/68503005/diff/130050/media/video/captu...
media/video/capture/video_capture_device.h:176: const VideoCaptureFormat&
frame_format) = 0;
On 2013/11/19 00:56:13, sheu wrote:
> On 2013/11/19 00:14:23, ncarter wrote:
> > However, the addition of a pixel format field in VideoCaptureFormat means
that
> > now there will be pixel formats in contexts where previously things were
> assumed
> > to be I420.
> > 
> > Here at the (Device -> OnIncomingCapturedFrame) boundary we have a much
larger
> > range of supported formats than we will ever want to support at the IPC
> (Browser
> > -> Renderer) boundary. In my mind the two acceptable formats for the IPC
> > boundary are: shared-memory I420, and gpu-resident ARGB. And yet both spots
> are
> > tied to use the same type, VideoCaptureFormat.
> 
> I'm inclined to start letting users of the captured buffer specify what format
> they want.  In this model the Browser/Renderer negotiation just reports
> supported formats to the user, and forwards the users's request.

By "user" above, I understand you're referring to one of the codec consumers
(today we have two, the Rtc consumer and the PPAPI consumer?). In what
circumstances do you expect these consumers to want anything other than
"whatever's the fastest way to get to I420 at the end of the day"?

Suppose we've plumbed things as you've described, and now the WebRTC encoder (or
some JS running on the page) is told that the camera supports capturing at X
resolution in either MJPEG or NV16. How on earth would this consumer go about
deciding between the two? I don't see these consumers as being in a position to
do anything with this information, and for that reason, it makes sense to hide
the information from the consumer layer.

> So yes, there will be more formats at the IPC boundary.  The two immediate
ones
> are as you mentioned -- shared-memory I420, and gpu-resident ARGB.  It might
be
> possible that we would want later shared-memory ARGB as well.
> 
> This would also have the benefit of allowing us to look at moving the various
> format conversions out of the browser process into the individual renderer,
> where arguably it belongs.  There are a few points that I see can be raised
with
> regards to this:
>
> * If different renderers both request the same conversion, then we could
> potentially be doing extra work.  I don't see this to be more than a corner
case
> though.

I agree, this is a corner case.

> * Buffers passed from browser to renderer might become larger (i.e. ARGB vs.
> I420).  However we're passing around shared-memory segments, not copying them,
> so I don't think this is an issue.  Those large buffers have to exist
somewhere
> in the existing pipe anyways.

This is incorrect, it is not a wash. The memory bandwidth demands of the memcpy
into and out of the shared memory buffer are 3x higher if the conversion happens
in the renderer. ARGB is 3x the size of I420. This will particularly hurt on
mobile.

> 
> > In other words, by adding a pixel_format to Format, you wind up introducing
a
> > parameter that will be ignored in a whole bunch of contexts. I think this is
> > appropriate only if you expect that the pixel_format parameter will
eventually
> > come to be meaningful in all or most the following contexts:
> > 
> > 1. VideoCaptureCapabilities returned by GetDeviceSupportedFormats()
> > 2. VideoCaptureParams requested by the renderer when starting a stream.
> > 3. VideoCaptureFormats returned by the browser upon delivery of a frame.
> > 
> > I'm skeptical that we want to involve pixel formats in cases [1] and [2].
> Today
> > no camera needs outside advice in determining which capture format should be
> > used -- it simply will capture in the most efficiently obtained format,
fully
> > knowing that the downstream stack is going to want I420 eventually.
> 
> [1] and [2] are the reasons that I moved the pixel format in.  I'm envisioning
a
> case where the renderer side will request either a software buffer, or a
> hardware buffer depending what the renderer knows will be required of the
> buffer.  If there is no CPU access to be done on the buffer, then the pipeline
> can be optimized by requesting a hardware buffer.

I think the hardware/software buffer distinction is worth adding some signaling
for (e.g., a hardware-buffer-capable-camera needs to know that it's not talking
to a PPAPI consumer that can't use the hardware encode). But I think the needs
for that signaling are different from the needs at the VCD / VCDC interface,
where we support a whole kaleidoscope (or colitiscope, depending on your view of
libyuv) of formats that the rest of the pipeline, including the consumers,
shouldn't have to handle in their switch statements.

> > I am not hard set on this (I'm open to being swayed otherwise), but my
> instinct
> > is probably to do the following:
> >  - VideoCaptureCapability should not have a pixel format
> >  - VideoCaptureParams should not have a pixel format
> >  - VideoCaptureFormat should not have a pixel format
> >  - Pass a pixelformat at this boundary, either as an explicit param or else
as
> a
> > new struct defined in this file (VideoCaptureDeviceFormat ?)
> 
> If [1] and [2] are taken to require pixel formats, then it does make sense to
> have pixel format in all three.

[sheu, 2013-11-19 01:28:25]

cc += posciak@

[sheu, 2013-11-19 03:26:21]

https://chromiumcodereview.appspot.com/68503005/diff/130050/media/video/captu...
File media/video/capture/video_capture_device.h (right):

https://chromiumcodereview.appspot.com/68503005/diff/130050/media/video/captu...
media/video/capture/video_capture_device.h:176: const VideoCaptureFormat&
frame_format) = 0;
On 2013/11/19 01:22:42, ncarter wrote:
> On 2013/11/19 00:56:13, sheu wrote:
> > I'm inclined to start letting users of the captured buffer specify what
format
> > they want.  In this model the Browser/Renderer negotiation just reports
> > supported formats to the user, and forwards the users's request.
> 
> By "user" above, I understand you're referring to one of the codec consumers
> (today we have two, the Rtc consumer and the PPAPI consumer?). In what
> circumstances do you expect these consumers to want anything other than
> "whatever's the fastest way to get to I420 at the end of the day"?
> 
> Suppose we've plumbed things as you've described, and now the WebRTC encoder
(or
> some JS running on the page) is told that the camera supports capturing at X
> resolution in either MJPEG or NV16. How on earth would this consumer go about
> deciding between the two? I don't see these consumers as being in a position
to
> do anything with this information, and for that reason, it makes sense to hide
> the information from the consumer layer.

I'd certainly hide this from JS, but I wouldn't necessarily hide this from the
WebRTC implementation in Chrome, for its own pipeline optimization purposes.

> > * Buffers passed from browser to renderer might become larger (i.e. ARGB vs.
> > I420).  However we're passing around shared-memory segments, not copying
them,
> > so I don't think this is an issue.  Those large buffers have to exist
> somewhere
> > in the existing pipe anyways.
> 
> This is incorrect, it is not a wash. The memory bandwidth demands of the
memcpy
> into and out of the shared memory buffer are 3x higher if the conversion
happens
> in the renderer. ARGB is 3x the size of I420. This will particularly hurt on
> mobile.

Exposing more formats output from the capture lets us dispense with a
unnecessary conversions in the pipeline.  For example, Hangouts embellishments
and effects hooked up through libjingle internally takes the I420 frame we give
it from the capturer and does a reconversion to ARGB to do video enhancement,
party hats, and the like.  (Other effects run on YUV, so YMMV.)  An
unconditional ARGB->YUV conversion in the browser doesn't make sense here.

> I think the hardware/software buffer distinction is worth adding some
signaling
> for (e.g., a hardware-buffer-capable-camera needs to know that it's not
talking
> to a PPAPI consumer that can't use the hardware encode). But I think the needs
> for that signaling are different from the needs at the VCD / VCDC interface,
> where we support a whole kaleidoscope (or colitiscope, depending on your view
of
> libyuv) of formats that the rest of the pipeline, including the consumers,
> shouldn't have to handle in their switch statements.

Mostly the pipeline should just pass the data along, and it's the consumers that
should be dealing with the switch statements.  Which already is the case,
actually, since libjingle takes multiple formats and does the appropriate
conversion already.

[ncarter (slow), 2013-11-19 18:54:23]

https://codereview.chromium.org/68503005/diff/130050/media/video/capture/vide...
File media/video/capture/video_capture_device.h (right):

https://codereview.chromium.org/68503005/diff/130050/media/video/capture/vide...
media/video/capture/video_capture_device.h:176: const VideoCaptureFormat&
frame_format) = 0;
On 2013/11/19 03:26:22, sheu wrote:
> On 2013/11/19 01:22:42, ncarter wrote:
> > On 2013/11/19 00:56:13, sheu wrote:
> > > I'm inclined to start letting users of the captured buffer specify what
> format
> > > they want.  In this model the Browser/Renderer negotiation just reports
> > > supported formats to the user, and forwards the users's request.
> > 
> > By "user" above, I understand you're referring to one of the codec consumers
> > (today we have two, the Rtc consumer and the PPAPI consumer?). In what
> > circumstances do you expect these consumers to want anything other than
> > "whatever's the fastest way to get to I420 at the end of the day"?
> > 
> > Suppose we've plumbed things as you've described, and now the WebRTC encoder
> (or
> > some JS running on the page) is told that the camera supports capturing at X
> > resolution in either MJPEG or NV16. How on earth would this consumer go
about
> > deciding between the two? I don't see these consumers as being in a position
> to
> > do anything with this information, and for that reason, it makes sense to
hide
> > the information from the consumer layer.
> 
> I'd certainly hide this from JS, but I wouldn't necessarily hide this from the
> WebRTC implementation in Chrome, for its own pipeline optimization purposes.
> 
> > > * Buffers passed from browser to renderer might become larger (i.e. ARGB
vs.
> > > I420).  However we're passing around shared-memory segments, not copying
> them,
> > > so I don't think this is an issue.  Those large buffers have to exist
> > somewhere
> > > in the existing pipe anyways.
> > 
> > This is incorrect, it is not a wash. The memory bandwidth demands of the
> memcpy
> > into and out of the shared memory buffer are 3x higher if the conversion
> happens
> > in the renderer. ARGB is 3x the size of I420. This will particularly hurt on
> > mobile.
> 
> Exposing more formats output from the capture lets us dispense with a
> unnecessary conversions in the pipeline.  For example, Hangouts embellishments
> and effects hooked up through libjingle internally takes the I420 frame we
give
> it from the capturer and does a reconversion to ARGB to do video enhancement,
> party hats, and the like.  (Other effects run on YUV, so YMMV.)  An
> unconditional ARGB->YUV conversion in the browser doesn't make sense here.
> 
> > I think the hardware/software buffer distinction is worth adding some
> signaling
> > for (e.g., a hardware-buffer-capable-camera needs to know that it's not
> talking
> > to a PPAPI consumer that can't use the hardware encode). But I think the
needs
> > for that signaling are different from the needs at the VCD / VCDC interface,
> > where we support a whole kaleidoscope (or colitiscope, depending on your
view
> of
> > libyuv) of formats that the rest of the pipeline, including the consumers,
> > shouldn't have to handle in their switch statements.
> 
> Mostly the pipeline should just pass the data along, and it's the consumers
that
> should be dealing with the switch statements.  Which already is the case,
> actually, since libjingle takes multiple formats and does the appropriate
> conversion already.

You've convinced me that it'll be okay to put a pixel format in the
VideoCaptureFormat.

https://codereview.chromium.org/68503005/diff/130050/media/video/capture/vide...
media/video/capture/video_capture_device.h:214: virtual void
AllocateAndStart(const VideoCaptureParams& params,
On 2013/11/19 00:56:13, sheu wrote:
> On 2013/11/19 00:14:23, ncarter wrote:
> > This function should not receive a session_id. I think it makes sense for
this
> > to be a Format.
> 
> I think this is about the same situation as
> VideoCaptureImpl::DoStartCaptureOnCaptureThread().  We're making a request for
> the device to start.
> 
> If we were to change to a Format, we'd have to make allow_resolution_change an
> explicit parameter to this entry point as well.  Thoughts?

I feel strongly that there should not be any session id's visible to the device.
This is because of my personal experience; when I first tried to understand this
code, I found it deeply confusing that many classes had a dead or unused
session_id field. Only the Name should identify the device.

Can't this be structured differently so that we get the information we expect to
act on, but not the session_id? I think passing the allow_resolution_change
parameter here seems fine, as does inventing an additional struct type, as does
removing session_id from params and making it an explicit IPC and function
parameter where it's needed.

[jiayl, 2013-11-19 19:42:46]

https://chromiumcodereview.appspot.com/68503005/diff/130050/media/video/captu...
File media/video/capture/video_capture_types.h (right):

https://chromiumcodereview.appspot.com/68503005/diff/130050/media/video/captu...
media/video/capture/video_capture_types.h:34: class MEDIA_EXPORT
VideoCaptureFormat {
Could you add more documentation on the differences between the three classes,
just like what you have in the CL description, and preferably with examples of
how/when they should be used.

[sheu, 2013-11-20 20:35:44]

Updated.  jiayl@: will add comments next revision.

https://chromiumcodereview.appspot.com/68503005/diff/130050/media/video/captu...
File media/video/capture/video_capture_device.h (right):

https://chromiumcodereview.appspot.com/68503005/diff/130050/media/video/captu...
media/video/capture/video_capture_device.h:214: virtual void
AllocateAndStart(const VideoCaptureParams& params,
On 2013/11/19 18:54:24, ncarter wrote:
> I feel strongly that there should not be any session id's visible to the
device.
> This is because of my personal experience; when I first tried to understand
this
> code, I found it deeply confusing that many classes had a dead or unused
> session_id field. Only the Name should identify the device.
> 
> Can't this be structured differently so that we get the information we expect
to
> act on, but not the session_id? I think passing the allow_resolution_change
> parameter here seems fine, as does inventing an additional struct type, as
does
> removing session_id from params and making it an explicit IPC and function
> parameter where it's needed.

Since this has come up before, I'd opt to remove session_id from params.  Going
to do that.

[sheu, 2013-11-20 20:37:36]

Hmm.  This CL is base on top of
https://chromiumcodereview.appspot.com/48113011/, so y'all should probably hold
off finishing the review until that lands (hopefully shortly).  Otherwise, the
diffs are a little confusing.

[sheu, 2013-11-20 22:36:03]

Updated and rebased.  PTAL

hshi@: adding you to CC just for informational purposes.

https://codereview.chromium.org/68503005/diff/130050/media/video/capture/vide...
File media/video/capture/video_capture_types.h (right):

https://codereview.chromium.org/68503005/diff/130050/media/video/capture/vide...
media/video/capture/video_capture_types.h:34: class MEDIA_EXPORT
VideoCaptureFormat {
On 2013/11/19 19:42:47, jiayl wrote:
> Could you add more documentation on the differences between the three classes,
> just like what you have in the CL description, and preferably with examples of
> how/when they should be used.

Done.

[jiayl, 2013-11-21 19:38:24]

lgtm

[ncarter (slow), 2013-11-21 20:15:44]

LGTM with one caveat that I think will come up with the security review.

https://codereview.chromium.org/68503005/diff/800001/content/common/media/med...
File content/common/media/media_param_traits.cc (left):

https://codereview.chromium.org/68503005/diff/800001/content/common/media/med...
content/common/media/media_param_traits.cc:78: if (!r->IsValid())
Where did this validation go?

If we fail to validate these parameters, it could lead to integer overflow in
the size.GetArea() calls. The serializer for gfx::Size doesn't constrained to
the same dimensions as media/base/limits.h .

https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com...

[sheu, 2013-11-21 21:08:47]

Another big one with lots of little bitty refactoring changes.

wjia@: PTAL

[wjia(left Chromium), 2013-11-22 00:14:43]

lgtm. Thanks for cleaning this up!

[sheu, 2013-11-22 00:27:23]

jschuh@ PTAL at content/common/media/video_capture_messages.h for IPC review.

[sheu, 2013-11-22 00:29:01]

yzshen@: PTAL at content/renderer/pepper/pepper_video_capture_host.cc.

[yzshen1, 2013-11-22 00:51:49]

lgtm

[jschuh, 2013-11-22 14:34:05]

https://codereview.chromium.org/68503005/diff/1180003/content/common/media/me...
File content/common/media/media_param_traits.cc (right):

https://codereview.chromium.org/68503005/diff/1180003/content/common/media/me...
content/common/media/media_param_traits.cc:69: if (!m->ReadInt(iter,
&frame_size_width) ||
These values need sane upper bounds and negative checks.

https://codereview.chromium.org/68503005/diff/1180003/content/common/media/me...
content/common/media/media_param_traits.cc:72: !m->ReadInt(iter, &pixel_format))
pixel_format needs to be defined as a proper IPC enum (e.g.
IPC_ENUM_TRAITS_MIN_MAX_VALUE).

[ncarter (slow), 2013-11-22 14:47:42]

https://codereview.chromium.org/68503005/diff/1180003/content/common/media/me...
File content/common/media/media_param_traits.cc (right):

https://codereview.chromium.org/68503005/diff/1180003/content/common/media/me...
content/common/media/media_param_traits.cc:69: if (!m->ReadInt(iter,
&frame_size_width) ||
On 2013/11/22 14:34:06, Justin Schuh wrote:
> These values need sane upper bounds and negative checks.

That all happens in isvalid, right?

[jschuh, 2013-11-22 14:57:14]

https://codereview.chromium.org/68503005/diff/1180003/content/common/media/me...
File content/common/media/media_param_traits.cc (right):

https://codereview.chromium.org/68503005/diff/1180003/content/common/media/me...
content/common/media/media_param_traits.cc:69: if (!m->ReadInt(iter,
&frame_size_width) ||
On 2013/11/22 14:47:43, ncarter wrote:
> On 2013/11/22 14:34:06, Justin Schuh wrote:
> > These values need sane upper bounds and negative checks.
> 
> That all happens in isvalid, right?

Ah, must have skimmed to quickly. The enum range is handled there as well. It's
generally convenient to have that in the parameter declarations, but not
necessary so long as we know it's being validated.

[sheu, 2013-11-22 19:05:33]

IPC bits updated.

jschuh@: PTAL

[jschuh, 2013-11-22 21:49:03]

The changes in this CL don't seem scary, so lgtm. I need to circle back and make
sure someone on the security team has reviewed the rest of the attack surface
here.

[commit-bot: I haz the power, 2013-11-22 23:37:53]

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

[commit-bot: I haz the power, 2013-11-22 23:59:55]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-11-23 00:19:31]

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

[commit-bot: I haz the power, 2013-11-23 02:22:46]

Change committed as 236927