Hook up EncodedVideoSource on the browser side

Hook up EncodedVideoSource on the browser side.

This change hooks up the browser side of the EncodedVideoSource interface.

* Add headers for EncodedVideoSource class and IPC messages
* Add headers and implementation fo EncodedBitstreamBuffer (for EVS)
* Implement renderer <-> browser EncodedVideoSource IPC receiving, and
  VideoCaptureHost/VideoCaptureController/VideoCaptureManager plumbing.
* Update unittests to cover encoded output functionality of
  VideoCaptureController.

BUG=chromium:221441
TEST=local build on desktop Linux, unittests

[sheu, 2013-06-11 23:50:34]

Hook up EncodedVideoSource on the browser side.

This change hooks up the browser side of the EncodedVideoSource interface.

* Add headers for EncodedVideoSource class and IPC messages
* Add headers and implementation fo EncodedBitstreamBuffer (for EVS)
* Implement renderer <-> browser EncodedVideoSource IPC receiving, and
  VideoCaptureHost/VideoCaptureController/VideoCaptureManager plumbing.
* Update unittests to cover encoded output functionality of
  VideoCaptureController.

BUG=chromium:221441
TEST=local build on desktop Linux, unittests

[sheu, 2013-06-12 08:04:21]

Ready.  fischman@, hshi@: PTAL

We are aware that the IPC definitions and some others don't match up with what
hshi@ has.  I've talked with him and we will be normalizing them shortly.

[sheu, 2013-06-12 08:06:21]

https://chromiumcodereview.appspot.com/15906019/diff/16001/content/browser/re...
File content/browser/renderer_host/media/video_capture_controller.h (right):

https://chromiumcodereview.appspot.com/15906019/diff/16001/content/browser/re...
content/browser/renderer_host/media/video_capture_controller.h:176:
media::VideoEncodingParameters encoded_frame_info_;
(see below)

https://chromiumcodereview.appspot.com/15906019/diff/16001/content/browser/re...
content/browser/renderer_host/media/video_capture_controller.h:185: bool
encoded_frame_info_available_;
This is bad and I feel bad.  This and its implications are probably the most
hack-ish part of this CL.

https://chromiumcodereview.appspot.com/15906019/diff/16001/content/browser/re...
File content/browser/renderer_host/media/video_capture_host.cc (right):

https://chromiumcodereview.appspot.com/15906019/diff/16001/content/browser/re...
content/browser/renderer_host/media/video_capture_host.cc:22: encoded(false) {}
Second most hack-ish part of this CL, but I don't feel nearly as bad about it.

[hshi1, 2013-06-12 18:30:45]

https://codereview.chromium.org/15906019/diff/16001/content/common/media/enco...
File content/common/media/encoded_video_source_messages.h (right):

https://codereview.chromium.org/15906019/diff/16001/content/common/media/enco...
content/common/media/encoded_video_source_messages.h:100: size_t /* buffer_size
*/)
What does |buffer_size| represent? I guess it is the size of a single buffer? So
we assume that all buffers in std::vector<base::SharedMemoryHandle> buffers have
the same size?

[hshi1, 2013-06-12 18:34:47]

https://codereview.chromium.org/15906019/diff/16001/media/video/encoded_video...
File media/video/encoded_video_source.h (right):

https://codereview.chromium.org/15906019/diff/16001/media/video/encoded_video...
media/video/encoded_video_source.h:6: #define
MEDIA_VIDEO_ENCODED_VIDEO_SOURCE_H_
I believe you don't really need this header file to be able to compile your CL?

In any case, please review this header in the PS#10 of
https://codereview.chromium.org/16320005/ -- as suggested by Ami, I have
significantly simplified this file by removing EVB and changing EVB::Client to
an inner class of EVS.

[sheu, 2013-06-12 19:51:18]

https://chromiumcodereview.appspot.com/15906019/diff/16001/content/common/med...
File content/common/media/encoded_video_source_messages.h (right):

https://chromiumcodereview.appspot.com/15906019/diff/16001/content/common/med...
content/common/media/encoded_video_source_messages.h:100: size_t /* buffer_size
*/)
On 2013/06/12 18:30:45, hshi1 wrote:
> What does |buffer_size| represent? I guess it is the size of a single buffer?
So
> we assume that all buffers in std::vector<base::SharedMemoryHandle> buffers
have
> the same size?

