Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(827)

Issue 23440015: Fix webrtc HW encode deadlock scenarios. (Closed)

Created:
7 years, 3 months ago by Pawel Osciak
Modified:
7 years, 3 months ago
CC:
chromium-reviews, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, jam, darin-cc_chromium.org
Base URL:
http://git.chromium.org/chromium/src.git@master
Visibility:
Public.

Description

Fix webrtc HW encode deadlock scenarios. Webrtc unfortunately likes to sleep in BaseChannel::Send on the renderer's ChildThread while directly or indirectly calling into HW encoder and we end up in a number of deadlocks of varying complexity in one way or another, while trying to also use the ChildThread to allocate shared memory to service those calls. The only way to avoid this is to not get onto the ChildThread while servicing webrtc requests, so we use the static ChildThread::AllocateSharedMemory() to send the request directly from the current thread. Also add VEA::RequireBitstreamBuffers() to the initialization sequence, so that RTCVideoEncoder::InitEncode() will only return after we've allocated requested buffers. VEA::RequireBitstreamBuffers() is effectively a part of initialization sequence anyway, because we can't really call VEA::Encode() without knowing the VEA impl's coded size requirements. This could also potentially reduce the latency of the first Encode() call. And separately, zero out header structures sent to the client. TEST=apprtc.appspot.com with HW encoding BUG=260210 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221395

Patch Set 1 #

Total comments: 8

Patch Set 2 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+17 lines, -48 lines) Patch
M content/renderer/media/renderer_gpu_video_accelerator_factories.h View 1 5 chunks +9 lines, -17 lines 0 comments Download
M content/renderer/media/renderer_gpu_video_accelerator_factories.cc View 1 4 chunks +5 lines, -30 lines 0 comments Download
M content/renderer/media/rtc_video_encoder.cc View 3 chunks +3 lines, -1 line 0 comments Download

Messages

Total messages: 15 (0 generated)
sheu
7 years, 3 months ago (2013-09-03 20:09:35 UTC) #1
Ami GONE FROM CHROMIUM
Launching a thread per web feature is a bad idea (resource exhaustion attacks). The ownership ...
7 years, 3 months ago (2013-09-03 20:09:53 UTC) #2
Pawel Osciak
On 2013/09/03 20:09:53, Ami Fischman wrote: > Launching a thread per web feature is a ...
7 years, 3 months ago (2013-09-04 01:34:47 UTC) #3
Ami GONE FROM CHROMIUM
> In any case, I'm operating under the assumption that if webrtc sleeps on > ...
7 years, 3 months ago (2013-09-04 02:19:11 UTC) #4
Pawel Osciak
> >> If RequireBB was really part of VEA initialization, then > >> > > ...
7 years, 3 months ago (2013-09-04 11:37:05 UTC) #5
Ami GONE FROM CHROMIUM
On Wed, Sep 4, 2013 at 4:37 AM, <posciak@chromium.org> wrote: > >> If RequireBB was ...
7 years, 3 months ago (2013-09-04 17:34:17 UTC) #6
sheu
> >> > Well, if you look at how this class does it right now, ...
7 years, 3 months ago (2013-09-04 17:55:59 UTC) #7
Ami GONE FROM CHROMIUM
https://chromiumcodereview.appspot.com/23440015/diff/1/content/renderer/media/renderer_gpu_video_accelerator_factories.cc File content/renderer/media/renderer_gpu_video_accelerator_factories.cc (right): https://chromiumcodereview.appspot.com/23440015/diff/1/content/renderer/media/renderer_gpu_video_accelerator_factories.cc#newcode393 content/renderer/media/renderer_gpu_video_accelerator_factories.cc:393: CHECK(shared_memory_segment_->CreateAndMapAnonymous(size)); Do you even sandbox, bro? How is this ...
7 years, 3 months ago (2013-09-04 20:47:55 UTC) #8
Pawel Osciak
https://chromiumcodereview.appspot.com/23440015/diff/1/content/renderer/media/renderer_gpu_video_accelerator_factories.cc File content/renderer/media/renderer_gpu_video_accelerator_factories.cc (right): https://chromiumcodereview.appspot.com/23440015/diff/1/content/renderer/media/renderer_gpu_video_accelerator_factories.cc#newcode393 content/renderer/media/renderer_gpu_video_accelerator_factories.cc:393: CHECK(shared_memory_segment_->CreateAndMapAnonymous(size)); On 2013/09/04 20:47:56, Ami Fischman wrote: > Do ...
7 years, 3 months ago (2013-09-05 05:46:33 UTC) #9
Ami GONE FROM CHROMIUM
LGTM % CL description needing a rewrite
7 years, 3 months ago (2013-09-05 06:18:28 UTC) #10
Pawel Osciak
On 2013/09/05 06:18:28, Ami Fischman wrote: > LGTM % CL description needing a rewrite Thanks. ...
7 years, 3 months ago (2013-09-05 06:57:44 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/posciak@chromium.org/23440015/13001
7 years, 3 months ago (2013-09-05 06:58:03 UTC) #12
commit-bot: I haz the power
Retried try job too often on ios_dbg_simulator for step(s) ui_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_simulator&number=83100
7 years, 3 months ago (2013-09-05 07:36:45 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/posciak@chromium.org/23440015/13001
7 years, 3 months ago (2013-09-05 07:46:19 UTC) #14
commit-bot: I haz the power
7 years, 3 months ago (2013-09-05 13:05:11 UTC) #15
Message was sent while issue was closed.
Change committed as 221395

Powered by Google App Engine
This is Rietveld 408576698