Tab Capture: Backing store readbacks to YV12 VideoFrames.

Tab Capture: Backing store readbacks to YV12 VideoFrames.

Goal here is performance; in tab capture there are
significant benefits to doing the readback as YV12:
it's less data to copy back, and it's cheap to do
on the GPU.

VideoFrame is used because it's the most capable YV12
container.

[render_widget_host.h] 
Add a new flavor of CopyFromBackingStore, called 
CopyFromBackingStoreToVideoFrame. The semantics are 
slightly different from the RGBA copy in ways that are 
video-appropriate: aspect ratio is preserved via 
letterboxing, meaning the output is returned at the target's
allocated size, no matter what (whereas CopyFromBackingStore
returns whatever it wants, treating |dst_size| as a hint).

Callers may only call CopyFromBackingStoreToVideoFrame
after checking the result of CanCopyToVideoFrame(). 
Support is only on Windows now, and only while accelerated
compositing is active. But the interface defined
here should make it possible to implement VideoFrame
readbacks on other platforms without having to touch
the callers.

[video_capture_controller.h]
Amend the interface to allow a VideoFrame to
be passed in, as an alternative to void*.
The buffer-based interface was inadequate for
our needs since stride was not handled. Using
VideoFrame allows strides to be accomodated,
and paves the way for the interface to
pre-reserve the VideoFrames.

[web_contents_video_capture_device.cc]
Start using CopyFromBackingStoreToVideoFrame. Handling
both copy flavors requires a bifurcation of much
of the processing pipeline. When dealing with a VideoFrame,
the Render stage can is bypassed completely.

[accelerated_surface_win.h]
Implementation of VideoFrame YV12 readback, using
AcceleratedSurfaceTransformer's d3d-accelerated routines.

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

[ncarter (slow), 2013-02-03 20:26:45]

+brettw for changes to render_widget_host interface
+wjia for media/video/capture
+apatrick for ui/surface
+scherkus for web_contents_video_capture_device

[apatrick_chromium, 2013-02-04 19:16:42]

ui/surface LGTM

https://codereview.chromium.org/12090109/diff/14008/ui/surface/accelerated_su...
File ui/surface/accelerated_surface_win.cc (right):

