I think there shouldn't be any hard limit on the number of frames in the pool and I'd avoid defensive programming in this case.
IIRC you noticed we ignore a DCHECK earlier that triggers in WebRTC when the number of video frames grew past a certain number.

We should make sure that that DCHECK is respected, not try to recover later.


On the other side, BindAndCreateMailboxesHardwareFrameResources shouldn't freeze, that seems like an issue worth investigating. I'm guessing it freezes since the GPU process dies and we might be waiting for it to restart.
Do you know if it eventually recovers? A freeze in the UI for a few seconds is expected in case of a GPU process crash, if it never recovers, we should look into that.

It does not recover after freeze as far as I tested. Note that it does not freeze each time either. It sometimes renders through them really fast and keep going. There might be deadlock, so it is worth investigating there.

These are the checks in WebRTC. VP8[0] is already a hard limit of 300 and not enough to cover the freeze case in hangouts. 
[0] https://cs.chromium.org/chromium/src/third_party/webrtc/common_video/i420_buffer_pool.cc?rcl=0&l=29
[1] https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/codecs/vp9/vp9_frame_buffer_pool.cc?rcl=0&l=76
GMBPool only grows with the number of frames and holds the memory until destructed. I think it still would be worth to add a limit within the pool's |resources_pool_| and directly call |frame_cb| after a certain limit.
https://cs.chromium.org/chromium/src/media/video/gpu_memory_buffer_video_frame_pool.cc?rcl=0&l=692|

It doesn't freeze all the times because not all the time we ran out of memory.

I'm not sure there is any deadlock in the pool itself, we use a scopedlock everywhere. We should at least try to understand what we're waiting on in BindAndCreateMailboxesHardwareFrameResources, as it is right now the deadlock in the pool is just a guess.

>> GMBPool only grows with the number of frames and holds the memory until destructed. I think it still would be worth to add a limit within the pool's |resources_pool_| and directly call |frame_cb| after a certain limit.

GMBPool is basically an allocator, we could shrink the pool after a while if you prefer, but putting a limit on the allocated memory doesn't really make any sense; we'd be just trying to recover a situation that went bad somewhere else, hiding the real problem instead of making it clear, and trying to alleviate a symptoms instead of fixing the root cause.

Whoever is trying to allocate all those video frames in the first place should not, and if they allocate hundreds of VF, OOMing is a completely reasonable way of dealing with it.

300 frames, if they're at 4k, it's about 3.5 GB of data. That limit doesn't seem reasonable. For video playback we have just a few frames in flight, not 300. Also, calling frame_cb might make compositing performance worse, and in the future we might not even support drawing without GBMs.

Bumping to M57. Please update if that's wrong.

Bumping this to M58. Please correct if that's wrong.

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

commit b035f281595474c98fef2c325e159e4bed351795
Author: Emircan Uysaler <emircan@chromium.org>
Date: Fri Feb 23 23:51:10 2018

Move GpuMemoryBufferVideoFramePool use into WebMediaPlayerMS::FrameDeliverer

This CL moves GMBVFP usage inside WebMediaPlayerMS. Since MediaStream no longer
passes frames through main thread, earlier blocker for the bugs have been
resolved. Uploading to GMBs in the WebMediaPlayerMS section where we pass on IO
thread makes more sense as we can track hidden or backgrounded state there.