Right.  The assumption is that they all share the same size.

https://chromiumcodereview.appspot.com/15906019/diff/16001/media/video/encode...
File media/video/encoded_video_source.h (right):

https://chromiumcodereview.appspot.com/15906019/diff/16001/media/video/encode...
media/video/encoded_video_source.h:6: #define
MEDIA_VIDEO_ENCODED_VIDEO_SOURCE_H_
On 2013/06/12 18:34:47, hshi1 wrote:
> I believe you don't really need this header file to be able to compile your
CL?
> 
> In any case, please review this header in the PS#10 of
> https://codereview.chromium.org/16320005/ -- as suggested by Ami, I have
> significantly simplified this file by removing EVB and changing EVB::Client to
> an inner class of EVS.

It's in here since it's included by some other header file, but I don't use any
of the interfaces directly.  It does appear that the inclusion was extraneous,
so I've removed it and this file.

[hshi1, 2013-06-12 20:16:32]

https://codereview.chromium.org/15906019/diff/16001/content/common/media/enco...
File content/common/media/encoded_video_source_messages.h (right):

https://codereview.chromium.org/15906019/diff/16001/content/common/media/enco...
content/common/media/encoded_video_source_messages.h:87:
media::VideoCaptureSessionId /* session_id */,
You're using session_id instead of device_id here, is the reason so that caps
can be reported prior to starting capture?

I realized that it would be difficult for VCMessageFilter to locate the delegate
(VCImpl) object associated with the session_id, because the session_id <->
VCImpl mapping is held by the VCImplManager. In order to do this I'll have to
add some plumbing for VCMessageFilter to get to the VCImplManager that owns it,
and another method to look up the VCImpl. Furthermore the manager returns a
VCImpl pointer, this would break the VCMessageFilter::Delegate abstraction.

[sheu, 2013-06-12 20:58:07]

https://codereview.chromium.org/15906019/diff/16001/content/common/media/enco...
File content/common/media/encoded_video_source_messages.h (right):

https://codereview.chromium.org/15906019/diff/16001/content/common/media/enco...
content/common/media/encoded_video_source_messages.h:87:
media::VideoCaptureSessionId /* session_id */,
On 2013/06/12 20:16:32, hshi1 wrote:
> You're using session_id instead of device_id here, is the reason so that caps
> can be reported prior to starting capture?
> 
> I realized that it would be difficult for VCMessageFilter to locate the
delegate
> (VCImpl) object associated with the session_id, because the session_id <->
> VCImpl mapping is held by the VCImplManager. In order to do this I'll have to
> add some plumbing for VCMessageFilter to get to the VCImplManager that owns
it,
> and another method to look up the VCImpl. Furthermore the manager returns a
> VCImpl pointer, this would break the VCMessageFilter::Delegate abstraction.

I suppose we could respec the IPC to say:

GetCapability: device_id and session_id
CapabilityAvailable: returns device_id

Since the device_ids are assigned on the client side, we'll just have the
CapabilityAvailable return from the browser with the device_id it was given in
GetCapability, without actually (yet) creating the VideoCaptureController yet on
the browser side.

Does this sound good?

[hshi1, 2013-06-12 20:59:00]

On 2013/06/12 20:58:07, sheu wrote:
>
https://codereview.chromium.org/15906019/diff/16001/content/common/media/enco...
> File content/common/media/encoded_video_source_messages.h (right):
> 
>
https://codereview.chromium.org/15906019/diff/16001/content/common/media/enco...
> content/common/media/encoded_video_source_messages.h:87:
> media::VideoCaptureSessionId /* session_id */,
> On 2013/06/12 20:16:32, hshi1 wrote:
> > You're using session_id instead of device_id here, is the reason so that
caps
> > can be reported prior to starting capture?
> > 
> > I realized that it would be difficult for VCMessageFilter to locate the
> delegate
> > (VCImpl) object associated with the session_id, because the session_id <->
> > VCImpl mapping is held by the VCImplManager. In order to do this I'll have
to
> > add some plumbing for VCMessageFilter to get to the VCImplManager that owns
> it,
> > and another method to look up the VCImpl. Furthermore the manager returns a
> > VCImpl pointer, this would break the VCMessageFilter::Delegate abstraction.
> 
> I suppose we could respec the IPC to say:
> 
> GetCapability: device_id and session_id
> CapabilityAvailable: returns device_id
> 
> Since the device_ids are assigned on the client side, we'll just have the
> CapabilityAvailable return from the browser with the device_id it was given in
> GetCapability, without actually (yet) creating the VideoCaptureController yet
on
> the browser side.
> 
> Does this sound good?

