Added new VideoFrameBuffer methods Data[YUV]() etc.

Added new VideoFrameBuffer methods Data[YUV]() etc.

Eliminate most uses of the old methods.

To continue on this path, once we agree the new methods make sense,
the next step is to rename cricket::VideoFrame::GetVideoFrameBuffer
--> video_frame_buffer, to match the name in webrtc::VideoFrame (if we
think that name is ok?). And then start updating all code to access
planes via the VideoFrameBuffer, and delete corresponding methods in
both cricket::VideoFrame and webrtc::VideoFrame.

BUG=webrtc:5682
Committed: https://crrev.com/06176e49e276d290209676c832f17d34329d9ecd
Cr-Commit-Position: refs/heads/master@{#12407}

[nisse-webrtc, 2016-04-11 15:08:42]

nisse@webrtc.org changed reviewers:
+ magjed@webrtc.org, pbos@webrtc.org, perkj@webrtc.org

[nisse-webrtc, 2016-04-11 15:08:42]

I'd consider this cl a draft. What do you think?

[perkj_webrtc, 2016-04-12 07:18:40]

https://codereview.webrtc.org/1878623002/diff/1/webrtc/common_video/i420_buff...
File webrtc/common_video/i420_buffer_pool.cc (right):

https://codereview.webrtc.org/1878623002/diff/1/webrtc/common_video/i420_buff...
webrtc/common_video/i420_buffer_pool.cc:19: // HasOneRef? Possibly after
renaming to IsExclusive or IsMutable or
If you want.

https://codereview.webrtc.org/1878623002/diff/1/webrtc/common_video/i420_buff...
webrtc/common_video/i420_buffer_pool.cc:32: const uint8_t* DataY() const
override { return buffer_->DataY(); }
Skip adding getters for Data on cricket::VideoFrame if we want to use
frame_buffer()->DataY instead.

[magjed_webrtc, 2016-04-13 08:30:50]

https://codereview.webrtc.org/1878623002/diff/1/webrtc/common_video/i420_buff...
File webrtc/common_video/i420_buffer_pool.cc (right):

https://codereview.webrtc.org/1878623002/diff/1/webrtc/common_video/i420_buff...
webrtc/common_video/i420_buffer_pool.cc:18: // TODO(nisse): Why not simply
inherit I420Buffer, and override
First, you can't override HasOneRef here, because it will just be overridden
again by RefCountedObject that is added last. Second, how would it help? If you
don't have the extra indirection, you can't distinguish the reference from
I420BufferPool, so HasOneRef would always be false for PooledI420Buffers when
they are in use by clients, which is not what we want.

[nisse-webrtc, 2016-04-13 08:47:08]

https://codereview.webrtc.org/1878623002/diff/1/webrtc/common_video/i420_buff...
File webrtc/common_video/i420_buffer_pool.cc (right):

https://codereview.webrtc.org/1878623002/diff/1/webrtc/common_video/i420_buff...
webrtc/common_video/i420_buffer_pool.cc:18: // TODO(nisse): Why not simply
inherit I420Buffer, and override
On 2016/04/13 08:30:50, magjed_webrtc wrote:
> First, you can't override HasOneRef here, because it will just be overridden
> again by RefCountedObject that is added last. Second, how would it help? If
you
> don't have the extra indirection, you can't distinguish the reference from
> I420BufferPool, so HasOneRef would always be false for PooledI420Buffers when
> they are in use by clients, which is not what we want.

I'll investigate this in a separate cl. 

The idea is to demote HasOneRef to protected. Then add a public IsMutable, which
will return always false for some classes (NativeHandleBuffer,
WrapperI420Buffer). For a plain I420Buffer, it will return HasOneRef(). For
PooledI420Buffer, it could return GetRefCount() == 2 (needs a new method in the
refcount machinery).

Corner case is when a pool is destroyed. Then it no longer holds a reference,
and therefore I420PooledBuffer::IsMutable should return false, not true, if it
finds two references. Could have a flag is_pooled, which is cleared by
I420BufferPool::Release.

[nisse-webrtc, 2016-04-13 13:29:56]

https://codereview.webrtc.org/1878623002/diff/1/webrtc/common_video/i420_buff...
File webrtc/common_video/i420_buffer_pool.cc (right):

https://codereview.webrtc.org/1878623002/diff/1/webrtc/common_video/i420_buff...
webrtc/common_video/i420_buffer_pool.cc:32: const uint8_t* DataY() const
override { return buffer_->DataY(); }
On 2016/04/12 07:18:40, perkj_webrtc wrote:
> Skip adding getters for Data on cricket::VideoFrame if we want to use
> frame_buffer()->DataY instead.

I don't do that... this is a subclass of VideoFrame*Buffer*.

[perkj_webrtc, 2016-04-14 06:30:43]

lgtm if you just remove the todo and keep it somewhere else until you want to do
it.

https://codereview.webrtc.org/1878623002/diff/1/webrtc/common_video/i420_buff...
File webrtc/common_video/i420_buffer_pool.cc (right):

https://codereview.webrtc.org/1878623002/diff/1/webrtc/common_video/i420_buff...
webrtc/common_video/i420_buffer_pool.cc:18: // TODO(nisse): Why not simply
inherit I420Buffer, and override
On 2016/04/13 08:47:08, nisse-webrtc wrote:
> On 2016/04/13 08:30:50, magjed_webrtc wrote:
> > First, you can't override HasOneRef here, because it will just be overridden
> > again by RefCountedObject that is added last. Second, how would it help? If
> you
> > don't have the extra indirection, you can't distinguish the reference from
> > I420BufferPool, so HasOneRef would always be false for PooledI420Buffers
when
> > they are in use by clients, which is not what we want.
> 
> I'll investigate this in a separate cl. 
> 
> The idea is to demote HasOneRef to protected. Then add a public IsMutable,
which
> will return always false for some classes (NativeHandleBuffer,
> WrapperI420Buffer). For a plain I420Buffer, it will return HasOneRef(). For
> PooledI420Buffer, it could return GetRefCount() == 2 (needs a new method in
the
> refcount machinery).
> 
> Corner case is when a pool is destroyed. Then it no longer holds a reference,
> and therefore I420PooledBuffer::IsMutable should return false, not true, if it
> finds two references. Could have a flag is_pooled, which is cleared by
> I420BufferPool::Release.

please remove this todo then and just keep that thought in a list somewhere
else.

[magjed_webrtc, 2016-04-14 07:31:52]

lgtm

[pbos-webrtc, 2016-04-14 13:06:39]

lgtm, feel free to ignore the deprecated part if you're removing them soon

https://codereview.webrtc.org/1878623002/diff/40001/webrtc/common_video/inclu...
File webrtc/common_video/include/video_frame_buffer.h (right):

https://codereview.webrtc.org/1878623002/diff/40001/webrtc/common_video/inclu...
webrtc/common_video/include/video_frame_buffer.h:49: // Deprecated method. Use
virtual final, to ensure we get warnings
Should this use RTC_DEPRECATED instead? (webrtc/base/deprecation.h)

https://codereview.webrtc.org/1878623002/diff/40001/webrtc/test/frame_utils.cc
File webrtc/test/frame_utils.cc (right):

https://codereview.webrtc.org/1878623002/diff/40001/webrtc/test/frame_utils.c...
webrtc/test/frame_utils.cc:54: f1->StrideU() != f2->StrideU() ||
maybe git cl format, not sure

[nisse-webrtc, 2016-04-14 14:08:47]

Thanks for the review. Trying to land this now.

https://codereview.webrtc.org/1878623002/diff/40001/webrtc/common_video/inclu...
File webrtc/common_video/include/video_frame_buffer.h (right):

https://codereview.webrtc.org/1878623002/diff/40001/webrtc/common_video/inclu...
webrtc/common_video/include/video_frame_buffer.h:49: // Deprecated method. Use
virtual final, to ensure we get warnings
On 2016/04/14 13:06:39, pbos-webrtc wrote:
> Should this use RTC_DEPRECATED instead? (webrtc/base/deprecation.h)

That will break Chrome, if it uses them. So I think we should just make sure
they aren't used, and then delete them.

https://codereview.webrtc.org/1878623002/diff/40001/webrtc/test/frame_utils.cc
File webrtc/test/frame_utils.cc (right):

https://codereview.webrtc.org/1878623002/diff/40001/webrtc/test/frame_utils.c...
webrtc/test/frame_utils.cc:54: f1->StrideU() != f2->StrideU() ||
On 2016/04/14 13:06:39, pbos-webrtc wrote:
> maybe git cl format, not sure

I think it looks nice as is. And I think there are other pending cls touching
this code.

[nisse-webrtc, 2016-04-14 14:09:21]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2016-04-14 14:09:21]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org,
perkj@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1878623002/#ps40001 (title:
"Add NOLINT for the unusual combination virtual final.")

