Passing Texture backed VideoFrame through WebRTC wrappers

Enable passing a texture backed media::VideoFrame through WebRTC wrappers in Renderer process. It is a refactor of https://codereview.chromium.org/1128213005/ as a lot of refactor in media::VideoFrame happened and I decided to create a new CL.

- VideoFrameWrapper points to the underlying media::VideoFrame via native_handle()
- WebRtcVideoCapturerAdapter uses VideoFrameWrapper to pass Native Texture backed VideoFrame
- RTCVideoEncoder accesses the media::VideoFrame via native_handle() and passes

BUG=440843, 503835
Committed: https://crrev.com/aeea60ad95bcc60ca463ec513d4953dedc1fbcfc
Cr-Commit-Position: refs/heads/master@{#338092}

[emircan, 2015-06-25 22:26:53]

emircan@chromium.org changed reviewers:
+ magjed@chromium.org, mcasas@chromium.org

[emircan, 2015-06-25 22:28:33]

Please take a look.

[mcasas, 2015-06-26 00:18:32]

https://codereview.chromium.org/1212133002/diff/1/content/renderer/media/rtc_...
File content/renderer/media/rtc_video_encoder.cc (right):

https://codereview.chromium.org/1212133002/diff/1/content/renderer/media/rtc_...
content/renderer/media/rtc_video_encoder.cc:475: DVLOG(3) <<
"Impl::EncodeOneFrame()";
DVLOG(3) << __func__;

https://codereview.chromium.org/1212133002/diff/1/content/renderer/media/rtc_...
content/renderer/media/rtc_video_encoder.cc:485: bool next_frame_keyframe =
input_next_frame_keyframe_;
const ?

https://codereview.chromium.org/1212133002/diff/1/content/renderer/media/webr...
File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right):

https://codereview.chromium.org/1212133002/diff/1/content/renderer/media/webr...
content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:221:
frame->HasTextures())) {
Is l.221 correct? I thought with this CL we can accept VideoFrames
that HasTextures().

https://codereview.chromium.org/1212133002/diff/1/content/renderer/media/webr...
content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:236:
DCHECK(frame_factory_ == frame_factory());
DCHECK_EQ?
what exactly is this line testing for? :?

[emircan, 2015-06-26 19:46:38]

Patchset #2 (id:20001) has been deleted

[emircan, 2015-06-26 19:47:09]

Please take a look.

https://codereview.chromium.org/1212133002/diff/1/content/renderer/media/rtc_...
File content/renderer/media/rtc_video_encoder.cc (right):

https://codereview.chromium.org/1212133002/diff/1/content/renderer/media/rtc_...
content/renderer/media/rtc_video_encoder.cc:475: DVLOG(3) <<
"Impl::EncodeOneFrame()";
On 2015/06/26 00:18:32, mcasas wrote:
> DVLOG(3) << __func__;

Done.

https://codereview.chromium.org/1212133002/diff/1/content/renderer/media/rtc_...
content/renderer/media/rtc_video_encoder.cc:485: bool next_frame_keyframe =
input_next_frame_keyframe_;
On 2015/06/26 00:18:32, mcasas wrote:
> const ?

Done.

https://codereview.chromium.org/1212133002/diff/1/content/renderer/media/webr...
File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right):