SGTM

[sheu, 2013-06-12 23:00:25]

9a6cfea4 Update due to hshi@

[sheu, 2013-06-12 23:00:58]

9a6cfea4 Update due to hshi@

[sheu, 2013-06-12 23:20:02]

Sorry for the spam; I hit the dreaded "Base files missing" bug.  Updated; PTAL.

For reference: I fixed it by deleting the error-ed patchsets, uploading a new
temporary patchset that has none of the files or changes of the running
patchsets, then uploading a further patchset on top of that that includes the
changes I want.  I then deleted the temporary patchset, and here I am.

[hshi1, 2013-06-13 00:28:34]

https://codereview.chromium.org/15906019/diff/33002/content/common/media/enco...
File content/common/media/encoded_video_capture_messages.h (right):

https://codereview.chromium.org/15906019/diff/33002/content/common/media/enco...
content/common/media/encoded_video_capture_messages.h:33:
IPC_STRUCT_TRAITS_MEMBER(frames_per_second)
Please move frames_per_second to the runtime_params (result of Ami's review in
the renderer-side CL). I don't expect the HW encoder to support dynamic
frame-rate changes but you can always ignore it.

https://codereview.chromium.org/15906019/diff/33002/content/common/media/enco...
content/common/media/encoded_video_capture_messages.h:125: size_t /* size */,
Per Ami: size_t is verboten in IPC. But I wonder if "int" suffices.

[sheu, 2013-06-13 06:08:18]

Updated to match your struct and IPC changes.  PTAL

https://chromiumcodereview.appspot.com/15906019/diff/33002/content/common/med...
File content/common/media/encoded_video_capture_messages.h (right):

https://chromiumcodereview.appspot.com/15906019/diff/33002/content/common/med...
content/common/media/encoded_video_capture_messages.h:33:
IPC_STRUCT_TRAITS_MEMBER(frames_per_second)
On 2013/06/13 00:28:35, hshi1 wrote:
> Please move frames_per_second to the runtime_params (result of Ami's review in
> the renderer-side CL). I don't expect the HW encoder to support dynamic
> frame-rate changes but you can always ignore it.

Done.

https://chromiumcodereview.appspot.com/15906019/diff/33002/content/common/med...
content/common/media/encoded_video_capture_messages.h:125: size_t /* size */,
On 2013/06/13 00:28:35, hshi1 wrote:
> Per Ami: size_t is verboten in IPC. But I wonder if "int" suffices.

I'm going to specify it as a uint32.

[hshi1, 2013-06-13 18:35:24]

https://codereview.chromium.org/15906019/diff/85001/content/common/media/enco...
File content/common/media/encoded_video_capture_messages.h (right):

https://codereview.chromium.org/15906019/diff/85001/content/common/media/enco...
content/common/media/encoded_video_capture_messages.h:100: size_t /* buffer_size
*/)
Can you make this a uint32 too?

[sheu, 2013-06-13 22:34:48]

https://chromiumcodereview.appspot.com/15906019/diff/85001/content/common/med...
File content/common/media/encoded_video_capture_messages.h (right):

https://chromiumcodereview.appspot.com/15906019/diff/85001/content/common/med...
content/common/media/encoded_video_capture_messages.h:100: size_t /* buffer_size
*/)
On 2013/06/13 18:35:24, hshi1 wrote:
> Can you make this a uint32 too?

Done, not uploaded yet, in case there's more.

[Ami GONE FROM CHROMIUM, 2013-06-18 18:35:55]

You probably want ncarter@ to sanity-check the browser-side capturer changes.
Not cc'ing him yet b/c I think the CL is way harder to CR until you take a
couple of my suggestions below: don't include files from other CLs in this CR,
and add do-nothing impls to the base class instead of the fine sprinkling of
copy/pasta you have now.