https://codereview.chromium.org/12090109/diff/14008/ui/surface/accelerated_su...
ui/surface/accelerated_surface_win.cc:61: void (*copier)(const uint8* source,
Any reason not to use a base::Callback here?

[ncarter (slow), 2013-02-04 19:38:04]

https://codereview.chromium.org/12090109/diff/14008/ui/surface/accelerated_su...
File ui/surface/accelerated_surface_win.cc (right):

https://codereview.chromium.org/12090109/diff/14008/ui/surface/accelerated_su...
ui/surface/accelerated_surface_win.cc:61: void (*copier)(const uint8* source,
On 2013/02/04 19:16:43, apatrick_chromium wrote:
> Any reason not to use a base::Callback here?

Not really; I just thought this was more straightforward. If you use
base::Callback you don't really avoid the function call syntax, and the use case
doesn't require currying:

if (!LockAndCopyPlane(y, frame, &media::CopyYPlane))
  return false;

[apatrick_chromium, 2013-02-04 19:58:57]

https://codereview.chromium.org/12090109/diff/14008/ui/surface/accelerated_su...
File ui/surface/accelerated_surface_win.cc (right):

https://codereview.chromium.org/12090109/diff/14008/ui/surface/accelerated_su...
ui/surface/accelerated_surface_win.cc:61: void (*copier)(const uint8* source,
Fair enough. I would have done it for consistency with the rest of the code but
this works fine.

[wjia(left Chromium), 2013-02-04 23:48:40]

Thanks for refactoring OnIncomingCapturedFrame()! We need support non-compact
frame buffers.

https://codereview.chromium.org/12090109/diff/14008/content/browser/renderer_...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/12090109/diff/14008/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_controller.cc:391:
media::VideoFrame::YV12,  // Actually I420, but it's equivalent here.
nit: indent by 4.

https://codereview.chromium.org/12090109/diff/14008/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_controller.cc:447:
target_as_frame = NULL;  // Wrapper was for temporary use only.
this is not needed.

https://codereview.chromium.org/12090109/diff/14008/content/browser/renderer_...
File content/browser/renderer_host/media/video_capture_manager_unittest.cc
(right):

https://codereview.chromium.org/12090109/diff/14008/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_manager_unittest.cc:50:
base::Time timestamp) OVERRIDE {}
For function declarations and definitions, put each argument on a separate line
unless the whole declaration fits on one line
(http://dev.chromium.org/developers/coding-style)

[ncarter (slow), 2013-02-05 17:36:07]

https://codereview.chromium.org/12090109/diff/14008/content/browser/renderer_...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/12090109/diff/14008/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_controller.cc:391:
media::VideoFrame::YV12,  // Actually I420, but it's equivalent here.
On 2013/02/04 23:48:40, wjia wrote:
> nit: indent by 4.

Done.

https://codereview.chromium.org/12090109/diff/14008/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_controller.cc:447:
target_as_frame = NULL;  // Wrapper was for temporary use only.
On 2013/02/04 23:48:40, wjia wrote:
> this is not needed.

Done.

https://codereview.chromium.org/12090109/diff/14008/content/browser/renderer_...
File content/browser/renderer_host/media/video_capture_manager_unittest.cc
(right):

https://codereview.chromium.org/12090109/diff/14008/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_manager_unittest.cc:50:
base::Time timestamp) OVERRIDE {}
On 2013/02/04 23:48:40, wjia wrote:
> For function declarations and definitions, put each argument on a separate
line
> unless the whole declaration fits on one line
> (http://dev.chromium.org/developers/coding-style)

Done.

[scherkus (not reviewing), 2013-02-05 22:40:44]

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_controller.cc:275:
CHECK(dib->created_size() >= static_cast<size_t>(frame_info_.width *
nit: CHECK_GE()?

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_controller.cc:390: return;  //
Only exact copies are supported.
if you reserve a DIB but return here, does the DIB stay permanently reserved?

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_controller.cc:415: // These
types will never work.
if possible, my preference would be to CHECK()since the caller is in the wrong

the runtime behaviour would be send an uninitialized target_as_frame pointing at
whatever used to be in the DIB, which doesn't seem very useful

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_controller.cc:446: default:
I like having the compiler break when enums are added/removed

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
File content/browser/renderer_host/media/video_capture_controller.h (right):

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_controller.h:97: // Helpers.
nit: this comment is useless -- mind removing?

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_controller.h:112: // DIB
allocator. Locate a free DIB object, reserve it, and return its id
nit: can you add blank lines before each of these // comments to avoid
walloftext code?

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_controller.h:114: bool
ReserveSharedMemory(int* buffer_id_out,
nit: the comment is a bit misleading as there is no allocation being performed
but rather finding an available DIB in owned_dibs_

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
File content/browser/renderer_host/media/web_contents_video_capture_device.cc
(right):

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:152:
void StartCopy(CaptureMachine* consumer,
nit: scoped-const-ref-& here + below

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:551:
frame_number, make_scoped_refptr(frame), frame_timestamp));
see my comments in video_capture_device.h about using const scoped_refptr<T>&
for passing around VideoFrame

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:672:
void OnSnapshotFailed(SnapshotError error,
hmm.... I believe one of my original aspirations for BackingStoreCopier was for
it to be standalone component and use a single callback for reporting
success+failure to maintain a 1:1 mapping between calls to StartCopy() and the
resulting call.

What do you think of something like:

enum SnapshotResult {
  SKBITMAP,
  VIDEO_FRAME,
  NO_SOURCE,
  TRANSIENT_ERROR
};
typedef base::Callback<void(SnapshotResult, int, Time, TimeDelta, SkBitmap,
VideoFrame)> SnapshotCB;

You could define a helper struct to package up the
int+Time+TimeDelta+SkBitmap+VideoFrame business and have said struct be NULL in
an error case.

Aside from CB vs calling CaptureMachine, I think having a single function call
isn't too horrible since it would cut down on the amount of boilerplate code
trampolining threads and checking FinishSnapshot().

WDYT?

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:798: //
Sets the color format used by OnIncomingCapturedFrame.
nit: () to function names

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:944:
BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
nit: formatting + indentation is incorrect (technically you should be wrapping
at the ( but that would leave you w/ no room)

either:
  a) drop BrowserThread::UI + FROM_HERE to next line
  b) put base::Bind( on previous line + 4 space indent the bind innards

FWIW (b) will work out better here

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:968:
base::Bind(&CaptureMachine::SnapshotCompleteBitmap, this,
ditto on formatting

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:969:
frame_number, start_time, duration, frame));
FYI the SkBitmap copy constructor says the pixels will be shared

I'm not very familiar w/ SkBitmap usage between threads, but I wonder if it's
possible for things to go bad here :\

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:996:
manager_thread_.message_loop()->PostTask(FROM_HERE,
ditto on formatting

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:1017:
manager_thread_.message_loop()->PostTask(FROM_HERE,
ditto on formatting

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:1028:
if (state_ != kCapturing) {
you can use FinishSnapshot() here

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
File
content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc
(right):

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc:299:
source()->SetUseVideoFrames(i % 2 == 1);
how will we know which code path failed if any of these expectations failed?

is running this test in a for loop required or are you trying to avoid
duplicating code?

I'd recommend one of the following, with a preference for either 3 or 4:
  1) Duplicate the code
  2) Move some of these expectations into WCVCDTest as a helper function
  3) Use SCOPED_TRACE() so we know the state of the test when something fails
  4) Make this a parameterized test using Bool() that flips SetUseVideoFrames()
via GetParam()

https://codereview.chromium.org/12090109/diff/35001/content/public/browser/re...
File content/public/browser/render_widget_host.h (right):

https://codereview.chromium.org/12090109/diff/35001/content/public/browser/re...
content/public/browser/render_widget_host.h:202: // Returns true if
CopyFromBackingStoreToVideoFrame is possible on the current
nit: add () to function names referenced in comments

https://codereview.chromium.org/12090109/diff/35001/content/public/browser/re...
content/public/browser/render_widget_host.h:204: // should check this before
each call to CopyFromBackingStoreToVideoFrame().
can we promote this "should" into a "must" and have CFBSTVF() crash if you
didn't check?

it appears that the behaviour today is to return an empty frame, which can lead
to problems -- refer to my other comment:
https://codereview.chromium.org/12090109/diff/35001/ui/surface/accelerated_su...

https://codereview.chromium.org/12090109/diff/35001/content/public/browser/re...
content/public/browser/render_widget_host.h:206: // Copies a given subset of the
backing store into a YV12 VideoFrame, and pass
nit: blank line before comments to avoid walloftext

https://codereview.chromium.org/12090109/diff/35001/media/video/capture/video...
File media/video/capture/video_capture_device.h (right):

https://codereview.chromium.org/12090109/diff/35001/media/video/capture/video...
media/video/capture/video_capture_device.h:41: // OnFrameInfo. |data| must be
packed, with no padding between rows and/or
pedantic nit: similar to using || around variable references in comments we like
to annotate function call references with () at the end to help distinguish
TypeNames from FunctionCalls().

https://codereview.chromium.org/12090109/diff/35001/media/video/capture/video...
media/video/capture/video_capture_device.h:48: // specified by an earlier call
to OnFrameInfo.
s/OnFrameInfo/OnFrameInfo()/

https://codereview.chromium.org/12090109/diff/35001/media/video/capture/video...
media/video/capture/video_capture_device.h:49: virtual void
OnIncomingCapturedVideoFrame(media::VideoFrame* frame,
I'm firmly in the camp that likes uses const scoped_refptr<T>& when passing
around ref-counted types (pros: self-documenting, callees won't forget to ref it
if they need to, no need for make_scoped_refptr(), cons: cumbersome)

https://codereview.chromium.org/12090109/diff/35001/media/video/capture/video...
media/video/capture/video_capture_device.h:50: base::Time timestamp) = 0;
FYI VideoFrame has a Get/SetTimestamp() field, although it uses TimeDeltas :|

https://codereview.chromium.org/12090109/diff/35001/ui/surface/accelerated_su...
File ui/surface/accelerated_surface_win.cc (right):

https://codereview.chromium.org/12090109/diff/35001/ui/surface/accelerated_su...
ui/surface/accelerated_surface_win.cc:61: void (*copier)(const uint8* source,
nit: typedef + docs?
typedef void(*CopyPlaneFunc)(const blah blah blah);

https://codereview.chromium.org/12090109/diff/35001/ui/surface/accelerated_su...
ui/surface/accelerated_surface_win.cc:402: 
nit: remove blank line

https://codereview.chromium.org/12090109/diff/35001/ui/surface/accelerated_su...
ui/surface/accelerated_surface_win.cc:412: target =
media::VideoFrame::CreateEmptyFrame();
I've never been a huge fan of the whole EMPTY frame business (you can end up
passing an empty frame all the way through the stack and into code that blindly
assumes its, say, YV12)

I haven't looked at all the callers, but do you feel having a
always-non-null-but-potentially-EMPTY VideoFrame is better than
potentially-null-but-always-YV12?

[ncarter (slow), 2013-02-06 23:54:44]

Fixed everything except the use of raw VideoFrame* pointers, which I discuss in
the comments below.

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_controller.cc:275:
CHECK(dib->created_size() >= static_cast<size_t>(frame_info_.width *
On 2013/02/05 22:40:44, scherkus wrote:
> nit: CHECK_GE()?

Done.

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_controller.cc:390: return;  //
Only exact copies are supported.
On 2013/02/05 22:40:44, scherkus wrote:
> if you reserve a DIB but return here, does the DIB stay permanently reserved?

Good catch! Reordered these operations.

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_controller.cc:415: // These
types will never work.
On 2013/02/05 22:40:44, scherkus wrote:
> if possible, my preference would be to CHECK()since the caller is in the wrong
> 
> the runtime behaviour would be send an uninitialized target_as_frame pointing
at
> whatever used to be in the DIB, which doesn't seem very useful

CHECK() isn't as much fun for the user when it happens in the browser process.
As a compromise, I added handling to the pre-ReserveSharedMemory validation
step, and do a "NOTREACHED()/return;".

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_controller.cc:446: default:
On 2013/02/05 22:40:44, scherkus wrote:
> I like having the compiler break when enums are added/removed

See above.

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
File content/browser/renderer_host/media/video_capture_controller.h (right):

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_controller.h:97: // Helpers.
On 2013/02/05 22:40:44, scherkus wrote:
> nit: this comment is useless -- mind removing?

Done, but I welcome wjia to contradict.

FWIW, I interpreted this comment as "from here on down these are implementation
helper functions, as opposed to trampoline targets." Touching this code for the
first time, I benefited from that hint. I also interpreted the lack of spacing
between the definitions was done so that it was clear that the 'Helpers' comment
applied to everything in the wall of text. The wall of text effect disappears if
you use an editor which syntax-colors the comments, there is a nice visual flow
in that case, so personally I am not bothered by such a lack of vertical
whitespace.

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_controller.h:112: // DIB
allocator. Locate a free DIB object, reserve it, and return its id
On 2013/02/05 22:40:44, scherkus wrote:
> nit: can you add blank lines before each of these // comments to avoid
> walloftext code?

Done.

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_controller.h:114: bool
ReserveSharedMemory(int* buffer_id_out,
On 2013/02/05 22:40:44, scherkus wrote:
> nit: the comment is a bit misleading as there is no allocation being performed
> but rather finding an available DIB in owned_dibs_

I see what you mean. Fixed the comment language.

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
File content/browser/renderer_host/media/web_contents_video_capture_device.cc
(right):

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:152:
void StartCopy(CaptureMachine* consumer,
On 2013/02/05 22:40:44, scherkus wrote:
> nit: scoped-const-ref-& here + below

Done.

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:551:
frame_number, make_scoped_refptr(frame), frame_timestamp));
On 2013/02/05 22:40:44, scherkus wrote:
> see my comments in video_capture_device.h about using const scoped_refptr<T>&
> for passing around VideoFrame

Done.

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:672:
void OnSnapshotFailed(SnapshotError error,
On 2013/02/05 22:40:44, scherkus wrote:
> hmm.... I believe one of my original aspirations for BackingStoreCopier was
for
> it to be standalone component and use a single callback for reporting
> success+failure to maintain a 1:1 mapping between calls to StartCopy() and the
> resulting call.
> 
> What do you think of something like:
> 
> enum SnapshotResult {
>   SKBITMAP,
>   VIDEO_FRAME,
>   NO_SOURCE,
>   TRANSIENT_ERROR
> };
> typedef base::Callback<void(SnapshotResult, int, Time, TimeDelta, SkBitmap,
> VideoFrame)> SnapshotCB;
> 
> You could define a helper struct to package up the
> int+Time+TimeDelta+SkBitmap+VideoFrame business and have said struct be NULL
in
> an error case.
> 
> Aside from CB vs calling CaptureMachine, I think having a single function call
> isn't too horrible since it would cut down on the amount of boilerplate code
> trampolining threads and checking FinishSnapshot().
> 
> WDYT?

I considered your suggested approach when writing this, and the "callback which
takes either an SkBitmap or a VideoFrame" seemed nasty to contemplate. It opens
the door to null-pointer dereferences all over the place, and pretty unreadable
code. I considered a few other designs too, and the one you see seemed like the
best compromise.

My intent here was actually for
OnSnapshotComplete/OnSnapshotComplete/OnSnapshotFailed to be extracted into an
interface, but the interface would be refcountedthreadsafe, and I was a little
worried about messing up the subtle destroy-time behavior of the CaptureMachine
by doing that (CaptureMachine uses some custom traits to RefCountedThreadSafe).
So I used the CaptureMachine type, and the interface is just a convention, hence
the trampolines.

My goal here was to land a new YV12 VideoFrame path for Windows, as simply as
possible, while minimizing the risk of regressing the ARGB paths. Longer term,
it's my hope for the SkBitmap path to go away. But we've gotta start somewhere.
One idea for the future is to return a RGB32 VideoFrame that's backed by an
SkBitmap. We've already got bugs filed for such follow-up work.

In M27, I think it's likely that we'll move to a model where Deliver happens in
the CopyFromBackingStore callback, and the goo in between will be restructured
(maybe resize+yuv convert will happen on a solitary worker thread, when not
hardware accelerated). Since there are no unit tests today -- none of these
classes is even declared in a header file that a unit test might #include -- I
feel we shouldn't invest too much effort perfecting the dependency-injection
story for this module at present.

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:798: //
Sets the color format used by OnIncomingCapturedFrame.
On 2013/02/05 22:40:44, scherkus wrote:
> nit: () to function names

Done.

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:944:
BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
On 2013/02/05 22:40:44, scherkus wrote:
> nit: formatting + indentation is incorrect (technically you should be wrapping
> at the ( but that would leave you w/ no room)
> 
> either:
>   a) drop BrowserThread::UI + FROM_HERE to next line
>   b) put base::Bind( on previous line + 4 space indent the bind innards
> 
> FWIW (b) will work out better here

My reading of the style guide allows what I put originally -- there's a little
ambiguity about what happens when you have a long argument but a short function
name. What I'm intuiting from your response is a rule saying that that "indent
just 4" is mutually exclusive of "arguments on the same line as LPAREN". If so
my style preference is for [A1], of the below options -- but let me know which
one you think is most Chrome-like, and I'll try to adjust my methodology.

[original]

    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
        base::Bind(&BackingStoreCopier::StartCopy,
                   base::Unretained(&copier_), make_scoped_refptr(this),
                   frame_number_, settings_.width, settings_.height));


[A1]

    BrowserThread::PostTask(
        BrowserThread::UI, FROM_HERE,
        base::Bind(&BackingStoreCopier::StartCopy,
                   base::Unretained(&copier_), make_scoped_refptr(this),
                   frame_number_, settings_.width, settings_.height));

[A2]

    BrowserThread::PostTask(
        BrowserThread::UI,
        FROM_HERE,
        base::Bind(&BackingStoreCopier::StartCopy,
                   base::Unretained(&copier_), make_scoped_refptr(this),
                   frame_number_, settings_.width, settings_.height));

[B1]

    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, base::Bind(
        &BackingStoreCopier::StartCopy, base::Unretained(&copier_),
        make_scoped_refptr(this), frame_number_, settings_.width,
        settings_.height));

[B2]

    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, base::Bind(
                                &BackingStoreCopier::StartCopy,
                                base::Unretained(&copier_),
                                make_scoped_refptr(this),
                                frame_number_,
                                settings_.width,
                                settings_.height));
[B3]

    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, 
                            base::Bind(
                                &BackingStoreCopier::StartCopy,
                                base::Unretained(&copier_),
                                make_scoped_refptr(this),
                                frame_number_,
                                settings_.width,
                                settings_.height));
[B4]

    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, base::Bind(
            &BackingStoreCopier::StartCopy,
            base::Unretained(&copier_),
            make_scoped_refptr(this),
            frame_number_,
            settings_.width,
            settings_.height));

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:968:
base::Bind(&CaptureMachine::SnapshotCompleteBitmap, this,
On 2013/02/05 22:40:44, scherkus wrote:
> ditto on formatting

Done.

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:969:
frame_number, start_time, duration, frame));
On 2013/02/05 22:40:44, scherkus wrote:
> FYI the SkBitmap copy constructor says the pixels will be shared
> 
> I'm not very familiar w/ SkBitmap usage between threads, but I wonder if it's
> possible for things to go bad here :\

The pixel-sharing behavior is intended. Think of SkBitmap as a
scoped_refptr<SkPixelRef>. SkPixelRef is intended to be thread safe. I spent
good time in the debugger, with memory breakpoints on the refcount, validating
all these behaviors.

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:996:
manager_thread_.message_loop()->PostTask(FROM_HERE,
On 2013/02/05 22:40:44, scherkus wrote:
> ditto on formatting

Done.

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:1017:
manager_thread_.message_loop()->PostTask(FROM_HERE,
On 2013/02/05 22:40:44, scherkus wrote:
> ditto on formatting

Done.

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:1028:
if (state_ != kCapturing) {
On 2013/02/05 22:40:44, scherkus wrote:
> you can use FinishSnapshot() here

Done.

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
File
content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc
(right):

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc:299:
source()->SetUseVideoFrames(i % 2 == 1);
On 2013/02/05 22:40:44, scherkus wrote:
> how will we know which code path failed if any of these expectations failed?
> 
> is running this test in a for loop required or are you trying to avoid
> duplicating code?
> 
> I'd recommend one of the following, with a preference for either 3 or 4:
>   1) Duplicate the code
>   2) Move some of these expectations into WCVCDTest as a helper function
>   3) Use SCOPED_TRACE() so we know the state of the test when something fails
>   4) Make this a parameterized test using Bool() that flips
SetUseVideoFrames()
> via GetParam()