https://codereview.chromium.org/1212133002/diff/1/content/renderer/media/webr...
content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:221:
frame->HasTextures())) {
On 2015/06/26 00:18:32, mcasas wrote:
> Is l.221 correct? I thought with this CL we can accept VideoFrames
> that HasTextures().

Correct. The initial ! applies to both statements.

https://codereview.chromium.org/1212133002/diff/1/content/renderer/media/webr...
content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:236:
DCHECK(frame_factory_ == frame_factory());
On 2015/06/26 00:18:32, mcasas wrote:
> DCHECK_EQ?
> what exactly is this line testing for? :?

l.152 passes the ownership of the frame_factory_ to the base class via
VideoCapturer::set_frame_factory(). AFAIU, this makes sure that they are still
pointing to the same thing.

[mcasas, 2015-06-26 23:52:57]

https://codereview.chromium.org/1212133002/diff/1/content/renderer/media/rtc_...
File content/renderer/media/rtc_video_encoder.cc (right):

https://codereview.chromium.org/1212133002/diff/1/content/renderer/media/rtc_...
content/renderer/media/rtc_video_encoder.cc:475: DVLOG(3) <<
"Impl::EncodeOneFrame()";
On 2015/06/26 19:47:08, emircan wrote:
> On 2015/06/26 00:18:32, mcasas wrote:
> > DVLOG(3) << __func__;
> 
> Done.

done?

https://codereview.chromium.org/1212133002/diff/1/content/renderer/media/webr...
File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right):

https://codereview.chromium.org/1212133002/diff/1/content/renderer/media/webr...
content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:236:
DCHECK(frame_factory_ == frame_factory());
On 2015/06/26 19:47:09, emircan wrote:
> On 2015/06/26 00:18:32, mcasas wrote:
> > DCHECK_EQ?
> > what exactly is this line testing for? :?
> 
> l.152 passes the ownership of the frame_factory_ to the base class via
> VideoCapturer::set_frame_factory(). AFAIU, this makes sure that they are still
> pointing to the same thing.
Ugh, there is a 
MediaVideoFrameFactory* frame_factory_;  // [1] 
and a
rtc::scoped_ptr<VideoFrameFactory> frame_factory_;  // [2]
in its base class :? :?

Why not 
set_frame_factory(new MediaVideoFrameFactory)
in [3] and remove the derived naked pointer?


