All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=667262

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICgy7--rgoM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICg6_TcvAoM


Bot(s) for this bug's original alert(s):

chromium-webrtc-rel-linux

emircan@ : These regressions seem to possibly be related to your CL https://codereview.chromium.org/2472273002. Could you please have a look to at this?

These seem to happen only on 360p, but not on 720p. I will take a look.

https://chromeperf.appspot.com/report?sid=7fedcd5006e41f08be44269f414745e7ef19a777bdabcead07bc401fd0cb5cde

 Issue 667271  has been merged into this issue.

 Issue 667270  has been merged into this issue.

 Issue 667273  has been merged into this issue.

 Issue 667697  has been merged into this issue.

This test uses MediaRecorder to record all the frames and analyzes afterwards to see unique_frames_count and max_repeated[0]. MediaRecorder is another sink like renderer, where frames are received and processes in their own thread. According to the regression metrics, it looks like we are dropping more frames after my change on compositing. This drop is clear in 360p video content but not in 720p interestingly. I tried to reproduce locally and understand why it is happening only for lower resolution but couldn't come to a conclusion. 

I added trace[1] to see how the behavior changed after my CL[2]. It looks like compositor thread is blocked for a long time with VideoResourceUpdater::CreateForSoftwarePlanes() call when there is SW decode and GMBPool isn't used. Then, all the frames posted on this thread would be waiting for ~15 ms for that blocking call. It is likely that we can drop frames in video_capture_device_client[3] because too many are in flight. It looks like compositor thread is busy as well as the main thread where we started.

In order to address the issue, I moved the render calls from compositor thread to media thread[4] which is also used by webmediaplayer_impl and traced[5]. Media thread seems to be available and sets the current frame quickly in WebMediaPlayerMSCompositor. 

dalecurtis@ and qiangchen@, what do you think about using media thread instead? Is there any restrictions about it that I am not aware of? If it sgtm to you, I will publish the CL for review. 

[0] https://cs.chromium.org/chromium/src/chrome/test/data/webrtc/video_extraction.js?rcl=0&l=54
[1] trace_compositor_thread
[2] https://codereview.chromium.org/2472273002
[3] https://cs.chromium.org/chromium/src/media/capture/video/video_capture_device_client.cc?rcl=0&l=154
[4] https://codereview.chromium.org/2529263004/
[5] trace_media_thread

Deleted: trace_compositor_thread.json.gz 3.3 MB

trace_compositor_thread.json.gz 3.3 MB Download

Deleted: trace_media_thread.json.gz 3.1 MB

trace_media_thread.json.gz 3.1 MB Download

Use the media thread for what exactly?

Use the media thread for delivering frames from MediaStreamVideoRendererSink to WebMediaPlayerMSCompositor. In the earlier CL you reviewed[0], I added FrameDeliverers to move these calls from main to compositor if you remember. Instead of compositor, I propose running these FrameDeliverer calls on media thread, see [1]. Main was blocked by JS calls and it looks like compositor can be blocked by CreateForSoftwarePlanes(), so they accumulate frames.

[0] https://codereview.chromium.org/2472273002/
[1]https://codereview.chromium.org/2529263004/

You should avoid any thread hops if possible. Can whatever code that's providing frames to the MediaStreamVideoRendererSink run on the media thread?

The model we use for <video> is all video rendering happens on media thread and compositor thread calls in for frames every now and then. Instead of pushing frames it's much better if you let the compositor pull them. qiangchen@ will know best here since this will greatly affect smoothness of video.

MediaStreamVideoRendererSink gets the frames from MediaStreamVideoTrack. MediaStreamVideoTrack delivers frames via running the sink callbacks on IO thread[0]. All these callbacks result in a thread hop to keep the IO thread responsive and immediately run the other callbacks as well. Therefore, we cannot avoid the thread hop from IO to media there. 

WebMediaPlayerMSCompositor::EnqueueFrame() would be completed on media thread and call SetCurrentFrame(). Compositor would be running WebMediaPlayerMSCompositor::GetCurrentFrame() independently a similar pull model. I believe this behavior would make it just better.  

[0] https://cs.chromium.org/chromium/src/content/renderer/media/video_track_adapter.cc?rcl=1475162463&l=306

