Move passing of WebRTC rendering frames to IO thread

Move passing of WebRTC rendering frames to IO thread

This is follow-up for CL[0] that moved passing of WebRTC rendering frames
off of main thread. That CL caused a regression in test[1] where the number
of unique frames encoded is less, suggesting that we are dropping frames.
Traces showed that compositor thread may be blocked by long texture upload
calls and frames get accumulated in the meantime. We might be dropping
frames due to the limit of having only 3 frames in flight.

This CL addresses this issue by moving these calls to IO thread directly.
In order to do it:
- FrameDeliverer classes in MediaStreamVideoRendererSink and WebMediaPlayerMS
are moved to IO thread.
- WebMediaPlayerMSCompositor::EnqueueFrame gets called on IO thread.
- Since WebMediaPlayerMSCompositor operates on three threads, I made it a ref
counted object. This way, we do not need to create multiple WeakPtr factories
or synchronize/block destruction on multiple threads.

[0] https://codereview.chromium.org/2472273002/
[1] crbug.com/667262

BUG=667262, 652923
TEST=Tested running JS alert() in console during an AppRTC loopback.

Committed: https://crrev.com/8f71ad428b65fc259b48c2a0917158a762a5e5f6
Cr-Commit-Position: refs/heads/master@{#437594}

[emircan, 2016-11-28 20:01:51]

Description was changed from

==========
move to media thread.

BUG=
==========

to

==========
Move passing of WebRTC rendering frames to media thread 

BUG=
==========

[emircan, 2016-11-30 00:28:10]

Description was changed from

==========
Move passing of WebRTC rendering frames to media thread 

BUG=
==========

to

==========
Move passing of WebRTC rendering frames to media thread 

BUG=667262,652923
TEST=
==========

[emircan, 2016-11-30 19:48:03]

Patchset #1 (id:1) has been deleted

[emircan, 2016-12-02 20:56:29]

Patchset #1 (id:20001) has been deleted

[emircan, 2016-12-02 21:18:06]

Patchset #1 (id:40001) has been deleted

[emircan, 2016-12-02 21:47:34]

The CQ bit was checked by emircan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 21:48:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 22:12:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-02 22:12:35]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[emircan, 2016-12-02 22:53:54]

The CQ bit was checked by emircan@chromium.org to run a CQ dry run

[emircan, 2016-12-02 22:54:26]

Patchset #1 (id:60001) has been deleted

[emircan, 2016-12-02 22:54:32]

Patchset #1 (id:80001) has been deleted

[commit-bot: I haz the power, 2016-12-02 22:54:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emircan, 2016-12-02 23:30:23]

Description was changed from

==========
Move passing of WebRTC rendering frames to media thread 

BUG=667262,652923
TEST=
==========

to

==========
Move passing of WebRTC rendering frames to media thread 

This is follow-up for CL[0] that moved passing of WebRTC rendering frames 
off of main thread. That CL caused a regression in test[1] where the number
of unique frames encoded is less, suggesting that we are dropping frames.
Traces showed that compositor thread may be blocked by long texture upload
calls and frames get accumulated in the meantime. We might be dropping 
frames due to the limit of frames in flight. 

This CL addresses this issue by moving these calls to more responsice media 
thread. In order to do it:
- FrameDeliverer classes in MediaStreamVideoRendererSink and WebMediaPlayerMS 
are moved to media thread.
- WebMediaPlayerMSCompositor::EnqueueFrame gets called on media thread.
- Since WebMediaPlayerMSCompositor operates on three threads, I made it a ref
counted object. This way, we do not need to create multiple WeakPtr factories
or synchronize destrcution on multiple threads. 

[0] https://codereview.chromium.org/2472273002/
[1] crbug.com/667262

BUG=667262,652923
TEST=Tested running JS alert() in console during an AppRTC loopback.
==========

[emircan, 2016-12-02 23:30:23]

emircan@chromium.org changed reviewers:
+ dalecurtis@chromium.org, mcasas@chromium.org, qiangchen@chromium.org

[emircan, 2016-12-02 23:31:22]

Description was changed from

==========
Move passing of WebRTC rendering frames to media thread 

This is follow-up for CL[0] that moved passing of WebRTC rendering frames 
off of main thread. That CL caused a regression in test[1] where the number
of unique frames encoded is less, suggesting that we are dropping frames.
Traces showed that compositor thread may be blocked by long texture upload
calls and frames get accumulated in the meantime. We might be dropping 
frames due to the limit of frames in flight. 

This CL addresses this issue by moving these calls to more responsice media 
thread. In order to do it:
- FrameDeliverer classes in MediaStreamVideoRendererSink and WebMediaPlayerMS 
are moved to media thread.
- WebMediaPlayerMSCompositor::EnqueueFrame gets called on media thread.
- Since WebMediaPlayerMSCompositor operates on three threads, I made it a ref
counted object. This way, we do not need to create multiple WeakPtr factories
or synchronize destrcution on multiple threads. 

[0] https://codereview.chromium.org/2472273002/
[1] crbug.com/667262

