Refactor VideoDestinationHandler to implement MediaStreamVideoSource.

Refactor VideoDestinationHandler to implement MediaStreamVideoSource.
VideoDestinationHandler is moved to media/webrtc since it needs to use libyuv. It also make sence since its a private API only used for WebRtc.

This cl also remove the WebRtcVideoCaptureAdapter::SetCurrrentFormat and make sure video resolution constraints are not passed to libJingle. Libjingle do not need to know the capture resolution or the used resolution constraints and that is infact how
VideoDestionationHandler can work today.
This is needed to simplify the adapter and the use of MediaStreamVideoSource since its currently not possible to call WebRtcVideoCaptureAdapter::SetCurrrentFormat before the MediaStreamVideoSource::OnSupportedFormat has been called.

BUG=334243
// TBR Jam for gyp changes, yzshen for the header file path change in Pepper.

R=joi@chromium.org, ronghuawu@chromium.org
TBR=jam, yzshen1

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

[perkj_chrome, 2014-03-26 14:28:56]

Can you please review?

[Jói, 2014-03-26 21:36:13]

LGTM, nits only.

https://codereview.chromium.org/212973002/diff/1/.gitmodules
File .gitmodules (right):

https://codereview.chromium.org/212973002/diff/1/.gitmodules#newcode1
.gitmodules:1: [submodule "breakpad/src"]
You probably don't want .gitmodules to appear in your CL.

https://codereview.chromium.org/212973002/diff/1/content/renderer/media/media...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/212973002/diff/1/content/renderer/media/media...
content/renderer/media/media_stream_video_source.cc:21: const char
MediaStreamVideoSource::kMaxAspectRatio[] = "maxAspectRatio";
For the rest, max comes before min. Can we do that here too, for consistency?

https://codereview.chromium.org/212973002/diff/1/content/renderer/media/media...
content/renderer/media/media_stream_video_source.cc:31:
MediaStreamVideoSource::kMinAspectRatio,
This is a duplicate.

https://codereview.chromium.org/212973002/diff/1/content/renderer/media/media...
content/renderer/media/media_stream_video_source.cc:32:
MediaStreamVideoSource::kMaxAspectRatio,
For all the others, max comes before min. Can we do that here too?

https://codereview.chromium.org/212973002/diff/1/content/renderer/media/webrt...
File content/renderer/media/webrtc/video_destination_handler.cc (right):

https://codereview.chromium.org/212973002/diff/1/content/renderer/media/webrt...
content/renderer/media/webrtc/video_destination_handler.cc:6: 
Any chance this file can show up as a move? Perhaps --similarity=30 would help?

https://codereview.chromium.org/212973002/diff/1/content/renderer/media/webrt...
content/renderer/media/webrtc/video_destination_handler.cc:84:
first_frame_received_ = true;
indenting is off, 2 spaces too much indent

https://codereview.chromium.org/212973002/diff/1/content/renderer/media/webrt...
File content/renderer/media/webrtc/video_destination_handler_unittest.cc
(right):

https://codereview.chromium.org/212973002/diff/1/content/renderer/media/webrt...
content/renderer/media/webrtc/video_destination_handler_unittest.cc:1: //
Copyright (c) 2013 The Chromium Authors. All rights reserved.
Is this file also a move? See if we can make it show up as such.

[Ronghua Wu (Left Chromium), 2014-03-26 22:27:53]

https://codereview.chromium.org/212973002/diff/1/.gitmodules
File .gitmodules (right):

https://codereview.chromium.org/212973002/diff/1/.gitmodules#newcode109
.gitmodules:109: [submodule "third_party/clang_format/script"]
changes to this file doesn't belong to this cl.

https://codereview.chromium.org/212973002/diff/1/content/content_tests.gypi
File content/content_tests.gypi (left):

https://codereview.chromium.org/212973002/diff/1/content/content_tests.gypi#o...
content/content_tests.gypi:572:
'renderer/media/video_destination_handler_unittest.cc',
the rule for gyp is to include all the files and exclude them when it's needed.
See line 704.

https://codereview.chromium.org/212973002/diff/1/content/renderer/media/mock_...
File content/renderer/media/mock_media_stream_video_sink.cc (right):

https://codereview.chromium.org/212973002/diff/1/content/renderer/media/mock_...
content/renderer/media/mock_media_stream_video_sink.cc:22: }
indention

https://codereview.chromium.org/212973002/diff/1/content/renderer/media/webrt...
File content/renderer/media/webrtc/video_destination_handler.cc (right):

