Use texture-backed VideoFrame pipeline for Aura desktop capturing.

Use texture-backed VideoFrame pipeline for Aura desktop capturing.

BUG=269312
TEST=local build, run on CrOS snow

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

[hshi1, 2014-04-24 18:32:41]

note: original CL from sheu is here: https://codereview.chromium.org/213253005/

this hasn't addressed any comments yet; I'm just copying the CL over (and
rebased at trunk) as a baseline.

[hshi1, 2014-04-25 21:15:56]

+fischman PTAL (+sheu as CC)

The Patch Set #1 is copied from sheu's original CL at
https://codereview.chromium.org/213253005/ but rebased and resolved conflict at
trunk. The subsequent patches addressed a few comments and fixed a DCHECK in
several content unittests.

[hshi1, 2014-04-30 00:23:45]

ping...?

[Ami GONE FROM CHROMIUM, 2014-04-30 00:30:24]

Yeah, still in my TODO stack, which is sadly on fire.  Soon, I hope.


On Tue, Apr 29, 2014 at 5:23 PM, <hshi@chromium.org> wrote:

> ping...?
>
> https://codereview.chromium.org/258663003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ami GONE FROM CHROMIUM, 2014-04-30 03:03:43]

Sorry for the delay; I somehow thought this was a huge CL :( 
but it's actually really straightforward :)

https://codereview.chromium.org/258663003/diff/60001/content/browser/media/ca...
File content/browser/media/capture/content_video_capture_device_core.cc (right):

https://codereview.chromium.org/258663003/diff/60001/content/browser/media/ca...
content/browser/media/capture/content_video_capture_device_core.cc:120: //
moment.  We do not construct those frames.
point at where those frames _are_ constructed :)

https://codereview.chromium.org/258663003/diff/60001/content/renderer/media/w...
File content/renderer/media/webrtc/media_stream_remote_video_source.cc (right):

https://codereview.chromium.org/258663003/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc/media_stream_remote_video_source.cc:67:
video_frame = static_cast<media::VideoFrame*>(frame->GetNativeHandle());
urr, wat?

What happened in this thunk?

[Ami GONE FROM CHROMIUM, 2014-04-30 03:04:10]

LGTM assuming that GetNativeHandle business was just craxy before.

[hshi1, 2014-04-30 18:05:19]

https://codereview.chromium.org/258663003/diff/60001/content/renderer/media/w...
File content/renderer/media/webrtc/media_stream_remote_video_source.cc (right):

https://codereview.chromium.org/258663003/diff/60001/content/renderer/media/w...
content/renderer/media/webrtc/media_stream_remote_video_source.cc:67:
video_frame = static_cast<media::VideoFrame*>(frame->GetNativeHandle());
On 2014/04/30 03:03:43, Ami Fischman wrote:
> urr, wat?
> 
> What happened in this thunk?

I actually don't think this change is correct, we should revert this file.

The cricket::VideoFrame::GetNativeHandle() returns a valid (void*) handle if the
VideoFrame object is an instance of class cricket::WebRtcTextureVideoFrame.
Aside from a trivial copy constructor, the only path I found to construct such
an instance is via call WebRtcVideoEngine::DeliverFrame() ->
WebRtcVideoEngine::DeliverTextureFrame().

Example of call to DeliverFrame() with valid handle: vie_renderer.cc:
ViEExternalRendererImpl::RenderFrame() where the handle comes from
webrtc::TextureVideoFrame::native_handle().

Example of constructor of webrtc::TextureVideoFrame is seen in
RTCVideoDecoder::PictureReady() where we supply the handle as new
webrtc::RefCountImpl<NativeHandleImpl>(media::VideoFrame).

Thus, the frame->GetNativeHandle() should be first static_cast'ed to
NativeHandleImpl* and then we obtain media::VideoFrame* pointer with
NativeHandleImpl::GetHandle(), as originally written.

[Ami GONE FROM CHROMIUM, 2014-04-30 18:06:26]

SGTM


On Wed, Apr 30, 2014 at 11:05 AM, <hshi@chromium.org> wrote:

>
> https://codereview.chromium.org/258663003/diff/60001/
> content/renderer/media/webrtc/media_stream_remote_video_source.cc
> File content/renderer/media/webrtc/media_stream_remote_video_source.cc
> (right):
>
> https://codereview.chromium.org/258663003/diff/60001/
> content/renderer/media/webrtc/media_stream_remote_video_
> source.cc#newcode67
> content/renderer/media/webrtc/media_stream_remote_video_source.cc:67:
> video_frame = static_cast<media::VideoFrame*>(frame->GetNativeHandle());
> On 2014/04/30 03:03:43, Ami Fischman wrote:
>
>> urr, wat?
>>
>
>  What happened in this thunk?
>>
>
> I actually don't think this change is correct, we should revert this
> file.
>
> The cricket::VideoFrame::GetNativeHandle() returns a valid (void*)
> handle if the VideoFrame object is an instance of class
> cricket::WebRtcTextureVideoFrame. Aside from a trivial copy constructor,
> the only path I found to construct such an instance is via call
> WebRtcVideoEngine::DeliverFrame() ->
> WebRtcVideoEngine::DeliverTextureFrame().
>
> Example of call to DeliverFrame() with valid handle: vie_renderer.cc:
> ViEExternalRendererImpl::RenderFrame() where the handle comes from
> webrtc::TextureVideoFrame::native_handle().
>
> Example of constructor of webrtc::TextureVideoFrame is seen in
> RTCVideoDecoder::PictureReady() where we supply the handle as new
> webrtc::RefCountImpl<NativeHandleImpl>(media::VideoFrame).
>
> Thus, the frame->GetNativeHandle() should be first static_cast'ed to
> NativeHandleImpl* and then we obtain media::VideoFrame* pointer with
> NativeHandleImpl::GetHandle(), as originally written.
>
> https://codereview.chromium.org/258663003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sheu, 2014-04-30 18:50:49]