BUG=667262,652923
TEST=Tested running JS alert() in console during an AppRTC loopback.
==========

to

==========
Move passing of WebRTC rendering frames to media thread 

This is follow-up for CL[0] that moved passing of WebRTC rendering frames 
off of main thread. That CL caused a regression in test[1] where the number
of unique frames encoded is less, suggesting that we are dropping frames.
Traces showed that compositor thread may be blocked by long texture upload
calls and frames get accumulated in the meantime. We might be dropping 
frames due to the limit of having only 3 frames in flight. 

This CL addresses this issue by moving these calls to more responsice media 
thread. In order to do it:
- FrameDeliverer classes in MediaStreamVideoRendererSink and WebMediaPlayerMS 
are moved to media thread.
- WebMediaPlayerMSCompositor::EnqueueFrame gets called on media thread.
- Since WebMediaPlayerMSCompositor operates on three threads, I made it a ref
counted object. This way, we do not need to create multiple WeakPtr factories
or synchronize destrcution on multiple threads. 

[0] https://codereview.chromium.org/2472273002/
[1] crbug.com/667262

BUG=667262,652923
TEST=Tested running JS alert() in console during an AppRTC loopback.
==========

[emircan, 2016-12-02 23:31:43]

Description was changed from

==========
Move passing of WebRTC rendering frames to media thread 

This is follow-up for CL[0] that moved passing of WebRTC rendering frames 
off of main thread. That CL caused a regression in test[1] where the number
of unique frames encoded is less, suggesting that we are dropping frames.
Traces showed that compositor thread may be blocked by long texture upload
calls and frames get accumulated in the meantime. We might be dropping 
frames due to the limit of having only 3 frames in flight. 

This CL addresses this issue by moving these calls to more responsice media 
thread. In order to do it:
- FrameDeliverer classes in MediaStreamVideoRendererSink and WebMediaPlayerMS 
are moved to media thread.
- WebMediaPlayerMSCompositor::EnqueueFrame gets called on media thread.
- Since WebMediaPlayerMSCompositor operates on three threads, I made it a ref
counted object. This way, we do not need to create multiple WeakPtr factories
or synchronize destrcution on multiple threads. 

[0] https://codereview.chromium.org/2472273002/
[1] crbug.com/667262

BUG=667262,652923
TEST=Tested running JS alert() in console during an AppRTC loopback.
==========

to

==========
Move passing of WebRTC rendering frames to media thread 

This is follow-up for CL[0] that moved passing of WebRTC rendering frames 
off of main thread. That CL caused a regression in test[1] where the number
of unique frames encoded is less, suggesting that we are dropping frames.
Traces showed that compositor thread may be blocked by long texture upload
calls and frames get accumulated in the meantime. We might be dropping 
frames due to the limit of having only 3 frames in flight. 

This CL addresses this issue by moving these calls to more responsive media 
thread. In order to do it:
- FrameDeliverer classes in MediaStreamVideoRendererSink and WebMediaPlayerMS 
are moved to media thread.
- WebMediaPlayerMSCompositor::EnqueueFrame gets called on media thread.
- Since WebMediaPlayerMSCompositor operates on three threads, I made it a ref
counted object. This way, we do not need to create multiple WeakPtr factories
or synchronize destruction on multiple threads. 

[0] https://codereview.chromium.org/2472273002/
[1] crbug.com/667262

BUG=667262,652923
TEST=Tested running JS alert() in console during an AppRTC loopback.
==========

[emircan, 2016-12-02 23:39:33]

The CQ bit was checked by emircan@chromium.org to run a CQ dry run

[emircan, 2016-12-02 23:39:41]

Patchset #1 (id:100001) has been deleted

[emircan, 2016-12-02 23:39:52]

PTAL.

[commit-bot: I haz the power, 2016-12-02 23:40:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emircan, 2016-12-02 23:40:28]

Description was changed from

==========
Move passing of WebRTC rendering frames to media thread 

This is follow-up for CL[0] that moved passing of WebRTC rendering frames 
off of main thread. That CL caused a regression in test[1] where the number
of unique frames encoded is less, suggesting that we are dropping frames.
Traces showed that compositor thread may be blocked by long texture upload
calls and frames get accumulated in the meantime. We might be dropping 
frames due to the limit of having only 3 frames in flight. 

This CL addresses this issue by moving these calls to more responsive media 
thread. In order to do it:
- FrameDeliverer classes in MediaStreamVideoRendererSink and WebMediaPlayerMS 
are moved to media thread.
- WebMediaPlayerMSCompositor::EnqueueFrame gets called on media thread.
- Since WebMediaPlayerMSCompositor operates on three threads, I made it a ref
counted object. This way, we do not need to create multiple WeakPtr factories
or synchronize destruction on multiple threads. 

[0] https://codereview.chromium.org/2472273002/
[1] crbug.com/667262

BUG=667262,652923
TEST=Tested running JS alert() in console during an AppRTC loopback.
==========

to

