Define EncodedVideoSource and RtcCapturedEncodingVideoCapturer.

Define EncodedVideoSource and RtcCapturedEncodingVideoCapturer.

Defined the interfaces for EVS (EncodedVideoSource) and related IPC messages
between renderer and browser processes for the purpose of encoded screen capture.

Added RtcCapturedEncodingVideoCapturerFactory which implements the
libjingle::WebRtcVideoEncoderFactory class, and the corresponding
RtcCapturedEncodingVideoCapturer which implements the webrtc::VideoEncoder.

Modify MediaStreamDependencyFactory and inject RtcCapturedEncodingVideoCaptureFactory
in PeerConnectionFactory. For the time being, the factory will only be instantiated
for Chrome OS and ARM.

BUG=238515
TEST=trybot

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

[hshi1, 2013-06-07 17:55:37]

Re-uploaded a fresh issue for review because Ami is having rietveld trouble with
the previous issue (too many deleted patch sets seems to cause confusion).

PTAL - fischman@ (OWNER)
+sheu@, zel@ as FYI

[Ami GONE FROM CHROMIUM, 2013-06-08 00:18:01]

Boom

https://codereview.chromium.org/16320005/diff/15001/content/common/content_me...
File content/common/content_message_generator.h (right):

https://codereview.chromium.org/16320005/diff/15001/content/common/content_me...
content/common/content_message_generator.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
Please update CL description to reflect the merge of your split CLs, and point
to URLs for other in-flight changes this depends on.

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

https://codereview.chromium.org/16320005/diff/15001/content/common/media/enco...
content/common/media/encoded_video_source_messages.h:46: // Message from
Renderer to Browser process to query the encoding capability
Please mimic gpu_messages.h where all the messages that go from process A to
process B go together, with a leading comment that says "Messages from A to B"
so you don't have to repeat the fact on each message.

