Delete VideoFrameFactory, CapturedFrame, and related code.

Delete VideoFrameFactory, CapturedFrame, and related code.

BUG=webrtc:5682
Committed: https://crrev.com/66ac50e58c790624d51ede10ae438cbadbca9d2e
Cr-Commit-Position: refs/heads/master@{#14315}

[pthatcher1, 2016-08-23 00:13:13]

pthatcher@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[pthatcher1, 2016-08-23 00:13:15]

https://codereview.webrtc.org/2262443003/diff/1/webrtc/media/base/fakevideoca...
File webrtc/media/base/fakevideocapturer.h (right):

https://codereview.webrtc.org/2262443003/diff/1/webrtc/media/base/fakevideoca...
webrtc/media/base/fakevideocapturer.h:113: // TODO(nisse): XXX Keep to avoid
warning.
So are you going to delete this too?

https://codereview.webrtc.org/2262443003/diff/1/webrtc/media/base/videoadapte...
File webrtc/media/base/videoadapter_unittest.cc (right):

https://codereview.webrtc.org/2262443003/diff/1/webrtc/media/base/videoadapte...
webrtc/media/base/videoadapter_unittest.cc:46: #endif
And these?

https://codereview.webrtc.org/2262443003/diff/1/webrtc/media/base/videocaptur...
File webrtc/media/base/videocapturer.h (left):

https://codereview.webrtc.org/2262443003/diff/1/webrtc/media/base/videocaptur...
webrtc/media/base/videocapturer.h:48: struct CapturedFrame {
Woh.  Are we sure we can delete this?  Isn't there a lot of code out there that
uses this?

https://codereview.webrtc.org/2262443003/diff/1/webrtc/media/base/videocaptur...
File webrtc/media/base/videocapturer.h (right):

https://codereview.webrtc.org/2262443003/diff/1/webrtc/media/base/videocaptur...
webrtc/media/base/videocapturer.h:276: bool apply_rotation_;
apply_rotation() is only used by unit tests, I think, so if it's not used
internally, then it can be removed.

[nisse-webrtc, 2016-08-23 06:46:40]

Thanks for the comments, I wanted to show you where I'm aiming.

I did a cl to eliminate use of VideoFrameFactory from chrome last week, but it
turned out it messed up timestamping in some way, so I had to revert that.
Obviously, those chrome changes have to be sorted out first.

https://codereview.webrtc.org/2262443003/diff/1/webrtc/media/base/fakevideoca...
File webrtc/media/base/fakevideocapturer.h (right):

https://codereview.webrtc.org/2262443003/diff/1/webrtc/media/base/fakevideoca...
webrtc/media/base/fakevideocapturer.h:113: // TODO(nisse): XXX Keep to avoid
warning.
On 2016/08/23 00:13:13, pthatcher1 wrote:
> So are you going to delete this too?

No, but the #if:ed out block needs to be rewritten to invoke
cricket::VideoCapturer::OnFrame. And write pixel data into an I420Buffer
(VideoFrameBuffer subclass), without going via the cricket::CapturedFrame
struct.

https://codereview.webrtc.org/2262443003/diff/1/webrtc/media/base/videoadapte...
File webrtc/media/base/videoadapter_unittest.cc (right):

https://codereview.webrtc.org/2262443003/diff/1/webrtc/media/base/videoadapte...
webrtc/media/base/videoadapter_unittest.cc:46: #endif
On 2016/08/23 00:13:13, pthatcher1 wrote:
> And these?

Same here, needs rewrite to use OnFrame and not use the CapturedFrame struct.

https://codereview.webrtc.org/2262443003/diff/1/webrtc/media/base/videocaptur...
File webrtc/media/base/videocapturer.h (left):

https://codereview.webrtc.org/2262443003/diff/1/webrtc/media/base/videocaptur...
webrtc/media/base/videocapturer.h:48: struct CapturedFrame {
On 2016/08/23 00:13:14, pthatcher1 wrote:
> Woh.  Are we sure we can delete this?  Isn't there a lot of code out there
that
> uses this?

Not 100% sure. I asked on the webrtc-discuss list some days ago, and so far
noone has spoken up to say they use this. If required, we can keep it and the
SignalCapturedFrame thing for compatibility, I guess.

All use I've seen within webrtc and chrome have been kludges to associate a
CapturedFrame struct with some data which doesn't quite fit.

https://codereview.webrtc.org/2262443003/diff/1/webrtc/media/base/videocaptur...
File webrtc/media/base/videocapturer.h (right):

https://codereview.webrtc.org/2262443003/diff/1/webrtc/media/base/videocaptur...
webrtc/media/base/videocapturer.h:276: bool apply_rotation_;
On 2016/08/23 00:13:14, pthatcher1 wrote:
> apply_rotation() is only used by unit tests, I think, so if it's not used
> internally, then it can be removed.

Before this change, it was used by set_frame_factory. Not sure if it needs to
stay here after that method is deleted. 

But it's also used by subclasses, in particular,
AndroidVideoCapturerJni::OnMemoryBufferFrame and
AndroidVideoCapturerJni::OnTextureFrame call capturer->apply_rotation() to
decide what to do.

[nisse-webrtc, 2016-08-24 12:19:54]

https://codereview.webrtc.org/2262443003/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoframe.h (right):

https://codereview.webrtc.org/2262443003/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoframe.h:57: // only by the LoadFrame function
used in the VideoFrame unittests.
I'd like to note that as a follow on to this cl, we can delete Init, Reset and
Validate. Then, most of the tests in videoframe_unittests become irrelevant and
can be deleted too; it seems the main purpose of those tests are to test Init
and Validate.

[nisse-webrtc, 2016-08-29 13:05:08]

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

[commit-bot: I haz the power, 2016-08-29 13:05:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[nisse-webrtc, 2016-08-29 13:05:23]

Description was changed from

==========
Delete VideoFrameFactory, CapturedFrame, and related code.

Work-in-progress, some test code needs to be reorganized.

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

to

==========
Delete VideoFrameFactory, CapturedFrame, and related code.

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

[commit-bot: I haz the power, 2016-08-29 13:06:26]

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

[commit-bot: I haz the power, 2016-08-29 13:06:27]

Dry run: Try jobs failed on following builders:
  android_compile_x86_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...)
  ios64_gn_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_dbg/builds/2280)
  ios64_gn_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_rel/builds/2297)
  linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)