Bug:  653200 
Change-Id: I95a9ca1cfa52d94e9001b4a499cc3ab71c2c71a3
Reviewed-on: https://chromium-review.googlesource.com/927707
Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Emircan Uysaler <emircan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538948}
[modify] https://crrev.com/b035f281595474c98fef2c325e159e4bed351795/content/public/renderer/media_stream_renderer_factory.h
[modify] https://crrev.com/b035f281595474c98fef2c325e159e4bed351795/content/renderer/media/stream/media_stream_renderer_factory_impl.cc
[modify] https://crrev.com/b035f281595474c98fef2c325e159e4bed351795/content/renderer/media/stream/media_stream_renderer_factory_impl.h
[modify] https://crrev.com/b035f281595474c98fef2c325e159e4bed351795/content/renderer/media/stream/media_stream_video_renderer_sink.cc
[modify] https://crrev.com/b035f281595474c98fef2c325e159e4bed351795/content/renderer/media/stream/media_stream_video_renderer_sink.h
[modify] https://crrev.com/b035f281595474c98fef2c325e159e4bed351795/content/renderer/media/stream/media_stream_video_renderer_sink_unittest.cc
[modify] https://crrev.com/b035f281595474c98fef2c325e159e4bed351795/content/renderer/media/stream/webmediaplayer_ms.cc
[modify] https://crrev.com/b035f281595474c98fef2c325e159e4bed351795/content/renderer/media/stream/webmediaplayer_ms.h
[modify] https://crrev.com/b035f281595474c98fef2c325e159e4bed351795/content/renderer/media/stream/webmediaplayer_ms_unittest.cc
[modify] https://crrev.com/b035f281595474c98fef2c325e159e4bed351795/content/shell/renderer/layout_test/test_media_stream_renderer_factory.cc
[modify] https://crrev.com/b035f281595474c98fef2c325e159e4bed351795/content/shell/renderer/layout_test/test_media_stream_renderer_factory.h

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

commit 1f31a184a7f74cf3f1bbafc21487d5ccb9b5bb08
Author: Emircan Uysaler <emircan@chromium.org>
Date: Tue Mar 13 23:33:38 2018

Stop using GpuMemoryBufferVideoFramePool when WebMediaPlayerMS is hidden

This CL adds calls to track when WebMediaPlayerMS is hidden or shown, so
that in the time period we can skip creating GMB backed frames. For that
time period, frames aren't going to be displayed, so copying them to GMBs
is extra work which should be avoided.

Bug:  653200 
Change-Id: I67e55c7f1150b434d82321ac90a08c7c3e3e6336
Reviewed-on: https://chromium-review.googlesource.com/954339
Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Commit-Queue: Emircan Uysaler <emircan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542950}
[modify] https://crrev.com/1f31a184a7f74cf3f1bbafc21487d5ccb9b5bb08/content/renderer/media/stream/webmediaplayer_ms.cc
[modify] https://crrev.com/1f31a184a7f74cf3f1bbafc21487d5ccb9b5bb08/content/renderer/media/stream/webmediaplayer_ms_unittest.cc

The new tests crashes under Fuchsia. I see that the test is not built/run under Android, but there's no comment to explain why not.  Will add OS_FUCHSIA to that conditional to un-break things for now.

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

commit f6b06f4d7c41c4e095fe52dd07701e9209e17928
Author: Caleb Rouleau <crouleau@chromium.org>
Date: Wed Mar 14 17:32:17 2018

Revert "Stop using GpuMemoryBufferVideoFramePool when WebMediaPlayerMS is hidden"

This reverts commit 1f31a184a7f74cf3f1bbafc21487d5ccb9b5bb08.

Reason for revert: "WebMediaPlayerMSTest.StopsCreatingHardwareFramesWhenHiddenOrClosed" is flaky. https://bugs.chromium.org/p/chromium/issues/detail?id=821839

Original change's description:
> Stop using GpuMemoryBufferVideoFramePool when WebMediaPlayerMS is hidden
> 
> This CL adds calls to track when WebMediaPlayerMS is hidden or shown, so
> that in the time period we can skip creating GMB backed frames. For that
> time period, frames aren't going to be displayed, so copying them to GMBs
> is extra work which should be avoided.
> 
> Bug:  653200 
> Change-Id: I67e55c7f1150b434d82321ac90a08c7c3e3e6336
> Reviewed-on: https://chromium-review.googlesource.com/954339
> Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
> Reviewed-by: Dan Sanders <sandersd@chromium.org>
> Commit-Queue: Emircan Uysaler <emircan@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#542950}

TBR=sandersd@chromium.org,dcastagna@chromium.org,emircan@chromium.org