==========
Move passing of WebRTC rendering frames to media thread 

This is follow-up for CL[0] that moved passing of WebRTC rendering frames 
off of main thread. That CL caused a regression in test[1] where the number
of unique frames encoded is less, suggesting that we are dropping frames.
Traces showed that compositor thread may be blocked by long texture upload
calls and frames get accumulated in the meantime. We might be dropping 
frames due to the limit of having only 3 frames in flight. 

This CL addresses this issue by moving these calls to more responsive media 
thread. In order to do it:
- FrameDeliverer classes in MediaStreamVideoRendererSink and WebMediaPlayerMS 
are moved to media thread.
- WebMediaPlayerMSCompositor::EnqueueFrame gets called on media thread.
- Since WebMediaPlayerMSCompositor operates on three threads, I made it a ref
counted object. This way, we do not need to create multiple WeakPtr factories
or synchronize/block destruction on multiple threads. 

[0] https://codereview.chromium.org/2472273002/
[1] crbug.com/667262

BUG=667262,652923
TEST=Tested running JS alert() in console during an AppRTC loopback.
==========

[DaleCurtis, 2016-12-03 01:17:54]

https://codereview.chromium.org/2529263004/diff/120001/content/renderer/media...
File content/renderer/media/webmediaplayer_ms.h (right):

https://codereview.chromium.org/2529263004/diff/120001/content/renderer/media...
content/renderer/media/webmediaplayer_ms.h:223:
scoped_refptr<WebMediaPlayerMSCompositor> compositor_;
Can we avoid this?

[commit-bot: I haz the power, 2016-12-03 01:21:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-03 01:21:47]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[emircan, 2016-12-03 01:42:13]

https://codereview.chromium.org/2529263004/diff/120001/content/renderer/media...
File content/renderer/media/webmediaplayer_ms.h (right):

https://codereview.chromium.org/2529263004/diff/120001/content/renderer/media...
content/renderer/media/webmediaplayer_ms.h:223:
scoped_refptr<WebMediaPlayerMSCompositor> compositor_;
On 2016/12/03 01:17:54, DaleCurtis wrote:
> Can we avoid this? 

I can think of a way of avoiding this, but I mentioned briefly in the CL
description why I preferred not to. Let me know if I am missing smt, you can
think of another way or prefer this.
I still need to post WebMediaPlayerMSCompositor::Enqueue frame on
media_task_runner. (Main and compositor are janky, and using IO all the way here
is too long of a task for IO.) If this class is not ref counted, I would need a
WeakPtr to access it. Then, I would need two WeakPtrFactory's since there is
already one in use for compositor thread here. In dtor(or earlier), I need to
make sure that both are invalidated in their respective threads. That would
require synchronization/blocking until the other is completed.

[qiangchen, 2016-12-05 16:51:58]

I think we've talked about using IO thread injecting frames. What's the reason
you still use media thread doing that?

[emircan, 2016-12-05 18:24:18]

On 2016/12/05 16:51:58, qiangchen wrote:
> I think we've talked about using IO thread injecting frames. What's the reason
> you still use media thread doing that?

I want to address the current regression with this CL, see
https://bugs.chromium.org/p/chromium/issues/detail?id=667262#c15. If this CL
does not sound like a good idea, then I have to revert my earlier CL as well and
go back to square one using main thread.

We cannot make a quick change to IO thread here. AFAIK from an earlier CL
discussion with piman@, IO thread needs to be responsive within 10 to maybe
100µs. Also, we need to be able to send the frame to the other sink callbacks
immediately using IO thread. Enqueing frames here involves acquiring a lock,
reading metadata, checking state, etc. These will all take time and block the IO
thread for more than necessary. We need a refactor of these classes such that
pooling immediately happens on IO thread, ideally on
MediaStreamVideoRendererSink, and WebMediaPlayerMSCompositor pulls frames from
there. I honestly don't know the details of rendering smoothness algorithm at
this point to do the refactor properly. I am currently just making a refactor of
the current code. Main thread was bridging the gap between IO and compositor
here; moving those tasks to compositor didn't work out; so I am proposing moving
them to media thread.

[qiangchen, 2016-12-05 18:54:57]

Some minor suggestions.

https://codereview.chromium.org/2529263004/diff/120001/content/renderer/media...
File content/renderer/media/webmediaplayer_ms_compositor.cc (right):

