create a separate WebMediaPlayer for URL derived from media stream

create a separate WebMediaPlayer for URL derived from media stream
When the "src" of <video> is derived from media stream, WebMediaPlayerMS is created.
Its real time charateristics allows it to simplify some controls in WebMediaPlayer, e.g., no buffering, no preload, etc.

WebMediaPlayerMS has 2 different VideoFrameProviders: LocalVideoCapture(for local preview) and RTCVideoRender(for remote view).

Audio will be added in the following patches.

BUG=142988, 110938
TEST=turn on WebMediaPlayer_MS, run apprtc.appspot.com/?debug=loopback
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=158490

[scherkus (not reviewing), 2012-09-07 11:44:03]

nice!

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/me...
File content/renderer/media/local_video_capture.cc (right):

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/me...
content/renderer/media/local_video_capture.cc:132: // Assume YV12 format.
that seems like a scary assumption!

in release mode this DCHECK isn't present and we'll certainly be susceptible to
memory overwrites/reads

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/me...
File content/renderer/media/local_video_capture.h (right):

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/me...
content/renderer/media/local_video_capture.h:22: 
all new code in content should be in the content namespace

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/me...
File content/renderer/media/media_stream_impl.cc (right):

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/me...
content/renderer/media/media_stream_impl.cc:208: // TODO(wjia): remove this to
switch to VideoFrameProvier.
I'd rather see this disabling w/ TODO + link to bug in
RenderViewImpl::createMediaPlayer() where this method is called

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/me...
File content/renderer/media/rtc_video_render.cc (right):

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/me...
content/renderer/media/rtc_video_render.cc:66: void RTCVideoRender::SetSize(int
width, int height) {
we don't need to implement this?

can it be removed from the interface?

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/me...
content/renderer/media/rtc_video_render.cc:70: DCHECK(frame);
dcheck not needed (you'll crash on subsequent lines)

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/me...
content/renderer/media/rtc_video_render.cc:82: DCHECK_EQ(frame->GetPixelWidth(),
1u);
what happens in release mode?

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/me...
File content/renderer/media/rtc_video_render.h (right):

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/me...
content/renderer/media/rtc_video_render.h:26: class CONTENT_EXPORT
RTCVideoRender
Shouldn't this be RTCVideoRenderer (extra "er" on end)?

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/me...
content/renderer/media/rtc_video_render.h:35: //
webkit_media::VideoFrameProvider implementation. Called in the main thread.
s/in/on/

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/me...
content/renderer/media/rtc_video_render.h:41: // webrtc::VideoRendererInterface
implementation. May be called in
s/in/on/

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/me...
content/renderer/media/rtc_video_render.h:50: enum ProviderState {
s/ProviderState/State/?

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/re...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/re...
content/renderer/render_view_impl.cc:2519: if (media_stream_impl_ &&
media_stream_impl_->IsMediaStream(url)) {
despite calling EnsureMediaStreamImpl() we still need to check for
media_stream_impl_ != NULL?

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/re...
content/renderer/render_view_impl.cc:2519: if (media_stream_impl_ &&
media_stream_impl_->IsMediaStream(url)) {
You should disable this new code path here instead of inside IsMediaStream()
returning false

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/re...
content/renderer/render_view_impl.cc:2520: media_player = new
webkit_media::WebMediaPlayerMS(
this can be an early return

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/media_...
File webkit/media/media_stream_client.h (right):

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/media_...
webkit/media/media_stream_client.h:10: #include
"webkit/media/video_frame_provider.h"
instead of #include use a fwd decl

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/media_...
webkit/media/media_stream_client.h:25: virtual bool IsMediaStream(const GURL&
url) = 0;
docs for interface methods?

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/simple...
File webkit/media/simple_video_frame_provider.h (right):

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/simple...
webkit/media/simple_video_frame_provider.h:20: class SimpleVideoFrameProvider :
public VideoFrameProvider {
I don't see this class isn't used anywhere in this CL -- remove for now?

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/simple...
webkit/media/simple_video_frame_provider.h:38: enum ProviderState {
s/ProviderState/State/?

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/video_...
File webkit/media/video_frame_provider.h (right):

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/video_...
webkit/media/video_frame_provider.h:17: typedef base::Callback<void(const
scoped_refptr<media::VideoFrame>&)> RepaintCB;
move this inside VFP class

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/video_...
webkit/media/video_frame_provider.h:21: : public
base::RefCountedThreadSafe<VideoFrameProvider> {
does this have to be refcounted?

would having implementors use weak pointers suffice?

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/video_...
webkit/media/video_frame_provider.h:23: virtual void Start() = 0;
docs for interface methods

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
File webkit/media/webmediaplayer_ms.cc (right):

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.cc:59: main_loop_(MessageLoop::current()),
You don't need this member variable as you're only using it for DCHECK purposes
and not posting tasks.

If you want to DCHECK single-threadedness use base::ThreadChecker (which
optimizes to nothing in release) or just remove all the DCHECK()s

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.cc:74: media_log_->AddEvent(
this will end up on chrome://media-internals but I can't guarantee you'll find
the data very useful :)

was it your intent to use it? (it's purely optional and you can remove if you
want)

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.cc:89: main_loop_->AddDestructionObserver(this);
I don't think you need a destruction observer as you don't have any teardown
code that requires the render loop.

Looking at the code you have here... in the worst case RTCVideoRender will fail
to post a task to execute the paint CB but since its using MLProxy it's OK.

If so, then you can also inline Destroy() into the dtor

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.cc:154: // TODO: add audio.
TODO(wjia)?

is there a bug filed for WebRTC audio?

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.cc:168: // TODO: add audio.
ditto

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.cc:201: // TODO: set audio volume.
TODO(wjia)?
NOTIMPLEMENTED?

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.cc:219: return !!video_frame_provider_;
use != NULL

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.cc:224: // TODO: add audio support.
TODO(wjia)?

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.cc:256: if (current_frame_.get()) {
how about:

if (current_frame_.get())
  return current_frame_->GetTimestamp().InSecondsF();
return 0.0f;

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.cc:281: return buffered_;
you don't need this variable -- just alloc + return a new empty range here
instead

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.cc:370: pending_repaint_ = true;
this seems odd -- what exactly is pending_repaint_ used for and what is it
tracking?

it looks like you're using it for both making sure getCurrentFrame() isn't
called too many times and for tracking dropped frames -- which one is it?

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.cc:407: if (pending_repaint_) {
considering you only set this in the HW decode path I don't think the stats
you're attempting to collect are accurate

in order to determine whether you really dropped a frame you have to track
whether current_frame_ was ever painted at all via software or hardware

let's remove the stats stuff for now and implement it in a later CL

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.cc:447: video_frame_provider_->Stop();
considering Stop() is synchronous how about replacing Stop() with destruction?

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
File webkit/media/webmediaplayer_ms.h (right):

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.h:5: // Delegate calls from
WebCore::MediaPlayerPrivate to Chrome's media player
move to class definition instead of top of file

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.h:132: void Repaint(const
scoped_refptr<media::VideoFrame>& frame);
how about OnFrameAvailable()?

Repaint() is a bit misleading

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.h:137: void OnSourceError();
this is a callback from VFP but the word "source" doesn't really tell me much
and neither does the comment

what is the "source" in this case? is there a better noun we can use to describe
where the error is coming from?

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.h:183: bool accelerated_compositing_reported_;
It looks like you copied code from WebMediaPlayerImpl that isn't applicable to
media streams

Can you make a pass to remove unneeded code/variables?

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/support/test...
File webkit/support/test_media_stream_client.h (right):

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/support/test...
webkit/support/test_media_stream_client.h:15: #include
"webkit/media/video_frame_provider.h"
media_stream_client.h handles this #include for yo

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/support/webk...
File webkit/support/webkit_support.cc (right):

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/support/webk...
webkit/support/webkit_support.cc:412: } else {
no need for else

[wjia(left Chromium), 2012-09-13 01:22:06]

Thanks! PTAL.

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/me...
File content/renderer/media/local_video_capture.cc (right):

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/me...
content/renderer/media/local_video_capture.cc:132: // Assume YV12 format.
On 2012/09/07 11:44:03, scherkus wrote:
> that seems like a scary assumption!
> 
> in release mode this DCHECK isn't present and we'll certainly be susceptible
to
> memory overwrites/reads

hrmm, for now, the media::VideoCapture::VideoFrameBuffer is fixed for color
media::VideoCaptureCapability::kI420. This |buf| is created in
VideoCaptureController::OnIncomingCapturedFrame in
content/browser/renderer_host/media/video_capture_controller.cc.

For the assumption of YV12 format, that's for output, i.e., the format needed by
WebMediaPlayer. That's a safe bet.

Could you elaborate if anything else is needed here to prevent memory
overwrites/reads?

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/me...
File content/renderer/media/local_video_capture.h (right):

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/me...
content/renderer/media/local_video_capture.h:22: 
On 2012/09/07 11:44:03, scherkus wrote:
> all new code in content should be in the content namespace

Done.

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/me...
File content/renderer/media/media_stream_impl.cc (right):

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/me...
content/renderer/media/media_stream_impl.cc:208: // TODO(wjia): remove this to
switch to VideoFrameProvier.
On 2012/09/07 11:44:03, scherkus wrote:
> I'd rather see this disabling w/ TODO + link to bug in
> RenderViewImpl::createMediaPlayer() where this method is called

Done.

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/me...
File content/renderer/media/rtc_video_render.cc (right):

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/me...
content/renderer/media/rtc_video_render.cc:66: void RTCVideoRender::SetSize(int
width, int height) {
On 2012/09/07 11:44:03, scherkus wrote:
> we don't need to implement this?
> 
> can it be removed from the interface?

Right, since the frame in RenderFrame() has the size, this function is not
really needed. But since this function is propagated from webrtc to libjingle to
Chromium, it will take some effort to remove it.

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/me...
content/renderer/media/rtc_video_render.cc:70: DCHECK(frame);
On 2012/09/07 11:44:03, scherkus wrote:
> dcheck not needed (you'll crash on subsequent lines)

Done.

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/me...
content/renderer/media/rtc_video_render.cc:82: DCHECK_EQ(frame->GetPixelWidth(),
1u);
On 2012/09/07 11:44:03, scherkus wrote:
> what happens in release mode?

It should be ok. According to
http://code.google.com/p/chromium/source/search?q=GetPixelWidth&origq=GetPixe...,
this aspect ratio checking exists due to the aspect_ratio_idc parameter of
H.264.

If any video frame has no square pixel, we have to resample it in order to keep
display correctly. For now, I don't see we would run into that case.

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/me...
File content/renderer/media/rtc_video_render.h (right):

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/me...
content/renderer/media/rtc_video_render.h:26: class CONTENT_EXPORT
RTCVideoRender
On 2012/09/07 11:44:03, scherkus wrote:
> Shouldn't this be RTCVideoRenderer (extra "er" on end)?

Done.

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/me...
content/renderer/media/rtc_video_render.h:35: //
webkit_media::VideoFrameProvider implementation. Called in the main thread.
On 2012/09/07 11:44:03, scherkus wrote:
> s/in/on/

Done.

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/me...
content/renderer/media/rtc_video_render.h:41: // webrtc::VideoRendererInterface
implementation. May be called in
On 2012/09/07 11:44:03, scherkus wrote:
> s/in/on/

Done.

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/me...
content/renderer/media/rtc_video_render.h:50: enum ProviderState {
On 2012/09/07 11:44:03, scherkus wrote:
> s/ProviderState/State/?

Done.

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/re...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/re...
content/renderer/render_view_impl.cc:2519: if (media_stream_impl_ &&
media_stream_impl_->IsMediaStream(url)) {
On 2012/09/07 11:44:03, scherkus wrote:
> despite calling EnsureMediaStreamImpl() we still need to check for
> media_stream_impl_ != NULL?

yes, inside EnsureMediaStreamImpl(), the creation of media_stream_impl is under
macro ENABLE_WEBRTC.

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/re...
content/renderer/render_view_impl.cc:2519: if (media_stream_impl_ &&
media_stream_impl_->IsMediaStream(url)) {
On 2012/09/07 11:44:03, scherkus wrote:
> You should disable this new code path here instead of inside IsMediaStream()
> returning false

Done.

https://chromiumcodereview.appspot.com/10918052/diff/7010/content/renderer/re...
content/renderer/render_view_impl.cc:2520: media_player = new
webkit_media::WebMediaPlayerMS(
On 2012/09/07 11:44:03, scherkus wrote:
> this can be an early return 

Done.

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/media_...
File webkit/media/media_stream_client.h (right):

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/media_...
webkit/media/media_stream_client.h:10: #include
"webkit/media/video_frame_provider.h"
On 2012/09/07 11:44:03, scherkus wrote:
> instead of #include use a fwd decl

This header file is needed because of RepaintCB is defined there.

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/media_...
webkit/media/media_stream_client.h:25: virtual bool IsMediaStream(const GURL&
url) = 0;
On 2012/09/07 11:44:03, scherkus wrote:
> docs for interface methods?

Done.

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/simple...
File webkit/media/simple_video_frame_provider.h (right):

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/simple...
webkit/media/simple_video_frame_provider.h:20: class SimpleVideoFrameProvider :
public VideoFrameProvider {
On 2012/09/07 11:44:03, scherkus wrote:
> I don't see this class isn't used anywhere in this CL -- remove for now?

It's used in webkit/support/test_media_stream_client.cc for DRT.

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/simple...
webkit/media/simple_video_frame_provider.h:38: enum ProviderState {
On 2012/09/07 11:44:03, scherkus wrote:
> s/ProviderState/State/?

Done.

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/video_...
File webkit/media/video_frame_provider.h (right):

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/video_...
webkit/media/video_frame_provider.h:17: typedef base::Callback<void(const
scoped_refptr<media::VideoFrame>&)> RepaintCB;
On 2012/09/07 11:44:03, scherkus wrote:
> move this inside VFP class

Done.

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/video_...
webkit/media/video_frame_provider.h:21: : public
base::RefCountedThreadSafe<VideoFrameProvider> {
On 2012/09/07 11:44:03, scherkus wrote:
> does this have to be refcounted?
> 
> would having implementors use weak pointers suffice?

Some derived class needs ref counted, because it will be essentially accessed on
at least 2 threads. For example, LocalVideoCapture is accessed on both main
thread and VideoCaptureImpl's working thread. For LocalVideoCapture, it's a bit
complicated since a Proxy is used. But the proxy is scoped_ptr'ed by
LocalVideoCapture. That means LocalVideoCapture can't go away before its user
confirms LocalVideoCapture will not be used any longer.

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/video_...
webkit/media/video_frame_provider.h:23: virtual void Start() = 0;
On 2012/09/07 11:44:03, scherkus wrote:
> docs for interface methods

Done.

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
File webkit/media/webmediaplayer_ms.cc (right):

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.cc:59: main_loop_(MessageLoop::current()),
On 2012/09/07 11:44:03, scherkus wrote:
> You don't need this member variable as you're only using it for DCHECK
purposes
> and not posting tasks.
> 
> If you want to DCHECK single-threadedness use base::ThreadChecker (which
> optimizes to nothing in release) or just remove all the DCHECK()s

Done.

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.cc:74: media_log_->AddEvent(
On 2012/09/07 11:44:03, scherkus wrote:
> this will end up on chrome://media-internals but I can't guarantee you'll find
> the data very useful :)
> 
> was it your intent to use it? (it's purely optional and you can remove if you
> want)

I'd keep it for now.

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.cc:89: main_loop_->AddDestructionObserver(this);
On 2012/09/07 11:44:03, scherkus wrote:
> I don't think you need a destruction observer as you don't have any teardown
> code that requires the render loop.
> 
> Looking at the code you have here... in the worst case RTCVideoRender will
fail
> to post a task to execute the paint CB but since its using MLProxy it's OK.
> 
> If so, then you can also inline Destroy() into the dtor

Done.

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.cc:154: // TODO: add audio.
On 2012/09/07 11:44:03, scherkus wrote:
> TODO(wjia)?
> 
> is there a bug filed for WebRTC audio?

Done. I'd use the same bug for adding WebRTC audio into WMPMS.

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.cc:168: // TODO: add audio.
On 2012/09/07 11:44:03, scherkus wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.cc:201: // TODO: set audio volume.
On 2012/09/07 11:44:03, scherkus wrote:
> TODO(wjia)?
> NOTIMPLEMENTED?

Done.

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.cc:219: return !!video_frame_provider_;
On 2012/09/07 11:44:03, scherkus wrote:
> use != NULL

Done.

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.cc:224: // TODO: add audio support.
On 2012/09/07 11:44:03, scherkus wrote:
> TODO(wjia)?

Done.

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.cc:256: if (current_frame_.get()) {
On 2012/09/07 11:44:03, scherkus wrote:
> how about:
> 
> if (current_frame_.get())
>   return current_frame_->GetTimestamp().InSecondsF();
> return 0.0f;

Done.

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.cc:281: return buffered_;
On 2012/09/07 11:44:03, scherkus wrote:
> you don't need this variable -- just alloc + return a new empty range here
> instead

This variable has to be here. If a WebTimeRanges is allocated and passed to
client, nobody will free it.

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.cc:370: pending_repaint_ = true;
On 2012/09/07 11:44:03, scherkus wrote:
> this seems odd -- what exactly is pending_repaint_ used for and what is it
> tracking?
> 
> it looks like you're using it for both making sure getCurrentFrame() isn't
> called too many times and for tracking dropped frames -- which one is it?

|pending_repaint_| is used to ensure client->repaint() will not be called
between getCurrentFrame() and putCurrentFrame(). I have seen some blinking
effect when it's done, e.g., client calls getCurrentFrame() when it tries to
repaint due to some reason other than the signal from WMPMS. Before client calls
putCurrentFrame(), a new frame comes from VideoFrameProvider and
client->repaint() is called. This results in embedded get/putCurrentFrame()
calls and blinking effect.

VideoFrameProvider always passes frames to WMPMS. When a frame is available, but
client->repaint() is not called, that frame is treated as dropped.

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.cc:407: if (pending_repaint_) {
On 2012/09/07 11:44:03, scherkus wrote:
> considering you only set this in the HW decode path I don't think the stats
> you're attempting to collect are accurate
> 
> in order to determine whether you really dropped a frame you have to track
> whether current_frame_ was ever painted at all via software or hardware
> 
> let's remove the stats stuff for now and implement it in a later CL

Removed dropped_frame_count_ for now. Is it possible both get/putCurrentFrame
and paint() could be called? For example, <video> display uses
get/putCurrentFrame(), while the same video is copied to <canvas>.

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.cc:447: video_frame_provider_->Stop();
On 2012/09/07 11:44:03, scherkus wrote:
> considering Stop() is synchronous how about replacing Stop() with destruction?

That won't work for LocalVideoCapture, since Start() will increase ref count.
WMPMS can only decrease the ref count, instead of triggering the destruction of
LVC.

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
File webkit/media/webmediaplayer_ms.h (right):

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.h:5: // Delegate calls from
WebCore::MediaPlayerPrivate to Chrome's media player
On 2012/09/07 11:44:03, scherkus wrote:
> move to class definition instead of top of file

Done.

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.h:132: void Repaint(const
scoped_refptr<media::VideoFrame>& frame);
On 2012/09/07 11:44:03, scherkus wrote:
> how about OnFrameAvailable()?
> 
> Repaint() is a bit misleading

Done.

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.h:137: void OnSourceError();
On 2012/09/07 11:44:03, scherkus wrote:
> this is a callback from VFP but the word "source" doesn't really tell me much
> and neither does the comment
> 
> what is the "source" in this case? is there a better noun we can use to
describe
> where the error is coming from?

The "source" means the real module which creates the frames, not the
VideoFrameProvider which is just a relayer. For example, the webcam for
LocalVideoCapture, the network for RTCVideoRenderer. 

There could be many types of error from those "sources", e.g., webcam is
unplugged, device goes wrong, network is lost, etc. I might just call it
OnError() without "source". What do you think?

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.h:183: bool accelerated_compositing_reported_;
On 2012/09/07 11:44:03, scherkus wrote:
> It looks like you copied code from WebMediaPlayerImpl that isn't applicable to
> media streams
> 
> Can you make a pass to remove unneeded code/variables?

yes, this one is copied from WMPImpl. Removing this one. I thought this variable
is needed since it doesn't check any condition. 

Please let me know what else is not applicable here.

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/support/test...
File webkit/support/test_media_stream_client.h (right):

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/support/test...
webkit/support/test_media_stream_client.h:15: #include
"webkit/media/video_frame_provider.h"
On 2012/09/07 11:44:03, scherkus wrote:
> media_stream_client.h handles this #include for yo

Done.

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/support/webk...
File webkit/support/webkit_support.cc (right):

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/support/webk...
webkit/support/webkit_support.cc:412: } else {
On 2012/09/07 11:44:03, scherkus wrote:
> no need for else

Done.

[scherkus (not reviewing), 2012-09-13 10:22:43]

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
File webkit/media/webmediaplayer_ms.cc (right):

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.cc:281: return buffered_;
On 2012/09/13 01:22:07, wjia wrote:
> On 2012/09/07 11:44:03, scherkus wrote:
> > you don't need this variable -- just alloc + return a new empty range here
> > instead
> 
> This variable has to be here. If a WebTimeRanges is allocated and passed to
> client, nobody will free it.

ah didn't see the const-ref return value

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.cc:407: if (pending_repaint_) {
On 2012/09/13 01:22:07, wjia wrote:
> On 2012/09/07 11:44:03, scherkus wrote:
> > considering you only set this in the HW decode path I don't think the stats
> > you're attempting to collect are accurate
> > 
> > in order to determine whether you really dropped a frame you have to track
> > whether current_frame_ was ever painted at all via software or hardware
> > 
> > let's remove the stats stuff for now and implement it in a later CL
> 
> Removed dropped_frame_count_ for now. Is it possible both get/putCurrentFrame
> and paint() could be called? For example, <video> display uses
> get/putCurrentFrame(), while the same video is copied to <canvas>.

I wouldn't say the two are mutually exclusive -- for example, if the GPU process
crashes and we fall back into software rendering we'll switch back to using
paint() calls

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.cc:447: video_frame_provider_->Stop();
On 2012/09/13 01:22:07, wjia wrote:
> On 2012/09/07 11:44:03, scherkus wrote:
> > considering Stop() is synchronous how about replacing Stop() with
destruction?
> 
> That won't work for LocalVideoCapture, since Start() will increase ref count.
> WMPMS can only decrease the ref count, instead of triggering the destruction
of
> LVC.

Let's see if I can untangle this a bit...

VideoFrameProvider is refcounted because...
LocalVideoCapture calls AddRef() in Start() because...
vc_manager_->AddDevice() is called and VCD::EventHandler is not refcounted

It seems what you really want is a way for LVC to stop / unregister itself so
that its guaranteed to be never called again via its VCD::EventHandler
interface. Relying on OnRemoved() so you can call balance refcounts via
Release() is a perilous path to go down.

Can this be handled via VideoCaptureHandlerProxy instead of requiring LVC to be
refcounted?

https://chromiumcodereview.appspot.com/10918052/diff/10006/content/renderer/m...
File content/renderer/media/local_video_capture.cc (right):

https://chromiumcodereview.appspot.com/10918052/diff/10006/content/renderer/m...
content/renderer/media/local_video_capture.cc:135: DCHECK_EQ(capability_.color,
media::VideoCaptureCapability::kI420);
If we're doing memory operations that require I420 layout then let's upgrade the
check.

For example, CHECK_EQ() + crash or DCHECK_EQ() + handle it by (for example)
skipping the copy plane code / repaint_cb_

https://chromiumcodereview.appspot.com/10918052/diff/10006/content/renderer/r...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10918052/diff/10006/content/renderer/r...
content/renderer/render_view_impl.cc:2532: //  return new
webkit_media::WebMediaPlayerMS(
instead of commented out code how about introducing a switch?

that way you can have the rest of the team / QA try it out in canary builds w/o
modifying the source

https://chromiumcodereview.appspot.com/10918052/diff/10006/webkit/media/media...
File webkit/media/media_stream_client.h (right):

https://chromiumcodereview.appspot.com/10918052/diff/10006/webkit/media/media...
webkit/media/media_stream_client.h:25: // Chcek if the |url| is derived from a
media stream object.
s/Chcek/Check/

https://chromiumcodereview.appspot.com/10918052/diff/10006/webkit/media/video...
File webkit/media/video_frame_provider.h (right):

https://chromiumcodereview.appspot.com/10918052/diff/10006/webkit/media/video...
webkit/media/video_frame_provider.h:26: // Stop to provide video frames to the
caller.
nit: can you add blank lines before // comments?

it's very wall-of-text-y looking like this

https://chromiumcodereview.appspot.com/10918052/diff/10006/webkit/media/webme...
File webkit/media/webmediaplayer_ms.cc (right):

https://chromiumcodereview.appspot.com/10918052/diff/10006/webkit/media/webme...
webkit/media/webmediaplayer_ms.cc:96: if
(incremented_externally_allocated_memory_) {
remove all this incremented memory / V8 stuff -- you don't need it

https://chromiumcodereview.appspot.com/10918052/diff/10006/webkit/media/webme...
webkit/media/webmediaplayer_ms.cc:153: // TODO(wjia): add audio.
can you reference the bug in the TODO?

https://chromiumcodereview.appspot.com/10918052/diff/10006/webkit/media/webme...
webkit/media/webmediaplayer_ms.cc:167: // TODO(wjia): add audio.
can you reference the bug in the TODO?

https://chromiumcodereview.appspot.com/10918052/diff/10006/webkit/media/webme...
webkit/media/webmediaplayer_ms.cc:200: // TODO(wjia): set audio volume.
can you reference the bug in the TODO?

https://chromiumcodereview.appspot.com/10918052/diff/10006/webkit/media/webme...
webkit/media/webmediaplayer_ms.cc:224: // TODO(wjia): add audio support.
can you reference the bug in the TODO?

[scherkus (not reviewing), 2012-09-13 10:25:23]

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/video_...
File webkit/media/video_frame_provider.h (right):

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/video_...
webkit/media/video_frame_provider.h:21: : public
base::RefCountedThreadSafe<VideoFrameProvider> {
On 2012/09/13 01:22:07, wjia wrote:
> On 2012/09/07 11:44:03, scherkus wrote:
> > does this have to be refcounted?
> > 
> > would having implementors use weak pointers suffice?
> 
> Some derived class needs ref counted, because it will be essentially accessed
on
> at least 2 threads. For example, LocalVideoCapture is accessed on both main
> thread and VideoCaptureImpl's working thread. For LocalVideoCapture, it's a
bit
> complicated since a Proxy is used. But the proxy is scoped_ptr'ed by
> LocalVideoCapture. That means LocalVideoCapture can't go away before its user
> confirms LocalVideoCapture will not be used any longer.

Being accessed on two threads doesn't imply a class needs to be refcounted.

See my other comments in WMPMS about this. It seems you really want a way for
LVC to tell VC to not call itself anymore OR move the use-after-free handling
inside of VideoCaptureHandlerProxy but not LVC.

[wjia(left Chromium), 2012-09-19 03:22:06]

Thanks! PTAL.

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/video_...
File webkit/media/video_frame_provider.h (right):

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/video_...
webkit/media/video_frame_provider.h:21: : public
base::RefCountedThreadSafe<VideoFrameProvider> {
On 2012/09/13 10:25:24, scherkus wrote:
> On 2012/09/13 01:22:07, wjia wrote:
> > On 2012/09/07 11:44:03, scherkus wrote:
> > > does this have to be refcounted?
> > > 
> > > would having implementors use weak pointers suffice?
> > 
> > Some derived class needs ref counted, because it will be essentially
accessed
> on
> > at least 2 threads. For example, LocalVideoCapture is accessed on both main
> > thread and VideoCaptureImpl's working thread. For LocalVideoCapture, it's a
> bit
> > complicated since a Proxy is used. But the proxy is scoped_ptr'ed by
> > LocalVideoCapture. That means LocalVideoCapture can't go away before its
user
> > confirms LocalVideoCapture will not be used any longer.
> 
> Being accessed on two threads doesn't imply a class needs to be refcounted.
> 
> See my other comments in WMPMS about this. It seems you really want a way for
> LVC to tell VC to not call itself anymore OR move the use-after-free handling
> inside of VideoCaptureHandlerProxy but not LVC.

You are right. I have thought about refactoring
media::VideoCapture::EventHandler to support weak ptr. When one EventHandler
registers with VideoCaptureImpl, it also provides a MessageLoopProxy on which
the EventHandler will be destructed. When sending an event to the EventHandler,
VideoCaptureImpl will always post a task to that MLP. This can guarantee that
EventHandler can be deleted any time without a use-after-free case.

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
File webkit/media/webmediaplayer_ms.cc (right):

https://chromiumcodereview.appspot.com/10918052/diff/7010/webkit/media/webmed...
webkit/media/webmediaplayer_ms.cc:447: video_frame_provider_->Stop();
On 2012/09/13 10:22:43, scherkus wrote:
> On 2012/09/13 01:22:07, wjia wrote:
> > On 2012/09/07 11:44:03, scherkus wrote:
> > > considering Stop() is synchronous how about replacing Stop() with
> destruction?
> > 
> > That won't work for LocalVideoCapture, since Start() will increase ref
count.
> > WMPMS can only decrease the ref count, instead of triggering the destruction
> of
> > LVC.
> 
> Let's see if I can untangle this a bit...
> 
> VideoFrameProvider is refcounted because...
> LocalVideoCapture calls AddRef() in Start() because...
> vc_manager_->AddDevice() is called and VCD::EventHandler is not refcounted
> 
> It seems what you really want is a way for LVC to stop / unregister itself so
> that its guaranteed to be never called again via its VCD::EventHandler
> interface. Relying on OnRemoved() so you can call balance refcounts via
> Release() is a perilous path to go down.
> 
> Can this be handled via VideoCaptureHandlerProxy instead of requiring LVC to
be
> refcounted?

Please see my comments in local_video_capture.cc about the plan to refactor
VideoCapture::EventHandler.

https://chromiumcodereview.appspot.com/10918052/diff/10006/content/renderer/m...
File content/renderer/media/local_video_capture.cc (right):

https://chromiumcodereview.appspot.com/10918052/diff/10006/content/renderer/m...
content/renderer/media/local_video_capture.cc:135: DCHECK_EQ(capability_.color,
media::VideoCaptureCapability::kI420);
On 2012/09/13 10:22:43, scherkus wrote:
> If we're doing memory operations that require I420 layout then let's upgrade
the
> check.
> 
> For example, CHECK_EQ() + crash or DCHECK_EQ() + handle it by (for example)
> skipping the copy plane code / repaint_cb_

Done.

https://chromiumcodereview.appspot.com/10918052/diff/10006/content/renderer/r...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10918052/diff/10006/content/renderer/r...
content/renderer/render_view_impl.cc:2532: //  return new
webkit_media::WebMediaPlayerMS(
On 2012/09/13 10:22:43, scherkus wrote:
> instead of commented out code how about introducing a switch?
> 
> that way you can have the rest of the team / QA try it out in canary builds
w/o
> modifying the source

Done.

https://chromiumcodereview.appspot.com/10918052/diff/10006/webkit/media/media...
File webkit/media/media_stream_client.h (right):

https://chromiumcodereview.appspot.com/10918052/diff/10006/webkit/media/media...
webkit/media/media_stream_client.h:25: // Chcek if the |url| is derived from a
media stream object.
On 2012/09/13 10:22:43, scherkus wrote:
> s/Chcek/Check/

Done.

https://chromiumcodereview.appspot.com/10918052/diff/10006/webkit/media/video...
File webkit/media/video_frame_provider.h (right):

https://chromiumcodereview.appspot.com/10918052/diff/10006/webkit/media/video...
webkit/media/video_frame_provider.h:26: // Stop to provide video frames to the
caller.
On 2012/09/13 10:22:43, scherkus wrote:
> nit: can you add blank lines before // comments?
> 
> it's very wall-of-text-y looking like this

Done.

https://chromiumcodereview.appspot.com/10918052/diff/10006/webkit/media/webme...
File webkit/media/webmediaplayer_ms.cc (right):

https://chromiumcodereview.appspot.com/10918052/diff/10006/webkit/media/webme...
webkit/media/webmediaplayer_ms.cc:96: if
(incremented_externally_allocated_memory_) {
On 2012/09/13 10:22:43, scherkus wrote:
> remove all this incremented memory / V8 stuff -- you don't need it

Done.

https://chromiumcodereview.appspot.com/10918052/diff/10006/webkit/media/webme...
webkit/media/webmediaplayer_ms.cc:153: // TODO(wjia): add audio.
On 2012/09/13 10:22:43, scherkus wrote:
> can you reference the bug in the TODO?

Done.

https://chromiumcodereview.appspot.com/10918052/diff/10006/webkit/media/webme...
webkit/media/webmediaplayer_ms.cc:167: // TODO(wjia): add audio.
On 2012/09/13 10:22:43, scherkus wrote:
> can you reference the bug in the TODO?

Done.

https://chromiumcodereview.appspot.com/10918052/diff/10006/webkit/media/webme...
webkit/media/webmediaplayer_ms.cc:200: // TODO(wjia): set audio volume.
On 2012/09/13 10:22:43, scherkus wrote:
> can you reference the bug in the TODO?

Done.

https://chromiumcodereview.appspot.com/10918052/diff/10006/webkit/media/webme...
webkit/media/webmediaplayer_ms.cc:224: // TODO(wjia): add audio support.
On 2012/09/13 10:22:43, scherkus wrote:
> can you reference the bug in the TODO?

Done.

[scherkus (not reviewing), 2012-09-20 19:27:00]

lgtm w/ nits

we're likely going to have to iterate and fix a few more things but this looks
like a good start

nice job!!!

https://chromiumcodereview.appspot.com/10918052/diff/18001/content/public/com...
File content/public/common/content_switches.cc (right):

https://chromiumcodereview.appspot.com/10918052/diff/18001/content/public/com...
content/public/common/content_switches.cc:311: const char
kEnableMediaPlayerSwitch[]       = "enable-media-player-switch";
This isn't quite accurate... this only affects MediaStream so how about:

kEnableWebMediaPlayerMS[] = "enable-web-media-player-ms" ?

That way in RenderViewImpl the switch will very closely resemble the code (i.e.,
if kEnableWebMediaPlayerMS is true then we create an object called
WebMediaPlayerMS)

https://chromiumcodereview.appspot.com/10918052/diff/18001/content/renderer/m...
File content/renderer/media/rtc_video_renderer.cc (right):

https://chromiumcodereview.appspot.com/10918052/diff/18001/content/renderer/m...
content/renderer/media/rtc_video_renderer.cc:85: int y_rows =
frame->GetHeight();
do we need to assert that |frame| is YV12?

https://chromiumcodereview.appspot.com/10918052/diff/18001/content/renderer/r...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10918052/diff/18001/content/renderer/r...
content/renderer/render_view_impl.cc:2500: if
(!CommandLine::ForCurrentProcess()->HasSwitch(
nit: can you switch this to use cmd_line?

https://chromiumcodereview.appspot.com/10918052/diff/18001/content/renderer/r...
content/renderer/render_view_impl.cc:2528: const CommandLine* cmd_line =
CommandLine::ForCurrentProcess();
nit: this is already declared at line 2486

https://chromiumcodereview.appspot.com/10918052/diff/18001/content/renderer/r...
content/renderer/render_view_impl.cc:2531: // TODO(wjia): when all patches
related to WebMediaPlayerMS have been
indent looks wrong here

https://chromiumcodereview.appspot.com/10918052/diff/18001/webkit/media/webme...
File webkit/media/webmediaplayer_ms.h (right):

https://chromiumcodereview.appspot.com/10918052/diff/18001/webkit/media/webme...
webkit/media/webmediaplayer_ms.h:45: //
nit: remove extra // line

https://chromiumcodereview.appspot.com/10918052/diff/18001/webkit/media/webme...
webkit/media/webmediaplayer_ms.h:46: 
nit: remove extra blank line

[wjia(left Chromium), 2012-09-21 19:47:27]

Thanks!

+piman@ for stamp on content/
+jamesr@ for stamp on webkit/support/

https://chromiumcodereview.appspot.com/10918052/diff/18001/content/public/com...
File content/public/common/content_switches.cc (right):

https://chromiumcodereview.appspot.com/10918052/diff/18001/content/public/com...
content/public/common/content_switches.cc:311: const char
kEnableMediaPlayerSwitch[]       = "enable-media-player-switch";
On 2012/09/20 19:27:00, scherkus wrote:
> This isn't quite accurate... this only affects MediaStream so how about:
> 
> kEnableWebMediaPlayerMS[] = "enable-web-media-player-ms" ?
> 
> That way in RenderViewImpl the switch will very closely resemble the code
(i.e.,
> if kEnableWebMediaPlayerMS is true then we create an object called
> WebMediaPlayerMS)

Done.

https://chromiumcodereview.appspot.com/10918052/diff/18001/content/renderer/m...
File content/renderer/media/rtc_video_renderer.cc (right):

https://chromiumcodereview.appspot.com/10918052/diff/18001/content/renderer/m...
content/renderer/media/rtc_video_renderer.cc:85: int y_rows =
frame->GetHeight();
On 2012/09/20 19:27:00, scherkus wrote:
> do we need to assert that |frame| is YV12?

The cricket::VideoFrame is for I420 only.
http://code.google.com/p/chromium/source/search?q=cricket+videoframe&origq=cr...

https://chromiumcodereview.appspot.com/10918052/diff/18001/content/renderer/r...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10918052/diff/18001/content/renderer/r...
content/renderer/render_view_impl.cc:2500: if
(!CommandLine::ForCurrentProcess()->HasSwitch(
On 2012/09/20 19:27:00, scherkus wrote:
> nit: can you switch this to use cmd_line?

Done.

https://chromiumcodereview.appspot.com/10918052/diff/18001/content/renderer/r...
content/renderer/render_view_impl.cc:2528: const CommandLine* cmd_line =
CommandLine::ForCurrentProcess();
On 2012/09/20 19:27:00, scherkus wrote:
> nit: this is already declared at line 2486

Done.

https://chromiumcodereview.appspot.com/10918052/diff/18001/content/renderer/r...
content/renderer/render_view_impl.cc:2531: // TODO(wjia): when all patches
related to WebMediaPlayerMS have been
On 2012/09/20 19:27:00, scherkus wrote:
> indent looks wrong here

Done.

https://chromiumcodereview.appspot.com/10918052/diff/18001/webkit/media/webme...
File webkit/media/webmediaplayer_ms.h (right):

https://chromiumcodereview.appspot.com/10918052/diff/18001/webkit/media/webme...
webkit/media/webmediaplayer_ms.h:45: //
On 2012/09/20 19:27:00, scherkus wrote:
> nit: remove extra // line

Done.

https://chromiumcodereview.appspot.com/10918052/diff/18001/webkit/media/webme...
webkit/media/webmediaplayer_ms.h:46: 
On 2012/09/20 19:27:00, scherkus wrote:
> nit: remove extra blank line

Done.

[piman, 2012-09-22 01:03:37]

One thing, the rest looks good.

https://chromiumcodereview.appspot.com/10918052/diff/35001/content/renderer/r...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10918052/diff/35001/content/renderer/r...
content/renderer/render_view_impl.cc:2526: EnsureMediaStreamImpl();
This looks heavy the first time (e.g. it creates a thread), but we'll pay that
cost even if the flag isn't set / it's not a media stream url.
Looking at IsMediaStream, it looks like it doesn't need anything from
MediaStreamImpl and could be implemented as a static method, so that you could
do this only the first time you need to create a WebMediaPlayerMS. WDYT?

[wjia(left Chromium), 2012-09-22 17:04:42]

Thanks! PTAL.

https://chromiumcodereview.appspot.com/10918052/diff/35001/content/renderer/r...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10918052/diff/35001/content/renderer/r...
content/renderer/render_view_impl.cc:2526: EnsureMediaStreamImpl();
On 2012/09/22 01:03:37, piman wrote:
> This looks heavy the first time (e.g. it creates a thread), but we'll pay that
> cost even if the flag isn't set / it's not a media stream url.
> Looking at IsMediaStream, it looks like it doesn't need anything from
> MediaStreamImpl and could be implemented as a static method, so that you could
> do this only the first time you need to create a WebMediaPlayerMS. WDYT?

That's a good point. In MediaStreamImpl, IsMediaStream doesn't need anything
from MediaStreamImpl. However, it uses GetMediaStream which is a virtual
function for test purpose
(http://code.google.com/p/chromium/source/search?q=getmediastream&origq=getmed...).
The unit test (in media_stream_impl_unittest.cc) has a different implementation
of GetMediaStream and it uses some member. It will be some refactoring effort to
make IsStreamMedia static. Could we do it in a follow-up patch?

For now, I changed the condition checking a bit so that it won't affect chrome
performance when the flag is not on.

[piman, 2012-09-22 17:07:26]

lgtm

[jamesr, 2012-09-24 18:07:43]

lgtm with one nit

https://chromiumcodereview.appspot.com/10918052/diff/38007/webkit/support/tes...
File webkit/support/test_media_stream_client.h (right):

https://chromiumcodereview.appspot.com/10918052/diff/38007/webkit/support/tes...
webkit/support/test_media_stream_client.h:12: #include "base/callback.h"
comment in base/callback.h says this:

// NOTE: Header files that do not require the full definition of Callback or
// Closure should #include "base/callback_forward.h" instead of this file.

[wjia(left Chromium), 2012-09-25 01:13:05]

Thanks!

https://chromiumcodereview.appspot.com/10918052/diff/38007/webkit/support/tes...
File webkit/support/test_media_stream_client.h (right):

https://chromiumcodereview.appspot.com/10918052/diff/38007/webkit/support/tes...
webkit/support/test_media_stream_client.h:12: #include "base/callback.h"
On 2012/09/24 18:07:44, jamesr wrote:
> comment in base/callback.h says this:
> 
> // NOTE: Header files that do not require the full definition of Callback or
> // Closure should #include "base/callback_forward.h" instead of this file.

Done.