Use texture-backed VideoFrane pipeline for Aura desktop capturing.

Use texture-backed VideoFrane pipeline for Aura desktop capturing.

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

[sheu, 2014-04-01 00:11:26]

Two comments added already on the ickiest parts.  Please feel free to comment or
ask if that's unclear.

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

https://codereview.chromium.org/213253005/diff/20001/content/browser/media/ca...
content/browser/media/capture/content_video_capture_device_core.cc:123: if
(video_frame_format_ != media::VideoFrame::NATIVE_TEXTURE) {
This part is a little icky, in that we return (or don't return) a VideoFrame
based on whether the format is NATIVE_TEXTURE or not.  I'd like to return a
VideoFrame or not unconditionally; the problem is that here I can't construct a
NATIVE_TEXTURE frame yet, as we won't know the coded_size until the
CopyOutputRequest returns with the captured texture.

Suggestions?  Do you think it would make sense for CopyOutputRequest to be able
to specify a texture size, if we provide it a mailbox to capture into?

https://codereview.chromium.org/213253005/diff/20001/content/browser/media/ca...
File content/browser/media/capture/desktop_capture_device_aura.cc (right):

https://codereview.chromium.org/213253005/diff/20001/content/browser/media/ca...
content/browser/media/capture/desktop_capture_device_aura.cc:328: return;
Cursor compositing doesn't happen, of course.

[sheu, 2014-04-01 00:38:52]

PTAL, especially w.r.t. the design here.  I've commented a bit already.

[Ami GONE FROM CHROMIUM, 2014-04-01 18:03:37]

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

https://codereview.chromium.org/213253005/diff/20001/content/browser/media/ca...
content/browser/media/capture/content_video_capture_device_core.cc:61:
video_frame_format_ = media::VideoFrame::UNKNOWN;
This needs to be handled elsewhere with some early-outs, no?

https://codereview.chromium.org/213253005/diff/20001/content/browser/media/ca...
content/browser/media/capture/content_video_capture_device_core.cc:123: if
(video_frame_format_ != media::VideoFrame::NATIVE_TEXTURE) {
On 2014/04/01 00:11:26, sheu wrote:
> This part is a little icky, in that we return (or don't return) a VideoFrame
> based on whether the format is NATIVE_TEXTURE or not.  I'd like to return a
> VideoFrame or not unconditionally; the problem is that here I can't construct
a
> NATIVE_TEXTURE frame yet, as we won't know the coded_size until the
> CopyOutputRequest returns with the captured texture.
> 
> Suggestions?  Do you think it would make sense for CopyOutputRequest to be
able
> to specify a texture size, if we provide it a mailbox to capture into?

Is it necessary to populate *storage here, considering the frame is passed to
DidCopyOutput anyway?
(IOW can we populate it in DCO() unconditionally)

https://codereview.chromium.org/213253005/diff/20001/content/browser/media/ca...
File content/browser/media/capture/content_video_capture_device_core.h (right):

https://codereview.chromium.org/213253005/diff/20001/content/browser/media/ca...
content/browser/media/capture/content_video_capture_device_core.h:56: // If
|success| is false, all other parameters are invalid.
Maybe make this the first param, then?

https://codereview.chromium.org/213253005/diff/20001/content/browser/media/ca...
File content/browser/media/capture/desktop_capture_device_aura.cc (right):

https://codereview.chromium.org/213253005/diff/20001/content/browser/media/ca...
content/browser/media/capture/desktop_capture_device_aura.cc:325:
base::TimeDelta(),
Why the empty TimeDelta?

https://codereview.chromium.org/213253005/diff/20001/content/browser/media/ca...
content/browser/media/capture/desktop_capture_device_aura.cc:326:
media::VideoFrame::ReadPixelsCB());
Why the empty ReadPixelsCB?

[Ami GONE FROM CHROMIUM, 2014-04-17 17:15:22]

Ping?  
@sheu: are you planning to look at this again?
@hshi: are you planning to take this over?

[sheu, 2014-04-21 20:39:39]

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

https://codereview.chromium.org/213253005/diff/20001/content/browser/media/ca...
content/browser/media/capture/content_video_capture_device_core.cc:123: if
(video_frame_format_ != media::VideoFrame::NATIVE_TEXTURE) {
On 2014/04/01 18:03:38, Ami Fischman wrote:
> On 2014/04/01 00:11:26, sheu wrote:
> > This part is a little icky, in that we return (or don't return) a VideoFrame
> > based on whether the format is NATIVE_TEXTURE or not.  I'd like to return a
> > VideoFrame or not unconditionally; the problem is that here I can't
construct
> a
> > NATIVE_TEXTURE frame yet, as we won't know the coded_size until the
> > CopyOutputRequest returns with the captured texture.
> > 
> > Suggestions?  Do you think it would make sense for CopyOutputRequest to be
> able
> > to specify a texture size, if we provide it a mailbox to capture into?
> 
> Is it necessary to populate *storage here, considering the frame is passed to
> DidCopyOutput anyway?
> (IOW can we populate it in DCO() unconditionally)

This frame population uses a lot of state that is internal to the
ThreadSafeCaptureOracle.  We'd have to break out that state in a clean way to
populate the VideoFrame in DCO.

https://codereview.chromium.org/213253005/diff/20001/content/browser/media/ca...
File content/browser/media/capture/desktop_capture_device_aura.cc (right):

https://codereview.chromium.org/213253005/diff/20001/content/browser/media/ca...
content/browser/media/capture/desktop_capture_device_aura.cc:325:
base::TimeDelta(),
On 2014/04/01 18:03:38, Ami Fischman wrote:
> Why the empty TimeDelta?

Just stubbed for now.  It would be better to have some sort of timestamp
reported back from the CopyOutputRequest.

https://codereview.chromium.org/213253005/diff/20001/content/browser/media/ca...
content/browser/media/capture/desktop_capture_device_aura.cc:326:
media::VideoFrame::ReadPixelsCB());
On 2014/04/01 18:03:38, Ami Fischman wrote:
> Why the empty ReadPixelsCB?

Not expecting this frame to be read back to software buffers, for now.

[hshi1, 2014-04-23 19:00:28]

+perkj to advise because this CL has conflicts with trunk; the class
WebRtcVideoSinkAdapter no longer exists.

The class MediaStreamRemoteVideoSource seems to have similar structure as
WebRtcVideoSinkAdapter but the name has "Source" in it... can you confirm it is
actually serving the role of the sink adapter?

[perkj_chrome, 2014-04-24 08:13:24]

On 2014/04/23 19:00:28, hshi1 wrote:
> +perkj to advise because this CL has conflicts with trunk; the class
> WebRtcVideoSinkAdapter no longer exists.
> 
> The class MediaStreamRemoteVideoSource seems to have similar structure as
> WebRtcVideoSinkAdapter but the name has "Source" in it... can you confirm it
is
> actually serving the role of the sink adapter?

Yes, frames received from a remote end over a pc is received in the Chrome class
MediaStreamRemoteVideoSource. The purpose is that such a stream can be used as a
source.

[hshi1, 2014-04-24 18:24:37]

On 2014/04/24 08:13:24, perkj wrote:
> On 2014/04/23 19:00:28, hshi1 wrote:
> > +perkj to advise because this CL has conflicts with trunk; the class
> > WebRtcVideoSinkAdapter no longer exists.
> > 
> > The class MediaStreamRemoteVideoSource seems to have similar structure as
> > WebRtcVideoSinkAdapter but the name has "Source" in it... can you confirm it
> is
> > actually serving the role of the sink adapter?
> 
> Yes, frames received from a remote end over a pc is received in the Chrome
class
> MediaStreamRemoteVideoSource. The purpose is that such a stream can be used as
a
> source.

Thanks! I've uploaded https://codereview.chromium.org/258663003/ which is under
my chromium account and is rebased at trunk.

I'll take over and address the comments over there.

[hshi1, 2014-04-24 20:55:45]

https://codereview.chromium.org/213253005/diff/20001/content/browser/media/ca...
File content/browser/media/capture/content_video_capture_device_core.h (right):

https://codereview.chromium.org/213253005/diff/20001/content/browser/media/ca...
content/browser/media/capture/content_video_capture_device_core.h:56: // If
|success| is false, all other parameters are invalid.
On 2014/04/01 18:03:38, Ami Fischman wrote:
> Maybe make this the first param, then?

It's a bit messy. See web_contents_video_capture_device.cc:
FrameSubscriber::ShouldCaptureFrame() where we return
base::Bind(capture_frame_cb, *storage) as type
content::RenderWidgetHostViewFrameSubscriber::DeliverFrameCallback, which
requires bool |frame_captured| as the last param. We'd either re-define
content::RenderWidgetHostViewFrameSubscriber::DeliverFrameCallback or add a
wrapper to swap the params.

[Ami GONE FROM CHROMIUM, 2014-04-24 21:05:27]

https://codereview.chromium.org/213253005/diff/20001/content/browser/media/ca...
File content/browser/media/capture/content_video_capture_device_core.h (right):

https://codereview.chromium.org/213253005/diff/20001/content/browser/media/ca...
content/browser/media/capture/content_video_capture_device_core.h:56: // If
|success| is false, all other parameters are invalid.
On 2014/04/24 20:55:45, hshi1 wrote:
> On 2014/04/01 18:03:38, Ami Fischman wrote:
> > Maybe make this the first param, then?
> 
> It's a bit messy. See web_contents_video_capture_device.cc:
> FrameSubscriber::ShouldCaptureFrame() where we return
> base::Bind(capture_frame_cb, *storage) as type
> content::RenderWidgetHostViewFrameSubscriber::DeliverFrameCallback, which
> requires bool |frame_captured| as the last param. We'd either re-define
> content::RenderWidgetHostViewFrameSubscriber::DeliverFrameCallback or add a
> wrapper to swap the params.

Yeah, not worth it.

[hshi1, 2014-04-24 21:22:34]

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

https://codereview.chromium.org/213253005/diff/20001/content/browser/media/ca...
content/browser/media/capture/content_video_capture_device_core.cc:61:
video_frame_format_ = media::VideoFrame::UNKNOWN;
On 2014/04/01 18:03:38, Ami Fischman wrote:
> This needs to be handled elsewhere with some early-outs, no?

I'll add a format check in the caller
(ContentVideoCaptureDeviceCore::AllocateAndStart) where I can early-out by
calling client->OnError.

In that case this "default" will never be reached. Do you want me to remove the
NOTREACHED() here?

[Ami GONE FROM CHROMIUM, 2014-04-24 22:02:37]

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

https://codereview.chromium.org/213253005/diff/20001/content/browser/media/ca...
content/browser/media/capture/content_video_capture_device_core.cc:61:
video_frame_format_ = media::VideoFrame::UNKNOWN;
On 2014/04/24 21:22:35, hshi1 wrote:
> On 2014/04/01 18:03:38, Ami Fischman wrote:
> > This needs to be handled elsewhere with some early-outs, no?
> 
> I'll add a format check in the caller
> (ContentVideoCaptureDeviceCore::AllocateAndStart) where I can early-out by
> calling client->OnError.
> 
> In that case this "default" will never be reached. Do you want me to remove
the
> NOTREACHED() here?

no code is best code so if you can delete it then that's great.
I suspect that you can't delete the default: clause without accounting for all
the enum values, so I think l.59-62 will turn into:
LOG(FATAL) << "Unpossible! " << params_.requested_format.pixel_format;