Added a SCOPED_TRACE() and a comment. The test is simulating intentionally
switching from the SkBitmap to the VideoFrame codepath, which the real code
would do e.g. as you fall in and out of accelerated compositing.

https://codereview.chromium.org/12090109/diff/35001/content/public/browser/re...
File content/public/browser/render_widget_host.h (right):

https://codereview.chromium.org/12090109/diff/35001/content/public/browser/re...
content/public/browser/render_widget_host.h:202: // Returns true if
CopyFromBackingStoreToVideoFrame is possible on the current
On 2013/02/05 22:40:44, scherkus wrote:
> nit: add () to function names referenced in comments

Done.

https://codereview.chromium.org/12090109/diff/35001/content/public/browser/re...
content/public/browser/render_widget_host.h:204: // should check this before
each call to CopyFromBackingStoreToVideoFrame().
On 2013/02/05 22:40:44, scherkus wrote:
> can we promote this "should" into a "must" and have CFBSTVF() crash if you
> didn't check?
> 
> it appears that the behaviour today is to return an empty frame, which can
lead
> to problems -- refer to my other comment:
>
https://codereview.chromium.org/12090109/diff/35001/ui/surface/accelerated_su...

Thanks for sharing your experience (which sounds hard-won) about empty frames
possibly leading to trouble. As mentioned in another comment, I am moving to a
"bool success" param with a caller-allocated VideoFrame, which will eliminate
the need to synthesize empty frames.

