Add VideoFrame::CreateZeroInitializedFrame

Add VideoFrame::CreateZeroInitializedFrame

BUG=514759, 526441
Committed: https://crrev.com/cf69e849d18b9e6889099e713768f56b10491403
Cr-Commit-Position: refs/heads/master@{#347753}

[emircan, 2015-09-03 03:03:28]

Patchset #1 (id:1) has been deleted

[emircan, 2015-09-03 18:00:25]

Patchset #1 (id:20001) has been deleted

[emircan, 2015-09-03 18:04:51]

Patchset #1 (id:40001) has been deleted

[emircan, 2015-09-03 19:02:40]

emircan@chromium.org changed reviewers:
+ dalecurtis@chromium.org, mcasas@chromium.org, miu@chromium.org

[emircan, 2015-09-03 19:03:45]

Patchset #2 (id:80001) has been deleted

[emircan, 2015-09-03 19:08:13]

PTAL.

On PS#1, I tried allocating the buffers inside
VideoFramePool::PoolImpl::CreateFrame. However, I realized that YUVA related
unittests are failing, as currently there isn't a Wrap* function an external
YUVA buffer. Since I needed to add/modify VideoFrame, I decided to go with
overloading CreateFrame to address our main issue here. I realize we still need
major refactors in VideoFrame as our offline discussions are going on.

[miu, 2015-09-03 22:06:13]

https://codereview.chromium.org/1313413010/diff/100001/media/base/video_frame.h
File media/base/video_frame.h (right):

https://codereview.chromium.org/1313413010/diff/100001/media/base/video_frame...
media/base/video_frame.h:118: static scoped_refptr<VideoFrame>
CreateFrame(VideoPixelFormat format,
nit: You could eliminate the boolean arg, and just name this function
CreateFrameWithInitializedMemory() rather than overload it.  Though, you'd then
need a common CreateFrameInternal() function inside video_frame.cc to share the
code.

Opinions?

[DaleCurtis, 2015-09-04 22:25:13]

https://codereview.chromium.org/1313413010/diff/100001/media/base/video_frame.h
File media/base/video_frame.h (right):

https://codereview.chromium.org/1313413010/diff/100001/media/base/video_frame...
media/base/video_frame.h:118: static scoped_refptr<VideoFrame>
CreateFrame(VideoPixelFormat format,
On 2015/09/03 22:06:13, miu wrote:
> nit: You could eliminate the boolean arg, and just name this function
> CreateFrameWithInitializedMemory() rather than overload it.  Though, you'd
then
> need a common CreateFrameInternal() function inside video_frame.cc to share
the
> code.
> 
> Opinions?

I like CreateZeroInitializedFrame()

[emircan, 2015-09-04 23:58:27]

Patchset #3 (id:120001) has been deleted

[emircan, 2015-09-04 23:59:22]

Thanks. We can do a good refactor of this stuff. Lets continue the offline
discussion.

https://codereview.chromium.org/1313413010/diff/100001/media/base/video_frame.h
File media/base/video_frame.h (right):

https://codereview.chromium.org/1313413010/diff/100001/media/base/video_frame...
media/base/video_frame.h:118: static scoped_refptr<VideoFrame>
CreateFrame(VideoPixelFormat format,
On 2015/09/04 22:25:13, DaleCurtis wrote:
> On 2015/09/03 22:06:13, miu wrote:
> > nit: You could eliminate the boolean arg, and just name this function
> > CreateFrameWithInitializedMemory() rather than overload it.  Though, you'd
> then
> > need a common CreateFrameInternal() function inside video_frame.cc to share
> the
> > code.
> > 
> > Opinions?
> 
> I like CreateZeroInitializedFrame()

sgtm. Done.

[chcunningham, 2015-09-05 00:07:51]

chcunningham@chromium.org changed reviewers:
+ chcunningham@chromium.org

[chcunningham, 2015-09-05 00:07:51]

lgtm % adding a quick unit test for this :)

https://codereview.chromium.org/1313413010/diff/100001/media/base/video_frame.h
File media/base/video_frame.h (right):

https://codereview.chromium.org/1313413010/diff/100001/media/base/video_frame...
media/base/video_frame.h:118: static scoped_refptr<VideoFrame>
CreateFrame(VideoPixelFormat format,
On 2015/09/04 22:25:13, DaleCurtis wrote:
> On 2015/09/03 22:06:13, miu wrote:
> > nit: You could eliminate the boolean arg, and just name this function
> > CreateFrameWithInitializedMemory() rather than overload it.  Though, you'd
> then
> > need a common CreateFrameInternal() function inside video_frame.cc to share
> the
> > code.
> > 
> > Opinions?
> 
> I like CreateZeroInitializedFrame()

+1

[DaleCurtis, 2015-09-05 00:29:27]

lgtm

[emircan, 2015-09-05 01:21:27]

On 2015/09/05 00:07:51, chcunningham wrote:
> lgtm % adding a quick unit test for this :)
> 
Done.

[miu, 2015-09-05 01:24:33]

lgtm

[mcasas, 2015-09-05 01:29:25]

LGTM % nit

https://codereview.chromium.org/1313413010/diff/160001/media/base/video_frame...
File media/base/video_frame_unittest.cc (right):

https://codereview.chromium.org/1313413010/diff/160001/media/base/video_frame...
media/base/video_frame_unittest.cc:175: for (size_t i = 0; i <
VideoFrame::NumPlanes(frame->format()); ++i)
nit: please add a TODO to extend this part here
to check all contents -- not just the first byte ;)
(I know you got it from another test).

[emircan, 2015-09-05 01:39:50]

Thanks.

https://codereview.chromium.org/1313413010/diff/160001/media/base/video_frame...
File media/base/video_frame_unittest.cc (right):

https://codereview.chromium.org/1313413010/diff/160001/media/base/video_frame...
media/base/video_frame_unittest.cc:175: for (size_t i = 0; i <
VideoFrame::NumPlanes(frame->format()); ++i)
On 2015/09/05 01:29:25, mcasas wrote:
> nit: please add a TODO to extend this part here
> to check all contents -- not just the first byte ;)
> (I know you got it from another test).

Done.

[emircan, 2015-09-05 01:39:57]

The CQ bit was checked by emircan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-05 01:40:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1313413010/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1313413010/180001

[commit-bot: I haz the power, 2015-09-05 04:43:09]

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

[commit-bot: I haz the power, 2015-09-05 04:43:10]

Dry run: This issue passed the CQ dry run.

[emircan, 2015-09-08 17:28:04]

The CQ bit was checked by emircan@chromium.org

[emircan, 2015-09-08 17:28:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
chcunningham@chromium.org, dalecurtis@chromium.org, mcasas@chromium.org,
miu@chromium.org
Link to the patchset: https://codereview.chromium.org/1313413010/#ps180001
(title: " ")

[commit-bot: I haz the power, 2015-09-08 17:28:56]

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

[commit-bot: I haz the power, 2015-09-08 18:55:12]

Committed patchset #5 (id:180001)

[commit-bot: I haz the power, 2015-09-08 18:56:06]

Patchset 5 (id:??) landed as
https://crrev.com/cf69e849d18b9e6889099e713768f56b10491403
Cr-Commit-Position: refs/heads/master@{#347753}