[commit-bot: I haz the power, 2016-04-14 14:09:35]

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

[commit-bot: I haz the power, 2016-04-14 16:10:18]

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

[commit-bot: I haz the power, 2016-04-14 16:10:19]

Try jobs failed on following builders:
  win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[nisse-webrtc, 2016-04-15 07:00:13]

The CQ bit was checked by nisse@webrtc.org

[commit-bot: I haz the power, 2016-04-15 07:00:22]

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

[commit-bot: I haz the power, 2016-04-15 07:37:06]

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

[commit-bot: I haz the power, 2016-04-15 07:37:07]

Try jobs failed on following builders:
  win_baremetal on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/10479)

[nisse-webrtc, 2016-04-15 09:04:21]

I just realized this can't be landed as is, due to the subclass in
WebRtcVideoFrameAdapter in Chrome. Not entirely obvious how to handle that.

[magjed_webrtc, 2016-04-15 11:16:23]

On 2016/04/15 09:04:21, nisse-webrtc wrote:
> I just realized this can't be landed as is, due to the subclass in
> WebRtcVideoFrameAdapter in Chrome. Not entirely obvious how to handle that.

I think you can just provide default implementations of Data[YUV] that do
RTC_NOT_REACHED(); return nullptr; You can make it pure virtual after you have
updated chrome.