This is not a great model and will lead to jank, but I don't have any objections to using the media thread here. It's likely idle in cases where media streams are being used.

As video track of the remote stream is naturally a push model, but compositing is a pull model, there must be a pool somewhere (now it is the WebMediaPlayerMS).

Compositing pulls happen on compositor thread.
We mainly use IO thread pushing the frames along the code path, but from MediaStreamVideoRendererSink to WebMediaPlayerMS, we used render main thread to do the push before CL 2472273002. After CL 2472273002, we use compositor thread to do the push.

I think the current issue is that compositor thread could be actually busy and thus blocking the push. 

It makes sense to me that we use IO thread to do the push, and thus avoid thread hops. A downside is that WebMediaPlayerMS would deal with three threads:
1. Main thread for handling JS commands;
2. IO thread to enqueue frames;
3. Compositor thread to submit frames for rendering. 

Thanks for the input. I started  issue 669276  to discuss these refactor ideas. Feel free to add them there. 

In the meantime, I will go ahead with this CL to move these call to media thread to address the regression. If not, I will revert all my changes back to using main thread.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8f71ad428b65fc259b48c2a0917158a762a5e5f6

commit 8f71ad428b65fc259b48c2a0917158a762a5e5f6
Author: emircan <emircan@chromium.org>
Date: Fri Dec 09 18:44:21 2016

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
Cr-Commit-Position: refs/heads/master@{#437594}

[modify] https://crrev.com/8f71ad428b65fc259b48c2a0917158a762a5e5f6/content/public/renderer/media_stream_renderer_factory.h
[modify] https://crrev.com/8f71ad428b65fc259b48c2a0917158a762a5e5f6/content/renderer/media/media_stream_renderer_factory_impl.cc
[modify] https://crrev.com/8f71ad428b65fc259b48c2a0917158a762a5e5f6/content/renderer/media/media_stream_renderer_factory_impl.h
[modify] https://crrev.com/8f71ad428b65fc259b48c2a0917158a762a5e5f6/content/renderer/media/media_stream_video_renderer_sink.cc
[modify] https://crrev.com/8f71ad428b65fc259b48c2a0917158a762a5e5f6/content/renderer/media/media_stream_video_renderer_sink.h
[modify] https://crrev.com/8f71ad428b65fc259b48c2a0917158a762a5e5f6/content/renderer/media/media_stream_video_renderer_sink_unittest.cc
[modify] https://crrev.com/8f71ad428b65fc259b48c2a0917158a762a5e5f6/content/renderer/media/webmediaplayer_ms.cc
[modify] https://crrev.com/8f71ad428b65fc259b48c2a0917158a762a5e5f6/content/renderer/media/webmediaplayer_ms.h
[modify] https://crrev.com/8f71ad428b65fc259b48c2a0917158a762a5e5f6/content/renderer/media/webmediaplayer_ms_compositor.cc
[modify] https://crrev.com/8f71ad428b65fc259b48c2a0917158a762a5e5f6/content/renderer/media/webmediaplayer_ms_compositor.h
[modify] https://crrev.com/8f71ad428b65fc259b48c2a0917158a762a5e5f6/content/renderer/media/webmediaplayer_ms_unittest.cc
[modify] https://crrev.com/8f71ad428b65fc259b48c2a0917158a762a5e5f6/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/8f71ad428b65fc259b48c2a0917158a762a5e5f6/content/shell/renderer/layout_test/test_media_stream_renderer_factory.cc
[modify] https://crrev.com/8f71ad428b65fc259b48c2a0917158a762a5e5f6/content/shell/renderer/layout_test/test_media_stream_renderer_factory.h
[modify] https://crrev.com/8f71ad428b65fc259b48c2a0917158a762a5e5f6/content/shell/renderer/layout_test/test_media_stream_video_renderer.cc
[modify] https://crrev.com/8f71ad428b65fc259b48c2a0917158a762a5e5f6/content/shell/renderer/layout_test/test_media_stream_video_renderer.h

The metrics have recovered after the fix, see https://chromeperf.appspot.com/group_report?bug_id=667262. Marking this as fixed.

Added some more recovery alerts to this bug.