https://chromiumcodereview.appspot.com/15906019/diff/85001/content/browser/re...
File content/browser/renderer_host/media/screen_capture_device.h (right):

https://chromiumcodereview.appspot.com/15906019/diff/85001/content/browser/re...
content/browser/renderer_host/media/screen_capture_device.h:1: // Copyright (c)
2013 The Chromium Authors. All rights reserved.
Please don't include code from different CLs in the same rietveld issue; it
makes review unnecessarily more difficult.

git cl upload can be told what to diff (it takes the same flags as git diff,
among other things) so if you need to include code from another CL in your
working branch then you should be explicit about the diffs you upload for
review.

https://chromiumcodereview.appspot.com/15906019/diff/85001/content/browser/re...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://chromiumcodereview.appspot.com/15906019/diff/85001/content/browser/re...
content/browser/renderer_host/media/video_capture_controller.cc:364: base::Bind(
fwiw here and elsewhere you can move and the preceding line to the PostTask(
line without affecting the indentation of the following lines.

https://chromiumcodereview.appspot.com/15906019/diff/85001/content/browser/re...
content/browser/renderer_host/media/video_capture_controller.cc:508:
buffer->shared_memory_handle()) >= 0) {
indent

https://chromiumcodereview.appspot.com/15906019/diff/85001/content/browser/re...
content/browser/renderer_host/media/video_capture_controller.cc:521: // it to a
frame from the buffer pool.
Is it important to support this "legacy" case for this brand-new codepath?
(can you instead CHECK the Recognize?)

https://chromiumcodereview.appspot.com/15906019/diff/85001/content/browser/re...
content/browser/renderer_host/media/video_capture_controller.cc:772:
DCHECK(!frame_info_available_);
did you mean to DCHECK(encoded_frame_info_available_)?

https://chromiumcodereview.appspot.com/15906019/diff/85001/content/browser/re...
File
content/browser/renderer_host/media/video_capture_controller_event_handler.h
(right):

https://chromiumcodereview.appspot.com/15906019/diff/85001/content/browser/re...
content/browser/renderer_host/media/video_capture_controller_event_handler.h:47:
// The (encoded) frame resolution the VideoCaptureDevice captures video in.
this is more than just resolution.

https://chromiumcodereview.appspot.com/15906019/diff/85001/content/browser/re...
File content/browser/renderer_host/media/video_capture_host.cc (right):

https://chromiumcodereview.appspot.com/15906019/diff/85001/content/browser/re...
content/browser/renderer_host/media/video_capture_host.cc:321: // Not used.
NOTREACHED?

https://chromiumcodereview.appspot.com/15906019/diff/85001/content/browser/re...
File content/browser/renderer_host/media/video_capture_host.h (right):

https://chromiumcodereview.appspot.com/15906019/diff/85001/content/browser/re...
content/browser/renderer_host/media/video_capture_host.h:71: //
VideoCaptureControllerEventHandler implementation.
s/rE/r::E/

https://chromiumcodereview.appspot.com/15906019/diff/85001/content/browser/re...
File
content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc
(right):

https://chromiumcodereview.appspot.com/15906019/diff/85001/content/browser/re...
content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc:403:
}
NOTREACHED?
What about actually testing this codepath?

https://chromiumcodereview.appspot.com/15906019/diff/85001/content/common/con...
File content/common/content_message_generator.h (right):