https://codereview.chromium.org/212973002/diff/1/content/renderer/media/webrt...
content/renderer/media/webrtc/video_destination_handler.cc:6: 
On 2014/03/26 21:36:13, Jói wrote:
> Any chance this file can show up as a move? Perhaps --similarity=30 would
help?

+1 would like see the diff

svn cp will do if you are using svn

https://codereview.chromium.org/212973002/diff/1/content/renderer/media/webrt...
File content/renderer/media/webrtc/video_destination_handler_unittest.cc
(right):

https://codereview.chromium.org/212973002/diff/1/content/renderer/media/webrt...
content/renderer/media/webrtc/video_destination_handler_unittest.cc:1: //
Copyright (c) 2013 The Chromium Authors. All rights reserved.
On 2014/03/26 21:36:13, Jói wrote:
> Is this file also a move? See if we can make it show up as such.

+1

[perkj_chrome, 2014-03-27 11:24:31]

Ronghua, PTAL
I removed code from MediaStreamDependency factory that is no longer used as well
in this patch set.

https://codereview.chromium.org/212973002/diff/1/.gitmodules
File .gitmodules (right):

https://codereview.chromium.org/212973002/diff/1/.gitmodules#newcode1
.gitmodules:1: [submodule "breakpad/src"]
On 2014/03/26 21:36:13, Jói wrote:
> You probably don't want .gitmodules to appear in your CL.

Done.

https://codereview.chromium.org/212973002/diff/1/.gitmodules#newcode109
.gitmodules:109: [submodule "third_party/clang_format/script"]
On 2014/03/26 22:27:53, Ronghua Wu wrote:
> changes to this file doesn't belong to this cl.

Done.

https://codereview.chromium.org/212973002/diff/1/content/content_tests.gypi
File content/content_tests.gypi (left):

https://codereview.chromium.org/212973002/diff/1/content/content_tests.gypi#o...
content/content_tests.gypi:572:
'renderer/media/video_destination_handler_unittest.cc',
On 2014/03/26 22:27:53, Ronghua Wu wrote:
> the rule for gyp is to include all the files and exclude them when it's
needed.
> See line 704.

So you want me to remove this on 704? I guess it don't make sence to have this
test unless webrtc is on anyway?

https://codereview.chromium.org/212973002/diff/1/content/renderer/media/media...
File content/renderer/media/media_stream_video_source.cc (right):

https://codereview.chromium.org/212973002/diff/1/content/renderer/media/media...
content/renderer/media/media_stream_video_source.cc:31:
MediaStreamVideoSource::kMinAspectRatio,
On 2014/03/26 21:36:13, Jói wrote:
> This is a duplicate.

Done.

https://codereview.chromium.org/212973002/diff/1/content/renderer/media/media...
content/renderer/media/media_stream_video_source.cc:32:
MediaStreamVideoSource::kMaxAspectRatio,
On 2014/03/26 21:36:13, Jói wrote:
> For all the others, max comes before min. Can we do that here too?

Done.

https://codereview.chromium.org/212973002/diff/1/content/renderer/media/mock_...
File content/renderer/media/mock_media_stream_video_sink.cc (right):

https://codereview.chromium.org/212973002/diff/1/content/renderer/media/mock_...
content/renderer/media/mock_media_stream_video_sink.cc:22: }
On 2014/03/26 22:27:53, Ronghua Wu wrote:
> indention

Done.

https://codereview.chromium.org/212973002/diff/1/content/renderer/media/webrt...
File content/renderer/media/webrtc/video_destination_handler.cc (right):

https://codereview.chromium.org/212973002/diff/1/content/renderer/media/webrt...
content/renderer/media/webrtc/video_destination_handler.cc:84:
first_frame_received_ = true;
On 2014/03/26 21:36:13, Jói wrote:
> indenting is off, 2 spaces too much indent

Done.

https://codereview.chromium.org/212973002/diff/1/content/renderer/media/webrt...
File content/renderer/media/webrtc/video_destination_handler_unittest.cc
(right):

https://codereview.chromium.org/212973002/diff/1/content/renderer/media/webrt...
content/renderer/media/webrtc/video_destination_handler_unittest.cc:1: //
Copyright (c) 2013 The Chromium Authors. All rights reserved.
On 2014/03/26 21:36:13, Jói wrote:
> Is this file also a move? See if we can make it show up as such.

Done.