Callers have to handle failure invocations of the callback, and the case
discussed here is just one (avoidable) case. Your suggestions about crashing the
browser don't mesh with either my instincts, or the recommendations here:
http://dev.chromium.org/developers/coding-style ["Use CHECK() if the consequence
of a failed assertion would be a security vulnerability, where crashing the
browser is preferable.  Because this takes down the whole browser, sometimes
there are better options than CHECK()."] I think NOTREACHED() is appropriate for
cases like this.

https://codereview.chromium.org/12090109/diff/35001/content/public/browser/re...
content/public/browser/render_widget_host.h:206: // Copies a given subset of the
backing store into a YV12 VideoFrame, and pass
On 2013/02/05 22:40:44, scherkus wrote:
> nit: blank line before comments to avoid walloftext

Done.

https://codereview.chromium.org/12090109/diff/35001/media/video/capture/video...
File media/video/capture/video_capture_device.h (right):

https://codereview.chromium.org/12090109/diff/35001/media/video/capture/video...
media/video/capture/video_capture_device.h:41: // OnFrameInfo. |data| must be
packed, with no padding between rows and/or
On 2013/02/05 22:40:44, scherkus wrote:
> pedantic nit: similar to using || around variable references in comments we
like
> to annotate function call references with () at the end to help distinguish
> TypeNames from FunctionCalls().