https://codereview.chromium.org/2529263004/diff/120001/content/renderer/media...
content/renderer/media/webmediaplayer_ms_compositor.cc:415: void
WebMediaPlayerMSCompositor::StartRenderingInternal() {
I think it is better to put these two functions where they are, as it would make
code review a little easier.

https://codereview.chromium.org/2529263004/diff/120001/content/renderer/media...
File content/renderer/media/webmediaplayer_ms_compositor.h (right):

https://codereview.chromium.org/2529263004/diff/120001/content/renderer/media...
content/renderer/media/webmediaplayer_ms_compositor.h:108: void
SetCurrentFrame(const scoped_refptr<media::VideoFrame>& frame);
Can you order this before Render function to kill this extra diff.

https://codereview.chromium.org/2529263004/diff/120001/content/renderer/media...
content/renderer/media/webmediaplayer_ms_compositor.h:159: bool
provider_in_use_;
Can you simply set video_frame_provider_client_ to nullptr in the implementation
of StopUsingProvider. And thus |provider_in_use_|==true is equivalent as
|video_frame_provier_client_|!=nullptr, and thus we can avoid |provider_in_use_|
variable.

[DaleCurtis, 2016-12-05 19:05:35]

Is it really that expensive to run this on the IO thread? The compositor thread
must also be very responsive, due to the whole UI thingy :p

[emircan, 2016-12-05 20:45:57]

On 2016/12/05 19:05:35, DaleCurtis wrote:
> Is it really that expensive to run this on the IO thread? The compositor
thread
> must also be very responsive, due to the whole UI thingy :p

I am moving tasks off of compositor thread in this CL for that exact reason :) 
As I pointed out before, pushing frames as it is now involves acquiring a lock,
reading metadata, checking state, so I would expect this to be a significant
time for IO thread in a low end machine. Let me know if you don't think media
thread is appropriate for pushing frames.

[DaleCurtis, 2016-12-05 21:00:29]

Media thread is fine, but I wonder if just keeping everything on the IO thread
is easier. Was there an explicit issue you moved things to the compositor thread
to fix?

[emircan, 2016-12-05 21:42:41]

On 2016/12/05 21:00:29, DaleCurtis wrote:
> Media thread is fine, but I wonder if just keeping everything on the IO thread
> is easier. Was there an explicit issue you moved things to the compositor
thread
> to fix?

In this earlier CL you reviewed, I moved the push frame tasks from main thread
to compositor: https://codereview.chromium.org/2472273002/ The explicit issue
there was that main thread can be busy/blocked due to other JS calls, i.e.
GetStats(), media recorder, copying arrays. Frames would accumulate in posted
tasks in the meantime and increase memory usage. If we can call
WebMediaPlayerMSCompositor::EnqueueFrame reliably on a different thread, it can
decide to use or discard the frames. Keeping all on IO thread is of course
easier, but I believe it is too long of a task for it as I explained before.

[DaleCurtis, 2016-12-05 22:15:50]

On 2016/12/05 at 21:42:41, emircan wrote:
> On 2016/12/05 21:00:29, DaleCurtis wrote:
> > Media thread is fine, but I wonder if just keeping everything on the IO
thread
> > is easier. Was there an explicit issue you moved things to the compositor
thread
> > to fix?
> 
> In this earlier CL you reviewed, I moved the push frame tasks from main thread
to compositor: https://codereview.chromium.org/2472273002/ The explicit issue
there was that main thread can be busy/blocked due to other JS calls, i.e.
GetStats(), media recorder, copying arrays. Frames would accumulate in posted
tasks in the meantime and increase memory usage. If we can call
WebMediaPlayerMSCompositor::EnqueueFrame reliably on a different thread, it can
decide to use or discard the frames. Keeping all on IO thread is of course
easier, but I believe it is too long of a task for it as I explained before.

The main thread can definitely be blocked for long periods of times due to JS,
etc; but are you saying that you actually saw the IO thread being blocked for
too long?

[emircan, 2016-12-06 00:25:45]

emircan@chromium.org changed reviewers:
- mcasas@chromium.org

[emircan, 2016-12-06 00:32:08]

Description was changed from

==========
Move passing of WebRTC rendering frames to media thread 

This is follow-up for CL[0] that moved passing of WebRTC rendering frames 
off of main thread. That CL caused a regression in test[1] where the number
of unique frames encoded is less, suggesting that we are dropping frames.
Traces showed that compositor thread may be blocked by long texture upload
calls and frames get accumulated in the meantime. We might be dropping 
frames due to the limit of having only 3 frames in flight. 

This CL addresses this issue by moving these calls to more responsive media 
thread. In order to do it:
- FrameDeliverer classes in MediaStreamVideoRendererSink and WebMediaPlayerMS 
are moved to media thread.
- WebMediaPlayerMSCompositor::EnqueueFrame gets called on media thread.
- Since WebMediaPlayerMSCompositor operates on three threads, I made it a ref
counted object. This way, we do not need to create multiple WeakPtr factories
or synchronize/block destruction on multiple threads. 

[0] https://codereview.chromium.org/2472273002/
[1] crbug.com/667262

BUG=667262,652923
TEST=Tested running JS alert() in console during an AppRTC loopback.
==========

to

==========
Move passing of WebRTC rendering frames to media thread 

This is follow-up for CL[0] that moved passing of WebRTC rendering frames 
off of main thread. That CL caused a regression in test[1] where the number
of unique frames encoded is less, suggesting that we are dropping frames.
Traces showed that compositor thread may be blocked by long texture upload
calls and frames get accumulated in the meantime. We might be dropping 
frames due to the limit of having only 3 frames in flight. 