https://codereview.chromium.org/16320005/diff/15001/content/common/media/enco...
content/common/media/encoded_video_source_messages.h:71:
IPC_MESSAGE_CONTROL2(EncodedVideoSourceHostMsg_DestroyBitstream,
"Destroy" or "Close"?  Please ensure that the IPC messages match the EVS
interface (there are other discrepancies) and also make a pass on the commentary
in this file.

https://codereview.chromium.org/16320005/diff/15001/content/common/media/enco...
content/common/media/encoded_video_source_messages.h:135: size_t /* size */,
"size_t" is verboten in IPC - use an explicitly-sized type.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/ca...
File content/renderer/media/captured_encoded_video_source_impl.cc (right):

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/ca...
content/renderer/media/captured_encoded_video_source_impl.cc:24:
source_->CloseBitstream(this);
This implies that a bitstream can close before the source removes it, but the
source has a ref on the bitstream so how can this happen?

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/ca...
File content/renderer/media/captured_encoded_video_source_impl.h (right):

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/ca...
content/renderer/media/captured_encoded_video_source_impl.h:4: #ifndef
CONTENT_RENDERER_MEDIA_CAPTURED_ENCODED_VIDEO_SOURCE_IMPL_H_
I _think_ you can actually drop this file (and it's .cc file) entirely, if you
switch from using control IPC messages to routed messages.
Routed messages are a generic answer to the need this file fills (AFAICT). 
If this is providing some service not served by routed messages, please let me
know.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/ca...
content/renderer/media/captured_encoded_video_source_impl.h:29: // device. It
lives in similar setting where it is proxying communication
"similar" to what?

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/ca...
content/renderer/media/captured_encoded_video_source_impl.h:31: class
CONTENT_EXPORT CapturedEncodedVideoBitstreamImpl :
Seems like an impl detail of CEVSI below; make it an inner class?

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/ca...
content/renderer/media/captured_encoded_video_source_impl.h:34:
CapturedEncodedVideoBitstreamImpl(
doco lifecycle of args

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/ca...
content/renderer/media/captured_encoded_video_source_impl.h:112: // Services
required from deriving class.
You need to say what these methods are supposed to do if you expect deriving
classes to provide them!

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/ca...
content/renderer/media/captured_encoded_video_source_impl.h:115: virtual
scoped_refptr<base::MessageLoopProxy> app_loop_proxy() = 0;
What is "app" in this context?

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/ca...
content/renderer/media/captured_encoded_video_source_impl.h:116: virtual
scoped_refptr<base::MessageLoopProxy> io_loop_proxy() = 0;
This is always
ChildProcess::current()->io_message_loop_proxy()
unclear why it needs to be cached and why clients wouldn't look this up
themselves.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/en...
File content/renderer/media/encoding_video_capture_message_filter.h (right):

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/en...
content/renderer/media/encoding_video_capture_message_filter.h:12: class
CONTENT_EXPORT EncodingVideoCaptureMessageFilter {
doco: what's the purpose of this interface?
This isn't just a request for better coding practices; I actually can't tell why
this needs to exist; no methods seem to take this as a parameter and the only
use is in the only impl of the type.  Why not just delete this file and its .cc
and avoid a layer of indirection?

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
File content/renderer/media/rtc_encoding_video_capturer.cc (right):

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:14: public
media::EncodedVideoBitstream::Client,
This implies the name of the class is poor - it's a Bitstream's Client, not a
Bitstream itself.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:15: public
base::RefCountedThreadSafe<RtcEncodingVideoCapturer::Bitstream> {
why refcount?

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:18:
media::EncodedVideoSource* encoded_video_source);
doco lifetime

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:20: // Function to
configure runtime encoding parameters.
Isn't this really just the EVB interface?

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:21: void Configure(const
media::RuntimeVideoEncodingParameters& params);
Try?

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:39: void
set_round_trip_time(int rtt_in_ms);
use TimeDelta for the param

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:62:
DISALLOW_COPY_AND_ASSIGN(Bitstream);
nit: extra \n before this

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:70:
encoded_video_source_(encoded_video_source) {
initialize all other members to known-reasonable sentinels (0, NULL, etc)

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:71: if
(encoded_video_source_)
how can this fail?

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:76: if
(encoded_video_source_ && bitstream_) {
how can this fail?

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:78: bitstream_ = NULL;
why bother?

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:84: if (bitstream_)
how can this fail?

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:103: callback_ = callback;
DCHECK(!callback_);
first?

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:114: finished_ = true;
bitstream_ = NULL;
?

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:121: // First buffer
constitutes the origin of the time for this bitstream
Isn't this true regardless of callback_ being set?

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:130: if (Convert(buffer,
image, codecInfo, fragInfo)) {
style: no non-const ref params.  These are output params and so should be passed
by pointer.
But really it seems strange to pass in output params just to use them in the
very next line.
Wouldn't it be simpler/better to move the Encoded() call into Convert() (after
renaming it)?

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:135: if
(encoded_video_source_)
can never be false.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:137: }
This collection of comments reduces l.120-136 into:
if (time_base_.is_null())
  time_base_ = buffer->metadata().timestamp;
ReportEncodedFrame(buffer);
encoded_video_source_->ReturnBitstreamBuffer(bitstream, buffer);

and ReportEncodedFrame is Convert with an early-return if !callback_ and a final
callback_->Encoded() call at its bottom.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:145: bool
RtcEncodingVideoCapturer::Bitstream::Convert(
return value is ignored at only callsite and no legit return false codepaths so
s/bool/void/ I think.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:155: // TODO: remove this
const_cast. Unfortunately webrtc::EncodedImage defines
TODO's should always be attributed like TODO(hshi)
(please make a pass)

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:160: image._size =
image._length;
Do you know what the diff is supposed to be between _size and _length?

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:164: (metadata.timestamp -
time_base_).InMilliseconds(); // 1 KHz
Comment doesn't make sense to me.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:165: image._timeStamp =
image.capture_time_ms_ * 90; // 90kHz
Once we finalize this CL IWBN to have mflodman@ or another webrtc-eng confirm
this is Doing It Right(tm).

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:204: if (!bitstream_) {
How can this fail?
(and if you wanted to handle InitEncode being called twice, you probably
wouldn't want to return OK)

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:210:
params.runtime_params.average_bitrate = codecSettings->maxBitrate;
s/max/start/ ?

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:221: // TODO: request
specific frame type.
TODO(hshi)

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:229:
bitstream_->set_callback(callback);
Are you really not guaranteed that RECC is called _after_ InitEncode?  Given
that they're all synchronous, this paranoia seems unwarranted.
(if indeed not necessary, then a bunch of code simplifies as
Bitstream::set_callback can go into Bitstream's ctor, etc)

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:235: bitstream_ = NULL;
Are you relying on this moving the refcount to 0?
Wouldn't it be better to 
bitstream_->OnBitstreamRemoved();
before 
bitstream_ = NULL;
?

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:240: uint32_t packetLoss,
/* packetLoss */ here & in the header

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:252: // TODO: wire up
runtime rate control.
TODO(hshi)

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
File content/renderer/media/rtc_encoding_video_capturer.h (right):

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.h:11: #include
"third_party/libjingle/source/talk/media/webrtc/webrtcvideoencoderfactory.h"
unused

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.h:12: #include
"third_party/libjingle/source/talk/media/webrtc/webrtcvideoframe.h"
unnecessary in this .h - this file doesn't make any more use of webrtc/cricket
types than the interface being impl'd.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.h:15: using
cricket::WebRtcVideoEncoderFactory;
unused

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.h:34: const
webrtc::I420VideoFrame& inputImage,
ignored params should be like this:
/* inputImage */

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.h:48: int stream_id_;
write-only variable?

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.h:54:
DISALLOW_COPY_AND_ASSIGN(RtcEncodingVideoCapturer);
nit: extra new line before this

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
File content/renderer/media/rtc_encoding_video_capturer_factory.cc (right):

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer_factory.cc:21: delete
it->second;
STLDeleteValues(&encoder_map_);

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer_factory.cc:29: // For now we
only support one instance of encoded video source at a time.
This doesn't seem super-legit, since the caller won't know it's being ignored...
Does this belong at the caller and a DCHECK here?

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer_factory.cc:54:
RtcEncodingVideoCapturer* capturer =
neither |type| nor |i| are used in l.54-58; suspicious!

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer_factory.cc:56: if (capturer)
can never fail

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer_factory.cc:66: for
(EncoderMap::iterator it = encoder_map_.begin(); it != encoder_map_.end();
lol it's a map but you never use it as such.
typedef std::set<VideoEncoder*> EncoderSet;
?

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer_factory.cc:86: return;
l.85-86 unnecessary

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer_factory.cc:97:
webrtc_codec_type = webrtc::kVideoCodecGeneric;
unnecessary given l.91

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
File content/renderer/media/rtc_encoding_video_capturer_factory.h (right):

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer_factory.h:13: #include
"third_party/libjingle/source/talk/media/webrtc/webrtcvie.h"
I suspect neither of l.12-13 is necessary in this .h.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer_factory.h:15: using
cricket::WebRtcVideoEncoderFactory;
style: "using" is verboten in headers.
(here & elsewhere; please make a pass)

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer_factory.h:32: 
remove newline

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer_factory.h:33: virtual const
std::vector<WebRtcVideoEncoderFactory::VideoCodec>& codecs()
WebRtcVideoEncoderFactory:: unnecessary

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer_factory.h:43: 
remove one of these two newlines

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer_factory.h:54: virtual
~RtcEncodingVideoCapturerFactory();
dtor goes at the top of the section

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
File content/renderer/media/video_capture_impl.cc (right):

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
content/renderer/media/video_capture_impl.cc:106: return this;
shouldn't this be conditionalized on encoding_supported_?
Further, I don't see where encoding_supported_ is set to anything but false; is
that supposed to be a flag?

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
content/renderer/media/video_capture_impl.cc:111:
CapturedEncodedVideoSourceImpl::OnCapabilityAvailable(capability);
having to write l.109-139 is usually a code smell of over-inheritance.  Is it
required?  Why?

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
content/renderer/media/video_capture_impl.cc:358:
CapturedEncodedVideoSourceImpl::StartFetchCapabilities();
why "CapturedEncodedVideoSourceImpl::"?

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
File content/renderer/media/video_capture_impl.h (right):

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
content/renderer/media/video_capture_impl.h:90: virtual void Send(IPC::Message*
message) OVERRIDE;
Keep order same as in interface definition.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
File content/renderer/media/video_capture_impl_manager.cc (right):

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
content/renderer/media/video_capture_impl_manager.cc:17:
encoding_capturer_factory_(new RtcEncodingVideoCapturerFactory()) {
flag-protect?

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
content/renderer/media/video_capture_impl_manager.cc:36: if
(encoding_capturer_factory_ && evs)
how can encoding_capturer_factory_ be false?

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
content/renderer/media/video_capture_impl_manager.cc:68: if
(encoding_capturer_factory_ && evs)
ditto encoding_capturer_factory_ is never NULL

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
File content/renderer/media/video_capture_impl_manager.h (right):

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
content/renderer/media/video_capture_impl_manager.h:55:
RtcEncodingVideoCapturerFactory* encoding_capturer_factory() const {
It's almost never a good idea to return a raw pointer to an object whose
lifetime is governed by a scoped_refptr<>.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
File content/renderer/media/video_capture_message_filter.cc (right):

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
content/renderer/media/video_capture_message_filter.cc:107: if
(delegates_.find(device_id) != delegates_.end())
Why not replace the other occurrences of this code in this file with a call to
this method?

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
File content/renderer/media/video_capture_message_filter.h (right):

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
content/renderer/media/video_capture_message_filter.h:27: : public
EncodingVideoCaptureMessageFilter,
This is a logical reversal; it implies that all VCMF's expect to handle EVS's
messages, even though while all EVS's are related to a VC, it's not true that
all VC's are related to an EVS.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
content/renderer/media/video_capture_message_filter.h:83: int32 last_device_id_;
Why are l.79-83 not private?

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
File content/renderer/media/video_capture_message_filter_unittest.cc (right):

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
content/renderer/media/video_capture_message_filter_unittest.cc:53: // TODO:
implement the following methods for encoded video capture.
I think this TODO is really 
// TODO(hshi): add tests for EVS functionality.

Which makes me wonder - how will we know we haven't broken this code once it
lands?

https://codereview.chromium.org/16320005/diff/15001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_platform_video_capture_impl.cc (right):

https://codereview.chromium.org/16320005/diff/15001/content/renderer/pepper/p...
content/renderer/pepper/pepper_platform_video_capture_impl.cc:99:
media::EncodedVideoSource*
Hopefully you can revert this file.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_platform_video_capture_impl.h (right):

https://codereview.chromium.org/16320005/diff/15001/content/renderer/pepper/p...
content/renderer/pepper/pepper_platform_video_capture_impl.h:46: virtual
media::EncodedVideoSource* GetEncodedVideoSource() OVERRIDE;
Hopefully you can revert this file

https://codereview.chromium.org/16320005/diff/15001/media/base/encoded_bitstr...
File media/base/encoded_bitstream_buffer.h (right):

https://codereview.chromium.org/16320005/diff/15001/media/base/encoded_bitstr...
media/base/encoded_bitstream_buffer.h:16: // General encoded video bitstream
buffer.
This sort of comment (which appears elsewhere as well) is not useful.

Things that can be useful to doco in a class-level comment include:
- intended use-cases
- clients
- caller conventions: lifecycle, thread-safety/friendliness/hostility
etc.

https://codereview.chromium.org/16320005/diff/15001/media/base/encoded_bitstr...
media/base/encoded_bitstream_buffer.h:17: class EncodedBitstreamBuffer :
Seems strange to have this time given that media::BitstreamBuffer exists.

Can you extend that with the metadata you need instead?
(might not be able to if you really don't want SHM; see below)

https://codereview.chromium.org/16320005/diff/15001/media/base/encoded_bitstr...
media/base/encoded_bitstream_buffer.h:23: uint8* buffer,
comment (and off-reviewlog convo) says "shared memory" but this param is a
pointer; is there SHM here or not?
if not, you need to doco the ownership/lifecycle of |buffer|; does |this| now
own |buffer| and expected to release it at some point?
I suspect the answer is "no" (since buffer_ is not a scoped_ptr and the dtor
doesn't delete it) but then something must be keeping track of the memory & the
need to free it later.  I'm surprised this doesn't have something like
VideoFrame::no_longer_needed_cb_.

https://codereview.chromium.org/16320005/diff/15001/media/base/encoded_bitstr...
media/base/encoded_bitstream_buffer.h:25: const media::BufferEncodingMetadata&
metadata);
Is there any reason for this to be a separate struct rather than part of EBB?

https://codereview.chromium.org/16320005/diff/15001/media/base/encoded_bitstr...
media/base/encoded_bitstream_buffer.h:32: protected:
private?

https://codereview.chromium.org/16320005/diff/15001/media/base/encoded_bitstr...
media/base/encoded_bitstream_buffer.h:33: // Destructor that deallocates the
buffers.
Comment is a lie given the impl.

https://codereview.chromium.org/16320005/diff/15001/media/base/encoded_bitstr...
media/base/encoded_bitstream_buffer.h:40: size_t size_;
IPC guidelines require this piece of data to be explicitly sized in the IPC so I
think you're better off making it uint64 everywhere.

https://codereview.chromium.org/16320005/diff/15001/media/video/capture/video...
File media/video/capture/video_capture.h (right):

https://codereview.chromium.org/16320005/diff/15001/media/video/capture/video...
media/video/capture/video_capture.h:104: // also stop the streaming of encoded
streams.
This is not enough information.  For example it implies one still calls
VideoCapture::StartCapture() to start getting encoded bitstreambuffers, but it
doesn't say what to pass for a  the |handler| param to StartCapture.  It sort of
implies that raw frames will be returned to the handler while
encodedbitstreambuffers will be returned to the EVS::Client registered with
Initialize.

https://codereview.chromium.org/16320005/diff/15001/media/video/capture/video...
media/video/capture/video_capture.h:105: // All platforms or cameras may have
not encoding support available.
Comment is written as if this returns a bool, not a pointer.

https://codereview.chromium.org/16320005/diff/15001/media/video/capture/video...
media/video/capture/video_capture.h:105: // All platforms or cameras may have
not encoding support available.
If this was impl'd to return NULL (in a new video_capture.cc file) then you
wouldn't need to propagate this as a do-nothing to
pepper_platform_video_capture_impl.cc and so on.

(there's no value to this being a pure abstract class)

https://codereview.chromium.org/16320005/diff/15001/media/video/capture/video...
media/video/capture/video_capture.h:106: virtual EncodedVideoSource*
GetEncodedVideoSource() = 0;
What are the ownership semantics of the returned pointer?

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

https://codereview.chromium.org/16320005/diff/15001/media/video/encoded_video...
media/video/encoded_video_source.h:5: #ifndef
MEDIA_VIDEO_ENCODED_VIDEO_SOURCE_H_
TODO+crbug(m30) for merging this into VEA

https://codereview.chromium.org/16320005/diff/15001/media/video/encoded_video...
media/video/encoded_video_source.h:14: // Class to represent encoded video
bitstream.
o rly?

https://codereview.chromium.org/16320005/diff/15001/media/video/encoded_video...
media/video/encoded_video_source.h:15: class MEDIA_EXPORT EncodedVideoBitstream
:
Is it not the case that bitstreams are owned by sources?  As such I'd expect to
see this as an inner class of EVS since these don't make sense by themselves.

https://codereview.chromium.org/16320005/diff/15001/media/video/encoded_video...
media/video/encoded_video_source.h:20: // After OnStreaming callback stream
shall be considered to be streaming.
Oh, shall it??

Can you make a pass and cull useless comments and add useful ones where mystery
might exist?

https://codereview.chromium.org/16320005/diff/15001/media/video/encoded_video...
media/video/encoded_video_source.h:21: virtual void OnBitstreamStreaming(
Drop "Bitstream" from the name of each of the functions in this interface?

https://codereview.chromium.org/16320005/diff/15001/media/video/encoded_video...
media/video/encoded_video_source.h:22: scoped_refptr<EncodedVideoBitstream>
bitstream,
The |bitstream| param to each of these methods is not actually needed (viz the
impl doesn't use them).

https://codereview.chromium.org/16320005/diff/15001/media/video/encoded_video...
media/video/encoded_video_source.h:35: virtual void OnBitstreamReady(
s/Bitstream/Buffer/

https://codereview.chromium.org/16320005/diff/15001/media/video/encoded_video...
media/video/encoded_video_source.h:45: // TrySetBitstreamConfig issues a
reconfiguration request. Old configuration
comment out of date (and tortured)

https://codereview.chromium.org/16320005/diff/15001/media/video/encoded_video...
media/video/encoded_video_source.h:51: protected:
Here and elsewhere you seem to think that RCTS<> requires dtor & friend decl to
be "protected" but actually they can be private instead.

https://codereview.chromium.org/16320005/diff/15001/media/video/encoded_video...
media/video/encoded_video_source.h:59: class MEDIA_EXPORT EncodedVideoSource {
Is there no need to report errors other than OnBitstreamRemoved?

https://codereview.chromium.org/16320005/diff/15001/media/video/encoded_video...
media/video/encoded_video_source.h:65: virtual void
OnCapabilitiesAvailable(EncodedVideoSource* source) = 0;
Single-method interfaces are better served by a callback parameter to the
relevant method, which leaves implementors more freedom for how to implement.

https://codereview.chromium.org/16320005/diff/15001/media/video/encoded_video...
media/video/encoded_video_source.h:86: virtual VideoEncodingCapability
GetCapabilities() = 0;
Observer, {Add,Remove}CapabilitiesObserver, StartFetchCapabilities, and
GetCapabilities() is a pretty baroque set of stuff that could all be replaced
with a single:

virtual void RequestCapabilities(const
base::Callback<void(VideoEncodingCapability)>& capability_cb) = 0;

don't you think?

https://codereview.chromium.org/16320005/diff/15001/media/video/encoded_video...
media/video/encoded_video_source.h:93: // EncodedVideoBitstream and client just
has the pointer to it.
Again, this comment is craxy.

https://codereview.chromium.org/16320005/diff/15001/media/video/encoded_video...
media/video/encoded_video_source.h:94: virtual
scoped_refptr<EncodedVideoBitstream> OpenBitstream(
This API was built to support the notion of multiple clients consuming the same
output of an encoding camera.  I suspect you could cut the amount of code in
this CL significantly by making it so that a VideoCapturer is limited to feeding
a single bitstream.

https://codereview.chromium.org/16320005/diff/15001/media/video/encoded_video...
media/video/encoded_video_source.h:95: EncodedVideoBitstream::Client* client,
doco lifecycle anytime ownership is not clarified by use of descriptive ptr
type.

https://codereview.chromium.org/16320005/diff/15001/media/video/encoded_video...
media/video/encoded_video_source.h:103: virtual void ReturnBitstreamBuffer(
It's craxy that buffers are handed to EVB::Client but are returned by EVS (not
EVB).

Looks like your model is that the EVS (browser process) creates & owns shared
memory buffers and the client (renderer process) notifies when they're no longer
needed.
This differs from VDA & VEA where the {de,en}coder requests buffers from the
client and the client ultimately owns them.
Might be worth calling out in a NOTE comment since this will require redoing
when dropping EVS for VEA.

https://codereview.chromium.org/16320005/diff/15001/media/video/video_encode_...
File media/video/video_encode_types.h (right):

https://codereview.chromium.org/16320005/diff/15001/media/video/video_encode_...
media/video/video_encode_types.h:17: // Data to represent limitations for a
particular encoder config.
We're talking about shipping the EVS stuff on only an extremely narrow set of
platforms.  Do we really need the configurability implied by this file or can we
get away with a much smaller set of predefined configs (possibly just one)?

https://codereview.chromium.org/16320005/diff/15001/media/video/video_encode_...
media/video/video_encode_types.h:26: typedef std::vector<VideoEncodingConfig>
VideoEncodingCapability;
VideoEncodingCapability is a particularly unintuitive name choice.
Why not simply pluralize the type of which this is a vector of?

https://codereview.chromium.org/16320005/diff/15001/media/video/video_encode_...
media/video/video_encode_types.h:31: int average_bitrate;
s/average/target/?

https://codereview.chromium.org/16320005/diff/15001/media/video/video_encode_...
media/video/video_encode_types.h:39: gfx::Size resolution;
Is there a reason why resolution & frames_per_second are not
runtime-configurable?

[hshi1, 2013-06-10 19:12:31]

Thanks a lot +@fischman for the comments!

I want to make the necessary changes related to the IPC message definition and
the video_encoding_types.h first, as John is also working on the renderer host
side based on these. So I've uploaded Patch Set#2 as a FYI.

I'm still in the process of addressing the remaining comments, particularly
those about unnecessary layers of indirection (vestiges of the rehashed hubble
work).

https://codereview.chromium.org/16320005/diff/15001/content/common/content_me...
File content/common/content_message_generator.h (right):

https://codereview.chromium.org/16320005/diff/15001/content/common/content_me...
content/common/content_message_generator.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2013/06/08 00:18:01, Ami Fischman wrote:
> Please update CL description to reflect the merge of your split CLs, and point
> to URLs for other in-flight changes this depends on.

Done.

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

https://codereview.chromium.org/16320005/diff/15001/content/common/media/enco...
content/common/media/encoded_video_source_messages.h:46: // Message from
Renderer to Browser process to query the encoding capability
On 2013/06/08 00:18:01, Ami Fischman wrote:
> Please mimic gpu_messages.h where all the messages that go from process A to
> process B go together, with a leading comment that says "Messages from A to B"
> so you don't have to repeat the fact on each message.

Done.

https://codereview.chromium.org/16320005/diff/15001/content/common/media/enco...
content/common/media/encoded_video_source_messages.h:71:
IPC_MESSAGE_CONTROL2(EncodedVideoSourceHostMsg_DestroyBitstream,
On 2013/06/08 00:18:01, Ami Fischman wrote:
> "Destroy" or "Close"?  Please ensure that the IPC messages match the EVS
> interface (there are other discrepancies) and also make a pass on the
commentary
> in this file.

Done. I'm unifying the names - use "Open" to replace "Add/Create" and "Close"
for "Remove/Destroy".

https://codereview.chromium.org/16320005/diff/15001/content/common/media/enco...
content/common/media/encoded_video_source_messages.h:135: size_t /* size */,
On 2013/06/08 00:18:01, Ami Fischman wrote:
> "size_t" is verboten in IPC - use an explicitly-sized type.

Done.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
File content/renderer/media/rtc_encoding_video_capturer.cc (right):

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:39: void
set_round_trip_time(int rtt_in_ms);
On 2013/06/08 00:18:01, Ami Fischman wrote:
> use TimeDelta for the param

Done.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:62:
DISALLOW_COPY_AND_ASSIGN(Bitstream);
On 2013/06/08 00:18:01, Ami Fischman wrote:
> nit: extra \n before this

Done.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:210:
params.runtime_params.average_bitrate = codecSettings->maxBitrate;
On 2013/06/08 00:18:01, Ami Fischman wrote:
> s/max/start/ ?

Done.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:221: // TODO: request
specific frame type.
On 2013/06/08 00:18:01, Ami Fischman wrote:
> TODO(hshi)

Done.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:240: uint32_t packetLoss,
On 2013/06/08 00:18:01, Ami Fischman wrote:
> /* packetLoss */ here & in the header

Done.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:252: // TODO: wire up
runtime rate control.
On 2013/06/08 00:18:01, Ami Fischman wrote:
> TODO(hshi)

Done.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
File content/renderer/media/rtc_encoding_video_capturer.h (right):

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.h:11: #include
"third_party/libjingle/source/talk/media/webrtc/webrtcvideoencoderfactory.h"
On 2013/06/08 00:18:01, Ami Fischman wrote:
> unused

Done.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.h:12: #include
"third_party/libjingle/source/talk/media/webrtc/webrtcvideoframe.h"
On 2013/06/08 00:18:01, Ami Fischman wrote:
> unnecessary in this .h - this file doesn't make any more use of webrtc/cricket
> types than the interface being impl'd.

Done.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.h:15: using
cricket::WebRtcVideoEncoderFactory;
On 2013/06/08 00:18:01, Ami Fischman wrote:
> unused

Done.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.h:34: const
webrtc::I420VideoFrame& inputImage,
On 2013/06/08 00:18:01, Ami Fischman wrote:
> ignored params should be like this:
> /* inputImage */

Done.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.h:48: int stream_id_;
On 2013/06/08 00:18:01, Ami Fischman wrote:
> write-only variable?

Removed.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.h:54:
DISALLOW_COPY_AND_ASSIGN(RtcEncodingVideoCapturer);
On 2013/06/08 00:18:01, Ami Fischman wrote:
> nit: extra new line before this

Done.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
File content/renderer/media/video_capture_impl.h (right):

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
content/renderer/media/video_capture_impl.h:90: virtual void Send(IPC::Message*
message) OVERRIDE;
On 2013/06/08 00:18:01, Ami Fischman wrote:
> Keep order same as in interface definition.

Done.

https://codereview.chromium.org/16320005/diff/15001/media/video/video_encode_...
File media/video/video_encode_types.h (right):

https://codereview.chromium.org/16320005/diff/15001/media/video/video_encode_...
media/video/video_encode_types.h:17: // Data to represent limitations for a
particular encoder config.
On 2013/06/08 00:18:01, Ami Fischman wrote:
> We're talking about shipping the EVS stuff on only an extremely narrow set of
> platforms.  Do we really need the configurability implied by this file or can
we
> get away with a much smaller set of predefined configs (possibly just one)?

I don't believe it is adding significant complexity to permit this kind of
configurability. Wouldn't you agree it is a bad idea to hard-coding encode
configs specific to a platform (especially if I can't even mention the specifics
of that platform in codereview)?

https://codereview.chromium.org/16320005/diff/15001/media/video/video_encode_...
media/video/video_encode_types.h:26: typedef std::vector<VideoEncodingConfig>
VideoEncodingCapability;
On 2013/06/08 00:18:01, Ami Fischman wrote:
> VideoEncodingCapability is a particularly unintuitive name choice.
> Why not simply pluralize the type of which this is a vector of?

Done.

https://codereview.chromium.org/16320005/diff/15001/media/video/video_encode_...
media/video/video_encode_types.h:31: int average_bitrate;
On 2013/06/08 00:18:01, Ami Fischman wrote:
> s/average/target/?

Done.

https://codereview.chromium.org/16320005/diff/15001/media/video/video_encode_...
media/video/video_encode_types.h:39: gfx::Size resolution;
On 2013/06/08 00:18:01, Ami Fischman wrote:
> Is there a reason why resolution & frames_per_second are not
> runtime-configurable?

I have moved the frames_per_second into the runtime-configurable parameters as
you suggested. However, the resolution is probably not runtime-configurable as
it involves re-allocating all reference and decoded picture buffers, so most
likely a resolution change would require tearing down the current bitstream and
creating a new one.

Note that the "runtime configurable parameters" i.e. bitrate and frame rate do
correspond to the SetRates method of a webrtc::VideoEncoder object. Webrtc does
not define any method for dynamic resolution change.

[Ami GONE FROM CHROMIUM, 2013-06-10 19:29:31]

https://codereview.chromium.org/16320005/diff/15001/media/video/video_encode_...
File media/video/video_encode_types.h (right):

https://codereview.chromium.org/16320005/diff/15001/media/video/video_encode_...
media/video/video_encode_types.h:17: // Data to represent limitations for a
particular encoder config.
On 2013/06/10 19:12:31, hshi1 wrote:
> On 2013/06/08 00:18:01, Ami Fischman wrote:
> > We're talking about shipping the EVS stuff on only an extremely narrow set
of
> > platforms.  Do we really need the configurability implied by this file or
can
> we
> > get away with a much smaller set of predefined configs (possibly just one)?
> 
> I don't believe it is adding significant complexity to permit this kind of
> configurability. Wouldn't you agree it is a bad idea to hard-coding encode
> configs specific to a platform (especially if I can't even mention the
specifics
> of that platform in codereview)?

This configurability requires in EVS:
Observer
AddCapabilitiesObserver
RemoveCapabilitiesObserver
StartFetchCapabilities
GetCapabilities
along with their attendant IPC surface.  Also, they require an unnecessary IPC
roundtrip.

None of this breaks the bank, I'm just not sure why bother in throwaway code.

[hshi1, 2013-06-10 19:35:56]

On 2013/06/10 19:29:31, Ami Fischman wrote:
>
https://codereview.chromium.org/16320005/diff/15001/media/video/video_encode_...
> File media/video/video_encode_types.h (right):
> 
>
https://codereview.chromium.org/16320005/diff/15001/media/video/video_encode_...
> media/video/video_encode_types.h:17: // Data to represent limitations for a
> particular encoder config.
> On 2013/06/10 19:12:31, hshi1 wrote:
> > On 2013/06/08 00:18:01, Ami Fischman wrote:
> > > We're talking about shipping the EVS stuff on only an extremely narrow set
> of
> > > platforms.  Do we really need the configurability implied by this file or
> can
> > we
> > > get away with a much smaller set of predefined configs (possibly just
one)?
> > 
> > I don't believe it is adding significant complexity to permit this kind of
> > configurability. Wouldn't you agree it is a bad idea to hard-coding encode
> > configs specific to a platform (especially if I can't even mention the
> specifics
> > of that platform in codereview)?
> 
> This configurability requires in EVS:
> Observer
> AddCapabilitiesObserver
> RemoveCapabilitiesObserver
> StartFetchCapabilities
> GetCapabilities
> along with their attendant IPC surface.  Also, they require an unnecessary IPC
> roundtrip.
> 
> None of this breaks the bank, I'm just not sure why bother in throwaway code.

Even if we just have one single config (e.g. 1280x720p30 with a fixed max
bitrate of X Mbps), we still would need that IPC trip to pass back a boolean
flag to indicate whether encoded capture is supported or not. Ultimately that
flag decides whether (1) the injected encoding capturer factory is used to
create external video encoders and register that with the VIE channel, or (2)
that we go through the SW encoder path. I don't see how we can avoid all this
complexity unless you just want to unconditionally enable this based on a combo
of #ifdef's.

[Ami GONE FROM CHROMIUM, 2013-06-10 19:41:25]

Runtime config doesn't have to be static for my comments to apply; only the
set of possible capabilities has to be static.
Resource availability (i.e. limits on how much encode can happen) can be
deduced from CreateBitstream/Initialize/Open/whatever failing, no?

[hshi1, 2013-06-10 19:49:57]

On 2013/06/10 19:41:25, Ami Fischman wrote:
> Resource availability (i.e. limits on how much encode can happen) can be
> deduced from CreateBitstream/Initialize/Open/whatever failing, no?

Unfortunately we cannot rely on failing OpenBitstream to indicate lack of
encoded capture support because there is no fallback logic in libjingle and
we'll just fail to set send codec on the channel. Unless someone makes
significant changes in libjingle to clean that up, we'll stuck with the status
quo where the caps reported by the factory affect how libjingle
(WebRtcVideoEngine) picks send codecs, and thus it must already be available
before the video engine decides to create external encoder and register that
with the VIE channel. It's one of the reason why we have bug 244626 (and the
related libjingle CL that I just landed last Friday that handles the plumbing of
cricket::WebRtcVideoEncoderFactory::Observer::OnCodecsAvailable().

[Ami GONE FROM CHROMIUM, 2013-06-10 19:57:32]

Like I said, this isn't the end of the world if you keep it.
Given the concerns you list I wonder how well this will all work if there
are two tabs trying to engage this codepath at the same time.

[hshi1, 2013-06-10 20:02:24]

On 2013/06/10 19:57:32, Ami Fischman wrote:
> Like I said, this isn't the end of the world if you keep it.
> Given the concerns you list I wonder how well this will all work if there
> are two tabs trying to engage this codepath at the same time.

Assuming that the HW encoder supports multiple concurrent contexts, I don't see
any particular problem with two tabs engaging in the caps query at the same
time. The caps queries IPC message have device ID specified (that corresponds to
the VideoCapturerControllerID on the browser process side) so the video capture
host should be able to respond to the correct renderer process.

[Ami GONE FROM CHROMIUM, 2013-06-10 20:49:38]

> Assuming that the HW encoder supports multiple concurrent contexts


I doubt this is the case.

[sheu, 2013-06-10 20:53:29]

On 2013/06/10 20:49:38, Ami Fischman wrote:
> > Assuming that the HW encoder supports multiple concurrent contexts
> 
> 
> I doubt this is the case.

In the Exynos case, the encoder does support multiple concurrent contexts. 
Whether this use case is entirely bug-free in the kernel driver -- that is
another matter.

[hshi1, 2013-06-10 21:49:34]

I've just uploaded Patch Set #3 which addresses an additional 35 comments,
mostly dealing with the RtcEncodingVideoCapturer and its factory class. These
are relatively straightforward to fix.

The remaining things that I still have to spend some time to figure out:
(1) how do I clearly define the lifetime of the EVB and EVB client? note that I
can't simply close the EVB then delete the client immediately, there's still an
IPC roundtrip that closes the bitstream on the browser side then send
OnBitstreamClosed back;
(2) how do I define the relationship between the general VCImpl and the
encoding-capable VCImpl, make it so that I won't need to touch the
non-encoding-capable classes such as the pepper VC, and eliminate unnecessary
layers of indirections.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
File content/renderer/media/rtc_encoding_video_capturer.cc (right):

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:14: public
media::EncodedVideoBitstream::Client,
On 2013/06/08 00:18:01, Ami Fischman wrote:
> This implies the name of the class is poor - it's a Bitstream's Client, not a
> Bitstream itself.

How about "BitstreamClient"? See updated CL.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:15: public
base::RefCountedThreadSafe<RtcEncodingVideoCapturer::Bitstream> {
On 2013/06/08 00:18:01, Ami Fischman wrote:
> why refcount?

Refcount is not really needed, I just need it to be a scoped ptr owned by the
RtcEncodingVideoCapturer. Would that work?

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:20: // Function to
configure runtime encoding parameters.
On 2013/06/08 00:18:01, Ami Fischman wrote:
> Isn't this really just the EVB interface?

... yes, what was I thinking. This is completely bogus. Removed.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:21: void Configure(const
media::RuntimeVideoEncodingParameters& params);
On 2013/06/08 00:18:01, Ami Fischman wrote:
> Try?

This is no longer needed.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:70:
encoded_video_source_(encoded_video_source) {
On 2013/06/08 00:18:01, Ami Fischman wrote:
> initialize all other members to known-reasonable sentinels (0, NULL, etc)

Done.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:71: if
(encoded_video_source_)
On 2013/06/08 00:18:01, Ami Fischman wrote:
> how can this fail?

Done.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:76: if
(encoded_video_source_ && bitstream_) {
On 2013/06/08 00:18:01, Ami Fischman wrote:
> how can this fail?

Well encoded_video_source_ should be valid, but bitstream_ can still be NULL if
OpenBitstream failed, or if bitstream is already closed. Updated here.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:78: bitstream_ = NULL;
On 2013/06/08 00:18:01, Ami Fischman wrote:
> why bother?

Done.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:84: if (bitstream_)
On 2013/06/08 00:18:01, Ami Fischman wrote:
> how can this fail?

Can fail if OpenBitstream failed, or bitstream is already closed.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:103: callback_ = callback;
On 2013/06/08 00:18:01, Ami Fischman wrote:
> DCHECK(!callback_);
> first?

Done.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:114: finished_ = true;
On 2013/06/08 00:18:01, Ami Fischman wrote:
> bitstream_ = NULL;
> ?

Done.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:121: // First buffer
constitutes the origin of the time for this bitstream
On 2013/06/08 00:18:01, Ami Fischman wrote:
> Isn't this true regardless of callback_ being set?

Yes but if callback_ is not set then we can't call callback_->Encoded. I've
moved the check to just before invoking callback_->Encoded.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:130: if (Convert(buffer,
image, codecInfo, fragInfo)) {
On 2013/06/08 00:18:01, Ami Fischman wrote:
> style: no non-const ref params.  These are output params and so should be
passed
> by pointer.
> But really it seems strange to pass in output params just to use them in the
> very next line.
> Wouldn't it be simpler/better to move the Encoded() call into Convert() (after
> renaming it)?

Done.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:135: if
(encoded_video_source_)
On 2013/06/08 00:18:01, Ami Fischman wrote:
> can never be false.

Done.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:137: }
On 2013/06/08 00:18:01, Ami Fischman wrote:
> This collection of comments reduces l.120-136 into:
> if (time_base_.is_null())
>   time_base_ = buffer->metadata().timestamp;
> ReportEncodedFrame(buffer);
> encoded_video_source_->ReturnBitstreamBuffer(bitstream, buffer);
> 
> and ReportEncodedFrame is Convert with an early-return if !callback_ and a
final
> callback_->Encoded() call at its bottom.

Done.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:145: bool
RtcEncodingVideoCapturer::Bitstream::Convert(
On 2013/06/08 00:18:01, Ami Fischman wrote:
> return value is ignored at only callsite and no legit return false codepaths
so
> s/bool/void/ I think.

Made it void.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:155: // TODO: remove this
const_cast. Unfortunately webrtc::EncodedImage defines
On 2013/06/08 00:18:01, Ami Fischman wrote:
> TODO's should always be attributed like TODO(hshi)
> (please make a pass)

Done.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:160: image._size =
image._length;
On 2013/06/08 00:18:01, Ami Fischman wrote:
> Do you know what the diff is supposed to be between _size and _length?

|_size| is supposed to indicate buffer capacity, and |_length| the amount of
valid bitstream data (in bytes). For our situation they're always identical.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:164: (metadata.timestamp -
time_base_).InMilliseconds(); // 1 KHz
On 2013/06/08 00:18:01, Ami Fischman wrote:
> Comment doesn't make sense to me.

Done.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:165: image._timeStamp =
image.capture_time_ms_ * 90; // 90kHz
On 2013/06/08 00:18:01, Ami Fischman wrote:
> Once we finalize this CL IWBN to have mflodman@ or another webrtc-eng confirm
> this is Doing It Right(tm).

I agree that this is not accurate; please review this new version.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:204: if (!bitstream_) {
On 2013/06/08 00:18:01, Ami Fischman wrote:
> How can this fail?
> (and if you wanted to handle InitEncode being called twice, you probably
> wouldn't want to return OK)

Done.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer.cc:229:
bitstream_->set_callback(callback);
On 2013/06/08 00:18:01, Ami Fischman wrote:
> Are you really not guaranteed that RECC is called _after_ InitEncode?  Given
> that they're all synchronous, this paranoia seems unwarranted.
> (if indeed not necessary, then a bunch of code simplifies as
> Bitstream::set_callback can go into Bitstream's ctor, etc)

You're right.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
File content/renderer/media/rtc_encoding_video_capturer_factory.cc (right):

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer_factory.cc:21: delete
it->second;
On 2013/06/08 00:18:01, Ami Fischman wrote:
> STLDeleteValues(&encoder_map_);

Thanks for the suggestion, done (although I'm using STLDeleteElements as the map
should really be a set.)

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer_factory.cc:29: // For now we
only support one instance of encoded video source at a time.
On 2013/06/08 00:18:01, Ami Fischman wrote:
> This doesn't seem super-legit, since the caller won't know it's being
ignored...
> Does this belong at the caller and a DCHECK here?

Done.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer_factory.cc:54:
RtcEncodingVideoCapturer* capturer =
On 2013/06/08 00:18:01, Ami Fischman wrote:
> neither |type| nor |i| are used in l.54-58; suspicious!

Oops. I should pass the type to the ctor of the encoding capturer. Done.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer_factory.cc:56: if (capturer)
On 2013/06/08 00:18:01, Ami Fischman wrote:
> can never fail

Done.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer_factory.cc:66: for
(EncoderMap::iterator it = encoder_map_.begin(); it != encoder_map_.end();
On 2013/06/08 00:18:01, Ami Fischman wrote:
> lol it's a map but you never use it as such.
> typedef std::set<VideoEncoder*> EncoderSet;
> ?
> 

Done.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer_factory.cc:86: return;
On 2013/06/08 00:18:01, Ami Fischman wrote:
> l.85-86 unnecessary

Done.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer_factory.cc:97:
webrtc_codec_type = webrtc::kVideoCodecGeneric;
On 2013/06/08 00:18:01, Ami Fischman wrote:
> unnecessary given l.91

Done.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
File content/renderer/media/rtc_encoding_video_capturer_factory.h (right):

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer_factory.h:13: #include
"third_party/libjingle/source/talk/media/webrtc/webrtcvie.h"
On 2013/06/08 00:18:01, Ami Fischman wrote:
> I suspect neither of l.12-13 is necessary in this .h.

Done.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer_factory.h:15: using
cricket::WebRtcVideoEncoderFactory;
On 2013/06/08 00:18:01, Ami Fischman wrote:
> style: "using" is verboten in headers.
> (here & elsewhere; please make a pass)

Done.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer_factory.h:32: 
On 2013/06/08 00:18:01, Ami Fischman wrote:
> remove newline

Done.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer_factory.h:33: virtual const
std::vector<WebRtcVideoEncoderFactory::VideoCodec>& codecs()
On 2013/06/08 00:18:01, Ami Fischman wrote:
> WebRtcVideoEncoderFactory:: unnecessary

Done.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer_factory.h:43: 
On 2013/06/08 00:18:01, Ami Fischman wrote:
> remove one of these two newlines

Done.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/rt...
content/renderer/media/rtc_encoding_video_capturer_factory.h:54: virtual
~RtcEncodingVideoCapturerFactory();
On 2013/06/08 00:18:01, Ami Fischman wrote:
> dtor goes at the top of the section

Done.

[Ami GONE FROM CHROMIUM, 2013-06-10 21:57:20]

On Mon, Jun 10, 2013 at 2:49 PM, <hshi@chromium.org> wrote:

> (1) how do I clearly define the lifetime of the EVB and EVB client? note
> that I
> can't simply close the EVB then delete the client immediately, there's
> still an
> IPC roundtrip that closes the bitstream on the browser side then send
> OnBitstreamClosed back;
>

The way VDA deals with this is saying that the VDA::Client must outlive the
VDA, and that calling VDA::Destroy() _synchronously_ guarantees that no
more calls to VDA::Client will be made.  So when async is involved (IPC- or
thread-based) you need your trampoliners to know to detach the connection
to their incarnation of "the client" before their Destroy returns.


> (2) how do I define the relationship between the general VCImpl and the
> encoding-capable VCImpl, make it so that I won't need to touch the
> non-encoding-capable classes such as the pepper VC, and eliminate
> unnecessary
> layers of indirections.
>

Default do-nothing impls can be your friend here.

[hshi1, 2013-06-11 00:48:20]

Still work in progress. Uploaded Patch Set #3 to address the following comments:

(1) Replace the observer interface for GetCapabilities with an asynchronous
RequestCapabilities method with an async callback function.
(2) After talking to John about it, I'm getting rid of the whole "stream_id"
concept in the IPC definitions. Only support one bitstream per EVS.

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

https://codereview.chromium.org/16320005/diff/15001/media/video/encoded_video...
media/video/encoded_video_source.h:5: #ifndef
MEDIA_VIDEO_ENCODED_VIDEO_SOURCE_H_
On 2013/06/08 00:18:01, Ami Fischman wrote:
> TODO+crbug(m30) for merging this into VEA

Done.

https://codereview.chromium.org/16320005/diff/15001/media/video/encoded_video...
media/video/encoded_video_source.h:21: virtual void OnBitstreamStreaming(
On 2013/06/08 00:18:01, Ami Fischman wrote:
> Drop "Bitstream" from the name of each of the functions in this interface?

Made the following changes:
OnBitstreamStreaming -> OnStreaming
OnBitstreamClosed -> OnClosed
OnBitstreamReady -> OnBufferReady
OnBitstreamConfigChanged -> OnConfigChanged

https://codereview.chromium.org/16320005/diff/15001/media/video/encoded_video...
media/video/encoded_video_source.h:22: scoped_refptr<EncodedVideoBitstream>
bitstream,
On 2013/06/08 00:18:01, Ami Fischman wrote:
> The |bitstream| param to each of these methods is not actually needed (viz the
> impl doesn't use them).

You're correct; removed.

https://codereview.chromium.org/16320005/diff/15001/media/video/encoded_video...
media/video/encoded_video_source.h:35: virtual void OnBitstreamReady(
On 2013/06/08 00:18:01, Ami Fischman wrote:
> s/Bitstream/Buffer/

Done.

https://codereview.chromium.org/16320005/diff/15001/media/video/encoded_video...
media/video/encoded_video_source.h:45: // TrySetBitstreamConfig issues a
reconfiguration request. Old configuration
On 2013/06/08 00:18:01, Ami Fischman wrote:
> comment out of date (and tortured)

Done.

https://codereview.chromium.org/16320005/diff/15001/media/video/encoded_video...
media/video/encoded_video_source.h:59: class MEDIA_EXPORT EncodedVideoSource {
On 2013/06/08 00:18:01, Ami Fischman wrote:
> Is there no need to report errors other than OnBitstreamRemoved?

Currently no; do you think I should add a separate OnBitstreamError IPC, or
re-use OnBitstreamClosed() IPC with an error code?

https://codereview.chromium.org/16320005/diff/15001/media/video/encoded_video...
media/video/encoded_video_source.h:65: virtual void
OnCapabilitiesAvailable(EncodedVideoSource* source) = 0;
On 2013/06/08 00:18:01, Ami Fischman wrote:
> Single-method interfaces are better served by a callback parameter to the
> relevant method, which leaves implementors more freedom for how to implement.

Removed this caps observer business entirely (see comment below also).

https://codereview.chromium.org/16320005/diff/15001/media/video/encoded_video...
media/video/encoded_video_source.h:86: virtual VideoEncodingCapability
GetCapabilities() = 0;
On 2013/06/08 00:18:01, Ami Fischman wrote:
> Observer, {Add,Remove}CapabilitiesObserver, StartFetchCapabilities, and
> GetCapabilities() is a pretty baroque set of stuff that could all be replaced
> with a single:
> 
> virtual void RequestCapabilities(const
> base::Callback<void(VideoEncodingCapability)>& capability_cb) = 0;
> 
> don't you think?
Thanks for the suggestion, I've removed the observer interface and added
RequestCapabilities. There is one minor snag though, namely the EVS impl can't
actually start sending the IPC for caps until capture has started (at which
point the device_id is actually registered on the render_host side). But the new
CL I've uploaded seems to work correctly.

https://codereview.chromium.org/16320005/diff/15001/media/video/encoded_video...
media/video/encoded_video_source.h:94: virtual
scoped_refptr<EncodedVideoBitstream> OpenBitstream(
On 2013/06/08 00:18:01, Ami Fischman wrote:
> This API was built to support the notion of multiple clients consuming the
same
> output of an encoding camera.  I suspect you could cut the amount of code in
> this CL significantly by making it so that a VideoCapturer is limited to
feeding
> a single bitstream.

Talked to sheu@. I'm removing all stream_id and assume that each device can only
have one bitstream.

[Ami GONE FROM CHROMIUM, 2013-06-11 17:29:54]

>
> https://codereview.chromium.**org/16320005/diff/15001/media/**
>
video/encoded_video_source.h#**newcode59<https://codereview.chromium.org/16320005/diff/15001/media/video/encoded_video_source.h#newcode59>
> media/video/encoded_video_**source.h:59: class MEDIA_EXPORT
> EncodedVideoSource {
> On 2013/06/08 00:18:01, Ami Fischman wrote:
>
>> Is there no need to report errors other than OnBitstreamRemoved?
>
> Currently no; do you think I should add a separate OnBitstreamError IPC,
> or re-use OnBitstreamClosed() IPC with an error code?


I suspect the former (dedicated IPC/callback) will be cleaner to code.

[hshi1, 2013-06-11 17:49:42]

Ami - I've uploaded a patch set named "Addressing comments - Part 4." I've
reverted changes to the base header video_capture.h and the pepper video capture
files.

Also for vc_manager_ I'd define the factory pointer for all platforms but only
initialize it for CrOS + ARM. Thus for other platforms we will not inject a
factory in CPCF.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
File content/renderer/media/video_capture_impl_manager.cc (right):

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
content/renderer/media/video_capture_impl_manager.cc:17:
encoding_capturer_factory_(new RtcEncodingVideoCapturerFactory()) {
On 2013/06/08 00:18:01, Ami Fischman wrote:
> flag-protect?

Per discussion with Zel I'm guarding this with #ifdef CrOS + ARM.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
content/renderer/media/video_capture_impl_manager.cc:36: if
(encoding_capturer_factory_ && evs)
On 2013/06/08 00:18:01, Ami Fischman wrote:
> how can encoding_capturer_factory_ be false?

Since I'm guarding encoding_capturer_factory_ with #ifdef (ARM + CrOS), it can
be NULL on other platforms. So I think it is still necessary to check here, or
if you prefer, also guard this with #ifdef.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
content/renderer/media/video_capture_impl_manager.cc:68: if
(encoding_capturer_factory_ && evs)
On 2013/06/08 00:18:01, Ami Fischman wrote:
> ditto encoding_capturer_factory_ is never NULL

ditto (NULL on non-ARM-CrOS platform).

https://codereview.chromium.org/16320005/diff/15001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_platform_video_capture_impl.cc (right):

https://codereview.chromium.org/16320005/diff/15001/content/renderer/pepper/p...
content/renderer/pepper/pepper_platform_video_capture_impl.cc:99:
media::EncodedVideoSource*
On 2013/06/08 00:18:01, Ami Fischman wrote:
> Hopefully you can revert this file.

Done.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_platform_video_capture_impl.h (right):

https://codereview.chromium.org/16320005/diff/15001/content/renderer/pepper/p...
content/renderer/pepper/pepper_platform_video_capture_impl.h:46: virtual
media::EncodedVideoSource* GetEncodedVideoSource() OVERRIDE;
On 2013/06/08 00:18:01, Ami Fischman wrote:
> Hopefully you can revert this file 

Done.

https://codereview.chromium.org/16320005/diff/15001/media/video/capture/video...
File media/video/capture/video_capture.h (right):

https://codereview.chromium.org/16320005/diff/15001/media/video/capture/video...
media/video/capture/video_capture.h:104: // also stop the streaming of encoded
streams.
I realized it is completely unnecessary to change this header at all. I'm
reverting it. There is no reason to add this method in the VideoCapture
interface. The base VideoCapture interface will always have no encoding
capabilities. This way I don't have to deal with pepper video capture either.

https://codereview.chromium.org/16320005/diff/15001/media/video/capture/video...
media/video/capture/video_capture.h:105: // All platforms or cameras may have
not encoding support available.
On 2013/06/08 00:18:01, Ami Fischman wrote:
> Comment is written as if this returns a bool, not a pointer.

N/A (file reverted).

https://codereview.chromium.org/16320005/diff/15001/media/video/capture/video...
media/video/capture/video_capture.h:105: // All platforms or cameras may have
not encoding support available.
On 2013/06/08 00:18:01, Ami Fischman wrote:
> Comment is written as if this returns a bool, not a pointer.

N/A (file reverted).

https://codereview.chromium.org/16320005/diff/15001/media/video/capture/video...
media/video/capture/video_capture.h:106: virtual EncodedVideoSource*
GetEncodedVideoSource() = 0;
On 2013/06/08 00:18:01, Ami Fischman wrote:
> What are the ownership semantics of the returned pointer?

N/A (file reverted).

[hshi1, 2013-06-11 19:51:22]

Ami - PTAL patch set #6 ("Addressing Ami's comments - part 5.").

I believe this has addressed the majority of your complaints regarding the
over-inheritance and the EVB/EVS relationship. The changes include:

(1) Remove EVB and move EVB::Client into EVS::Client
(2) Remove encoded VC message filter. Move the message handling logic into the
VC message filter.
(3) Remove the CapturedEncodedVideoCaptureImpl. Make VideoCaptureImpl inherit
EVS directly.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/ca...
File content/renderer/media/captured_encoded_video_source_impl.cc (right):

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/ca...
content/renderer/media/captured_encoded_video_source_impl.cc:24:
source_->CloseBitstream(this);
On 2013/06/08 00:18:01, Ami Fischman wrote:
> This implies that a bitstream can close before the source removes it, but the
> source has a ref on the bitstream so how can this happen?

N/A (file removed)

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/ca...
File content/renderer/media/captured_encoded_video_source_impl.h (right):

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/ca...
content/renderer/media/captured_encoded_video_source_impl.h:4: #ifndef
CONTENT_RENDERER_MEDIA_CAPTURED_ENCODED_VIDEO_SOURCE_IMPL_H_
On 2013/06/08 00:18:01, Ami Fischman wrote:
> I _think_ you can actually drop this file (and it's .cc file) entirely, if you
> switch from using control IPC messages to routed messages.
> Routed messages are a generic answer to the need this file fills (AFAICT). 
> If this is providing some service not served by routed messages, please let me
> know.

I'm removing the entire file and moved EVS stuff to CaptureImpl itself.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/ca...
content/renderer/media/captured_encoded_video_source_impl.h:29: // device. It
lives in similar setting where it is proxying communication
On 2013/06/08 00:18:01, Ami Fischman wrote:
> "similar" to what?

N/A (this class is removed).

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/ca...
content/renderer/media/captured_encoded_video_source_impl.h:31: class
CONTENT_EXPORT CapturedEncodedVideoBitstreamImpl :
On 2013/06/08 00:18:01, Ami Fischman wrote:
> Seems like an impl detail of CEVSI below; make it an inner class?

N/A (this class is removed).

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/ca...
content/renderer/media/captured_encoded_video_source_impl.h:34:
CapturedEncodedVideoBitstreamImpl(
On 2013/06/08 00:18:01, Ami Fischman wrote:
> doco lifecycle of args

N/A (this class is removed).

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/ca...
content/renderer/media/captured_encoded_video_source_impl.h:112: // Services
required from deriving class.
On 2013/06/08 00:18:01, Ami Fischman wrote:
> You need to say what these methods are supposed to do if you expect deriving
> classes to provide them!

Removed this indirection.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/ca...
content/renderer/media/captured_encoded_video_source_impl.h:115: virtual
scoped_refptr<base::MessageLoopProxy> app_loop_proxy() = 0;
On 2013/06/08 00:18:01, Ami Fischman wrote:
> What is "app" in this context?

Removed.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/ca...
content/renderer/media/captured_encoded_video_source_impl.h:116: virtual
scoped_refptr<base::MessageLoopProxy> io_loop_proxy() = 0;
On 2013/06/08 00:18:01, Ami Fischman wrote:
> This is always
> ChildProcess::current()->io_message_loop_proxy()
> unclear why it needs to be cached and why clients wouldn't look this up
> themselves.

Removed.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/en...
File content/renderer/media/encoding_video_capture_message_filter.h (right):

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/en...
content/renderer/media/encoding_video_capture_message_filter.h:12: class
CONTENT_EXPORT EncodingVideoCaptureMessageFilter {
On 2013/06/08 00:18:01, Ami Fischman wrote:
> doco: what's the purpose of this interface?
> This isn't just a request for better coding practices; I actually can't tell
why
> this needs to exist; no methods seem to take this as a parameter and the only
> use is in the only impl of the type.  Why not just delete this file and its
.cc
> and avoid a layer of indirection?

I'm removing this header and its .cc file. The encoded-capture related messages
and delegates are merged into the VC message filter.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
File content/renderer/media/video_capture_impl.cc (right):

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
content/renderer/media/video_capture_impl.cc:106: return this;
On 2013/06/08 00:18:01, Ami Fischman wrote:
> shouldn't this be conditionalized on encoding_supported_?
> Further, I don't see where encoding_supported_ is set to anything but false;
is
> that supposed to be a flag?

Got rid of this.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
content/renderer/media/video_capture_impl.cc:111:
CapturedEncodedVideoSourceImpl::OnCapabilityAvailable(capability);
On 2013/06/08 00:18:01, Ami Fischman wrote:
> having to write l.109-139 is usually a code smell of over-inheritance.  Is it
> required?  Why?

I've flattened the VCImpl - CapturedEncodedVideoCaptureImpl hierarchy, and hence
got rid of this extra level of indirection.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
content/renderer/media/video_capture_impl.cc:358:
CapturedEncodedVideoSourceImpl::StartFetchCapabilities();
On 2013/06/08 00:18:01, Ami Fischman wrote:
> why "CapturedEncodedVideoSourceImpl::"?

No longer needed.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
File content/renderer/media/video_capture_message_filter.cc (right):

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
content/renderer/media/video_capture_message_filter.cc:107: if
(delegates_.find(device_id) != delegates_.end())
On 2013/06/08 00:18:01, Ami Fischman wrote:
> Why not replace the other occurrences of this code in this file with a call to
> this method?

Done.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
File content/renderer/media/video_capture_message_filter.h (right):

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
content/renderer/media/video_capture_message_filter.h:27: : public
EncodingVideoCaptureMessageFilter,
On 2013/06/08 00:18:01, Ami Fischman wrote:
> This is a logical reversal; it implies that all VCMF's expect to handle EVS's
> messages, even though while all EVS's are related to a VC, it's not true that
> all VC's are related to an EVS.

I agree it is wrong. I think it is really not necessary to have this separate
EncodingVideoCaptureMessageFilter interface; just move the encoded message
handling here.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
content/renderer/media/video_capture_message_filter.h:83: int32 last_device_id_;
On 2013/06/08 00:18:01, Ami Fischman wrote:
> Why are l.79-83 not private?

Now that I've gotten rid of the encoded VC message filter, I'm restoring this to
the way it used to be.

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

https://codereview.chromium.org/16320005/diff/15001/media/video/encoded_video...
media/video/encoded_video_source.h:14: // Class to represent encoded video
bitstream.
On 2013/06/08 00:18:01, Ami Fischman wrote:
> o rly?

Proposed change: remove the entire EVB definition. Move EVB::Client into
EVS::Client.

https://codereview.chromium.org/16320005/diff/15001/media/video/encoded_video...
media/video/encoded_video_source.h:20: // After OnStreaming callback stream
shall be considered to be streaming.
On 2013/06/08 00:18:01, Ami Fischman wrote:
> Oh, shall it??
> 
> Can you make a pass and cull useless comments and add useful ones where
mystery
> might exist?

Got rid of the EVB altogether.

https://codereview.chromium.org/16320005/diff/15001/media/video/encoded_video...
media/video/encoded_video_source.h:51: protected:
On 2013/06/08 00:18:01, Ami Fischman wrote:
> Here and elsewhere you seem to think that RCTS<> requires dtor & friend decl
to
> be "protected" but actually they can be private instead.

N/A (removed).

https://codereview.chromium.org/16320005/diff/15001/media/video/encoded_video...
media/video/encoded_video_source.h:93: // EncodedVideoBitstream and client just
has the pointer to it.
On 2013/06/08 00:18:01, Ami Fischman wrote:
> Again, this comment is craxy.

Now that EVB is gone, I've simplified this to be a void function. Only one
bitstream can be opened per EVS.

https://codereview.chromium.org/16320005/diff/15001/media/video/encoded_video...
media/video/encoded_video_source.h:103: virtual void ReturnBitstreamBuffer(
On 2013/06/08 00:18:01, Ami Fischman wrote:
> It's craxy that buffers are handed to EVB::Client but are returned by EVS (not
> EVB).
> 
> Looks like your model is that the EVS (browser process) creates & owns shared
> memory buffers and the client (renderer process) notifies when they're no
longer
> needed.
> This differs from VDA & VEA where the {de,en}coder requests buffers from the
> client and the client ultimately owns them.
> Might be worth calling out in a NOTE comment since this will require redoing
> when dropping EVS for VEA.

EVB is now gone and EVB::Client is moved to become EVS::Client.

[hshi1, 2013-06-12 00:50:24]

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
File content/renderer/media/video_capture_impl_manager.h (right):

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
content/renderer/media/video_capture_impl_manager.h:55:
RtcEncodingVideoCapturerFactory* encoding_capturer_factory() const {
On 2013/06/08 00:18:01, Ami Fischman wrote:
> It's almost never a good idea to return a raw pointer to an object whose
> lifetime is governed by a scoped_refptr<>.

The problem I'm facing is that the factory pointer is supposed to be owned by
the PeerConnectionFactory (which is the pc_factory_ member of
MediaStreamDependencyFactory). In the current libjingle design, factory lives in
a talk_base::scoped_ptr in pc_factory_. So, it seems wrong to also hold
encoding_capturer_factory_ in a scoped_refptr of vc_manager_. Furthermore, I
believe the vc_manager_ outlives the pc_factory_ (the latter is owned by
MediaStreamDependencyFactory which is outlived by vc_manager_), so we need to
avoid vc_manager_ using the encoder_factory_ after pc_factory_ is freed in
MediaStreamDependencyFactory::CleanupPeerConnectionFactory().

Would the following work?
(1) vc_manager_ ctor constructs and holds a (not scoped) pointer to
encoder_factory_, and pass itself to encoder_factory_;
(2) at CPCF, the encoder_factory_ is passed to the PC factory;
(3) the dtor of encoder_factory_ notifies vc_manager_ that it is destroyed.
After that vc_manager_ will clear the pointer and no longer call
encoder_factory_ when video capturer impls are added/removed.

[Ami GONE FROM CHROMIUM, 2013-06-12 01:04:39]

The key is that scoped_refptr<> and scoped_ptr<> each impose their own
lifecycle model.
It is never correct to hold a ref-counted object in a scoped_ptr<> and it
is never correct to hold a non-refcounted object in a scoped_refptr<>.
Thus no type should ever be held by both scoped_refptr & scoped_ptr.
Additionally to the inviolate rules above, there's also the fact that it is
almost never a good idea to store a raw pointer to a ref-counted object.

libjingle is dictating the API that RtcEncodingVideoCapturerFactory is
implementing, and that API includes ownership transfer into
PeerConnectionFactory, which dictates that the thing whose ownership is
thus transferred not be refcounted.  The real question in my mind is why is
RtcEncodingVideoCapturerFactory refcounted in your CL.
In general you should have very good reasons to make things refcounted;
reasoning about lifecycles/ownership of refcounted objects becomes much
harder than with explicitly-owned objects.

On Tue, Jun 11, 2013 at 5:50 PM, <hshi@chromium.org> wrote:

>
> https://codereview.chromium.**org/16320005/diff/15001/**
>
content/renderer/media/video_**capture_impl_manager.h<https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/video_capture_impl_manager.h>
> File content/renderer/media/video_**capture_impl_manager.h (right):
>
> https://codereview.chromium.**org/16320005/diff/15001/**
>
content/renderer/media/video_**capture_impl_manager.h#**newcode55<https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/video_capture_impl_manager.h#newcode55>
> content/renderer/media/video_**capture_impl_manager.h:55:
> RtcEncodingVideoCapturerFactor**y* encoding_capturer_factory() const {
> On 2013/06/08 00:18:01, Ami Fischman wrote:
>
>> It's almost never a good idea to return a raw pointer to an object
>>
> whose
>
>> lifetime is governed by a scoped_refptr<>.
>>
>
> The problem I'm facing is that the factory pointer is supposed to be
> owned by the PeerConnectionFactory (which is the pc_factory_ member of
> MediaStreamDependencyFactory). In the current libjingle design, factory
> lives in a talk_base::scoped_ptr in pc_factory_. So, it seems wrong to
> also hold encoding_capturer_factory_ in a scoped_refptr of vc_manager_.
> Furthermore, I believe the vc_manager_ outlives the pc_factory_ (the
> latter is owned by MediaStreamDependencyFactory which is outlived by
> vc_manager_), so we need to avoid vc_manager_ using the encoder_factory_
> after pc_factory_ is freed in
> MediaStreamDependencyFactory::**CleanupPeerConnectionFactory()**.
>
> Would the following work?
> (1) vc_manager_ ctor constructs and holds a (not scoped) pointer to
> encoder_factory_, and pass itself to encoder_factory_;
> (2) at CPCF, the encoder_factory_ is passed to the PC factory;
> (3) the dtor of encoder_factory_ notifies vc_manager_ that it is
> destroyed. After that vc_manager_ will clear the pointer and no longer
> call encoder_factory_ when video capturer impls are added/removed.
>
>
https://codereview.chromium.**org/16320005/<https://codereview.chromium.org/1...
>

[Ami GONE FROM CHROMIUM, 2013-06-12 01:44:05]

Comments on PS#6

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/r...
File content/renderer/media/rtc_encoding_video_capturer.cc (right):

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/r...
content/renderer/media/rtc_encoding_video_capturer.cc:17:
media::EncodedVideoSource* encoded_video_source,
doco lifetime

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/r...
content/renderer/media/rtc_encoding_video_capturer.cc:67:
RtcEncodingVideoCapturer::EncodedVideoSourceClient::~EncodedVideoSourceClient()
{
80-cols

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/r...
content/renderer/media/rtc_encoding_video_capturer.cc:208:
encoded_video_source_client_.reset(NULL);
I think you missed:

Are you relying on this moving the refcount to 0?
Wouldn't it be better to 
bitstream_->OnBitstreamRemoved();
before 
bitstream_ = NULL;
?

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/r...
File content/renderer/media/rtc_encoding_video_capturer_factory.cc (right):

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/r...
content/renderer/media/rtc_encoding_video_capturer_factory.cc:20:
STLDeleteElements(&encoder_set_);
I'm surprised you need to delete these explicitly given that they are passed to
libjingle; doesn't libjingle call DestroyVideoEncoder on each of them?
(perhaps this just wants to be
DCHECK(encoder_set_.empty());
?)

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/r...
File content/renderer/media/rtc_encoding_video_capturer_factory.h (right):

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/r...
content/renderer/media/rtc_encoding_video_capturer_factory.h:19: public
base::RefCountedThreadSafe<RtcEncodingVideoCapturerFactory> {
Per convo elsewhere, drop refcounting.

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/r...
content/renderer/media/rtc_encoding_video_capturer_factory.h:29: const OVERRIDE;
fits on previous line now?

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
File content/renderer/media/video_capture_impl.cc (right):

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:110: // Invoke callback immediately
if capabilities is already available.
s/is/are/

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:112: callback_.Run(capabilities_);
Don't you want to clear the callback once it's run.
base::ResetAndReturn(&callback_).Run(capabilities_);

(here and elsewhere)

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:118: base::Unretained(this)));
Is it obvious why Unretained is safe here and below?

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:472: Send(new
EncodedVideoSourceHostMsg_GetCapabilities(device_id_));
Why proxy through this thread just to jump to the io thread next?
(ditto below)

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:532: if (shm->Map(size)) {
what if this fails?
better yet, why not map all SHMs in the map handed to OnOpened once, and avoid
the need to map/unmap them later?

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
File content/renderer/media/video_capture_impl.h (right):

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
content/renderer/media/video_capture_impl.h:75: // media::EncoderVideoSource
interface.
s/er/ed/

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
content/renderer/media/video_capture_impl.h:186: // Bitstream buffers returned
by the video capture device.
s/$/  Unowned./

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
File content/renderer/media/video_capture_impl_manager.cc (right):

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
content/renderer/media/video_capture_impl_manager.cc:22:
encoding_capturer_factory_ = new RtcEncodingVideoCapturerFactory();
Are you planning to hold off landing this CL until all the rest of the machinery
lands (sheu@'s CLs)?  Because if you don't, then this will break the use cases
in question on cros/arm.  I think you want a runtime flag.

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
File content/renderer/media/video_capture_message_filter.cc (right):

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
content/renderer/media/video_capture_message_filter.cc:104:
VideoCaptureMessageFilter::Delegate* VideoCaptureMessageFilter::find_delegate(
This pattern of demultiplexing messages from a single channel based on an id is
precisely what routed messages are for. 
I wonder if there's a good reason why everything is done with control messages
here instead of routed ones.

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
content/renderer/media/video_capture_message_filter.cc:189: std::map<int,
base::SharedMemoryHandle> buffers) {
.h file passes this as const& but here it's passed by copy.
Should pass by const&

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
File content/renderer/media/video_capture_message_filter.h (right):

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
content/renderer/media/video_capture_message_filter.h:52: virtual void
OnCapabilitiesAvailable(
Here and below, the operative word is "encoding", not "bitstream".  Given the
context of the interface I'd expect each of the new methods in the delegate to
include one of Encoder/Encoding/EVS/etc in them.

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
content/renderer/media/video_capture_message_filter.h:58: const std::map<int,
base::SharedMemoryHandle>& buffers) = 0;
How can the EVS impl know how big the SHMs will need to be up-front, before
runtime encoding params are known?
I doubt this can work (without being wasteful by way over-allocating). 
VEA and VDA instead have a Client callback where the {en,de}coder can ask for
buffers of a given size and the client provides them on demand.

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
File content/renderer/media/video_capture_message_filter_unittest.cc (right):

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
content/renderer/media/video_capture_message_filter_unittest.cc:53: // TODO:
implement the following methods for encoded video capture.
I think you missed this comment:

I think this TODO is really 
// TODO(hshi): add tests for EVS functionality.

Which makes me wonder - how will we know we haven't broken this code once it
lands?

https://codereview.chromium.org/16320005/diff/115001/media/video/encoded_vide...
File media/video/encoded_video_source.h (right):

https://codereview.chromium.org/16320005/diff/115001/media/video/encoded_vide...
media/video/encoded_video_source.h:16: // encoder and webcam that has encoding
capabilities.
Add a TODO pointing to http://crbug.com/248334.

https://codereview.chromium.org/16320005/diff/115001/media/video/encoded_vide...
media/video/encoded_video_source.h:21: // After OnStreaming callback stream
shall be considered to be streaming.
I think you missed the point of my comment:

Oh, shall it??
Can you make a pass and cull useless comments and add useful ones where mystery
might exist?

Your response was that you "Got rid of the EVB altogether" but my request was
that you make a pass over the commentary in this CL and make it more useful and
less useless / faux spec'ish.  This particular comment is a shining beacon
example of extremely formal and at the same time extremely pointless.

https://codereview.chromium.org/16320005/diff/115001/media/video/encoded_vide...
media/video/encoded_video_source.h:22: virtual void OnStreaming(const
VideoEncodingParameters& params) = 0;
s/OnStreaming/OnOpened/ to mirror {Open,Close}Bitstream and OnClosed ?

https://codereview.chromium.org/16320005/diff/115001/media/video/encoded_vide...
media/video/encoded_video_source.h:46: // encoded bitstream supported by the
encoder. This call should invoked only
s/should/should be/

https://codereview.chromium.org/16320005/diff/115001/media/video/encoded_vide...
media/video/encoded_video_source.h:54: virtual void OpenBitstream(Client*
client,
I think you missed my comment to the effect of:
here and elsewhere please document lifecycle/ownership semantics when raw
pointers are passed.

https://codereview.chromium.org/16320005/diff/115001/media/video/encoded_vide...
media/video/encoded_video_source.h:55: const VideoEncodingParameters& params) =
0;
indent is off

https://codereview.chromium.org/16320005/diff/160002/media/base/encoded_bitst...
File media/base/encoded_bitstream_buffer.h (right):

https://codereview.chromium.org/16320005/diff/160002/media/base/encoded_bitst...
media/base/encoded_bitstream_buffer.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
I think you missed my comments on this file in PS#1.

[hshi1, 2013-06-12 17:52:39]

https://codereview.chromium.org/16320005/diff/15001/media/base/encoded_bitstr...
File media/base/encoded_bitstream_buffer.h (right):

https://codereview.chromium.org/16320005/diff/15001/media/base/encoded_bitstr...
media/base/encoded_bitstream_buffer.h:17: class EncodedBitstreamBuffer :
On 2013/06/08 00:18:01, Ami Fischman wrote:
> Seems strange to have this time given that media::BitstreamBuffer exists.
> 
> Can you extend that with the metadata you need instead?
> (might not be able to if you really don't want SHM; see below)

I'm leaving this as-is for now. Need to review sheu@'s CL to see how EBB is used
there.

https://codereview.chromium.org/16320005/diff/15001/media/base/encoded_bitstr...
media/base/encoded_bitstream_buffer.h:32: protected:
On 2013/06/08 00:18:01, Ami Fischman wrote:
> private?

Done.

https://codereview.chromium.org/16320005/diff/15001/media/base/encoded_bitstr...
media/base/encoded_bitstream_buffer.h:33: // Destructor that deallocates the
buffers.
On 2013/06/08 00:18:01, Ami Fischman wrote:
> Comment is a lie given the impl.

Removed; but see sheu@'s CL. I believe he intends to re-use this class as a
container for EVB on the brower side also, which creates a situation where the
EVB may or may not own the SHM.

https://codereview.chromium.org/16320005/diff/15001/media/base/encoded_bitstr...
media/base/encoded_bitstream_buffer.h:40: size_t size_;
On 2013/06/08 00:18:01, Ami Fischman wrote:
> IPC guidelines require this piece of data to be explicitly sized in the IPC so
I
> think you're better off making it uint64 everywhere.

Wouldn't an int do? I don't think bitstream buffer is ever going to exceed 32
bits.

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/r...
File content/renderer/media/rtc_encoding_video_capturer.cc (right):

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/r...
content/renderer/media/rtc_encoding_video_capturer.cc:67:
RtcEncodingVideoCapturer::EncodedVideoSourceClient::~EncodedVideoSourceClient()
{
On 2013/06/12 01:44:06, Ami Fischman wrote:
> 80-cols

Done.

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/r...
File content/renderer/media/rtc_encoding_video_capturer_factory.cc (right):

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/r...
content/renderer/media/rtc_encoding_video_capturer_factory.cc:20:
STLDeleteElements(&encoder_set_);
On 2013/06/12 01:44:06, Ami Fischman wrote:
> I'm surprised you need to delete these explicitly given that they are passed
to
> libjingle; doesn't libjingle call DestroyVideoEncoder on each of them?
> (perhaps this just wants to be
> DCHECK(encoder_set_.empty());
> ?)

Done.

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/r...
File content/renderer/media/rtc_encoding_video_capturer_factory.h (right):

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/r...
content/renderer/media/rtc_encoding_video_capturer_factory.h:19: public
base::RefCountedThreadSafe<RtcEncodingVideoCapturerFactory> {
On 2013/06/12 01:44:06, Ami Fischman wrote:
> Per convo elsewhere, drop refcounting.

Done. However I still need a weak_ptr_factory_ for this class (see the new patch
set) as I'm paranoid that GetCapabilities callback may come back after
destruction.

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/r...
content/renderer/media/rtc_encoding_video_capturer_factory.h:29: const OVERRIDE;
On 2013/06/12 01:44:06, Ami Fischman wrote:
> fits on previous line now?

Done.

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
File content/renderer/media/video_capture_impl.cc (right):

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:110: // Invoke callback immediately
if capabilities is already available.
On 2013/06/12 01:44:06, Ami Fischman wrote:
> s/is/are/

Done.

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:112: callback_.Run(capabilities_);
On 2013/06/12 01:44:06, Ami Fischman wrote:
> Don't you want to clear the callback once it's run.
> base::ResetAndReturn(&callback_).Run(capabilities_);
> 
> (here and elsewhere)

Done. Thanks for the suggestion!

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:118: base::Unretained(this)));
On 2013/06/12 01:44:06, Ami Fischman wrote:
> Is it obvious why Unretained is safe here and below?

No it is not obvious. I'm using Unretained only because it is used for the
existing methods such as Start/Stop Capture.

Would you suggest that I use a weak pointer?

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:472: Send(new
EncodedVideoSourceHostMsg_GetCapabilities(device_id_));
On 2013/06/12 01:44:06, Ami Fischman wrote:
> Why proxy through this thread just to jump to the io thread next?
> (ditto below)

I am puzzled as well but all the existing functions seem to go through this
two-step hoops. How about just post the IPC to IO thread directly? That would
get rid of a bunch of DoXxxxOnCaptureThread nonsense. See the new CL.

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:532: if (shm->Map(size)) {
On 2013/06/12 01:44:06, Ami Fischman wrote:
> what if this fails?
> better yet, why not map all SHMs in the map handed to OnOpened once, and avoid
> the need to map/unmap them later?

This wasn't possible with the IPC messages I defined because OnOpened does not
return a |size| argument. But I just looked at sheu@'s review CL and it does
have |size|, so I can indeed map all SHMs there. I'll be modifying the whole CL
to match his IPC definition and ping you for review later today.

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
File content/renderer/media/video_capture_impl.h (right):

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
content/renderer/media/video_capture_impl.h:75: // media::EncoderVideoSource
interface.
On 2013/06/12 01:44:06, Ami Fischman wrote:
> s/er/ed/

Done.

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
content/renderer/media/video_capture_impl.h:186: // Bitstream buffers returned
by the video capture device.
On 2013/06/12 01:44:06, Ami Fischman wrote:
> s/$/  Unowned./

Done.

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
File content/renderer/media/video_capture_impl_manager.cc (right):

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
content/renderer/media/video_capture_impl_manager.cc:22:
encoding_capturer_factory_ = new RtcEncodingVideoCapturerFactory();
On 2013/06/12 01:44:06, Ami Fischman wrote:
> Are you planning to hold off landing this CL until all the rest of the
machinery
> lands (sheu@'s CLs)?  Because if you don't, then this will break the use cases
> in question on cros/arm.  I think you want a runtime flag.

Theoretically this wouldn't "break" cros/arm, because if the browser-side is not
implemented then no encoding capabilities will be reported, thus no caps will be
exposed by the factory, and no external encoder would be created by libjingle,
and we'll never go through the rest of the EVS stuff.

But you're probably right that things could go unpredictably wrong with the
browser-side unimplemented. Will update later today.

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
File content/renderer/media/video_capture_message_filter.cc (right):

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
content/renderer/media/video_capture_message_filter.cc:104:
VideoCaptureMessageFilter::Delegate* VideoCaptureMessageFilter::find_delegate(
On 2013/06/12 01:44:06, Ami Fischman wrote:
> This pattern of demultiplexing messages from a single channel based on an id
is
> precisely what routed messages are for. 
> I wonder if there's a good reason why everything is done with control messages
> here instead of routed ones.

There's no good reason; the existing messages are handled this way and I'm just
following these examples for the sake of uniformity. Would it be ok if we move
to routed messages in another refactor CL?

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
content/renderer/media/video_capture_message_filter.cc:189: std::map<int,
base::SharedMemoryHandle> buffers) {
On 2013/06/12 01:44:06, Ami Fischman wrote:
> .h file passes this as const& but here it's passed by copy.
> Should pass by const&

No the .h passes this as a copy for VCMessageFilter::OnBitstreamOpened. The
const you're referring to is the VCMessageFilter::Delegate::OnBitstreamOpened.

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
File content/renderer/media/video_capture_message_filter.h (right):

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
content/renderer/media/video_capture_message_filter.h:52: virtual void
OnCapabilitiesAvailable(
On 2013/06/12 01:44:06, Ami Fischman wrote:
> Here and below, the operative word is "encoding", not "bitstream".  Given the
> context of the interface I'd expect each of the new methods in the delegate to
> include one of Encoder/Encoding/EVS/etc in them.

Done.

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
content/renderer/media/video_capture_message_filter.h:58: const std::map<int,
base::SharedMemoryHandle>& buffers) = 0;
On 2013/06/12 01:44:06, Ami Fischman wrote:
> How can the EVS impl know how big the SHMs will need to be up-front, before
> runtime encoding params are known?
> I doubt this can work (without being wasteful by way over-allocating). 
> VEA and VDA instead have a Client callback where the {en,de}coder can ask for
> buffers of a given size and the client provides them on demand.

The CL sheu@ uploaded has |size| defined for the OnOpened IPC. In his proposed
model, browser process receives OpenBitstream call with parameters and
determines the buffer size, then returns that along with the encoding params
back to OnOpened.

If you're ok with this (see the other CL sheu@ has just uploaded for your
review) then I'm going to make appropriate changes on the renderer side also.

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
File content/renderer/media/video_capture_message_filter_unittest.cc (right):

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/v...
content/renderer/media/video_capture_message_filter_unittest.cc:53: // TODO:
implement the following methods for encoded video capture.
On 2013/06/12 01:44:06, Ami Fischman wrote:
> I think you missed this comment:
> 
> I think this TODO is really 
> // TODO(hshi): add tests for EVS functionality.
> 
> Which makes me wonder - how will we know we haven't broken this code once it
> lands?

I'm working on the unit tests right now and will update later today.

https://codereview.chromium.org/16320005/diff/115001/media/video/encoded_vide...
File media/video/encoded_video_source.h (right):

https://codereview.chromium.org/16320005/diff/115001/media/video/encoded_vide...
media/video/encoded_video_source.h:16: // encoder and webcam that has encoding
capabilities.
On 2013/06/12 01:44:06, Ami Fischman wrote:
> Add a TODO pointing to http://crbug.com/248334.

Done.

https://codereview.chromium.org/16320005/diff/115001/media/video/encoded_vide...
media/video/encoded_video_source.h:21: // After OnStreaming callback stream
shall be considered to be streaming.
On 2013/06/12 01:44:06, Ami Fischman wrote:
> I think you missed the point of my comment:
> 
> Oh, shall it??
> Can you make a pass and cull useless comments and add useful ones where
mystery
> might exist?
> 
> Your response was that you "Got rid of the EVB altogether" but my request was
> that you make a pass over the commentary in this CL and make it more useful
and
> less useless / faux spec'ish.  This particular comment is a shining beacon
> example of extremely formal and at the same time extremely pointless.

Done.

https://codereview.chromium.org/16320005/diff/115001/media/video/encoded_vide...
media/video/encoded_video_source.h:22: virtual void OnStreaming(const
VideoEncodingParameters& params) = 0;
On 2013/06/12 01:44:06, Ami Fischman wrote:
> s/OnStreaming/OnOpened/ to mirror {Open,Close}Bitstream and OnClosed ?

Done.

https://codereview.chromium.org/16320005/diff/115001/media/video/encoded_vide...
media/video/encoded_video_source.h:46: // encoded bitstream supported by the
encoder. This call should invoked only
On 2013/06/12 01:44:06, Ami Fischman wrote:
> s/should/should be/

Done.

https://codereview.chromium.org/16320005/diff/115001/media/video/encoded_vide...
media/video/encoded_video_source.h:54: virtual void OpenBitstream(Client*
client,
On 2013/06/12 01:44:06, Ami Fischman wrote:
> I think you missed my comment to the effect of:
> here and elsewhere please document lifecycle/ownership semantics when raw
> pointers are passed.

I'm going to make a pass and update later today.

https://codereview.chromium.org/16320005/diff/115001/media/video/encoded_vide...
media/video/encoded_video_source.h:55: const VideoEncodingParameters& params) =
0;
On 2013/06/12 01:44:06, Ami Fischman wrote:
> indent is off

Done. ("const" is aligned with "Client", is that correct?)

[hshi1, 2013-06-12 21:02:14]

PS#11 addresses the following:
(1) Renamed encoded_video_source_messages.h to encoded_video_capture_messages.h;

(2) OnBitstreamOpened passes back vector instead of map;
(3) OnBitstreamOpened passes back buffer size so that VCImpl can map buffers
upfront

[sheu, 2013-06-12 22:35:04]

https://chromiumcodereview.appspot.com/16320005/diff/115001/content/renderer/...
File content/renderer/media/video_capture_message_filter.cc (right):

https://chromiumcodereview.appspot.com/16320005/diff/115001/content/renderer/...
content/renderer/media/video_capture_message_filter.cc:104:
VideoCaptureMessageFilter::Delegate* VideoCaptureMessageFilter::find_delegate(
On 2013/06/12 17:52:39, hshi1 wrote:
> On 2013/06/12 01:44:06, Ami Fischman wrote:
> > This pattern of demultiplexing messages from a single channel based on an id
> is
> > precisely what routed messages are for. 
> > I wonder if there's a good reason why everything is done with control
messages
> > here instead of routed ones.
> 
> There's no good reason; the existing messages are handled this way and I'm
just
> following these examples for the sake of uniformity. Would it be ok if we move
> to routed messages in another refactor CL?

I believe it's due to the fact that routing IDs are generated on the browser
side and not the renderer side for this.  We'd have to make the create call
synchronous for the VideoCaptureMessageFilter::Delegate* to be able to find out
what device id to use.

https://chromiumcodereview.appspot.com/16320005/diff/194001/content/renderer/...
File content/renderer/media/video_capture_impl.cc (right):

https://chromiumcodereview.appspot.com/16320005/diff/194001/content/renderer/...
content/renderer/media/video_capture_impl.cc:148: capabilities_ = capabilities;
This is why we do that little trampoline thing through
capture_message_loop_proxy_.

VideoCaptureMessageFilter is registered on io_message_loop_proxy_, so we get the
callback here on io_message_loop_proxy_, and set capabilities_ accordingly.

At the same time, in RequestCapabilities(), we're accessing capability_ on
another thread, possibly the capture_message_loop_proxy_ (or something else). 
We've got a data race here.

You're going to have to make sure the callback_ also gets called on the correct
thread.

https://chromiumcodereview.appspot.com/16320005/diff/194001/content/renderer/...
content/renderer/media/video_capture_impl.cc:482: bitstream_buffers_.clear();
I think we leak base::SharedMemory objects at this point.  The std::vector<>
does not claim ownership of its buffers and does not destroy them.

(discussion continued below)

https://chromiumcodereview.appspot.com/16320005/diff/194001/content/renderer/...
content/renderer/media/video_capture_impl.cc:499: buffer_id,
(uint8*)shm->memory(), size, metadata);
What are the lifetime semantics of the shared memory backing the
EncodedBitstreamBuffer?

The way EVS was spec-ed, the EncodedBitstreamBuffer represented one invariant
piece of the encoded bitstream held by a refcounted object.  Clients of EVS
would get EncodedBitstreamBuffers for to use, and the refcounting would take
care of notifying the EVS when the EBB was destroyed, without having to
explicitly return the EBB to the EVS.  In our particular case, when the last
reference is dropped, then the internal shared memory buffer can be recycled,
and the empty buffer can be returned to the browser.

The way EVS and EBB are implemented here, it's possible for some observer's view
of an EBB's underlying memory contents to change, if the EBB is passed returned
back to the EVS while there are still outstanding references elsewhere on the
EBB.  I think this is a little bit outside the spirit of the fact that EBB is a
refcounted object.

https://chromiumcodereview.appspot.com/16320005/diff/194001/content/renderer/...
File content/renderer/media/video_capture_message_filter.cc (right):

https://chromiumcodereview.appspot.com/16320005/diff/194001/content/renderer/...
content/renderer/media/video_capture_message_filter.cc:106:
VideoCaptureMessageFilter::Delegate* delegate = NULL;
Can we make "delegate" the iterator type, and just do this find() once?

[hshi1, 2013-06-13 00:25:31]

https://codereview.chromium.org/16320005/diff/194001/content/renderer/media/v...
File content/renderer/media/video_capture_impl.cc (right):

https://codereview.chromium.org/16320005/diff/194001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:148: capabilities_ = capabilities;
On 2013/06/12 22:35:05, sheu wrote:
> This is why we do that little trampoline thing through
> capture_message_loop_proxy_.
> 
I've changed this so that we always update capabilities_ and invoke callback on
the capture thread. However I believe that for the renderer->browser IPCs such
as Open/Close Bistream and TrySetConfig, it is safe to directly post IPC to IO
thread.

https://codereview.chromium.org/16320005/diff/194001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:482: bitstream_buffers_.clear();
On 2013/06/12 22:35:05, sheu wrote:
> I think we leak base::SharedMemory objects at this point.  The std::vector<>
> does not claim ownership of its buffers and does not destroy them.
> 
> (discussion continued below)

Done.

https://codereview.chromium.org/16320005/diff/194001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:499: buffer_id,
(uint8*)shm->memory(), size, metadata);
On 2013/06/12 22:35:05, sheu wrote:
> What are the lifetime semantics of the shared memory backing the
> EncodedBitstreamBuffer?
> 
> The way EVS was spec-ed, the EncodedBitstreamBuffer represented one invariant
> piece of the encoded bitstream held by a refcounted object.  Clients of EVS
> would get EncodedBitstreamBuffers for to use, and the refcounting would take
> care of notifying the EVS when the EBB was destroyed, without having to
> explicitly return the EBB to the EVS.  In our particular case, when the last
> reference is dropped, then the internal shared memory buffer can be recycled,
> and the empty buffer can be returned to the browser.
> 
> The way EVS and EBB are implemented here, it's possible for some observer's
view
> of an EBB's underlying memory contents to change, if the EBB is passed
returned
> back to the EVS while there are still outstanding references elsewhere on the
> EBB.  I think this is a little bit outside the spirit of the fact that EBB is
a
> refcounted object.

The EBB object is passed linearly from one point to another and never has a
refcount greater than 1. It is first created here to be and then passed to the
client (RtcEncodingVideoCapturer::EncodedVideoSourceClient::OnBufferReady()),
where the raw pointer to the mapped buffer is passed to webrtc via the
synchronous Encoded callback. Webrtc copies (with necesssary packetiziation) the
buffer content and no longer needs the buffer when the callback returns, so the
client calls evs->ReturnBitstreamBuffer at the end of OnBufferReady(). The EBB
will be released at the end of VideoCaptureImpl::ReturnBitstreamBuffer where its
refcount is back to 0.

[hshi1, 2013-06-13 00:46:52]

Updated IPC definitions for GetCaps and OpenBistream (added session_id).

https://codereview.chromium.org/16320005/diff/194001/content/renderer/media/v...
File content/renderer/media/video_capture_message_filter.cc (right):

https://codereview.chromium.org/16320005/diff/194001/content/renderer/media/v...
content/renderer/media/video_capture_message_filter.cc:106:
VideoCaptureMessageFilter::Delegate* delegate = NULL;
On 2013/06/12 22:35:05, sheu wrote:
> Can we make "delegate" the iterator type, and just do this find() once?

Done.

[sheu, 2013-06-13 05:36:50]

https://chromiumcodereview.appspot.com/16320005/diff/194001/content/renderer/...
File content/renderer/media/video_capture_impl.cc (right):

https://chromiumcodereview.appspot.com/16320005/diff/194001/content/renderer/...
content/renderer/media/video_capture_impl.cc:499: buffer_id,
(uint8*)shm->memory(), size, metadata);
On 2013/06/13 00:25:31, hshi1 wrote:
> On 2013/06/12 22:35:05, sheu wrote:
> > What are the lifetime semantics of the shared memory backing the
> > EncodedBitstreamBuffer?
> > 
> > The way EVS was spec-ed, the EncodedBitstreamBuffer represented one
invariant
> > piece of the encoded bitstream held by a refcounted object.  Clients of EVS
> > would get EncodedBitstreamBuffers for to use, and the refcounting would take
> > care of notifying the EVS when the EBB was destroyed, without having to
> > explicitly return the EBB to the EVS.  In our particular case, when the last
> > reference is dropped, then the internal shared memory buffer can be
recycled,
> > and the empty buffer can be returned to the browser.
> > 
> > The way EVS and EBB are implemented here, it's possible for some observer's
> view
> > of an EBB's underlying memory contents to change, if the EBB is passed
> returned
> > back to the EVS while there are still outstanding references elsewhere on
the
> > EBB.  I think this is a little bit outside the spirit of the fact that EBB
is
> a
> > refcounted object.
> 
> The EBB object is passed linearly from one point to another and never has a
> refcount greater than 1. It is first created here to be and then passed to the
> client (RtcEncodingVideoCapturer::EncodedVideoSourceClient::OnBufferReady()),
> where the raw pointer to the mapped buffer is passed to webrtc via the
> synchronous Encoded callback. Webrtc copies (with necesssary packetiziation)
the
> buffer content and no longer needs the buffer when the callback returns, so
the
> client calls evs->ReturnBitstreamBuffer at the end of OnBufferReady(). The EBB
> will be released at the end of VideoCaptureImpl::ReturnBitstreamBuffer where
its
> refcount is back to 0.

I'm confident that you've got the lifetime nailed down so we don't have problems
with this use model.  My concern is only that it doesn't seem to be as robust as
the initial design for EVS.

Though since our use of EVS is pretty dirty here anyways, I suppose we can let
this slide.

https://chromiumcodereview.appspot.com/16320005/diff/212001/content/renderer/...
File content/renderer/media/video_capture_impl.cc (right):

https://chromiumcodereview.appspot.com/16320005/diff/212001/content/renderer/...
content/renderer/media/video_capture_impl.cc:107: const
media::EncodedVideoSource::RequestCapabilitiesCallback& callback) {
We're assuming that this is called always on capture_message_loop_proxy_, since
it modifies callback_.  Can you confirm this and put in a DCHECK here to be
safe?

https://chromiumcodereview.appspot.com/16320005/diff/212001/content/renderer/...
content/renderer/media/video_capture_impl.cc:121: void
VideoCaptureImpl::OpenBitstream(
We're assuming that this is called always on capture_message_loop_proxy_, since
it modifies client_.  Can you confirm this and put in a DCHECK here to be safe?

https://chromiumcodereview.appspot.com/16320005/diff/212001/content/renderer/...
content/renderer/media/video_capture_impl.cc:130: void
VideoCaptureImpl::CloseBitstream() {
This and other entry points that don't require trampolining -- can you add a
comment to this effect?  e.g. "No trampolining necessary -- no state changed.".

[hshi1, 2013-06-13 19:18:46]

Uploaded PS#14 with buffer sizes changed to uint32.

Re:sheu@ - unfortunately the DCHECK for capture thread failed. It seems we're
indeed getting RequestCapabilities and Open/CloseBitstream calls on non-capture
threads, although I haven't determined which thread it is. In light of this, I'm
adding back the trampolining for these.

https://codereview.chromium.org/16320005/diff/212001/content/renderer/media/v...
File content/renderer/media/video_capture_impl.cc (right):

https://codereview.chromium.org/16320005/diff/212001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:107: const
media::EncodedVideoSource::RequestCapabilitiesCallback& callback) {
On 2013/06/13 05:36:51, sheu wrote:
> We're assuming that this is called always on capture_message_loop_proxy_,
since
> it modifies callback_.  Can you confirm this and put in a DCHECK here to be
> safe?

I'm afraid that the DCHECK failed. During my testing, I see this called from a
different thread (not sure if IO or other). Thus I do have to post this to
capture thread.

https://codereview.chromium.org/16320005/diff/212001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:121: void
VideoCaptureImpl::OpenBitstream(
On 2013/06/13 05:36:51, sheu wrote:
> We're assuming that this is called always on capture_message_loop_proxy_,
since
> it modifies client_.  Can you confirm this and put in a DCHECK here to be
safe?

I'm afraid that the DCHECK failed. I see this called from a different thread
(not sure if IO or other). Thus I do have to post this to capture thread.

https://codereview.chromium.org/16320005/diff/212001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:130: void
VideoCaptureImpl::CloseBitstream() {
On 2013/06/13 05:36:51, sheu wrote:
> This and other entry points that don't require trampolining -- can you add a
> comment to this effect?  e.g. "No trampolining necessary -- no state
changed.".

I'm posting this to capture thread as well, because it changes state (clears
client_ pointer). The rest of the entry points are fine.

[Ami GONE FROM CHROMIUM, 2013-06-13 19:33:04]

On Thu, Jun 13, 2013 at 12:18 PM, <hshi@chromium.org> wrote:

> Re:sheu@ - unfortunately the DCHECK for capture thread failed. It seems
> we're
> indeed getting RequestCapabilities and Open/CloseBitstream calls on
> non-capture
> threads, although I haven't determined which thread it is. In light of
> this, I'm
> adding back the trampolining for these.
>

Trampolining as a strategy is fine when you know where you're being called
from and that those are legit callpaths.
Trampolining as a strategy for not understanding how you're being called is
a great way to hide silly bugs elsewhere that cause unnecessary jank &
latency.
I recommend figuring out where the calls are coming from and whether they
are legit.

[hshi1, 2013-06-13 19:33:40]

Note: ronghuawu@ just confirmed that he has rolled libjingle DEPS this morning
to r342. So I'm merging the factory injection CL into this one and updated the
description. Please see PS#15.

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

On 2013/06/13 19:33:04, Ami Fischman wrote:
> On Thu, Jun 13, 2013 at 12:18 PM, <mailto:hshi@chromium.org> wrote:
> 
> > Re:sheu@ - unfortunately the DCHECK for capture thread failed. It seems
> > we're
> > indeed getting RequestCapabilities and Open/CloseBitstream calls on
> > non-capture
> > threads, although I haven't determined which thread it is. In light of
> > this, I'm
> > adding back the trampolining for these.
> >
> 
> Trampolining as a strategy is fine when you know where you're being called
> from and that those are legit callpaths.
> Trampolining as a strategy for not understanding how you're being called is
> a great way to hide silly bugs elsewhere that cause unnecessary jank &
> latency.
> I recommend figuring out where the calls are coming from and whether they
> are legit.

Ami, these are legit callpaths. The RtcEncodingVideoCapturerFactory and
RtcEncodingVideoCapturer are
driven by libjingle (specifically the WebRtcVideoEngine and related media
channels) which run on the
libjingle working thread, that is neither capture thread nor io thread. Both
OpenBitstream and CloseBitstream
will be run on the working thread (corresponding to the
WebRtcVideoEncoder::InitEncode and WebRtcVideoEncoder::Release
calls).

[hshi1, 2013-06-13 22:03:46]

Ami, PTAL at PS#16. I believe this has clarified questions on the lifetime of
the RTC encoding capturer factory and the EVS client.

The encoding capturer factory is owned by the PeerConnectionFactory thus its
lifetime is limited by the PC factory. VCImplManager outlives it. I propose to
pass a weak pointer to the VCImplManager.

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
File content/renderer/media/video_capture_impl_manager.h (right):

https://codereview.chromium.org/16320005/diff/15001/content/renderer/media/vi...
content/renderer/media/video_capture_impl_manager.h:55:
RtcEncodingVideoCapturerFactory* encoding_capturer_factory() const {
On 2013/06/08 00:18:01, Ami Fischman wrote:
> It's almost never a good idea to return a raw pointer to an object whose
> lifetime is governed by a scoped_refptr<>.

I've made the following change:
(1) the encoding_capturer_factory is now created by
MediaStreamDependencyFactory, then passed to and owned by its
PeerConnectionFactory;
(2) VCImplManager receives a weak pointer to the factory. VCImplManager will
outlive the PeerConnectionFactory.

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/r...
File content/renderer/media/rtc_encoding_video_capturer.cc (right):

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/r...
content/renderer/media/rtc_encoding_video_capturer.cc:17:
media::EncodedVideoSource* encoded_video_source,
On 2013/06/12 01:44:06, Ami Fischman wrote:
> doco lifetime

Done.

https://codereview.chromium.org/16320005/diff/115001/content/renderer/media/r...
content/renderer/media/rtc_encoding_video_capturer.cc:208:
encoded_video_source_client_.reset(NULL);
On 2013/06/12 01:44:06, Ami Fischman wrote:
> I think you missed:
> 
> Are you relying on this moving the refcount to 0?
> Wouldn't it be better to 
> bitstream_->OnBitstreamRemoved();
> before 
> bitstream_ = NULL;
> ?

This will invoke the dtor of the EVSClient (see
EncodedVideoSourceClient::~EncodedVideoSourceClient()) which will invoke
EVS->CloseBitstream(), prior to resetting the pointer to NULL.

[Ami GONE FROM CHROMIUM, 2013-06-13 22:56:57]

Reviewed PS#16

https://codereview.chromium.org/16320005/diff/248001/content/renderer/media/m...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/16320005/diff/248001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory.cc:499:
RtcEncodingVideoCapturerFactory* rtc_encoding_capturer_factory =
Needs to be flag-protected.

https://codereview.chromium.org/16320005/diff/248001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory.cc:501: if
(rtc_encoding_capturer_factory) {
this can never fail

https://codereview.chromium.org/16320005/diff/248001/content/renderer/media/r...
File content/renderer/media/rtc_encoding_video_capturer.cc (right):

https://codereview.chromium.org/16320005/diff/248001/content/renderer/media/r...
content/renderer/media/rtc_encoding_video_capturer.cc:68: ) {
interesting wrap

https://codereview.chromium.org/16320005/diff/248001/content/renderer/media/v...
File content/renderer/media/video_capture_impl.cc (right):

https://codereview.chromium.org/16320005/diff/248001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:110: base::Unretained(this),
callback));
Yowza.  So, Unretained(this) is safe (even though the I/O thread is scoped to
the renderer process and the capture_message_loop_proxy_ thread is scoped to the
VideoCaptureImplManager) because VideoCaptureImplManager only triggers deletion
of its VideoCaptureImpl's by calling DeInit which detours through the capture &
I/O threads, so as long as nobody posts tasks after the DeInit() call is made,
it is guaranteed none of these Unretained posted tasks will dangle after the
delete goes through.

https://codereview.chromium.org/16320005/diff/248001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:527: if (buffer_id >= 0 &&
buffer_id < (int)bitstream_buffers_.size()) {
s/(int)/static_cast<int>/ but that risks overflow; better to go the other way
once you assure non-negativity:
buffer_id >= 0 && static_cast<size_t>(buffer_id) < bitstream_buffers_.size()

https://codereview.chromium.org/16320005/diff/248001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:529: if (shm && shm->Map(size)) {
shm can't be NULL here.

https://codereview.chromium.org/16320005/diff/248001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:529: if (shm && shm->Map(size)) {
You test shm->Map() for failing but you don't do anything with that fact.  I
think CHECK-failing would be a fine thing to do here.

https://codereview.chromium.org/16320005/diff/248001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:534: shm->Unmap();
Is it clear why you want to Map & Unmap on every bitstreambuffer delivery?
ISTM much better to Map each SHM once in DoNotifyBitstreamOpenedOnCaptureThread
and then Unmap each of them once in DoNotifyBitstreamClosedOnCaptureThread.

https://codereview.chromium.org/16320005/diff/240007/media/video/encoded_vide...
File media/video/encoded_video_source.h (right):

https://codereview.chromium.org/16320005/diff/240007/media/video/encoded_vide...
media/video/encoded_video_source.h:48: // only once. EncodedVideoSource will
invoke |callback| when capabilities
Given this should be called only once, would it make more sense to always return
Capabilities unconditionally in Client::OnOpened?

https://codereview.chromium.org/16320005/diff/240007/media/video/encoded_vide...
media/video/encoded_video_source.h:55: virtual void OpenBitstream(Client*
client,
doco lifetime of |client|
(probably the answer is that it must outlive the call to OnClosed()).

[hshi1, 2013-06-13 23:33:40]

PTAL @ PS#19, thanks!

https://codereview.chromium.org/16320005/diff/248001/content/renderer/media/m...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/16320005/diff/248001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory.cc:499:
RtcEncodingVideoCapturerFactory* rtc_encoding_capturer_factory =
On 2013/06/13 22:56:58, Ami Fischman wrote:
> Needs to be flag-protected.

Do you still think we need a runtime switch (is #ifdef CrOS+ARM not acceptable?)

https://codereview.chromium.org/16320005/diff/248001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory.cc:501: if
(rtc_encoding_capturer_factory) {
On 2013/06/13 22:56:58, Ami Fischman wrote:
> this can never fail

Done.

https://codereview.chromium.org/16320005/diff/248001/content/renderer/media/r...
File content/renderer/media/rtc_encoding_video_capturer.cc (right):

https://codereview.chromium.org/16320005/diff/248001/content/renderer/media/r...
content/renderer/media/rtc_encoding_video_capturer.cc:68: ) {
On 2013/06/13 22:56:58, Ami Fischman wrote:
> interesting wrap

Done.

https://codereview.chromium.org/16320005/diff/248001/content/renderer/media/v...
File content/renderer/media/video_capture_impl.cc (right):

https://codereview.chromium.org/16320005/diff/248001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:110: base::Unretained(this),
callback));
On 2013/06/13 22:56:58, Ami Fischman wrote:
> Yowza.  So, Unretained(this) is safe (even though the I/O thread is scoped to
> the renderer process and the capture_message_loop_proxy_ thread is scoped to
the
> VideoCaptureImplManager) because VideoCaptureImplManager only triggers
deletion
> of its VideoCaptureImpl's by calling DeInit which detours through the capture
&
> I/O threads, so as long as nobody posts tasks after the DeInit() call is made,
> it is guaranteed none of these Unretained posted tasks will dangle after the
> delete goes through.

Ok so I assume there's no action item for me then.

https://codereview.chromium.org/16320005/diff/248001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:527: if (buffer_id >= 0 &&
buffer_id < (int)bitstream_buffers_.size()) {
On 2013/06/13 22:56:58, Ami Fischman wrote:
> s/(int)/static_cast<int>/ but that risks overflow; better to go the other way
> once you assure non-negativity:
> buffer_id >= 0 && static_cast<size_t>(buffer_id) < bitstream_buffers_.size()

Done.

https://codereview.chromium.org/16320005/diff/248001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:529: if (shm && shm->Map(size)) {
On 2013/06/13 22:56:58, Ami Fischman wrote:
> shm can't be NULL here.

Done.

https://codereview.chromium.org/16320005/diff/248001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:529: if (shm && shm->Map(size)) {
On 2013/06/13 22:56:58, Ami Fischman wrote:
> You test shm->Map() for failing but you don't do anything with that fact.  I
> think CHECK-failing would be a fine thing to do here.

Done.

https://codereview.chromium.org/16320005/diff/248001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:534: shm->Unmap();
On 2013/06/13 22:56:58, Ami Fischman wrote:
> Is it clear why you want to Map & Unmap on every bitstreambuffer delivery?
> ISTM much better to Map each SHM once in
DoNotifyBitstreamOpenedOnCaptureThread
> and then Unmap each of them once in DoNotifyBitstreamClosedOnCaptureThread.

Done. (Notice: no need to Unmap on close - bitstream_buffers[i]->Close()
automatically does Unmap).

https://codereview.chromium.org/16320005/diff/240007/media/video/encoded_vide...
File media/video/encoded_video_source.h (right):

https://codereview.chromium.org/16320005/diff/240007/media/video/encoded_vide...
media/video/encoded_video_source.h:48: // only once. EncodedVideoSource will
invoke |callback| when capabilities
On 2013/06/13 22:56:58, Ami Fischman wrote:
> Given this should be called only once, would it make more sense to always
return
> Capabilities unconditionally in Client::OnOpened?

Unfortunately Client::OnOpened is too late for passing capabilities. The caps
needs to be sent to the factory very early (before any encoded bitstream is
opened), because factory reports its supported codecs list to libjingle based on
the caps, and the that codec list determines the list of SendCodecs that is
signalled in the SDP header. No caps, no send codecs, and we won't have a
generic codec listed in the offer sent to the receiver end.

https://codereview.chromium.org/16320005/diff/240007/media/video/encoded_vide...
media/video/encoded_video_source.h:55: virtual void OpenBitstream(Client*
client,
On 2013/06/13 22:56:58, Ami Fischman wrote:
> doco lifetime of |client|
> (probably the answer is that it must outlive the call to OnClosed()).

Well the answer is more complicated. The lifetime of |client| is limited to the
owner, encoding capturer factory (due to the scoped_ptr) but it may be released
earlier. In the current design, it may or may not outlive the call to
OnClosed().

Case (1): bitstream is closed upon request of libjingle (coming from
WebRtcVideoEncoder::Release): this invokes the dtor of client, which calls
EVS->CloseBitstream(). Note that EVS->CloseBitstream() immedialy sets its
client_ pointer to NULL, so when the IPC roundtrip comes back, client_ will not
be notified of OnClosed().

Case (2): bitstream is closed by the browser process (may be due to an error for
example). In this case, renderer process never requested CloseBitstream, but got
an OnBitstreamClosed message. This will cause EVS to call EVSClient::OnClosed
(where we set finished_ to true), and then EVS will set its client_ to NULL.

[hshi1, 2013-06-14 18:18:33]

PTAL at PS#21. This updates the definition of EncodedBitstreamBuffer to match
that of sheu@'s CL (15906019).

Note: for the |size| argument I propose to use uint32 instead of size_t.

[hshi1, 2013-06-15 00:02:11]

PTAL at PS#23. Per offline discussion, I've removed the static guard (#if
defined(ARCH_CPU_ARMEL)) and defined a runtime switch to control the encoded
capture behavior.

[Ami GONE FROM CHROMIUM, 2013-06-18 20:56:27]

LGTM % nits (and you have red bots)

https://codereview.chromium.org/16320005/diff/248001/content/renderer/media/v...
File content/renderer/media/video_capture_impl.cc (right):

https://codereview.chromium.org/16320005/diff/248001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:110: base::Unretained(this),
callback));
On 2013/06/13 23:33:40, hshi1 wrote:
> On 2013/06/13 22:56:58, Ami Fischman wrote:
> > Yowza.  So, Unretained(this) is safe (even though the I/O thread is scoped
to
> > the renderer process and the capture_message_loop_proxy_ thread is scoped to
> the
> > VideoCaptureImplManager) because VideoCaptureImplManager only triggers
> deletion
> > of its VideoCaptureImpl's by calling DeInit which detours through the
capture
> &
> > I/O threads, so as long as nobody posts tasks after the DeInit() call is
made,
> > it is guaranteed none of these Unretained posted tasks will dangle after the
> > delete goes through.
> 
> Ok so I assume there's no action item for me then.

If you felt like adding this comment to the class header comments that talk
about threading I think that would be an improvement.  Otherwise I'll do it.

https://codereview.chromium.org/16320005/diff/375001/content/renderer/media/v...
File content/renderer/media/video_capture_impl.h (right):

https://codereview.chromium.org/16320005/diff/375001/content/renderer/media/v...
content/renderer/media/video_capture_impl.h:195: bool bitstream_opened_;
s/ed// since this is temporal (once closed, it is still true that the bitstream
_was_ opened, but it is no longer true that it _is_ open).

https://codereview.chromium.org/16320005/diff/375001/media/base/encoded_bitst...
File media/base/encoded_bitstream_buffer.h (right):

https://codereview.chromium.org/16320005/diff/375001/media/base/encoded_bitst...
media/base/encoded_bitstream_buffer.h:26: const base::Closure&
destroy_callback);
fwiw media code usually names this sort of thing with _cb, not with _callback.

[hshi1, 2013-06-18 23:01:19]

Thanks! I've uploaded PS#27 and re-running trybots.

Adding additional owners for review:
darin@: ipc/
piman@: content/

Thanks!

https://codereview.chromium.org/16320005/diff/248001/content/renderer/media/v...
File content/renderer/media/video_capture_impl.cc (right):

https://codereview.chromium.org/16320005/diff/248001/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:110: base::Unretained(this),
callback));
On 2013/06/18 20:56:27, Ami Fischman wrote:
> On 2013/06/13 23:33:40, hshi1 wrote:
> > On 2013/06/13 22:56:58, Ami Fischman wrote:
> > > Yowza.  So, Unretained(this) is safe (even though the I/O thread is scoped
> to
> > > the renderer process and the capture_message_loop_proxy_ thread is scoped
to
> > the
> > > VideoCaptureImplManager) because VideoCaptureImplManager only triggers
> > deletion
> > > of its VideoCaptureImpl's by calling DeInit which detours through the
> capture
> > &
> > > I/O threads, so as long as nobody posts tasks after the DeInit() call is
> made,
> > > it is guaranteed none of these Unretained posted tasks will dangle after
the
> > > delete goes through.
> > 
> > Ok so I assume there's no action item for me then.
> 
> If you felt like adding this comment to the class header comments that talk
> about threading I think that would be an improvement.  Otherwise I'll do it.

I'd feel more comfortable if you add the appropriate comments. I'm not confident
that I'll be able to phrase it correctly and concisely. Thanks!

https://codereview.chromium.org/16320005/diff/375001/content/renderer/media/v...
File content/renderer/media/video_capture_impl.h (right):

https://codereview.chromium.org/16320005/diff/375001/content/renderer/media/v...
content/renderer/media/video_capture_impl.h:195: bool bitstream_opened_;
On 2013/06/18 20:56:27, Ami Fischman wrote:
> s/ed// since this is temporal (once closed, it is still true that the
bitstream
> _was_ opened, but it is no longer true that it _is_ open).

Done.

https://codereview.chromium.org/16320005/diff/375001/media/base/encoded_bitst...
File media/base/encoded_bitstream_buffer.h (right):

https://codereview.chromium.org/16320005/diff/375001/media/base/encoded_bitst...
media/base/encoded_bitstream_buffer.h:26: const base::Closure&
destroy_callback);
On 2013/06/18 20:56:27, Ami Fischman wrote:
> fwiw media code usually names this sort of thing with _cb, not with _callback.

Done.

[hshi1, 2013-06-19 01:25:27]

Sorry I just realized that security review is required for
ipc/ipc_message_start.h.

+cevans@, palmer@ - PTAL ipc/ipc_message_start.h, thanks!

[piman, 2013-06-19 18:23:58]

https://codereview.chromium.org/16320005/diff/389002/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/16320005/diff/389002/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:912:
switches::kEnableEncodedScreenCapture,
Also needs to go in kForwardSwitches in
chrome/browser/chromeos/login/chrome_restart_request.cc so that it also works in
guest mode.

https://codereview.chromium.org/16320005/diff/389002/content/common/media/enc...
File content/common/media/encoded_video_capture_messages.h (right):

https://codereview.chromium.org/16320005/diff/389002/content/common/media/enc...
content/common/media/encoded_video_capture_messages.h:51:
media::VideoCaptureSessionId /* session_id */)
Where is the browser side of all these messages?
It's hard to review in a vacuum.

[hshi1, 2013-06-19 18:25:43]

https://codereview.chromium.org/16320005/diff/389002/content/common/media/enc...
File content/common/media/encoded_video_capture_messages.h (right):

https://codereview.chromium.org/16320005/diff/389002/content/common/media/enc...
content/common/media/encoded_video_capture_messages.h:51:
media::VideoCaptureSessionId /* session_id */)
On 2013/06/19 18:23:58, piman wrote:
> Where is the browser side of all these messages?
> It's hard to review in a vacuum.

The browser side are implemented in a separate CL by sheu@
(https://codereview.chromium.org/15906019/ currently under review)

[palmer, 2013-06-19 20:24:08]

LGTM % nits, and a type/design question.

https://codereview.chromium.org/16320005/diff/389002/content/common/media/enc...
File content/common/media/encoded_video_capture_messages.h (right):

https://codereview.chromium.org/16320005/diff/389002/content/common/media/enc...
content/common/media/encoded_video_capture_messages.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
2013

https://codereview.chromium.org/16320005/diff/389002/content/common/media/enc...
content/common/media/encoded_video_capture_messages.h:96: // occurs, bitstream
can be considered to be streaming, and Renderer should be
Nit: I would rather not capitalize "renderer", and I would capitalize "IDs".

https://codereview.chromium.org/16320005/diff/389002/content/renderer/media/v...
File content/renderer/media/video_capture_impl.cc (right):

https://codereview.chromium.org/16320005/diff/389002/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:561: static_cast<size_t>(buffer_id)
< bitstream_buffers_.size()) {
As written this looks OK, but it strongly suggests that |buffer_id| should be a
uint32_t, uint16_t, uint64_t, or something like that.

Alternately, or in addition, use std::vector::at, if testing shows that it
really is bounds-checked on all platforms. (Josuttis claims that such checks are
part of the interface, but I have my doubts about some of the implementations we
have to use.)

Alternately, use a std::map for |bitstream_buffers_| (but I like the vector, as
long as there is a sanity check on the index).

[hshi1, 2013-06-19 21:16:24]

https://codereview.chromium.org/16320005/diff/389002/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/16320005/diff/389002/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:912:
switches::kEnableEncodedScreenCapture,
On 2013/06/19 18:23:58, piman wrote:
> Also needs to go in kForwardSwitches in
> chrome/browser/chromeos/login/chrome_restart_request.cc so that it also works
in
> guest mode.

Done.

https://codereview.chromium.org/16320005/diff/389002/content/common/media/enc...
File content/common/media/encoded_video_capture_messages.h (right):

https://codereview.chromium.org/16320005/diff/389002/content/common/media/enc...
content/common/media/encoded_video_capture_messages.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
On 2013/06/19 20:24:08, Chromium Palmer wrote:
> 2013

Done.

https://codereview.chromium.org/16320005/diff/389002/content/common/media/enc...
content/common/media/encoded_video_capture_messages.h:96: // occurs, bitstream
can be considered to be streaming, and Renderer should be
On 2013/06/19 20:24:08, Chromium Palmer wrote:
> Nit: I would rather not capitalize "renderer", and I would capitalize "IDs".

Done.

https://codereview.chromium.org/16320005/diff/389002/content/renderer/media/v...
File content/renderer/media/video_capture_impl.cc (right):

https://codereview.chromium.org/16320005/diff/389002/content/renderer/media/v...
content/renderer/media/video_capture_impl.cc:561: static_cast<size_t>(buffer_id)
< bitstream_buffers_.size()) {
On 2013/06/19 20:24:08, Chromium Palmer wrote:
> As written this looks OK, but it strongly suggests that |buffer_id| should be
a
> uint32_t, uint16_t, uint64_t, or something like that.
> 
> Alternately, or in addition, use std::vector::at, if testing shows that it
> really is bounds-checked on all platforms. (Josuttis claims that such checks
are
> part of the interface, but I have my doubts about some of the implementations
we
> have to use.)
> 
> Alternately, use a std::map for |bitstream_buffers_| (but I like the vector,
as
> long as there is a sanity check on the index).

I'd opt for using std::vector::at.

[hshi1, 2013-06-19 23:21:51]

piman@ - since sheu@'s OOTO, I don't think the browser-side CL can land very
soon. Are you ok with me landing the renderer side or do you want me to hold on
until R29 branches? Thanks.

[hshi1, 2013-06-28 20:51:17]

piman@ - ping again... now that R29 is branched can I land this? Thanks.

[piman, 2013-07-01 23:05:34]

LGTM if Ami is ok with the extra IPC surface - I'll redirect further CLs in the
area to him.

[Ami GONE FROM CHROMIUM, 2013-07-01 23:09:18]

On 2013/07/01 23:05:34, piman wrote:
> LGTM if Ami is ok with the extra IPC surface - I'll redirect further CLs in
the
> area to him.

If you're talking about the overlap between EVS & VEA,
https://code.google.com/p/chromium/issues/detail?id=248334 tracks dropping EVS
(TODO'd in this CL).
If you're talking about something else, let me know ;)

[piman, 2013-07-01 23:20:57]

On Mon, Jul 1, 2013 at 4:09 PM, <fischman@chromium.org> wrote:

> On 2013/07/01 23:05:34, piman wrote:
>
>> LGTM if Ami is ok with the extra IPC surface - I'll redirect further CLs
>> in
>>
> the
>
>> area to him.
>>
>
> If you're talking about the overlap between EVS & VEA,
>
https://code.google.com/p/**chromium/issues/detail?id=**248334<https://code.g...
dropping EVS
> (TODO'd in this CL).
> If you're talking about something else, let me know ;)
>

I mean the extra set of IPCs in this CL. I haven't attempted to understand
how it all works across the many CLs. If you do, then good.


>
>
https://codereview.chromium.**org/16320005/<https://codereview.chromium.org/1...
>

[commit-bot: I haz the power, 2013-07-01 23:28:16]

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

[commit-bot: I haz the power, 2013-07-02 01:01:04]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-07-02 02:28:45]

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

[commit-bot: I haz the power, 2013-07-02 03:20:04]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-07-02 16:47:24]

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

[commit-bot: I haz the power, 2013-07-02 20:03:44]

Change committed as 209760

[Ami GONE FROM CHROMIUM, 2013-07-31 19:24:59]

Since the browser side of EVS never landed and sheu@ is not planning to land it
(favoring instead to land VEA support that replaces EVS), I believe this is all
dead code that will never be revived.  Can you remove it?

[hshi1, 2013-07-31 19:47:54]

On 2013/07/31 19:24:59, Ami Fischman wrote:
> Since the browser side of EVS never landed and sheu@ is not planning to land
it
> (favoring instead to land VEA support that replaces EVS), I believe this is
all
> dead code that will never be revived.  Can you remove it?

Yes it is on my TODO list. I just talked to sheu@ and he'd like to keep the
files around a bit longer until he makes sure all the VEA support gets landed.
Then I'll just delete all the EVS stuff.

[Ami GONE FROM CHROMIUM, 2013-07-31 20:34:37]

I don't understand this logic.
If VEA fails to land for some reason, we can revert the delete of EVS.
If VEA lands as expected, EVS will just be noise for an extra few weeks.
We should strongly strive to minimize the codebase.  Dead code is an easy
decision to make to remove.


On Wed, Jul 31, 2013 at 12:47 PM, <hshi@chromium.org> wrote:

> On 2013/07/31 19:24:59, Ami Fischman wrote:
>
>> Since the browser side of EVS never landed and sheu@ is not planning to
>> land
>>
> it
>
>> (favoring instead to land VEA support that replaces EVS), I believe this
>> is
>>
> all
>
>> dead code that will never be revived.  Can you remove it?
>>
>
> Yes it is on my TODO list. I just talked to sheu@ and he'd like to keep
> the
> files around a bit longer until he makes sure all the VEA support gets
> landed.
> Then I'll just delete all the EVS stuff.
>
>
https://chromiumcodereview.**appspot.com/16320005/<https://chromiumcodereview...
>

[hshi1, 2013-07-31 22:55:35]

FWIW I've uploaded the revert CL (that reverts 3 related chromium revisions) to
https://codereview.chromium.org/21421002/