Cast: deliver video frames on the IO thread

Cast: deliver video frames on the IO thread

This is the last change to deliver video frames to Cast streaming API
on the IO thread. It will no longer be blocked by Javascript events.

This needs a change in the MediaStreamVideoSink interface. Also getting
rid of the interface of MediaStreamVideoTrack that delivers frames on
the render thread. All clients now handle the thread hopping directly.

BUG=335327, 371775
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269616

[Alpha Left Google, 2014-05-08 07:08:02]

https://codereview.chromium.org/264363005/diff/40001/chrome/renderer/media/ca...
File chrome/renderer/media/cast_rtp_stream.cc (left):

https://codereview.chromium.org/264363005/diff/40001/chrome/renderer/media/ca...
chrome/renderer/media/cast_rtp_stream.cc:233: // Capture time is calculated
using the time when the first frame
This part needs to be merged. Please ignore this diff here.

[perkj_chrome, 2014-05-08 08:15:07]

This looks great! 

lgtm provided the below is addressed. I will try to talk to you later tonight
again.

Adding Ronghua for pepper and video destination handler and as much as you want.

Tommi, would you mind a quit look?

https://codereview.chromium.org/264363005/diff/60001/content/renderer/media/m...
File content/renderer/media/mock_media_stream_video_sink.cc (right):