This CL addresses this issue by moving these calls to IO thread directly. 
In order to do it:
- FrameDeliverer classes in MediaStreamVideoRendererSink and WebMediaPlayerMS 
are moved to IO thread.
- WebMediaPlayerMSCompositor::EnqueueFrame gets called on IO thread.
- Since WebMediaPlayerMSCompositor operates on three threads, I made it a ref
counted object. This way, we do not need to create multiple WeakPtr factories
or synchronize/block destruction on multiple threads. 

[0] https://codereview.chromium.org/2472273002/
[1] crbug.com/667262

BUG=667262,652923
TEST=Tested running JS alert() in console during an AppRTC loopback.
==========

[emircan, 2016-12-06 00:36:44]

After offline discussion, I ran a trace to see how long these tasks take on an
average case. In a simple AppRTC loopback, it looks like we spend overall ~0.03
ms in these functions with details below. Note that this will scale with the
number of connections. We decided that it is reasonable to use Io thread, so I
updated the CL and description accordingly. PTAL again.

On Macbook Air 2013:
- WebMediaPlayerMSCompositor::EnqueueFrame 0.021
- WebMediaPlayerMS::OnVideoFrame 0.05
- MediaStreamVideoRendererSink::FrameDeliverer::OnVideoFrame 0.01

https://codereview.chromium.org/2529263004/diff/120001/content/renderer/media...
File content/renderer/media/webmediaplayer_ms_compositor.h (right):

https://codereview.chromium.org/2529263004/diff/120001/content/renderer/media...
content/renderer/media/webmediaplayer_ms_compositor.h:108: void
SetCurrentFrame(const scoped_refptr<media::VideoFrame>& frame);
On 2016/12/05 18:54:57, qiangchen wrote:
> Can you order this before Render function to kill this extra diff.

I wanted to sync the order of declaration/definition in these files while I am
already adding new functions.

https://codereview.chromium.org/2529263004/diff/120001/content/renderer/media...
content/renderer/media/webmediaplayer_ms_compositor.h:159: bool
provider_in_use_;
On 2016/12/05 18:54:57, qiangchen wrote:
> Can you simply set video_frame_provider_client_ to nullptr in the
implementation
> of StopUsingProvider. And thus |provider_in_use_|==true is equivalent as
> |video_frame_provier_client_|!=nullptr, and thus we can avoid
|provider_in_use_|
> variable.

Done.

[emircan, 2016-12-06 00:37:13]

The CQ bit was checked by emircan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 00:37:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[DaleCurtis, 2016-12-06 01:00:42]

https://codereview.chromium.org/2529263004/diff/120001/content/renderer/media...
File content/renderer/media/webmediaplayer_ms.h (right):

https://codereview.chromium.org/2529263004/diff/120001/content/renderer/media...
content/renderer/media/webmediaplayer_ms.h:223:
scoped_refptr<WebMediaPlayerMSCompositor> compositor_;
On 2016/12/03 at 01:42:13, emircan wrote:
> On 2016/12/03 01:17:54, DaleCurtis wrote:
> > Can we avoid this? 
> 
> I can think of a way of avoiding this, but I mentioned briefly in the CL
description why I preferred not to. Let me know if I am missing smt, you can
think of another way or prefer this.
> I still need to post WebMediaPlayerMSCompositor::Enqueue frame on
media_task_runner. (Main and compositor are janky, and using IO all the way here
is too long of a task for IO.) If this class is not ref counted, I would need a
WeakPtr to access it. Then, I would need two WeakPtrFactory's since there is
already one in use for compositor thread here. In dtor(or earlier), I need to
make sure that both are invalidated in their respective threads. That would
require synchronization/blocking until the other is completed.

Is this true? Or would it be sufficient to just generate a cancellable callback
you can hand out?

[commit-bot: I haz the power, 2016-12-06 01:02:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-06 01:03:00]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[qiangchen, 2016-12-06 17:48:17]

https://codereview.chromium.org/2529263004/diff/120001/content/renderer/media...
File content/renderer/media/webmediaplayer_ms_compositor.cc (right):

https://codereview.chromium.org/2529263004/diff/120001/content/renderer/media...
content/renderer/media/webmediaplayer_ms_compositor.cc:415: void
WebMediaPlayerMSCompositor::StartRenderingInternal() {
On 2016/12/05 18:54:57, qiangchen wrote:
> I think it is better to put these two functions where they are, as it would
make
> code review a little easier.

Acknowledged.

https://codereview.chromium.org/2529263004/diff/140001/content/renderer/media...
File content/renderer/media/webmediaplayer_ms_compositor.h (right):

https://codereview.chromium.org/2529263004/diff/140001/content/renderer/media...
content/renderer/media/webmediaplayer_ms_compositor.h:159: bool
provider_in_use_;
Remove this declaration, too.

[emircan, 2016-12-06 19:37:09]

The CQ bit was checked by emircan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 19:37:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emircan, 2016-12-06 19:38:27]

https://codereview.chromium.org/2529263004/diff/140001/content/renderer/media...
File content/renderer/media/webmediaplayer_ms_compositor.h (right):

