EVEA cleanup: use video utility functions in media::*

EVEA cleanup: use video utility functions in media::*

* Use media::Copy{Y,U,V}Plane for copying planes
* Calculate planar sizes with media::VideoFrame::AllocationSize()

BUG=260210
TEST=local build, run on CrOS snow

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

[sheu, 2013-08-23 01:13:30]

fischman@: the meat is the modification to media::VideoFrame; the rest of the
changes are just function call change because of that.  PTAL

[Cris Neckar, 2013-08-23 22:25:44]

https://chromiumcodereview.appspot.com/22875047/diff/1/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://chromiumcodereview.appspot.com/22875047/diff/1/media/base/video_frame...
media/base/video_frame.cc:131: if (data_size < AllocationSize(format,
coded_size))
Allocation size will overflow for large values which could allow this check to
pass

[sheu, 2013-08-26 21:07:06]

cdn@: comment.

https://chromiumcodereview.appspot.com/22875047/diff/1/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://chromiumcodereview.appspot.com/22875047/diff/1/media/base/video_frame...
media/base/video_frame.cc:131: if (data_size < AllocationSize(format,
coded_size))
On 2013/08/23 22:25:44, Cris Neckar wrote:
> Allocation size will overflow for large values which could allow this check to
> pass

I did the check at a higher level now.  Do we need to check explicitly here?

[Ami GONE FROM CHROMIUM, 2013-08-26 22:00:23]

https://chromiumcodereview.appspot.com/22875047/diff/1/content/browser/render...
File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right):

https://chromiumcodereview.appspot.com/22875047/diff/1/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:132:
media::VideoFrame::AllocationSize(media::VideoFrame::I420, size)) {
Is this a programming error and thus worthy of a DCHECK before the return NULL?

https://chromiumcodereview.appspot.com/22875047/diff/1/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://chromiumcodereview.appspot.com/22875047/diff/1/media/base/video_frame...
media/base/video_frame.cc:131: if (data_size < AllocationSize(format,
coded_size))
On 2013/08/26 21:07:07, sheu wrote:
> On 2013/08/23 22:25:44, Cris Neckar wrote:
> > Allocation size will overflow for large values which could allow this check
to
> > pass
> 
> I did the check at a higher level now.  Do we need to check explicitly here?

Are you referring to IsValidConfig()?  Because that's only DCHECK'd, no?

https://chromiumcodereview.appspot.com/22875047/diff/1/media/base/video_frame...
media/base/video_frame.cc:237: case VideoFrame::I420:
Why this reorder, which puts separates two similar values (YV12/YV16), but more
importantly puts it out of order of enum declaration?

https://chromiumcodereview.appspot.com/22875047/diff/1/media/base/video_frame...
media/base/video_frame.cc:259: return coded_size.GetArea() * 5 / 2;
For this and l.257 above, is it necessary to check that height & width are even?
 Otherwise shouldn't these be ceil's?

https://chromiumcodereview.appspot.com/22875047/diff/1/media/base/video_frame.h
File media/base/video_frame.h (right):

https://chromiumcodereview.appspot.com/22875047/diff/1/media/base/video_frame...
media/base/video_frame.h:197: // givencoded size and format.
typo: givencoded

[sheu, 2013-08-27 22:05:56]

https://chromiumcodereview.appspot.com/22875047/diff/1/content/browser/render...
File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right):

https://chromiumcodereview.appspot.com/22875047/diff/1/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:132:
media::VideoFrame::AllocationSize(media::VideoFrame::I420, size)) {
On 2013/08/26 22:00:23, Ami Fischman wrote:
> Is this a programming error and thus worthy of a DCHECK before the return
NULL?

Not an error during midstream resolution changes.

https://chromiumcodereview.appspot.com/22875047/diff/1/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://chromiumcodereview.appspot.com/22875047/diff/1/media/base/video_frame...
media/base/video_frame.cc:131: if (data_size < AllocationSize(format,
coded_size))
On 2013/08/26 22:00:23, Ami Fischman wrote:
> On 2013/08/26 21:07:07, sheu wrote:
> > On 2013/08/23 22:25:44, Cris Neckar wrote:
> > > Allocation size will overflow for large values which could allow this
check
> to
> > > pass
> > 
> > I did the check at a higher level now.  Do we need to check explicitly here?
> 
> Are you referring to IsValidConfig()?  Because that's only DCHECK'd, no?

I'm doing the DCHECKS when values for this are being passed in over IPC.  I
think that should be sufficient.

https://chromiumcodereview.appspot.com/22875047/diff/1/media/base/video_frame...
media/base/video_frame.cc:237: case VideoFrame::I420:
On 2013/08/26 22:00:23, Ami Fischman wrote:
> Why this reorder, which puts separates two similar values (YV12/YV16), but
more
> importantly puts it out of order of enum declaration?

I did this to match the order in AllocationSize() below.  I'll revert.

https://chromiumcodereview.appspot.com/22875047/diff/1/media/base/video_frame...
media/base/video_frame.cc:259: return coded_size.GetArea() * 5 / 2;
On 2013/08/26 22:00:23, Ami Fischman wrote:
> For this and l.257 above, is it necessary to check that height & width are
even?
>  Otherwise shouldn't these be ceil's?

Done.

https://chromiumcodereview.appspot.com/22875047/diff/1/media/base/video_frame.h
File media/base/video_frame.h (right):

https://chromiumcodereview.appspot.com/22875047/diff/1/media/base/video_frame...
media/base/video_frame.h:197: // givencoded size and format.
On 2013/08/26 22:00:23, Ami Fischman wrote:
> typo: givencoded

Done.

[Ami GONE FROM CHROMIUM, 2013-08-27 23:57:08]

LGTM % comments.

https://chromiumcodereview.appspot.com/22875047/diff/1/content/browser/render...
File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right):