[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...
[3]
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

https://codereview.chromium.org/1212133002/diff/60001/content/renderer/media/...
File content/renderer/media/rtc_video_encoder.cc (right):

https://codereview.chromium.org/1212133002/diff/60001/content/renderer/media/...
content/renderer/media/rtc_video_encoder.cc:539:
frame->AddDestructionObserver(media::BindToCurrentLoop(
This will register EncodeFrameFinished() a second time
for memory-backed VideoFrames, after l. 511. Is that
what we want?

[emircan, 2015-06-27 00:37:17]

https://codereview.chromium.org/1212133002/diff/1/content/renderer/media/rtc_...
File content/renderer/media/rtc_video_encoder.cc (right):

https://codereview.chromium.org/1212133002/diff/1/content/renderer/media/rtc_...
content/renderer/media/rtc_video_encoder.cc:475: DVLOG(3) <<
"Impl::EncodeOneFrame()";
On 2015/06/26 23:52:56, mcasas wrote:
> On 2015/06/26 19:47:08, emircan wrote:
> > On 2015/06/26 00:18:32, mcasas wrote:
> > > DVLOG(3) << __func__;
> > 
> > Done.
> 
> done?

It was done on PatchSet#2, but all the Windows bots failed at compilation when
__func__ is used. Then, I removed and uploaded a new patch.

It should be an issue with MSVC version running on the bots, as it looks like
WIN specific functions are used:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/mesa/s...

https://codereview.chromium.org/1212133002/diff/1/content/renderer/media/webr...
File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right):

https://codereview.chromium.org/1212133002/diff/1/content/renderer/media/webr...
content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:236:
DCHECK(frame_factory_ == frame_factory());
On 2015/06/26 23:52:56, mcasas wrote:
> On 2015/06/26 19:47:09, emircan wrote:
> > On 2015/06/26 00:18:32, mcasas wrote:
> > > DCHECK_EQ?
> > > what exactly is this line testing for? :?
> > 
> > l.152 passes the ownership of the frame_factory_ to the base class via
> > VideoCapturer::set_frame_factory(). AFAIU, this makes sure that they are
still
> > pointing to the same thing.
> Ugh, there is a 
> MediaVideoFrameFactory* frame_factory_;  // [1] 
> and a
> rtc::scoped_ptr<VideoFrameFactory> frame_factory_;  // [2]
> in its base class :? :?
> 
> Why not 
> set_frame_factory(new MediaVideoFrameFactory)
> in [3] and remove the derived naked pointer?
> 
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
> [2]
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...
> [3]
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

The method used from |MediaVideoFrameFactory* frame_factory_| in this class are
not a part of VideoFrameFactory interface, such as SetFrame(),
GetCapturedFrame(), ReleaseFrame(). I guess, it was kept as member so that we
don't need to cast it, considering OnFrameCaptured() is called a lot. What do
you think? 

Still, I went ahead and implemented it though.

https://codereview.chromium.org/1212133002/diff/60001/content/renderer/media/...
File content/renderer/media/rtc_video_encoder.cc (right):

https://codereview.chromium.org/1212133002/diff/60001/content/renderer/media/...
content/renderer/media/rtc_video_encoder.cc:539:
frame->AddDestructionObserver(media::BindToCurrentLoop(
On 2015/06/26 23:52:56, mcasas wrote:
> This will register EncodeFrameFinished() a second time
> for memory-backed VideoFrames, after l. 511. Is that
> what we want?

Thanks for pointing out, I removed l.511. I must have missed it while rebasing.

[mcasas, 2015-06-27 01:03:39]

LGTM % the comments below.

https://codereview.chromium.org/1212133002/diff/80001/content/renderer/media/...
File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right):

https://codereview.chromium.org/1212133002/diff/80001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:148:
first_frame_timestamp_(media::kNoTimestamp()) {
Not your problem but kNoTimestamp() is
a misleading name.

https://codereview.chromium.org/1212133002/diff/80001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:235:
MediaVideoFrameFactory* media_video_frame_factory =
MediaVideoFrameFactory* const ?
:-)

https://codereview.chromium.org/1212133002/diff/80001/content/renderer/media/...
File content/renderer/media/webrtc/webrtc_video_capturer_adapter.h (right):

https://codereview.chromium.org/1212133002/diff/80001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_video_capturer_adapter.h:57: class
MediaVideoFrameFactory;
I guess this is not needed either.
Remove it here and move MediaVideoFrameFactory 
to be a class in the anonymous namespace inside
content namespace in webrtc_video_capturer_adapter.cc
file.

[emircan, 2015-06-30 22:48:54]

Ping 
magjed@ please take a look.

[magjed_chromium, 2015-07-03 10:51:45]

lgtm

https://codereview.chromium.org/1212133002/diff/80001/content/renderer/media/...
File content/renderer/media/webrtc/webrtc_video_capturer_adapter.h (right):

https://codereview.chromium.org/1212133002/diff/80001/content/renderer/media/...
content/renderer/media/webrtc/webrtc_video_capturer_adapter.h:57: class
MediaVideoFrameFactory;
On 2015/06/27 01:03:38, mcasas wrote:
> I guess this is not needed either.
> Remove it here and move MediaVideoFrameFactory 
> to be a class in the anonymous namespace inside
> content namespace in webrtc_video_capturer_adapter.cc
> file.

+1

[emircan, 2015-07-06 17:33:58]

Thanks. 

dalecurtis@ RS owners review please.

[emircan, 2015-07-06 17:34:19]

emircan@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[DaleCurtis, 2015-07-06 17:40:03]

dalecurtis@chromium.org changed reviewers:
+ perkj@chromium.org, tommi@chromium.org
- dalecurtis@chromium.org

[DaleCurtis, 2015-07-06 17:40:03]

tommi/perkj are specific owners for this code area, so I defer to them for this
patch.

[tommi (sloooow) - chröme, 2015-07-09 14:30:19]

lgtm

[emircan, 2015-07-09 17:07:44]

The CQ bit was checked by emircan@chromium.org

[commit-bot: I haz the power, 2015-07-09 17:09:18]

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

[commit-bot: I haz the power, 2015-07-09 18:30:54]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-07-09 18:31:40]

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