On 2014/04/30 18:05:19, hshi1 wrote:
> I actually don't think this change is correct, we should revert this file.
> 
> The cricket::VideoFrame::GetNativeHandle() returns a valid (void*) handle if
the
> VideoFrame object is an instance of class cricket::WebRtcTextureVideoFrame.
> Aside from a trivial copy constructor, the only path I found to construct such
> an instance is via call WebRtcVideoEngine::DeliverFrame() ->
> WebRtcVideoEngine::DeliverTextureFrame().
> 
> Example of call to DeliverFrame() with valid handle: vie_renderer.cc:
> ViEExternalRendererImpl::RenderFrame() where the handle comes from
> webrtc::TextureVideoFrame::native_handle().
> 
> Example of constructor of webrtc::TextureVideoFrame is seen in
> RTCVideoDecoder::PictureReady() where we supply the handle as new
> webrtc::RefCountImpl<NativeHandleImpl>(media::VideoFrame).
> 
> Thus, the frame->GetNativeHandle() should be first static_cast'ed to
> NativeHandleImpl* and then we obtain media::VideoFrame* pointer with
> NativeHandleImpl::GetHandle(), as originally written.

It's probably not correct as WebRTC handles NativeHandleImpls right now.  I used
this path for the internal hacks I put into WebRTC for passing the frames
around; the "proper" method has probably changed by now.

Out of curiosity: does libjingle/WebRTC properly hand off ownership of the
native handles now?  media::VideoFrames are ref-counted, so that's kind of
important.

[hshi1, 2014-04-30 18:52:53]

The CQ bit was checked by hshi@chromium.org

[commit-bot: I haz the power, 2014-04-30 18:53:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/258663003/80001

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

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

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

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

[hshi1, 2014-04-30 22:21:19]

The CQ bit was checked by hshi@chromium.org

[commit-bot: I haz the power, 2014-04-30 22:22:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/258663003/80001

[commit-bot: I haz the power, 2014-05-01 01:47:55]

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

[commit-bot: I haz the power, 2014-05-01 01:47:55]

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

[hshi1, 2014-05-01 21:15:01]

The CQ bit was checked by hshi@chromium.org

[commit-bot: I haz the power, 2014-05-01 21:15:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/258663003/80001

[hshi1, 2014-05-01 21:22:27]

The CQ bit was unchecked by hshi@chromium.org

[hshi1, 2014-05-01 21:23:36]

On 2014/05/01 21:15:43, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/hshi@chromium.org/258663003/80001

I'm aborting CQ for now. Looks like r267290 has changed the typedef of
ReleaseMailboxCB while my CL was in flight. Will re-base and fix.

[hshi1, 2014-05-01 22:18:47]

The CQ bit was checked by hshi@chromium.org

[commit-bot: I haz the power, 2014-05-01 22:19:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/258663003/100001

[commit-bot: I haz the power, 2014-05-01 23:16:11]

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

[commit-bot: I haz the power, 2014-05-01 23:16:12]

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

[hshi1, 2014-05-01 23:18:57]

The CQ bit was checked by hshi@chromium.org

[commit-bot: I haz the power, 2014-05-01 23:19:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hshi@chromium.org/258663003/100001

[commit-bot: I haz the power, 2014-05-02 01:24:20]

Change committed as 267717

[Lei Zhang, 2014-05-02 03:49:20]

A revert of this CL has been created in
https://codereview.chromium.org/267813002/ by thestig@chromium.org.

The reason for reverting is: Likely cause of all Mac browser_tests failures

[4296:71427:0501/193356:FATAL:video_util.cc(145)] Check failed:
!(view_area.height() & 1).
0   libbase.dylib                       0x698ac3ef
base::debug::StackTrace::StackTrace() + 63
1   libbase.dylib                       0x698ac44b
base::debug::StackTrace::StackTrace() + 43
2   libbase.dylib                       0x69951672
logging::LogMessage::~LogMessage() + 82
3   libbase.dylib                       0x699500eb
logging::LogMessage::~LogMessage() + 43
4   libmedia.dylib                      0x67756110
media::LetterboxYUV(media::VideoFrame*, gfx::Rect const&) + 928
5   libmedia.dylib                      0x6775773c
media::CopyRGBToVideoFrame(unsigned char const*, int, gfx::Rect const&,
media::VideoFrame*) + 492
6   libcontent.dylib                    0x75f39814 content::(anonymous
namespace)::RenderVideoFrame(SkBitmap const&,
scoped_refptr\u003Cmedia::VideoFrame> const&, base::Callback\u003Cvoid ()(bool)>
const&) + 2420.