https://chromiumcodereview.appspot.com/15906019/diff/85001/content/common/con...
content/common/content_message_generator.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
Ignoring file (b/c I think it's part of a different CL).

https://chromiumcodereview.appspot.com/15906019/diff/85001/content/common/med...
File content/common/media/encoded_video_capture_messages.h (right):

https://chromiumcodereview.appspot.com/15906019/diff/85001/content/common/med...
content/common/media/encoded_video_capture_messages.h:2: // Use of this source
code is governed by a BSD-style license that can be
Ignoring file (b/c I think it's part of a different CL).

https://chromiumcodereview.appspot.com/15906019/diff/85001/content/content_co...
File content/content_common.gypi (right):

https://chromiumcodereview.appspot.com/15906019/diff/85001/content/content_co...
content/content_common.gypi:1: # Copyright (c) 2012 The Chromium Authors. All
rights reserved.
Ignoring file (b/c I think it's part of a different CL).

https://chromiumcodereview.appspot.com/15906019/diff/85001/ipc/ipc_message_st...
File ipc/ipc_message_start.h (right):

https://chromiumcodereview.appspot.com/15906019/diff/85001/ipc/ipc_message_st...
ipc/ipc_message_start.h:1: // Copyright 2012 The Chromium Authors. All rights
reserved.
Ignoring file (b/c I think it's part of a different CL).

https://chromiumcodereview.appspot.com/15906019/diff/85001/media/base/encoded...
File media/base/encoded_bitstream_buffer.cc (right):

https://chromiumcodereview.appspot.com/15906019/diff/85001/media/base/encoded...
media/base/encoded_bitstream_buffer.cc:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
Ignoring file (b/c I think it's part of a different CL).

https://chromiumcodereview.appspot.com/15906019/diff/85001/media/base/encoded...
File media/base/encoded_bitstream_buffer.h (right):

https://chromiumcodereview.appspot.com/15906019/diff/85001/media/base/encoded...
media/base/encoded_bitstream_buffer.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
Ignoring file (b/c I think it's part of a different CL).

https://chromiumcodereview.appspot.com/15906019/diff/85001/media/media.gyp
File media/media.gyp (right):

https://chromiumcodereview.appspot.com/15906019/diff/85001/media/media.gyp#ne...
media/media.gyp:2: # Use of this source code is governed by a BSD-style license
that can be
Ignoring file (b/c I think it's part of a different CL).

https://chromiumcodereview.appspot.com/15906019/diff/85001/media/video/captur...
File media/video/capture/fake_video_capture_device.cc (right):

https://chromiumcodereview.appspot.com/15906019/diff/85001/media/video/captur...
media/video/capture/fake_video_capture_device.cc:20: namespace {
not adding value

https://chromiumcodereview.appspot.com/15906019/diff/85001/media/video/captur...
media/video/capture/fake_video_capture_device.cc:189: memset(fake_frame_.get(),
0, frame_size_);
only used in the not-encoding case

https://chromiumcodereview.appspot.com/15906019/diff/85001/media/video/captur...
File media/video/capture/fake_video_capture_device.h (right):

https://chromiumcodereview.appspot.com/15906019/diff/85001/media/video/captur...
media/video/capture/fake_video_capture_device.h:22: bool encoded_capture);
indent is off here and elsewhere in the CL.  Please make a pass.

https://chromiumcodereview.appspot.com/15906019/diff/85001/media/video/captur...
media/video/capture/fake_video_capture_device.h:28: static void
GetDeviceNames(Names* device_names, bool encoded_capture);
Why pass this in if not to inspect it in Create() (and thus not have to pass it
to Create)?

https://chromiumcodereview.appspot.com/15906019/diff/85001/media/video/captur...
media/video/capture/fake_video_capture_device.h:64:
scoped_refptr<media::EncodedBitstreamBuffer> fake_encoded_bitstream_buffer_;
unused

https://chromiumcodereview.appspot.com/15906019/diff/85001/media/video/captur...
File media/video/capture/video_capture_device.h (right):

https://chromiumcodereview.appspot.com/15906019/diff/85001/media/video/captur...
media/video/capture/video_capture_device.h:67: // ReserveOutputVideoFrame().
TODO to drop this & point to EVS->VEA crbug?

https://chromiumcodereview.appspot.com/15906019/diff/85001/media/video/captur...
media/video/capture/video_capture_device.h:144: const
media::RuntimeVideoEncodingParameters& params) = 0;
I think the majority of files & lines in this CL will evaporate if you give
these methods default impls in v_c_d.cc.
Default for TCEB() should probably NOTIMPLEMENTED not just silently no-op.

https://chromiumcodereview.appspot.com/15906019/diff/85001/media/video/video_...
File media/video/video_encode_types.h (right):

https://chromiumcodereview.appspot.com/15906019/diff/85001/media/video/video_...
media/video/video_encode_types.h:1: // Copyright (c) 2013 The Chromium Authors.
All rights reserved.
Ignoring file (b/c I think it's part of a different CL).

[piman, 2013-06-19 18:33:51]

https://codereview.chromium.org/15906019/diff/85001/content/browser/renderer_...
File content/browser/renderer_host/media/video_capture_host.cc (right):

https://codereview.chromium.org/15906019/diff/85001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_host.cc:321: // Not used.
On 2013/06/18 18:35:55, Ami Fischman wrote:
> NOTREACHED?

Not NOTREACHED since it can be reached by a malicious renderer.

https://codereview.chromium.org/15906019/diff/85001/content/browser/renderer_...
content/browser/renderer_host/media/video_capture_host.cc:391:
DCHECK(entries_.find(controller_id) == entries_.end());
This data comes from the untrusted renderer.
This can't just be a DCHECK, but must be handled safely.

[hshi1, 2013-06-20 22:39:16]

https://chromiumcodereview.appspot.com/15906019/diff/85001/media/media.gyp
File media/media.gyp (right):

https://chromiumcodereview.appspot.com/15906019/diff/85001/media/media.gyp#ne...
media/media.gyp:258: 'baise/encoded_bitstream_buffer.h',
typo: "baise" -> "base"

[hshi1, 2013-06-20 22:51:02]

https://chromiumcodereview.appspot.com/15906019/diff/85001/content/browser/re...
File content/browser/renderer_host/media/video_capture_host.cc (right):

https://chromiumcodereview.appspot.com/15906019/diff/85001/content/browser/re...
content/browser/renderer_host/media/video_capture_host.cc:158:
controller_id.device_id));
How can browser send EncodedVideoCaptureHostMsg to renderer?

I believe in this case you need to send EncodedVideoCaptureMsg_BitstreamClosed.

[hshi1, 2013-06-20 23:23:26]

https://chromiumcodereview.appspot.com/15906019/diff/85001/content/common/med...
File content/common/media/encoded_video_capture_messages.h (right):

https://chromiumcodereview.appspot.com/15906019/diff/85001/content/common/med...
content/common/media/encoded_video_capture_messages.h:56:
IPC_MESSAGE_CONTROL3(EncodedVideoCaptureHostMsg_CreateBitstream,
Please make a pass to replace "create" to "open" and "destroy" to "close" in the
message names, so as to match that in
https://codereview.chromium.org/16320005/diff/415002/content/common/media/enc...

[sheu, 2013-08-22 22:40:31]

Posting comments, just to clear out the "draft" queue.  I will be abandoning
this CL shortly.

https://chromiumcodereview.appspot.com/15906019/diff/85001/content/browser/re...
File content/browser/renderer_host/media/video_capture_host.cc (right):

https://chromiumcodereview.appspot.com/15906019/diff/85001/content/browser/re...
content/browser/renderer_host/media/video_capture_host.cc:158:
controller_id.device_id));
On 2013/06/20 22:51:02, hshi1 wrote:
> How can browser send EncodedVideoCaptureHostMsg to renderer?
> 
> I believe in this case you need to send
EncodedVideoCaptureMsg_BitstreamClosed.

Done.

https://chromiumcodereview.appspot.com/15906019/diff/85001/content/browser/re...
content/browser/renderer_host/media/video_capture_host.cc:391:
DCHECK(entries_.find(controller_id) == entries_.end());
On 2013/06/19 18:33:51, piman wrote:
> This data comes from the untrusted renderer.
> This can't just be a DCHECK, but must be handled safely.

Done.  Fixed also for OnStartCapture.

https://chromiumcodereview.appspot.com/15906019/diff/85001/content/browser/re...
File content/browser/renderer_host/media/video_capture_host.h (right):

https://chromiumcodereview.appspot.com/15906019/diff/85001/content/browser/re...
content/browser/renderer_host/media/video_capture_host.h:71: //
VideoCaptureControllerEventHandler implementation.
On 2013/06/18 18:35:55, Ami Fischman wrote:
> s/rE/r::E/

It's actually not a nested class.  <dramatic prairie dog />

https://chromiumcodereview.appspot.com/15906019/diff/85001/content/browser/re...
File
content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc
(right):

https://chromiumcodereview.appspot.com/15906019/diff/85001/content/browser/re...
content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc:403:
}
On 2013/06/18 18:35:55, Ami Fischman wrote:
> NOTREACHED?
> What about actually testing this codepath?

NOTREACHED() them.

Testing this codepath is unnecessary until encoded tab capture is implemented.

https://chromiumcodereview.appspot.com/15906019/diff/85001/content/common/med...
File content/common/media/encoded_video_capture_messages.h (right):

https://chromiumcodereview.appspot.com/15906019/diff/85001/content/common/med...
content/common/media/encoded_video_capture_messages.h:56:
IPC_MESSAGE_CONTROL3(EncodedVideoCaptureHostMsg_CreateBitstream,
On 2013/06/20 23:23:26, hshi1 wrote:
> Please make a pass to replace "create" to "open" and "destroy" to "close" in
the
> message names, so as to match that in
>
https://codereview.chromium.org/16320005/diff/415002/content/common/media/enc...

Done, also for EncodedVideoCaptureMsg_BitstreamOpened.

https://chromiumcodereview.appspot.com/15906019/diff/85001/media/media.gyp
File media/media.gyp (right):

https://chromiumcodereview.appspot.com/15906019/diff/85001/media/media.gyp#ne...
media/media.gyp:258: 'baise/encoded_bitstream_buffer.h',
On 2013/06/20 22:39:17, hshi1 wrote:
> typo: "baise" -> "base"

Done.

Not sure how this even built?

https://chromiumcodereview.appspot.com/15906019/diff/85001/media/video/captur...
File media/video/capture/fake_video_capture_device.cc (right):

https://chromiumcodereview.appspot.com/15906019/diff/85001/media/video/captur...
media/video/capture/fake_video_capture_device.cc:20: namespace {
On 2013/06/18 18:35:55, Ami Fischman wrote:
> not adding value

Fine :-P

https://chromiumcodereview.appspot.com/15906019/diff/85001/media/video/captur...
media/video/capture/fake_video_capture_device.cc:189: memset(fake_frame_.get(),
0, frame_size_);
On 2013/06/18 18:35:55, Ami Fischman wrote:
> only used in the not-encoding case

Also used for encoding.  Changed up a bit here.

https://chromiumcodereview.appspot.com/15906019/diff/85001/media/video/captur...
File media/video/capture/fake_video_capture_device.h (right):

https://chromiumcodereview.appspot.com/15906019/diff/85001/media/video/captur...
media/video/capture/fake_video_capture_device.h:22: bool encoded_capture);
On 2013/06/18 18:35:55, Ami Fischman wrote:
> indent is off here and elsewhere in the CL.  Please make a pass.

Not sure what you mean here?

https://chromiumcodereview.appspot.com/15906019/diff/85001/media/video/captur...
media/video/capture/fake_video_capture_device.h:28: static void
GetDeviceNames(Names* device_names, bool encoded_capture);
On 2013/06/18 18:35:55, Ami Fischman wrote:
> Why pass this in if not to inspect it in Create() (and thus not have to pass
it
> to Create)?

Not quite following here.  GetDeviceNames() constructs a different name if it's
encoding, and Create() creates an encoding fake device if it's encoding.

https://chromiumcodereview.appspot.com/15906019/diff/85001/media/video/captur...
media/video/capture/fake_video_capture_device.h:64:
scoped_refptr<media::EncodedBitstreamBuffer> fake_encoded_bitstream_buffer_;
On 2013/06/18 18:35:55, Ami Fischman wrote:
> unused

Done.

https://chromiumcodereview.appspot.com/15906019/diff/85001/media/video/captur...
File media/video/capture/video_capture_device.h (right):

https://chromiumcodereview.appspot.com/15906019/diff/85001/media/video/captur...
media/video/capture/video_capture_device.h:67: // ReserveOutputVideoFrame().
On 2013/06/18 18:35:55, Ami Fischman wrote:
> TODO to drop this & point to EVS->VEA crbug? 

Done.

https://chromiumcodereview.appspot.com/15906019/diff/85001/media/video/captur...
media/video/capture/video_capture_device.h:144: const
media::RuntimeVideoEncodingParameters& params) = 0;
On 2013/06/18 18:35:55, Ami Fischman wrote:
> I think the majority of files & lines in this CL will evaporate if you give
> these methods default impls in v_c_d.cc.
> Default for TCEB() should probably NOTIMPLEMENTED not just silently no-op.

I thought we tried to keep interfaces pure virtual?

It would certainly simplify things.  I'll take your suggestion as
yes-we-can-do-this and blame you accordingly if it turns out not to plan :-)