Done.

https://codereview.chromium.org/12090109/diff/35001/media/video/capture/video...
media/video/capture/video_capture_device.h:48: // specified by an earlier call
to OnFrameInfo.
On 2013/02/05 22:40:44, scherkus wrote:
> s/OnFrameInfo/OnFrameInfo()/

Done.

https://codereview.chromium.org/12090109/diff/35001/media/video/capture/video...
media/video/capture/video_capture_device.h:49: virtual void
OnIncomingCapturedVideoFrame(media::VideoFrame* frame,
On 2013/02/05 22:40:44, scherkus wrote:
> I'm firmly in the camp that likes uses const scoped_refptr<T>& when passing
> around ref-counted types (pros: self-documenting, callees won't forget to ref
it
> if they need to, no need for make_scoped_refptr(), cons: cumbersome)

Changing T*'s to const scoped_refptr<T>&'s would be a whole bunch of text
editing, and I happen to know that all affected lines are likely to churn in the
very near future -- because of the possibility of allocating a VideoFrame backed
by the DIB, I think we should move to a model where VideoFrames are allocated by
the caller of CopyFromBackingStoreToVideoFrame, and the callback gets a 'bool
success' param (Ironically, this is the model we just moved away from, for
CopyFromBackingStore/PlatformBitmap).

So, my inclination is to simply move to the new caller-allocated model
immediately, in this CL. I'm prepping it now.

https://codereview.chromium.org/12090109/diff/35001/media/video/capture/video...
media/video/capture/video_capture_device.h:50: base::Time timestamp) = 0;
On 2013/02/05 22:40:44, scherkus wrote:
> FYI VideoFrame has a Get/SetTimestamp() field, although it uses TimeDeltas :|

Yeah, I'm aware of that and I think it would be nice to unify it, but certainly
not in this change (I gather it would mean plumbing an origin timestamp to a
number of places).

https://codereview.chromium.org/12090109/diff/35001/ui/surface/accelerated_su...
File ui/surface/accelerated_surface_win.cc (right):

https://codereview.chromium.org/12090109/diff/35001/ui/surface/accelerated_su...
ui/surface/accelerated_surface_win.cc:61: void (*copier)(const uint8* source,
On 2013/02/05 22:40:44, scherkus wrote:
> nit: typedef + docs?
> typedef void(*CopyPlaneFunc)(const blah blah blah);

I got rid of the function pointer altogether by simply exposing the
CopyPlane(size_t plane_index ... ) helper function in video_util.h. Make sure
you have a look at that.

https://codereview.chromium.org/12090109/diff/35001/ui/surface/accelerated_su...
ui/surface/accelerated_surface_win.cc:402: 
On 2013/02/05 22:40:44, scherkus wrote:
> nit: remove blank line

Done.

https://codereview.chromium.org/12090109/diff/35001/ui/surface/accelerated_su...
ui/surface/accelerated_surface_win.cc:412: target =
media::VideoFrame::CreateEmptyFrame();
On 2013/02/05 22:40:44, scherkus wrote:
> I've never been a huge fan of the whole EMPTY frame business (you can end up
> passing an empty frame all the way through the stack and into code that
blindly
> assumes its, say, YV12)
> 
> I haven't looked at all the callers, but do you feel having a
> always-non-null-but-potentially-EMPTY VideoFrame is better than
> potentially-null-but-always-YV12?