[nisse-webrtc, 2016-08-29 13:18:30]

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

[commit-bot: I haz the power, 2016-08-29 13:18:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-08-29 13:21:49]

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

[commit-bot: I haz the power, 2016-08-29 13:21:50]

Dry run: Try jobs failed on following builders:
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/12302)

[nisse-webrtc, 2016-08-29 13:42:58]

nisse@webrtc.org changed reviewers:
+ perkj@webrtc.org

[nisse-webrtc, 2016-08-29 13:42:59]

Tests works for me now. ios bots are broken, because I haven't updated
talk/app/webrtc/objc/avfoundationvideocapturer.mm to not use
cricket::CapturedFrame. I really hope the files under talk will die soon.

[pthatcher1, 2016-08-29 18:55:36]

https://codereview.webrtc.org/2262443003/diff/1/webrtc/media/base/videocaptur...
File webrtc/media/base/videocapturer.h (left):

https://codereview.webrtc.org/2262443003/diff/1/webrtc/media/base/videocaptur...
webrtc/media/base/videocapturer.h:48: struct CapturedFrame {
On 2016/08/23 06:46:40, nisse-webrtc wrote:
> On 2016/08/23 00:13:14, pthatcher1 wrote:
> > Woh.  Are we sure we can delete this?  Isn't there a lot of code out there
> that
> > uses this?
> 
> Not 100% sure. I asked on the webrtc-discuss list some days ago, and so far
> noone has spoken up to say they use this. If required, we can keep it and the
> SignalCapturedFrame thing for compatibility, I guess.
> 
> All use I've seen within webrtc and chrome have been kludges to associate a
> CapturedFrame struct with some data which doesn't quite fit. 

I was under impression that lots of VideoSources are implemented by subclassing
VideoCapturer and then listening to the CapturedFrame that comes out and then
converting it to a VideoFrame.  But I would be very happy to be wrong about
that, because it would be great to get rid of this if we can.

[nisse-webrtc, 2016-09-02 11:23:34]

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

[commit-bot: I haz the power, 2016-09-02 11:23:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-09-02 13:24:20]

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

[commit-bot: I haz the power, 2016-09-02 13:24:21]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[pthatcher1, 2016-09-07 18:03:03]

lgtm

But can you add a comment about where apply_rotation_ is used, rather than
having a "Still needed?"?

https://codereview.webrtc.org/2262443003/diff/80001/webrtc/media/base/videoca...
File webrtc/media/base/videocapturer.h (right):

https://codereview.webrtc.org/2262443003/diff/80001/webrtc/media/base/videoca...
webrtc/media/base/videocapturer.h:274: // TODO(nisse): Still needed?
It's still used here:

https://cs.chromium.org/chromium/src/third_party/webrtc/sdk/objc/Framework/Cl...


If we remove that use, then I think we could remove it.

[perkj_webrtc, 2016-09-14 07:12:28]

https://codereview.webrtc.org/2262443003/diff/80001/webrtc/media/base/fakevid...
File webrtc/media/base/fakevideocapturer.h (right):

https://codereview.webrtc.org/2262443003/diff/80001/webrtc/media/base/fakevid...
webrtc/media/base/fakevideocapturer.h:90: // class. Child classes of
VideoFrameFactory are expected to call
This comment is a bit confusing if you delete VideoFrameFactory.

https://codereview.webrtc.org/2262443003/diff/80001/webrtc/media/base/fakevid...
webrtc/media/base/fakevideocapturer.h:96: rtc::scoped_refptr<webrtc::I420Buffer>
buffer(
nit : use I420Buffer::Create

https://codereview.webrtc.org/2262443003/diff/80001/webrtc/media/base/videoca...
File webrtc/media/base/videocapturer.cc (right):

https://codereview.webrtc.org/2262443003/diff/80001/webrtc/media/base/videoca...
webrtc/media/base/videocapturer.cc:212: broadcaster_.OnFrame(WebRtcVideoFrame(
this will not work for textures since this only rotates i420 buffers. Do you
remember the discussion with magjed on where Rotate should be implemented?  
apply_rotation_ is set in OnSinkWantsChanged. Are you saying that this class can
only be used in none texture case?

[nisse-webrtc, 2016-09-14 07:58:15]

Easy nits addressed.

https://codereview.webrtc.org/2262443003/diff/80001/webrtc/media/base/fakevid...
File webrtc/media/base/fakevideocapturer.h (right):

https://codereview.webrtc.org/2262443003/diff/80001/webrtc/media/base/fakevid...
webrtc/media/base/fakevideocapturer.h:90: // class. Child classes of
VideoFrameFactory are expected to call
On 2016/09/14 07:12:28, perkj_webrtc wrote:
> This comment is a bit confusing if you delete VideoFrameFactory.

Should be "Child classes of VideoCapturer". Good catch.

https://codereview.webrtc.org/2262443003/diff/80001/webrtc/media/base/fakevid...
webrtc/media/base/fakevideocapturer.h:96: rtc::scoped_refptr<webrtc::I420Buffer>
buffer(
On 2016/09/14 07:12:28, perkj_webrtc wrote:
> nit : use I420Buffer::Create 

Done.

https://codereview.webrtc.org/2262443003/diff/80001/webrtc/media/base/videoca...
File webrtc/media/base/videocapturer.cc (right):

https://codereview.webrtc.org/2262443003/diff/80001/webrtc/media/base/videoca...
webrtc/media/base/videocapturer.cc:212: broadcaster_.OnFrame(WebRtcVideoFrame(
On 2016/09/14 07:12:28, perkj_webrtc wrote:
> this will not work for textures since this only rotates i420 buffers. Do you
> remember the discussion with magjed on where Rotate should be implemented?  
> apply_rotation_ is set in OnSinkWantsChanged. Are you saying that this class
can
> only be used in none texture case? 

I'm not sure we really should implement rotation here at all; since we're moving
away from using cricket::VideoCapturer, it doesn't make so much sense to add new
features.

But to answer the question: With this cl, any capturer that produces texture
frames must handle apply_rotation_ itself, before passing frames on to this
method. 

Hmm, but for this to work, I420Buffer::Rotate would need to be changed to do the
check 

  if (rotation == webrtc::kVideoRotation_0) {
    return src;
  }

before accessing planar data. 

I'll have to think a bit more about this, but I'm leaning towards deleting this
code and possibly add a log or DCHECK for the invalid case apply_rotation_ true
and frame.rotation() != 0.

https://codereview.webrtc.org/2262443003/diff/80001/webrtc/media/base/videoca...
File webrtc/media/base/videocapturer.h (right):

https://codereview.webrtc.org/2262443003/diff/80001/webrtc/media/base/videoca...
webrtc/media/base/videocapturer.h:274: // TODO(nisse): Still needed?
On 2016/09/07 18:03:03, pthatcher1 wrote:
> It's still used here:
> 
>
https://cs.chromium.org/chromium/src/third_party/webrtc/sdk/objc/Framework/Cl...
> 
> 
> If we remove that use, then I think we could remove it.
> 

I'm changing the comment to say that is is used by child classes. And we not too
far from being able to delete all of cricket::VideoCapturer now...

[nisse-webrtc, 2016-09-20 07:45:10]

I have updated all dependencies I have found, and there have been some advance
warning on the discuss-webrtc list.

Are we ready?

[perkj_webrtc, 2016-09-20 12:46:16]

lgtm

[nisse-webrtc, 2016-09-20 13:08:13]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2016-09-20 13:08:14]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2262443003/#ps160001 (title:
"Update apply_rotation_ comment.")

[commit-bot: I haz the power, 2016-09-20 13:08:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-09-20 15:08:57]

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

[commit-bot: I haz the power, 2016-09-20 15:08:59]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_clang_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_drmemory_light on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_clang_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_gyp_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_gyp_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[nisse-webrtc, 2016-09-21 06:14:33]

The CQ bit was checked by nisse@webrtc.org

[commit-bot: I haz the power, 2016-09-21 06:14:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-09-21 06:51:38]

Description was changed from

==========
Delete VideoFrameFactory, CapturedFrame, and related code.

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

to

==========
Delete VideoFrameFactory, CapturedFrame, and related code.

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

[commit-bot: I haz the power, 2016-09-21 06:51:39]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-09-21 06:51:56]

Description was changed from

==========
Delete VideoFrameFactory, CapturedFrame, and related code.

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

to

==========
Delete VideoFrameFactory, CapturedFrame, and related code.

BUG=webrtc:5682

Committed: https://crrev.com/66ac50e58c790624d51ede10ae438cbadbca9d2e
Cr-Commit-Position: refs/heads/master@{#14315}
==========

[commit-bot: I haz the power, 2016-09-21 06:51:58]

Patchset 9 (id:??) landed as
https://crrev.com/66ac50e58c790624d51ede10ae438cbadbca9d2e
Cr-Commit-Position: refs/heads/master@{#14315}

[nisse-webrtc, 2016-09-21 09:09:34]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.webrtc.org/2357113002/ by nisse@webrtc.org.

The reason for reverting is: Breaks downstream testcode, still using
CapturedFrame..