Change-Id: Ib88d6f0419271bf7d5e65b1b017eef1e12aeecbb
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  653200 
Reviewed-on: https://chromium-review.googlesource.com/962824
Reviewed-by: Caleb Rouleau <crouleau@chromium.org>
Commit-Queue: Caleb Rouleau <crouleau@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543118}
[modify] https://crrev.com/f6b06f4d7c41c4e095fe52dd07701e9209e17928/content/renderer/media/stream/webmediaplayer_ms.cc
[modify] https://crrev.com/f6b06f4d7c41c4e095fe52dd07701e9209e17928/content/renderer/media/stream/webmediaplayer_ms_unittest.cc

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

commit c3089ac93ef14584f30f20ea82e219f297d4dfb5
Author: Emircan Uysaler <emircan@chromium.org>
Date: Wed Mar 14 23:57:43 2018

Reland "Stop using GpuMemoryBufferVideoFramePool when WebMediaPlayerMS is hidden"

This is a reland of 1f31a184a7f74cf3f1bbafc21487d5ccb9b5bb08

Original change's description:
> Stop using GpuMemoryBufferVideoFramePool when WebMediaPlayerMS is hidden
>
> This CL adds calls to track when WebMediaPlayerMS is hidden or shown, so
> that in the time period we can skip creating GMB backed frames. For that
> time period, frames aren't going to be displayed, so copying them to GMBs
> is extra work which should be avoided.
>
> Bug:  653200 
> Change-Id: I67e55c7f1150b434d82321ac90a08c7c3e3e6336
> Reviewed-on: https://chromium-review.googlesource.com/954339
> Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
> Reviewed-by: Dan Sanders <sandersd@chromium.org>
> Commit-Queue: Emircan Uysaler <emircan@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#542950}

Bug:  653200 
TBR: sandersd@chromium.org, dcastagna@chromium.org
Change-Id: I277eca14cac19e80b4b00dae75188842d7b49c97
Reviewed-on: https://chromium-review.googlesource.com/963581
Reviewed-by: Emircan Uysaler <emircan@chromium.org>
Commit-Queue: Emircan Uysaler <emircan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543241}
[modify] https://crrev.com/c3089ac93ef14584f30f20ea82e219f297d4dfb5/content/renderer/media/stream/webmediaplayer_ms.cc
[modify] https://crrev.com/c3089ac93ef14584f30f20ea82e219f297d4dfb5/content/renderer/media/stream/webmediaplayer_ms_unittest.cc

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

commit e269e69cccd1561bb8270760d46cabc40c0c60b9
Author: Emircan Uysaler <emircan@chromium.org>
Date: Thu Mar 22 18:40:24 2018

Refactor GpuMemoryBufferVideoFramePool usage in WebMediaPlayerMS

- Posts GpuMemoryBufferVideoFramePool::Abort() calls on the correct thread.
- Avoids unnecessary Abort calls when there isn't an active task.
- Renames FrameReady() to EnqueueFrame().

Bug:  653200 
Change-Id: Iaa98be89f34cf4d76855c51b6121b877bf44a6c5
Reviewed-on: https://chromium-review.googlesource.com/974308
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Commit-Queue: Emircan Uysaler <emircan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545168}
[modify] https://crrev.com/e269e69cccd1561bb8270760d46cabc40c0c60b9/content/renderer/media/stream/webmediaplayer_ms.cc

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

commit 88424202fe0a6059b0e516fa011b0be7ef7af1ac
Author: Emircan Uysaler <emircan@chromium.org>
Date: Thu Mar 29 06:48:49 2018

Enable WebRtcUseGpuMemoryBufferVideoFrame feature by default

Now that all the blocker issues(see bug below) are fixed, we can reenable this
feature.

Bug:  653200 
Change-Id: I42c6bc582c4b7e4af770e093e46bbf12357c7753
Reviewed-on: https://chromium-review.googlesource.com/963642
Commit-Queue: Daniele Castagna <dcastagna@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546751}
[modify] https://crrev.com/88424202fe0a6059b0e516fa011b0be7ef7af1ac/content/public/common/content_features.cc

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