https://codereview.chromium.org/264363005/diff/60001/content/renderer/media/m...
content/renderer/media/mock_media_stream_video_sink.cc:16: weak_factory_(this) {
Can you add thread checks to this class? Just to make it obvious that all still
happen on the main render thread.

https://codereview.chromium.org/264363005/diff/60001/content/renderer/media/r...
File content/renderer/media/rtc_video_renderer.h (right):

https://codereview.chromium.org/264363005/diff/60001/content/renderer/media/r...
content/renderer/media/rtc_video_renderer.h:60: // VideoTrackSink
implementation. Called on the main thread.
please change to MediaStreamVideoSink implementation.

https://codereview.chromium.org/264363005/diff/60001/content/renderer/pepper/...
File content/renderer/pepper/pepper_media_stream_video_track_host.cc (right):

https://codereview.chromium.org/264363005/diff/60001/content/renderer/pepper/...
content/renderer/pepper/pepper_media_stream_video_track_host.cc:408: ppformat =
Prefer the old way- don't change a local variable if its used for something else
prior to this.

https://codereview.chromium.org/264363005/diff/70001/chrome/renderer/media/ca...
File chrome/renderer/media/cast_rtp_stream.cc (right):

https://codereview.chromium.org/264363005/diff/70001/chrome/renderer/media/ca...
chrome/renderer/media/cast_rtp_stream.cc:236: if (frame->coded_size() !=
expected_coded_size) {
FYI- coded_size() is the size from the source before any resolution constraints
are applied for scaling /cropping . So if you want track constraints to have
effect - you must look at the natural_size and visible_rect.

https://codereview.chromium.org/264363005/diff/70001/content/public/renderer/...
File content/public/renderer/media_stream_video_sink.h (right):

https://codereview.chromium.org/264363005/diff/70001/content/public/renderer/...
content/public/renderer/media_stream_video_sink.h:13: #include
"media/video/capture/video_capture_types.h"
 Can you forward declare const media::VideoCaptureFormat& the same way as
VideoFrame instead.

https://codereview.chromium.org/264363005/diff/70001/content/public/renderer/...
content/public/renderer/media_stream_video_sink.h:40: // Note that |callback|
for frame delivery can be called on an arbitrary
I like not making promises, but I think Tommmi wanted us to guarantee this is on
specific thread, namely the IO-thread currently.

https://codereview.chromium.org/264363005/diff/70001/content/renderer/media/m...
File content/renderer/media/mock_media_stream_video_sink.h (right):

https://codereview.chromium.org/264363005/diff/70001/content/renderer/media/m...
content/renderer/media/mock_media_stream_video_sink.h:43: int number_of_frames_;
nit: empty line before  first member.

https://codereview.chromium.org/264363005/diff/70001/content/renderer/media/v...
File content/renderer/media/video_source_handler.h (right):

https://codereview.chromium.org/264363005/diff/70001/content/renderer/media/v...
content/renderer/media/video_source_handler.h:69: void
DeliverFrameForTesting(FrameReaderInterface* reader,
Prefer to make this protected and create a test class 

VideoSourceHandlerForTest : public VideoSourceHandler {
public:
using VideoSourceHandler::DeliverFrameForTesting
...


Remove FRIEND_TEST_ALL_PREFIXES(VideoSourceHandlerTest, OpenClose);

[perkj_chrome, 2014-05-08 08:20:54]

And actually adding Ronghua and Tommi.

[Alpha Left Google, 2014-05-08 18:45:51]

https://codereview.chromium.org/264363005/diff/70001/chrome/renderer/media/ca...
File chrome/renderer/media/cast_rtp_stream.cc (right):

https://codereview.chromium.org/264363005/diff/70001/chrome/renderer/media/ca...
chrome/renderer/media/cast_rtp_stream.cc:236: if (frame->coded_size() !=
expected_coded_size) {
On 2014/05/08 08:15:08, perkj wrote:
> FYI- coded_size() is the size from the source before any resolution
constraints
> are applied for scaling /cropping . So if you want track constraints to have
> effect - you must look at the natural_size and visible_rect. 

I see. I think this is good for now. We're not applying any constraints to crop.

https://codereview.chromium.org/264363005/diff/70001/content/public/renderer/...
File content/public/renderer/media_stream_video_sink.h (right):

https://codereview.chromium.org/264363005/diff/70001/content/public/renderer/...
content/public/renderer/media_stream_video_sink.h:13: #include
"media/video/capture/video_capture_types.h"
On 2014/05/08 08:15:08, perkj wrote:
>  Can you forward declare const media::VideoCaptureFormat& the same way as
> VideoFrame instead.

Done.

https://codereview.chromium.org/264363005/diff/70001/content/public/renderer/...
content/public/renderer/media_stream_video_sink.h:40: // Note that |callback|
for frame delivery can be called on an arbitrary
On 2014/05/08 08:15:08, perkj wrote:
> I like not making promises, but I think Tommmi wanted us to guarantee this is
on
> specific thread, namely the IO-thread currently. 

Done.

https://codereview.chromium.org/264363005/diff/70001/content/renderer/media/m...
File content/renderer/media/mock_media_stream_video_sink.h (right):

https://codereview.chromium.org/264363005/diff/70001/content/renderer/media/m...
content/renderer/media/mock_media_stream_video_sink.h:43: int number_of_frames_;
On 2014/05/08 08:15:08, perkj wrote:
> nit: empty line before  first member.

Done.

https://codereview.chromium.org/264363005/diff/70001/content/renderer/media/v...
File content/renderer/media/video_source_handler.h (right):

https://codereview.chromium.org/264363005/diff/70001/content/renderer/media/v...
content/renderer/media/video_source_handler.h:69: void
DeliverFrameForTesting(FrameReaderInterface* reader,
On 2014/05/08 08:15:08, perkj wrote:
> Prefer to make this protected and create a test class 
> 
> VideoSourceHandlerForTest : public VideoSourceHandler {
> public:
> using VideoSourceHandler::DeliverFrameForTesting
> ...
> 
> 
> Remove FRIEND_TEST_ALL_PREFIXES(VideoSourceHandlerTest, OpenClose);

There's already VideoSourceHandlerTest in the unit test. I would prefer to keep
this design because it's the main point of this patch.

[Alpha Left Google, 2014-05-08 18:46:01]

ping ronghua.

[perkj_chrome, 2014-05-08 19:14:03]

see explanation- but I leave it up to you and Ronghua.

https://codereview.chromium.org/264363005/diff/70001/content/renderer/media/v...
File content/renderer/media/video_source_handler.h (right):

https://codereview.chromium.org/264363005/diff/70001/content/renderer/media/v...
content/renderer/media/video_source_handler.h:69: void
DeliverFrameForTesting(FrameReaderInterface* reader,
On 2014/05/08 18:45:51, Alpha wrote:
> On 2014/05/08 08:15:08, perkj wrote:
> > Prefer to make this protected and create a test class 
> > 
> > VideoSourceHandlerForTest : public VideoSourceHandler {
> > public:
> > using VideoSourceHandler::DeliverFrameForTesting
> > ...
> > 
> > 


See for example media_stream_remote_video_source_unittest.cc
class MediaStreamRemoteVideoSourceUnderTest
    : public MediaStreamRemoteVideoSource {
 public:
 using MediaStreamRemoteVideoSource::RenderInterfaceForTest;
};


> > Remove FRIEND_TEST_ALL_PREFIXES(VideoSourceHandlerTest, OpenClose);
> 
> There's already VideoSourceHandlerTest in the unit test. I would prefer to
keep
> this design because it's the main point of this patch.

[Ronghua Wu (Left Chromium), 2014-05-08 21:32:14]

Reviewed:
video_source_handler*
pepper_media_stream_video_track_host*

+David and Peng for pepper_media_stream_video_track_host changes.

https://codereview.chromium.org/264363005/diff/110001/content/renderer/media/...
File content/renderer/media/video_source_handler.cc (right):

https://codereview.chromium.org/264363005/diff/110001/content/renderer/media/...
content/renderer/media/video_source_handler.cc:58: if (reader_) {
OnVideoFrame is called from the io thread, and SetReader will be called on
render thread. How do you protect |reader_| from data racing?

[Alpha Left Google, 2014-05-08 21:48:41]

https://codereview.chromium.org/264363005/diff/110001/content/renderer/media/...
File content/renderer/media/video_source_handler.cc (right):

https://codereview.chromium.org/264363005/diff/110001/content/renderer/media/...
content/renderer/media/video_source_handler.cc:58: if (reader_) {
On 2014/05/08 21:32:14, Ronghua Wu wrote:
> OnVideoFrame is called from the io thread, and SetReader will be called on
> render thread. How do you protect |reader_| from data racing?

OnVideoFrame is *NOT* called on the IO thread because of the use of
BindToCurrentLoop. It will be called on the render thread.

[dmichael (off chromium), 2014-05-08 21:53:26]

content/renderer/pepper lgtm

[Ronghua Wu (Left Chromium), 2014-05-08 22:00:03]

video_source_handler*
pepper_media_stream_video_track_host*

LGTM

https://codereview.chromium.org/264363005/diff/110001/content/renderer/media/...
File content/renderer/media/video_source_handler.cc (right):

https://codereview.chromium.org/264363005/diff/110001/content/renderer/media/...
content/renderer/media/video_source_handler.cc:58: if (reader_) {
On 2014/05/08 21:48:41, Alpha wrote:
> On 2014/05/08 21:32:14, Ronghua Wu wrote:
> > OnVideoFrame is called from the io thread, and SetReader will be called on
> > render thread. How do you protect |reader_| from data racing?
> 
> OnVideoFrame is *NOT* called on the IO thread because of the use of
> BindToCurrentLoop. It will be called on the render thread.

ic

[Alpha Left Google, 2014-05-08 23:04:22]

+piman for OWNERS approval of content/public

[Alpha Left Google, 2014-05-09 03:27:47]

I have updated the code such that the added callback is guaranteed to be
released on the render thread.

[piman, 2014-05-09 03:44:22]

https://codereview.chromium.org/264363005/diff/170001/content/renderer/media/...
File content/renderer/media/video_frame_deliverer.cc (right):

https://codereview.chromium.org/264363005/diff/170001/content/renderer/media/...
content/renderer/media/video_frame_deliverer.cc:60:
message_loop->PostTask(FROM_HERE, base::Bind(&ResetCallback, callback));
So, this is racy.
If the target message loop is free, then ResetCallback can be called immediately
(releasing its reference), but then the local variable |callback| still is in
scope and has a reference to the callback storage, so the last one would be
removed on the IO thread.
(it also shows the difficulty of writing a full test for this :( )

Maybe something along the lines of this would work:

scoped_ptr<VideoCaptureDeliverFrameCB> callback;
{
  callback.reset(new VideoCaptureDeliverFrameCB(it->second));
  callbacks_.erase(it);
}
message_loop->PostTask(FROM_HERE, base::Bind(&ResetCallback, Passed(callback)));

(with big warnings about the subtlety of this).
You really can't have any temporary or anything still have a copy of the
callback, by the time you post the task.


Note that this will only work if message_loop outlives the current thread
(fortunately it is the case here).

[Alpha Left Google, 2014-05-09 04:32:23]

I've updated the code to prevent this race condition. It is however very
difficult to test this scenario because it requires the message loop to be
exiting..

[tommi (sloooow) - chröme, 2014-05-09 16:20:49]

Sorry for the delay, things have been rather crazy.
I took a quick look over everything and it lgtm.

One concern below that I'll leave up to you.

https://codereview.chromium.org/264363005/diff/210001/content/renderer/media/...
File content/renderer/media/media_stream_video_track_unittest.cc (right):

https://codereview.chromium.org/264363005/diff/210001/content/renderer/media/...
content/renderer/media/media_stream_video_track_unittest.cc:119: *correct_ =
thread_checker_.CalledOnValidThread();
doesn't CalledOnValidThread always return true in release builds?

https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr...

Perhaps you want to use ThreadCheckerImpl directly? (not sure if that's frowned
upon or not)

[piman, 2014-05-09 16:53:00]

lgtm

[Alpha Left Google, 2014-05-09 17:33:22]

https://codereview.chromium.org/264363005/diff/210001/content/renderer/media/...
File content/renderer/media/media_stream_video_track_unittest.cc (right):

https://codereview.chromium.org/264363005/diff/210001/content/renderer/media/...
content/renderer/media/media_stream_video_track_unittest.cc:119: *correct_ =
thread_checker_.CalledOnValidThread();
Sneaky! Changed it to use ThreadCheckerImpl directly.

[Alpha Left Google, 2014-05-09 17:33:28]

The CQ bit was checked by hclam@chromium.org

[commit-bot: I haz the power, 2014-05-09 17:35:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hclam@chromium.org/264363005/230001

[commit-bot: I haz the power, 2014-05-10 19:18:45]

Change committed as 269616