I'm moving to a model where VideoFrame is allocated and passed in by the
original caller, the callback just receives a |bool status|, and we never
synthesize empty frames. I think this will address your concern.

[ncarter (slow), 2013-02-07 00:00:12]

https://codereview.chromium.org/12090109/diff/14008/ui/surface/accelerated_su...
File ui/surface/accelerated_surface_win.cc (right):

https://codereview.chromium.org/12090109/diff/14008/ui/surface/accelerated_su...
ui/surface/accelerated_surface_win.cc:61: void (*copier)(const uint8* source,
On 2013/02/04 19:58:58, apatrick_chromium wrote:
> Fair enough. I would have done it for consistency with the rest of the code
but
> this works fine.

This has been addressed.

[ncarter (slow), 2013-02-07 13:07:07]

scherkus, wjia: Please take another look.

[scherkus (not reviewing), 2013-02-07 18:25:03]

LGTM w/ some nits + replies'n'stuff

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
File content/browser/renderer_host/media/video_capture_controller.h (right):

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_controller.h:97: // Helpers.
On 2013/02/06 23:54:44, ncarter wrote:
> On 2013/02/05 22:40:44, scherkus wrote:
> > nit: this comment is useless -- mind removing?
> 
> Done, but I welcome wjia to contradict.
> 
> FWIW, I interpreted this comment as "from here on down these are
implementation
> helper functions, as opposed to trampoline targets." Touching this code for
the
> first time, I benefited from that hint. I also interpreted the lack of spacing
> between the definitions was done so that it was clear that the 'Helpers'
comment
> applied to everything in the wall of text. The wall of text effect disappears
if
> you use an editor which syntax-colors the comments, there is a nice visual
flow
> in that case, so personally I am not bothered by such a lack of vertical
> whitespace.

My $0.02 is making it readable for the lowest common denominator means
readability will always be improved regardless of someone's editor preference.

Example: this code review

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
File content/browser/renderer_host/media/web_contents_video_capture_device.cc
(right):

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:672:
void OnSnapshotFailed(SnapshotError error,
On 2013/02/06 23:54:44, ncarter wrote:
> On 2013/02/05 22:40:44, scherkus wrote:
> > hmm.... I believe one of my original aspirations for BackingStoreCopier was
> for
> > it to be standalone component and use a single callback for reporting
> > success+failure to maintain a 1:1 mapping between calls to StartCopy() and
the
> > resulting call.
> > 
> > What do you think of something like:
> > 
> > enum SnapshotResult {
> >   SKBITMAP,
> >   VIDEO_FRAME,
> >   NO_SOURCE,
> >   TRANSIENT_ERROR
> > };
> > typedef base::Callback<void(SnapshotResult, int, Time, TimeDelta, SkBitmap,
> > VideoFrame)> SnapshotCB;
> > 
> > You could define a helper struct to package up the
> > int+Time+TimeDelta+SkBitmap+VideoFrame business and have said struct be NULL
> in
> > an error case.
> > 
> > Aside from CB vs calling CaptureMachine, I think having a single function
call
> > isn't too horrible since it would cut down on the amount of boilerplate code
> > trampolining threads and checking FinishSnapshot().
> > 
> > WDYT?
> 
> I considered your suggested approach when writing this, and the "callback
which
> takes either an SkBitmap or a VideoFrame" seemed nasty to contemplate. It
opens
> the door to null-pointer dereferences all over the place, and pretty
unreadable
> code. I considered a few other designs too, and the one you see seemed like
the
> best compromise.
> 
> My intent here was actually for
> OnSnapshotComplete/OnSnapshotComplete/OnSnapshotFailed to be extracted into an
> interface, but the interface would be refcountedthreadsafe, and I was a little
> worried about messing up the subtle destroy-time behavior of the
CaptureMachine
> by doing that (CaptureMachine uses some custom traits to
RefCountedThreadSafe).
> So I used the CaptureMachine type, and the interface is just a convention,
hence
> the trampolines.
> 
> My goal here was to land a new YV12 VideoFrame path for Windows, as simply as
> possible, while minimizing the risk of regressing the ARGB paths. Longer term,
> it's my hope for the SkBitmap path to go away. But we've gotta start
somewhere.
> One idea for the future is to return a RGB32 VideoFrame that's backed by an
> SkBitmap. We've already got bugs filed for such follow-up work.

SGTM

> In M27, I think it's likely that we'll move to a model where Deliver happens
in
> the CopyFromBackingStore callback, and the goo in between will be restructured
> (maybe resize+yuv convert will happen on a solitary worker thread, when not
> hardware accelerated). Since there are no unit tests today -- none of these
> classes is even declared in a header file that a unit test might #include -- I
> feel we shouldn't invest too much effort perfecting the dependency-injection
> story for this module at present.

FWIW this isn't so much about dependency injection but the
layered-onion-composition approach of building bigger blocks out of small blocks
and having tests for said small blocks (but as you note [and I agree with] this
isn't something that needs to be done this very moment [but that won't stop me
from suggesting it each time someone touches this code!])

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:944:
BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
On 2013/02/06 23:54:44, ncarter wrote:
> On 2013/02/05 22:40:44, scherkus wrote:
> > nit: formatting + indentation is incorrect (technically you should be
wrapping
> > at the ( but that would leave you w/ no room)
> > 
> > either:
> >   a) drop BrowserThread::UI + FROM_HERE to next line
> >   b) put base::Bind( on previous line + 4 space indent the bind innards
> > 
> > FWIW (b) will work out better here
> 
> My reading of the style guide allows what I put originally -- there's a little
> ambiguity about what happens when you have a long argument but a short
function
> name. What I'm intuiting from your response is a rule saying that that "indent
> just 4" is mutually exclusive of "arguments on the same line as LPAREN". If so
> my style preference is for [A1], of the below options -- but let me know which
> one you think is most Chrome-like, and I'll try to adjust my methodology.
> 
> [original]
> 
>     BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
>         base::Bind(&BackingStoreCopier::StartCopy,
>                    base::Unretained(&copier_), make_scoped_refptr(this),
>                    frame_number_, settings_.width, settings_.height));
> 
> 
> [A1]
> 
>     BrowserThread::PostTask(
>         BrowserThread::UI, FROM_HERE,
>         base::Bind(&BackingStoreCopier::StartCopy,
>                    base::Unretained(&copier_), make_scoped_refptr(this),
>                    frame_number_, settings_.width, settings_.height));
> 
> [A2]
> 
>     BrowserThread::PostTask(
>         BrowserThread::UI,
>         FROM_HERE,
>         base::Bind(&BackingStoreCopier::StartCopy,
>                    base::Unretained(&copier_), make_scoped_refptr(this),
>                    frame_number_, settings_.width, settings_.height));
> 
> [B1]
> 
>     BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, base::Bind(
>         &BackingStoreCopier::StartCopy, base::Unretained(&copier_),
>         make_scoped_refptr(this), frame_number_, settings_.width,
>         settings_.height));
> 
> [B2]
> 
>     BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, base::Bind(
>                                 &BackingStoreCopier::StartCopy,
>                                 base::Unretained(&copier_),
>                                 make_scoped_refptr(this),
>                                 frame_number_,
>                                 settings_.width,
>                                 settings_.height));
> [B3]
> 
>     BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, 
>                             base::Bind(
>                                 &BackingStoreCopier::StartCopy,
>                                 base::Unretained(&copier_),
>                                 make_scoped_refptr(this),
>                                 frame_number_,
>                                 settings_.width,
>                                 settings_.height));
> [B4]
> 
>     BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, base::Bind(
>             &BackingStoreCopier::StartCopy,
>             base::Unretained(&copier_),
>             make_scoped_refptr(this),
>             frame_number_,
>             settings_.width,
>             settings_.height));