commit 4b1011cb419e8d2f6e52d3b378a5db4c56e934b0
Author: Markus Heintz <markusheintz@chromium.org>
Date: Thu Mar 29 10:38:52 2018

Revert "Enable WebRtcUseGpuMemoryBufferVideoFrame feature by default"

This reverts commit 88424202fe0a6059b0e516fa011b0be7ef7af1ac.

Reason for revert:
Findit (https://goo.gl/kROfz5) identified this CL at revision 546751 as the culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtLzg4NDI0MjAyZmUwYTYwNTliMGU1MTZmYTAxMWIwYmU3ZWY3YWYxYWMM

See 
https://ci.chromium.org/buildbot/chromium.linux/Linux%20Tests%20(dbg)(1)(32)/49093
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20Tests%20(dbg)(1)/71241 
https://logs.chromium.org/v/?s=chromium%2Fbuildbucket%2Fcr-buildbucket.appspot.com%2F8950737227952100496%2F%2B%2Fsteps%2Fviz_content_browsertests%2F0%2Flogs%2FWebRtcGetUserMediaBrowserTest.SrcObjectRemoveFirstOfTwoVideoTracks%2F0

Failing Test: WebRtcGetUserMediaBrowserTest.SrcObjectRemoveFirstOfTwoVideoTracks

Original change's description:
> Enable WebRtcUseGpuMemoryBufferVideoFrame feature by default
> 
> Now that all the blocker issues(see bug below) are fixed, we can reenable this
> feature.
> 
> Bug:  653200 
> Change-Id: I42c6bc582c4b7e4af770e093e46bbf12357c7753
> Reviewed-on: https://chromium-review.googlesource.com/963642
> Commit-Queue: Daniele Castagna <dcastagna@chromium.org>
> Reviewed-by: Avi Drissman <avi@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#546751}

TBR=avi@chromium.org,dcastagna@chromium.org,emircan@chromium.org

Change-Id: I1d760473421747078cade100b76ed209f3899c3a
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  653200 
Reviewed-on: https://chromium-review.googlesource.com/986252
Reviewed-by: Markus Heintz <markusheintz@chromium.org>
Commit-Queue: Markus Heintz <markusheintz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546785}
[modify] https://crrev.com/4b1011cb419e8d2f6e52d3b378a5db4c56e934b0/content/public/common/content_features.cc

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

commit a1b68fd522584c38840cc4adc3396812e59f6809
Author: Emircan Uysaler <emircan@chromium.org>
Date: Fri Mar 30 15:44:19 2018

Reland "Enable WebRtcUseGpuMemoryBufferVideoFrame feature by default"

This is a reland of 88424202fe0a6059b0e516fa011b0be7ef7af1ac
The flakiness caused in tests are fixed at
https://chromium-review.googlesource.com/c/chromium/src/+/986988

Original change's description:
> Enable WebRtcUseGpuMemoryBufferVideoFrame feature by default
>
> Now that all the blocker issues(see bug below) are fixed, we can reenable this
> feature.
>
> Bug:  653200 
> Change-Id: I42c6bc582c4b7e4af770e093e46bbf12357c7753
> Reviewed-on: https://chromium-review.googlesource.com/963642
> Commit-Queue: Daniele Castagna <dcastagna@chromium.org>
> Reviewed-by: Avi Drissman <avi@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#546751}

Bug:  653200 
TBR: avi@chromium.org
Change-Id: I1d16ecdfe79e8b4d2809182d73ac39c8ea569003
Reviewed-on: https://chromium-review.googlesource.com/986725
Reviewed-by: Emircan Uysaler <emircan@chromium.org>
Commit-Queue: Emircan Uysaler <emircan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547170}
[modify] https://crrev.com/a1b68fd522584c38840cc4adc3396812e59f6809/content/public/common/content_features.cc

Issue 837792 has been merged into this issue.