https://codereview.chromium.org/2529263004/diff/140001/content/renderer/media...
content/renderer/media/webmediaplayer_ms_compositor.h:159: bool
provider_in_use_;
On 2016/12/06 17:48:17, qiangchen wrote:
> Remove this declaration, too.

Done.

[emircan, 2016-12-06 19:38:59]

On 2016/12/06 01:00:42, DaleCurtis wrote:
>
https://codereview.chromium.org/2529263004/diff/120001/content/renderer/media...
> File content/renderer/media/webmediaplayer_ms.h (right):
> 
>
https://codereview.chromium.org/2529263004/diff/120001/content/renderer/media...
> content/renderer/media/webmediaplayer_ms.h:223:
> scoped_refptr<WebMediaPlayerMSCompositor> compositor_;
> On 2016/12/03 at 01:42:13, emircan wrote:
> > On 2016/12/03 01:17:54, DaleCurtis wrote:
> > > Can we avoid this? 
> > 
> > I can think of a way of avoiding this, but I mentioned briefly in the CL
> description why I preferred not to. Let me know if I am missing smt, you can
> think of another way or prefer this.
> > I still need to post WebMediaPlayerMSCompositor::Enqueue frame on
> media_task_runner. (Main and compositor are janky, and using IO all the way
here
> is too long of a task for IO.) If this class is not ref counted, I would need
a
> WeakPtr to access it. Then, I would need two WeakPtrFactory's since there is
> already one in use for compositor thread here. In dtor(or earlier), I need to
> make sure that both are invalidated in their respective threads. That would
> require synchronization/blocking until the other is completed.
> 
> Is this true? Or would it be sufficient to just generate a cancellable
callback
> you can hand out?

CancelableCallback objects must be created on, posted to, cancelled on, and
destroyed on the same thread. AFAIU, we will still have a similar problem.