While most media-ish code uses [B1] (having more room when using base::Bind()
instead of an indentation-fest is always nice!), [A1] is totally cool. As
always, file-level consistency is best.

The clang C++ formatter likes [A1] so we can settle on that. It'll produce a
smaller diff when I run the formatter over the code ;)

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
File
content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc
(right):

https://codereview.chromium.org/12090109/diff/35001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc:299:
source()->SetUseVideoFrames(i % 2 == 1);
On 2013/02/06 23:54:44, ncarter wrote:
> On 2013/02/05 22:40:44, scherkus wrote:
> > how will we know which code path failed if any of these expectations failed?
> > 
> > is running this test in a for loop required or are you trying to avoid
> > duplicating code?
> > 
> > I'd recommend one of the following, with a preference for either 3 or 4:
> >   1) Duplicate the code
> >   2) Move some of these expectations into WCVCDTest as a helper function
> >   3) Use SCOPED_TRACE() so we know the state of the test when something
fails
> >   4) Make this a parameterized test using Bool() that flips
> SetUseVideoFrames()
> > via GetParam()
> 
> Added a SCOPED_TRACE() and a comment. The test is simulating intentionally
> switching from the SkBitmap to the VideoFrame codepath, which the real code
> would do e.g. as you fall in and out of accelerated compositing.

I had a feeling this test was doing that! Cool!

Thanks for updating the comment.

https://codereview.chromium.org/12090109/diff/35001/content/public/browser/re...
File content/public/browser/render_widget_host.h (right):

https://codereview.chromium.org/12090109/diff/35001/content/public/browser/re...
content/public/browser/render_widget_host.h:204: // should check this before
each call to CopyFromBackingStoreToVideoFrame().
On 2013/02/06 23:54:44, ncarter wrote:
> On 2013/02/05 22:40:44, scherkus wrote:
> > can we promote this "should" into a "must" and have CFBSTVF() crash if you
> > didn't check?
> > 
> > it appears that the behaviour today is to return an empty frame, which can
> lead
> > to problems -- refer to my other comment:
> >
>
https://codereview.chromium.org/12090109/diff/35001/ui/surface/accelerated_su...
> 
> Thanks for sharing your experience (which sounds hard-won) about empty frames
> possibly leading to trouble. As mentioned in another comment, I am moving to a
> "bool success" param with a caller-allocated VideoFrame, which will eliminate
> the need to synthesize empty frames.
> 
> Callers have to handle failure invocations of the callback, and the case
> discussed here is just one (avoidable) case. Your suggestions about crashing
the
> browser don't mesh with either my instincts, or the recommendations here:
> http://dev.chromium.org/developers/coding-style ["Use CHECK() if the
consequence
> of a failed assertion would be a security vulnerability, where crashing the
> browser is preferable.  Because this takes down the whole browser, sometimes
> there are better options than CHECK()."] I think NOTREACHED() is appropriate
for
> cases like this.

SGTM

https://codereview.chromium.org/12090109/diff/35001/media/video/capture/video...
File media/video/capture/video_capture_device.h (right):

https://codereview.chromium.org/12090109/diff/35001/media/video/capture/video...
media/video/capture/video_capture_device.h:49: virtual void
OnIncomingCapturedVideoFrame(media::VideoFrame* frame,
On 2013/02/06 23:54:44, ncarter wrote:
> On 2013/02/05 22:40:44, scherkus wrote:
> > I'm firmly in the camp that likes uses const scoped_refptr<T>& when passing
> > around ref-counted types (pros: self-documenting, callees won't forget to
ref
> it
> > if they need to, no need for make_scoped_refptr(), cons: cumbersome)
> 
> Changing T*'s to const scoped_refptr<T>&'s would be a whole bunch of text
> editing, and I happen to know that all affected lines are likely to churn in
the
> very near future -- because of the possibility of allocating a VideoFrame
backed
> by the DIB, I think we should move to a model where VideoFrames are allocated
by
> the caller of CopyFromBackingStoreToVideoFrame, and the callback gets a 'bool
> success' param (Ironically, this is the model we just moved away from, for
> CopyFromBackingStore/PlatformBitmap).
> 
> So, my inclination is to simply move to the new caller-allocated model
> immediately, in this CL. I'm prepping it now.