[magjed_webrtc, 2016-04-15 11:22:11]

On 2016/04/15 11:16:23, magjed_webrtc wrote:
> On 2016/04/15 09:04:21, nisse-webrtc wrote:
> > I just realized this can't be landed as is, due to the subclass in
> > WebRtcVideoFrameAdapter in Chrome. Not entirely obvious how to handle that.
> 
> I think you can just provide default implementations of Data[YUV] that do
> RTC_NOT_REACHED(); return nullptr; You can make it pure virtual after you have
> updated chrome.

No wait, you need to make proper default implementations to the old interface,
e.g. uint8_t* VideoFrameBuffer::DataY() { return data(kYPlane); }. Then update
Chrome, and then remove old interface and make the new interface pure virtual.

[nisse-webrtc, 2016-04-15 12:21:32]

On 2016/04/15 11:22:11, magjed_webrtc wrote:
 No wait, you need to make proper default implementations to the old interface,
> e.g. uint8_t* VideoFrameBuffer::DataY() { return data(kYPlane); }. Then update
> Chrome, and then remove old interface and make the new interface pure virtual.

Done. Default implementation of data() calls DataX(), default implementation of
DataX calls data(). Subclasses have to override one or the other, or we get a
nice infinite recursion.

Do you think this solves the transition problem?

[magjed_webrtc, 2016-04-18 09:51:38]

On 2016/04/15 12:21:32, nisse-webrtc wrote:
> On 2016/04/15 11:22:11, magjed_webrtc wrote:
>  No wait, you need to make proper default implementations to the old
interface,
> > e.g. uint8_t* VideoFrameBuffer::DataY() { return data(kYPlane); }. Then
update
> > Chrome, and then remove old interface and make the new interface pure
virtual.
> 
> Done. Default implementation of data() calls DataX(), default implementation
of
> DataX calls data(). Subclasses have to override one or the other, or we get a
> nice infinite recursion.
> 
> Do you think this solves the transition problem?

Yes.

[nisse-webrtc, 2016-04-18 11:03:58]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2016-04-18 11:03:58]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org,
pbos@webrtc.org, perkj@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1878623002/#ps60001 (title:
"Added default implementations, to aid transition.")

[commit-bot: I haz the power, 2016-04-18 11:04:08]

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

[commit-bot: I haz the power, 2016-04-18 11:04:58]

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

[commit-bot: I haz the power, 2016-04-18 11:05:00]

Try jobs failed on following builders:
  ios32_sim_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/6747)
  ios_arm64_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/9068)
  ios_arm64_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/8987)
  ios_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/14277)
  ios_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/12920)
  mac_asan on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/13908)
  mac_compile_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
  mac_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/2388)
  mac_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/8614)
  mac_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/14343)

[nisse-webrtc, 2016-04-18 11:33:37]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-18 11:33:46]

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

[nisse-webrtc, 2016-04-18 12:28:50]

The CQ bit was unchecked by nisse@webrtc.org

[nisse-webrtc, 2016-04-18 12:28:50]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2016-04-18 12:28:51]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org,
pbos@webrtc.org, perkj@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1878623002/#ps80001 (title:
"Rebase.")

[commit-bot: I haz the power, 2016-04-18 12:29:01]

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

[commit-bot: I haz the power, 2016-04-18 12:34:44]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-18 12:34:48]

Description was changed from

==========
Added new VideoFrameBuffer methods Data[YUV]() etc.

Eliminate most uses of the old methods.

To continue on this path, once we agree the new methods make sense,
the next step is to rename cricket::VideoFrame::GetVideoFrameBuffer
--> video_frame_buffer, to match the name in webrtc::VideoFrame (if we
think that name is ok?). And then start updating all code to access
planes via the VideoFrameBuffer, and delete corresponding methods in
both cricket::VideoFrame and webrtc::VideoFrame.

BUG=webrtc:5682
==========

to

==========
Added new VideoFrameBuffer methods Data[YUV]() etc.

Eliminate most uses of the old methods.

To continue on this path, once we agree the new methods make sense,
the next step is to rename cricket::VideoFrame::GetVideoFrameBuffer
--> video_frame_buffer, to match the name in webrtc::VideoFrame (if we
think that name is ok?). And then start updating all code to access
planes via the VideoFrameBuffer, and delete corresponding methods in
both cricket::VideoFrame and webrtc::VideoFrame.

BUG=webrtc:5682

Committed: https://crrev.com/06176e49e276d290209676c832f17d34329d9ecd
Cr-Commit-Position: refs/heads/master@{#12407}
==========

[commit-bot: I haz the power, 2016-04-18 12:34:50]

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