[commit-bot: I haz the power, 2016-12-06 20:20:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-06 20:20:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[DaleCurtis, 2016-12-06 21:43:26]

lgtm, though I'm always sad to see more scoped_refptr

[qiangchen, 2016-12-07 17:44:13]

A question about the newly added override function.

https://codereview.chromium.org/2529263004/diff/160001/content/renderer/media...
File content/renderer/media/webmediaplayer_ms.h (right):

https://codereview.chromium.org/2529263004/diff/160001/content/renderer/media...
content/renderer/media/webmediaplayer_ms.h:160: bool
texImageImpl(TexImageFunctionID functionID,
I did not see where this new function gets used.

[emircan, 2016-12-07 18:10:05]

https://codereview.chromium.org/2529263004/diff/160001/content/renderer/media...
File content/renderer/media/webmediaplayer_ms.h (right):

https://codereview.chromium.org/2529263004/diff/160001/content/renderer/media...
content/renderer/media/webmediaplayer_ms.h:160: bool
texImageImpl(TexImageFunctionID functionID,
On 2016/12/07 17:44:13, qiangchen wrote:
> I did not see where this new function gets used.

It is added by rebase, not related to my CL.

[qiangchen, 2016-12-07 18:21:23]

lgtm

[emircan, 2016-12-07 18:23:20]

The CQ bit was checked by emircan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 18:23:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emircan, 2016-12-07 18:26:57]

emircan@chromium.org changed reviewers:
+ avi@chromium.org, mcasas@chromium.org

[emircan, 2016-12-07 18:26:58]

mcasas@chromium.org: Please review changes in content/renderer/media/*

avi@chromium.org: Please review changes in content/public/renderer/*,
content/renderer/* and content/shell/renderer/*

[commit-bot: I haz the power, 2016-12-07 20:27:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-07 20:27:17]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[emircan, 2016-12-07 22:03:36]

The CQ bit was checked by emircan@chromium.org to run a CQ dry run

[emircan, 2016-12-07 22:03:48]

Patchset #4 (id:180001) has been deleted

[commit-bot: I haz the power, 2016-12-07 22:04:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 23:26:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-07 23:26:51]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[emircan, 2016-12-08 00:15:07]

The CQ bit was checked by emircan@chromium.org to run a CQ dry run

[emircan, 2016-12-08 00:15:19]

Patchset #4 (id:200001) has been deleted

[commit-bot: I haz the power, 2016-12-08 00:15:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 01:48:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-08 01:48:57]

Dry run: This issue passed the CQ dry run.

[mcasas, 2016-12-08 19:00:47]

On 2016/12/07 18:26:58, emircan wrote:
> mailto:mcasas@chromium.org: Please review changes in content/renderer/media/*
> 
> mailto:avi@chromium.org: Please review changes in content/public/renderer/*,
> content/renderer/* and content/shell/renderer/*

RS lgtm content/renderer/media/*

[emircan, 2016-12-08 21:46:29]

emircan@chromium.org changed reviewers:
- avi@chromium.org

[emircan, 2016-12-08 21:50:17]

emircan@chromium.org changed reviewers:
+ avi@chromium.org, nick@chromium.org

[emircan, 2016-12-08 21:50:18]

It looks like avi@ is OOO.

nick@chromium.org: Can you please review changes in content/public/renderer/*,
content/renderer/* and content/shell/renderer/*

[ncarter (slow), 2016-12-08 22:43:44]

lgtm

https://codereview.chromium.org/2529263004/diff/220001/content/renderer/media...
File content/renderer/media/media_stream_video_renderer_sink.cc (right):

https://codereview.chromium.org/2529263004/diff/220001/content/renderer/media...
content/renderer/media/media_stream_video_renderer_sink.cc:259:
io_task_runner_->PostTaskAndReply(FROM_HERE, base::Bind([] {}),
This doesn't belong in the .cc file for production code; it looks like a random
hack. Can you move it out to wherever you call GetStateForTesting?

[emircan, 2016-12-08 23:25:59]

The CQ bit was checked by emircan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 23:26:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emircan, 2016-12-08 23:26:50]

Thanks.

https://codereview.chromium.org/2529263004/diff/220001/content/renderer/media...
File content/renderer/media/media_stream_video_renderer_sink.cc (right):

https://codereview.chromium.org/2529263004/diff/220001/content/renderer/media...
content/renderer/media/media_stream_video_renderer_sink.cc:259:
io_task_runner_->PostTaskAndReply(FROM_HERE, base::Bind([] {}),
On 2016/12/08 22:43:44, ncarter wrote:
> This doesn't belong in the .cc file for production code; it looks like a
random
> hack. Can you move it out to wherever you call GetStateForTesting?

I moved it to unittest.

[commit-bot: I haz the power, 2016-12-08 23:38:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-08 23:38:19]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[emircan, 2016-12-09 04:36:51]

Description was changed from

==========
Move passing of WebRTC rendering frames to media thread 

This is follow-up for CL[0] that moved passing of WebRTC rendering frames 
off of main thread. That CL caused a regression in test[1] where the number
of unique frames encoded is less, suggesting that we are dropping frames.
Traces showed that compositor thread may be blocked by long texture upload
calls and frames get accumulated in the meantime. We might be dropping 
frames due to the limit of having only 3 frames in flight. 

This CL addresses this issue by moving these calls to IO thread directly. 
In order to do it:
- FrameDeliverer classes in MediaStreamVideoRendererSink and WebMediaPlayerMS 
are moved to IO thread.
- WebMediaPlayerMSCompositor::EnqueueFrame gets called on IO thread.
- Since WebMediaPlayerMSCompositor operates on three threads, I made it a ref
counted object. This way, we do not need to create multiple WeakPtr factories
or synchronize/block destruction on multiple threads. 

[0] https://codereview.chromium.org/2472273002/
[1] crbug.com/667262

BUG=667262,652923
TEST=Tested running JS alert() in console during an AppRTC loopback.
==========

to

==========
Move passing of WebRTC rendering frames to IO thread 

This is follow-up for CL[0] that moved passing of WebRTC rendering frames 
off of main thread. That CL caused a regression in test[1] where the number
of unique frames encoded is less, suggesting that we are dropping frames.
Traces showed that compositor thread may be blocked by long texture upload
calls and frames get accumulated in the meantime. We might be dropping 
frames due to the limit of having only 3 frames in flight. 

This CL addresses this issue by moving these calls to IO thread directly. 
In order to do it:
- FrameDeliverer classes in MediaStreamVideoRendererSink and WebMediaPlayerMS 
are moved to IO thread.
- WebMediaPlayerMSCompositor::EnqueueFrame gets called on IO thread.
- Since WebMediaPlayerMSCompositor operates on three threads, I made it a ref
counted object. This way, we do not need to create multiple WeakPtr factories
or synchronize/block destruction on multiple threads. 

[0] https://codereview.chromium.org/2472273002/
[1] crbug.com/667262

BUG=667262,652923
TEST=Tested running JS alert() in console during an AppRTC loopback.
==========

[emircan, 2016-12-09 04:37:05]

The CQ bit was checked by emircan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 04:37:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-09 06:08:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-09 06:08:03]

Dry run: This issue passed the CQ dry run.

[emircan, 2016-12-09 18:12:50]

The CQ bit was checked by emircan@chromium.org

[emircan, 2016-12-09 18:12:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
qiangchen@chromium.org, dalecurtis@chromium.org, mcasas@chromium.org,
nick@chromium.org
Link to the patchset: https://codereview.chromium.org/2529263004/#ps240001
(title: "ncarter@ comment.")

[commit-bot: I haz the power, 2016-12-09 18:13:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-09 18:44:26]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1481307170188110,
"parent_rev": "25c7933ac7e4e1b143fc4791648d5094e2b79418", "commit_rev":
"d22f81c971f50ed62e3052de9162bba54207d16f"}

[commit-bot: I haz the power, 2016-12-09 18:44:59]

Description was changed from

==========
Move passing of WebRTC rendering frames to IO thread 

This is follow-up for CL[0] that moved passing of WebRTC rendering frames 
off of main thread. That CL caused a regression in test[1] where the number
of unique frames encoded is less, suggesting that we are dropping frames.
Traces showed that compositor thread may be blocked by long texture upload
calls and frames get accumulated in the meantime. We might be dropping 
frames due to the limit of having only 3 frames in flight. 

This CL addresses this issue by moving these calls to IO thread directly. 
In order to do it:
- FrameDeliverer classes in MediaStreamVideoRendererSink and WebMediaPlayerMS 
are moved to IO thread.
- WebMediaPlayerMSCompositor::EnqueueFrame gets called on IO thread.
- Since WebMediaPlayerMSCompositor operates on three threads, I made it a ref
counted object. This way, we do not need to create multiple WeakPtr factories
or synchronize/block destruction on multiple threads. 

[0] https://codereview.chromium.org/2472273002/
[1] crbug.com/667262

BUG=667262,652923
TEST=Tested running JS alert() in console during an AppRTC loopback.
==========

to

==========
Move passing of WebRTC rendering frames to IO thread 

This is follow-up for CL[0] that moved passing of WebRTC rendering frames 
off of main thread. That CL caused a regression in test[1] where the number
of unique frames encoded is less, suggesting that we are dropping frames.
Traces showed that compositor thread may be blocked by long texture upload
calls and frames get accumulated in the meantime. We might be dropping 
frames due to the limit of having only 3 frames in flight. 

This CL addresses this issue by moving these calls to IO thread directly. 
In order to do it:
- FrameDeliverer classes in MediaStreamVideoRendererSink and WebMediaPlayerMS 
are moved to IO thread.
- WebMediaPlayerMSCompositor::EnqueueFrame gets called on IO thread.
- Since WebMediaPlayerMSCompositor operates on three threads, I made it a ref
counted object. This way, we do not need to create multiple WeakPtr factories
or synchronize/block destruction on multiple threads. 

[0] https://codereview.chromium.org/2472273002/
[1] crbug.com/667262

BUG=667262,652923
TEST=Tested running JS alert() in console during an AppRTC loopback.

Review-Url: https://codereview.chromium.org/2529263004
==========

[commit-bot: I haz the power, 2016-12-09 18:45:02]

Committed patchset #5 (id:240001)

[commit-bot: I haz the power, 2016-12-12 14:36:28]

Description was changed from

==========
Move passing of WebRTC rendering frames to IO thread 

This is follow-up for CL[0] that moved passing of WebRTC rendering frames 
off of main thread. That CL caused a regression in test[1] where the number
of unique frames encoded is less, suggesting that we are dropping frames.
Traces showed that compositor thread may be blocked by long texture upload
calls and frames get accumulated in the meantime. We might be dropping 
frames due to the limit of having only 3 frames in flight. 

This CL addresses this issue by moving these calls to IO thread directly. 
In order to do it:
- FrameDeliverer classes in MediaStreamVideoRendererSink and WebMediaPlayerMS 
are moved to IO thread.
- WebMediaPlayerMSCompositor::EnqueueFrame gets called on IO thread.
- Since WebMediaPlayerMSCompositor operates on three threads, I made it a ref
counted object. This way, we do not need to create multiple WeakPtr factories
or synchronize/block destruction on multiple threads. 

[0] https://codereview.chromium.org/2472273002/
[1] crbug.com/667262

BUG=667262,652923
TEST=Tested running JS alert() in console during an AppRTC loopback.

Review-Url: https://codereview.chromium.org/2529263004
==========

to

==========
Move passing of WebRTC rendering frames to IO thread

This is follow-up for CL[0] that moved passing of WebRTC rendering frames
off of main thread. That CL caused a regression in test[1] where the number
of unique frames encoded is less, suggesting that we are dropping frames.
Traces showed that compositor thread may be blocked by long texture upload
calls and frames get accumulated in the meantime. We might be dropping
frames due to the limit of having only 3 frames in flight.

This CL addresses this issue by moving these calls to IO thread directly.
In order to do it:
- FrameDeliverer classes in MediaStreamVideoRendererSink and WebMediaPlayerMS
are moved to IO thread.
- WebMediaPlayerMSCompositor::EnqueueFrame gets called on IO thread.
- Since WebMediaPlayerMSCompositor operates on three threads, I made it a ref
counted object. This way, we do not need to create multiple WeakPtr factories
or synchronize/block destruction on multiple threads.

[0] https://codereview.chromium.org/2472273002/
[1] crbug.com/667262

BUG=667262,652923
TEST=Tested running JS alert() in console during an AppRTC loopback.

Committed: https://crrev.com/8f71ad428b65fc259b48c2a0917158a762a5e5f6
Cr-Commit-Position: refs/heads/master@{#437594}
==========

[commit-bot: I haz the power, 2016-12-12 14:36:29]

Patchset 5 (id:??) landed as
https://crrev.com/8f71ad428b65fc259b48c2a0917158a762a5e5f6
Cr-Commit-Position: refs/heads/master@{#437594}