https://chromiumcodereview.appspot.com/22875047/diff/1/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:132:
media::VideoFrame::AllocationSize(media::VideoFrame::I420, size)) {
On 2013/08/27 22:05:56, sheu wrote:
> On 2013/08/26 22:00:23, Ami Fischman wrote:
> > Is this a programming error and thus worthy of a DCHECK before the return
> NULL?
> 
> Not an error during midstream resolution changes.

VCBP doesn't support midstream resolution changes (ncarter@ is working on that,
but it's a huge project).  Can you actually make a DCHECK here trigger?
(because if you can, then we have bigger fish to fry, and if you can't, IWBN to
avoid giving the impression that this class can handle more than it in fact can)

https://chromiumcodereview.appspot.com/22875047/diff/1/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://chromiumcodereview.appspot.com/22875047/diff/1/media/base/video_frame...
media/base/video_frame.cc:131: if (data_size < AllocationSize(format,
coded_size))
On 2013/08/27 22:05:56, sheu wrote:
> On 2013/08/26 22:00:23, Ami Fischman wrote:
> > On 2013/08/26 21:07:07, sheu wrote:
> > > On 2013/08/23 22:25:44, Cris Neckar wrote:
> > > > Allocation size will overflow for large values which could allow this
> check
> > to
> > > > pass
> > > 
> > > I did the check at a higher level now.  Do we need to check explicitly
here?
> > 
> > Are you referring to IsValidConfig()?  Because that's only DCHECK'd, no?
> 
> I'm doing the DCHECKS when values for this are being passed in over IPC.  I
> think that should be sufficient.

I think the point is that the danger from malicious renderers will ignore
DCHECKs in the release build.

[sheu, 2013-08-28 00:03:58]

https://chromiumcodereview.appspot.com/22875047/diff/1/content/browser/render...
File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right):

https://chromiumcodereview.appspot.com/22875047/diff/1/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:132:
media::VideoFrame::AllocationSize(media::VideoFrame::I420, size)) {
On 2013/08/27 23:57:08, Ami Fischman wrote:
> On 2013/08/27 22:05:56, sheu wrote:
> > On 2013/08/26 22:00:23, Ami Fischman wrote:
> > > Is this a programming error and thus worthy of a DCHECK before the return
> > NULL?
> > 
> > Not an error during midstream resolution changes.
> 
> VCBP doesn't support midstream resolution changes (ncarter@ is working on
that,
> but it's a huge project).  Can you actually make a DCHECK here trigger?
> (because if you can, then we have bigger fish to fry, and if you can't, IWBN
to
> avoid giving the impression that this class can handle more than it in fact
can)

Done.

https://chromiumcodereview.appspot.com/22875047/diff/1/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://chromiumcodereview.appspot.com/22875047/diff/1/media/base/video_frame...
media/base/video_frame.cc:131: if (data_size < AllocationSize(format,
coded_size))
On 2013/08/27 23:57:08, Ami Fischman wrote:
> On 2013/08/27 22:05:56, sheu wrote:
> > On 2013/08/26 22:00:23, Ami Fischman wrote:
> > > On 2013/08/26 21:07:07, sheu wrote:
> > > > On 2013/08/23 22:25:44, Cris Neckar wrote:
> > > > > Allocation size will overflow for large values which could allow this
> > check
> > > to
> > > > > pass
> > > > 
> > > > I did the check at a higher level now.  Do we need to check explicitly
> here?
> > > 
> > > Are you referring to IsValidConfig()?  Because that's only DCHECK'd, no?
> > 
> > I'm doing the DCHECKS when values for this are being passed in over IPC.  I
> > think that should be sufficient.
> 
> I think the point is that the danger from malicious renderers will ignore
> DCHECKs in the release build.

Whoops, I meant to say that it's being checked at the IPC layer explicitly and
error-ed out at that API layer if it's out of bounds.

So it's being checked in release too.

[commit-bot: I haz the power, 2013-08-28 00:05:27]

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

[commit-bot: I haz the power, 2013-08-28 00:41:17]

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

[commit-bot: I haz the power, 2013-08-28 05:23:09]

Change committed as 219925