SGTM

https://codereview.chromium.org/12090109/diff/35001/media/video/capture/video...
media/video/capture/video_capture_device.h:50: base::Time timestamp) = 0;
On 2013/02/06 23:54:44, ncarter wrote:
> On 2013/02/05 22:40:44, scherkus wrote:
> > FYI VideoFrame has a Get/SetTimestamp() field, although it uses TimeDeltas
:|
> 
> Yeah, I'm aware of that and I think it would be nice to unify it, but
certainly
> not in this change (I gather it would mean plumbing an origin timestamp to a
> number of places).

Agreed -- I believe VF (and other media buffer-ish classes) should be using
base::Time but that's way beyond the scope of this CL.

https://codereview.chromium.org/12090109/diff/39088/content/browser/renderer_...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/12090109/diff/39088/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_controller.cc:448: default:
you can still replace this default w/ all other values of
media::VideoFrame::Format (this is more about forcing people who change the enum
to re-inspect this code)

https://codereview.chromium.org/12090109/diff/39088/content/browser/renderer_...
File
content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc
(right):

https://codereview.chromium.org/12090109/diff/39088/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc:301:
testing::Message() << "Using "
nit: you can also use base::StringPrintf() if you like printfs() over
operator<<()

[ncarter (slow), 2013-02-07 18:54:50]

brettw: This is ready for your attention ( content/ OWNERS )

[brettw, 2013-02-08 21:44:51]

When I discussed this with John he pointed out that we do not need to add
anything to the public API since the new backing store readback function is only
used from inside content (the backing store reader).

And I don't think we want this forwarding function in RWH. Instead the backing
store copyier can just access the port directly (GetView to get the RWHV, then
RenderWidgetHostViewPort::FromRWHV which takes a RWHV). The other backing store
copying functions should either be cleaned up or need to be exposed on this
class, so I don't think they're necessarily a guide for what we need to do for
this patch.

https://codereview.chromium.org/12090109/diff/43012/content/browser/renderer_...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/12090109/diff/43012/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_controller.cc:284:
///////////////////////////////////////////////////////////////////////////////
Remove /////...

[ncarter (slow), 2013-02-08 22:23:35]

On 2013/02/08 21:44:51, brettw wrote:
> When I discussed this with John he pointed out that we do not need to add
> anything to the public API since the new backing store readback function is
only
> used from inside content (the backing store reader).
> 
> And I don't think we want this forwarding function in RWH. Instead the backing
> store copyier can just access the port directly (GetView to get the RWHV, then
> RenderWidgetHostViewPort::FromRWHV which takes a RWHV). The other backing
store
> copying functions should either be cleaned up or need to be exposed on this
> class, so I don't think they're necessarily a guide for what we need to do for
> this patch.
> 
>
https://codereview.chromium.org/12090109/diff/43012/content/browser/renderer_...
> File content/browser/renderer_host/media/video_capture_controller.cc (right):
> 
>
https://codereview.chromium.org/12090109/diff/43012/content/browser/renderer_...
> content/browser/renderer_host/media/video_capture_controller.cc:284:
>
///////////////////////////////////////////////////////////////////////////////
> Remove /////...

Awesome, thank you. I believe it can be made to work in the way you describe,
and am implementing the following model: RWHVP gets two new methods, which will
either be #defined to be Windows-only, or else stub impls on all platforms
except Windows:
  CanCopyToVideoFrame() {
    return render_widget_host_->is_accelerated_compositing_active();
  }
  CopyFromCompositingSurfaceToVideoFrame(), as before.

[brettw, 2013-02-08 22:28:39]

If it can be done in a reasonable way without ifdefs, that sounds preferrable.

[ncarter (slow), 2013-02-09 06:17:54]

PTAL.

https://codereview.chromium.org/12090109/diff/39088/content/browser/renderer_...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/12090109/diff/39088/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_controller.cc:448: default:
On 2013/02/07 18:25:03, scherkus wrote:
> you can still replace this default w/ all other values of
> media::VideoFrame::Format (this is more about forcing people who change the
enum
> to re-inspect this code)

Done.

[wjia(left Chromium), 2013-02-10 06:55:06]

lgtm with nit.

https://codereview.chromium.org/12090109/diff/32001/content/browser/renderer_...
File content/browser/renderer_host/media/video_capture_controller.h (right):

https://codereview.chromium.org/12090109/diff/32001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_controller.h:69: base::Time
timestamp) OVERRIDE;
nit: For function declarations and definitions, put each argument on a separate
line
unless the whole declaration fits on one line
(http://dev.chromium.org/developers/coding-style)

[ncarter (slow), 2013-02-11 02:12:42]

https://codereview.chromium.org/12090109/diff/32001/content/browser/renderer_...
File content/browser/renderer_host/media/video_capture_controller.h (right):

https://codereview.chromium.org/12090109/diff/32001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_controller.h:69: base::Time
timestamp) OVERRIDE;
On 2013/02/10 06:55:06, wjia wrote:
> nit: For function declarations and definitions, put each argument on a
separate
> line
> unless the whole declaration fits on one line
> (http://dev.chromium.org/developers/coding-style)

Done.

[ncarter (slow), 2013-02-11 19:43:46]

Waiting on brettw.

[brettw, 2013-02-11 20:50:51]

LGTM. This is only for high-level content integration; I didn't look at the
details.

[commit-bot: I haz the power, 2013-02-11 20:53:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nick@chromium.org/12090109/34057

[commit-bot: I haz the power, 2013-02-11 23:06:29]

Change committed as 181785