https://codereview.chromium.org/212973002/diff/1/content/renderer/media/webrt...
content/renderer/media/webrtc/video_destination_handler_unittest.cc:1: //
Copyright (c) 2013 The Chromium Authors. All rights reserved.
On 2014/03/26 22:27:53, Ronghua Wu wrote:
> On 2014/03/26 21:36:13, Jói wrote:
> > Is this file also a move? See if we can make it show up as such.
> 
> +1

Done.

[perkj_chrome, 2014-03-27 14:55:03]

yschen , can you help with the pepper file and the gyp changes? 

Thanks
Per

[Ronghua Wu (Left Chromium), 2014-03-28 02:30:11]

https://codereview.chromium.org/212973002/diff/50001/content/renderer/media/w...
File content/renderer/media/webrtc/video_destination_handler.cc (left):

https://codereview.chromium.org/212973002/diff/50001/content/renderer/media/w...
content/renderer/media/webrtc/video_destination_handler.cc:181: if
(stream.isNull() || !stream.extraData()) {
why change?

https://codereview.chromium.org/212973002/diff/50001/content/renderer/media/w...
File content/renderer/media/webrtc/video_destination_handler.cc (right):

https://codereview.chromium.org/212973002/diff/50001/content/renderer/media/w...
content/renderer/media/webrtc/video_destination_handler.cc:103:
frame_pool_.CreateFrame(media::VideoFrame::I420, frame_size,
looks like you claimed supporting PIXEL_FORMAT_YV12 but sending a I420, or did i
miss anything?

https://codereview.chromium.org/212973002/diff/50001/content/renderer/media/w...
content/renderer/media/webrtc/video_destination_handler.cc:106:
libyuv::BGRAToI420(reinterpret_cast<uint8*>(bitmap->getPixels()),
so we added a one more memcpy after this change, correct?

https://codereview.chromium.org/212973002/diff/50001/content/renderer/media/w...
content/renderer/media/webrtc/video_destination_handler.cc:119: //
PpFrameWriterProxy is a helper class to make sure the user won't use
looks like we no longer need PpFrameWriterProxy since the Writer is a Source now

https://codereview.chromium.org/212973002/diff/50001/content/renderer/media/w...
content/renderer/media/webrtc/video_destination_handler.cc:170: //
base::Base64Encode(base::RandBytesAsString(64), &track_id);
why remove?

[perkj_chrome, 2014-03-28 08:36:44]

PTAL

https://codereview.chromium.org/212973002/diff/50001/content/renderer/media/w...
File content/renderer/media/webrtc/video_destination_handler.cc (left):

https://codereview.chromium.org/212973002/diff/50001/content/renderer/media/w...
content/renderer/media/webrtc/video_destination_handler.cc:181: if
(stream.isNull() || !stream.extraData()) {
On 2014/03/28 02:30:11, Ronghua Wu wrote:
> why change?

This code should just worry about being able to add a video track to the MS. It
is not sure that we need to have anything in extraData of the MS when we are
done with all refactorings.

https://codereview.chromium.org/212973002/diff/50001/content/renderer/media/w...
File content/renderer/media/webrtc/video_destination_handler.cc (right):

https://codereview.chromium.org/212973002/diff/50001/content/renderer/media/w...
content/renderer/media/webrtc/video_destination_handler.cc:103:
frame_pool_.CreateFrame(media::VideoFrame::I420, frame_size,
On 2014/03/28 02:30:11, Ronghua Wu wrote:
> looks like you claimed supporting PIXEL_FORMAT_YV12 but sending a I420, or did
i
> miss anything?

Done.

https://codereview.chromium.org/212973002/diff/50001/content/renderer/media/w...
content/renderer/media/webrtc/video_destination_handler.cc:106:
libyuv::BGRAToI420(reinterpret_cast<uint8*>(bitmap->getPixels()),
On 2014/03/28 02:30:11, Ronghua Wu wrote:
> so we added a one more memcpy after this change, correct?

Isn't it a null sum? We move the copy from libjingle where a conversion is
currently done to here. 
Since the frame is an I420 frame, libjingle can just wrapp this frame and use
without the need of a copy.

https://codereview.chromium.org/212973002/diff/50001/content/renderer/media/w...
content/renderer/media/webrtc/video_destination_handler.cc:119: //
PpFrameWriterProxy is a helper class to make sure the user won't use
On 2014/03/28 02:30:11, Ronghua Wu wrote:
> looks like we no longer need PpFrameWriterProxy since the Writer is a Source
now

I think we need it since Pepper take ownership of the proxy while the 
PpFrameWriter is owned by WebMediaStreamSource.

https://codereview.chromium.org/212973002/diff/50001/content/renderer/media/w...
content/renderer/media/webrtc/video_destination_handler.cc:170: //
base::Base64Encode(base::RandBytesAsString(64), &track_id);
On 2014/03/28 02:30:11, Ronghua Wu wrote:
> why remove?

oops...

[perkj_chrome, 2014-03-31 08:15:28]

On 2014/03/28 08:36:44, perkj wrote:
> PTAL
> 
>
https://codereview.chromium.org/212973002/diff/50001/content/renderer/media/w...
> File content/renderer/media/webrtc/video_destination_handler.cc (left):
> 
>
https://codereview.chromium.org/212973002/diff/50001/content/renderer/media/w...
> content/renderer/media/webrtc/video_destination_handler.cc:181: if
> (stream.isNull() || !stream.extraData()) {
> On 2014/03/28 02:30:11, Ronghua Wu wrote:
> > why change?
> 
> This code should just worry about being able to add a video track to the MS.
It
> is not sure that we need to have anything in extraData of the MS when we are
> done with all refactorings.
> 
>
https://codereview.chromium.org/212973002/diff/50001/content/renderer/media/w...
> File content/renderer/media/webrtc/video_destination_handler.cc (right):
> 
>
https://codereview.chromium.org/212973002/diff/50001/content/renderer/media/w...
> content/renderer/media/webrtc/video_destination_handler.cc:103:
> frame_pool_.CreateFrame(media::VideoFrame::I420, frame_size,
> On 2014/03/28 02:30:11, Ronghua Wu wrote:
> > looks like you claimed supporting PIXEL_FORMAT_YV12 but sending a I420, or
did
> i
> > miss anything?
> 
> Done.
> 
>
https://codereview.chromium.org/212973002/diff/50001/content/renderer/media/w...
> content/renderer/media/webrtc/video_destination_handler.cc:106:
> libyuv::BGRAToI420(reinterpret_cast<uint8*>(bitmap->getPixels()),
> On 2014/03/28 02:30:11, Ronghua Wu wrote:
> > so we added a one more memcpy after this change, correct?
> 
> Isn't it a null sum? We move the copy from libjingle where a conversion is
> currently done to here. 
> Since the frame is an I420 frame, libjingle can just wrapp this frame and use
> without the need of a copy.
> 
>
https://codereview.chromium.org/212973002/diff/50001/content/renderer/media/w...
> content/renderer/media/webrtc/video_destination_handler.cc:119: //
> PpFrameWriterProxy is a helper class to make sure the user won't use
> On 2014/03/28 02:30:11, Ronghua Wu wrote:
> > looks like we no longer need PpFrameWriterProxy since the Writer is a Source
> now
> 
> I think we need it since Pepper take ownership of the proxy while the 
> PpFrameWriter is owned by WebMediaStreamSource.
> 
>
https://codereview.chromium.org/212973002/diff/50001/content/renderer/media/w...
> content/renderer/media/webrtc/video_destination_handler.cc:170: //
> base::Base64Encode(base::RandBytesAsString(64), &track_id);
> On 2014/03/28 02:30:11, Ronghua Wu wrote:
> > why remove?
> 
> oops...

Friendly ping.

[Ronghua Wu (Left Chromium), 2014-04-01 00:01:26]

LGTM with minor comments.

https://codereview.chromium.org/212973002/diff/50001/content/renderer/media/w...
File content/renderer/media/webrtc/video_destination_handler.cc (right):

https://codereview.chromium.org/212973002/diff/50001/content/renderer/media/w...
content/renderer/media/webrtc/video_destination_handler.cc:106:
libyuv::BGRAToI420(reinterpret_cast<uint8*>(bitmap->getPixels()),
On 2014/03/28 08:36:44, perkj wrote:
> On 2014/03/28 02:30:11, Ronghua Wu wrote:
> > so we added a one more memcpy after this change, correct?
> 
> Isn't it a null sum? We move the copy from libjingle where a conversion is
> currently done to here. 
> Since the frame is an I420 frame, libjingle can just wrapp this frame and use
> without the need of a copy.

Hmm, true.

https://codereview.chromium.org/212973002/diff/50001/content/renderer/media/w...
content/renderer/media/webrtc/video_destination_handler.cc:119: //
PpFrameWriterProxy is a helper class to make sure the user won't use
On 2014/03/28 08:36:44, perkj wrote:
> On 2014/03/28 02:30:11, Ronghua Wu wrote:
> > looks like we no longer need PpFrameWriterProxy since the Writer is a Source
> now
> 
> I think we need it since Pepper take ownership of the proxy while the 
> PpFrameWriter is owned by WebMediaStreamSource.

Ok, then can you update the comment here? E.g. the owner is now
WebMediaStreamSource.

https://codereview.chromium.org/212973002/diff/70001/content/renderer/media/w...
File content/renderer/media/webrtc/video_destination_handler_unittest.cc
(right):

https://codereview.chromium.org/212973002/diff/70001/content/renderer/media/w...
content/renderer/media/webrtc/video_destination_handler_unittest.cc:73:
EXPECT_EQ(1, sink.number_of_frames());
can you please verify we got i420 frame.

[perkj_chrome, 2014-04-01 10:06:57]

https://codereview.chromium.org/212973002/diff/50001/content/renderer/media/w...
File content/renderer/media/webrtc/video_destination_handler.cc (right):

https://codereview.chromium.org/212973002/diff/50001/content/renderer/media/w...
content/renderer/media/webrtc/video_destination_handler.cc:119: //
PpFrameWriterProxy is a helper class to make sure the user won't use
On 2014/04/01 00:01:27, Ronghua Wu wrote:
> On 2014/03/28 08:36:44, perkj wrote:
> > On 2014/03/28 02:30:11, Ronghua Wu wrote:
> > > looks like we no longer need PpFrameWriterProxy since the Writer is a
Source
> > now
> > 
> > I think we need it since Pepper take ownership of the proxy while the 
> > PpFrameWriter is owned by WebMediaStreamSource.
> 
> Ok, then can you update the comment here? E.g. the owner is now
> WebMediaStreamSource.

Done.

https://codereview.chromium.org/212973002/diff/70001/content/renderer/media/w...
File content/renderer/media/webrtc/video_destination_handler_unittest.cc
(right):

https://codereview.chromium.org/212973002/diff/70001/content/renderer/media/w...
content/renderer/media/webrtc/video_destination_handler_unittest.cc:73:
EXPECT_EQ(1, sink.number_of_frames());
On 2014/04/01 00:01:27, Ronghua Wu wrote:
> can you please verify we got i420 frame.

...- The sink will get YV12 since the VideoTrack change to YV12. However the
capture adapter will get I420. 

Alpha is changin the track back to not convert in
https://codereview.chromium.org/213423006/

[perkj_chrome, 2014-04-01 10:09:32]

The CQ bit was checked by perkj@chromium.org

[commit-bot: I haz the power, 2014-04-01 10:09:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/212973002/90001

[commit-bot: I haz the power, 2014-04-01 11:30:30]

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

[commit-bot: I haz the power, 2014-04-01 11:30:31]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[perkj_chrome, 2014-04-01 13:05:19]

The CQ bit was checked by perkj@chromium.org

[commit-bot: I haz the power, 2014-04-01 13:05:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/212973002/110001

[commit-bot: I haz the power, 2014-04-01 13:42:28]

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

[commit-bot: I haz the power, 2014-04-01 13:42:29]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_rel

[perkj_chrome, 2014-04-02 05:09:05]

The CQ bit was checked by perkj@chromium.org

[commit-bot: I haz the power, 2014-04-02 05:09:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/212973002/110001

[commit-bot: I haz the power, 2014-04-02 06:22:14]

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

[commit-bot: I haz the power, 2014-04-02 06:22:15]

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

[perkj_chrome, 2014-04-02 09:20:38]

The CQ bit was checked by perkj@chromium.org

[commit-bot: I haz the power, 2014-04-02 09:20:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/212973002/130001

[commit-bot: I haz the power, 2014-04-02 12:45:32]

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

[commit-bot: I haz the power, 2014-04-02 12:45:33]

Retried try job too often on win_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[perkj_chrome, 2014-04-02 13:36:56]

The CQ bit was checked by perkj@chromium.org

[commit-bot: I haz the power, 2014-04-02 13:37:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/212973002/130001

[commit-bot: I haz the power, 2014-04-02 18:15:14]

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

[commit-bot: I haz the power, 2014-04-02 18:15:15]

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

[perkj_chrome, 2014-04-03 07:35:22]

The CQ bit was checked by perkj@chromium.org

[commit-bot: I haz the power, 2014-04-03 07:37:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/212973002/130001

[commit-bot: I haz the power, 2014-04-03 10:12:21]

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

[commit-bot: I haz the power, 2014-04-03 10:12:22]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[perkj_chrome, 2014-04-03 12:10:51]

Committed patchset #8 manually as r261364 (presubmit successful).