Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(22)

Issue 2750993002: Move destruction of VEAEncoder to encoding task runner (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 months, 1 week ago by emircan
Modified:
3 months, 1 week ago
Reviewers:
mcasas
CC:
chromium-reviews, mlamouri+watch-content_chromium.org, emircan+watch+mediarecorder_chromium.org, posciak+watch_chromium.org, jam, mcasas+watch+mediarecorder_chromium.org, darin-cc_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Move destruction of VEAEncoder to encoding task runner We were seeing random crashes in WebRtcMediaRecorderTest.*FiresErrorEvent tests because: - GpuVideoEncodeAcceleratorHost::Initialize() takes a naked ptr to |client| which is VEAEncoder. - GpuVideoEncodeAcceleratorHost::OnRequireBitstreamBuffers() calls client->OnRequireBitstreamBuffers(). - While VEAEncoder::RequireBitstreamBuffers() is running on |encoding_task_runner_|, ~VEAEncoder() gets called on main render thread through ~VideoTrackRecorder() after the error event. ~ VEAEncoder() needs a mechanism similar to H264Encoder::ShutdownEncoder() to make sure that shutdown tasks on |encoding_task_runner_| are completed before moving on. This CL solves the issues by making sure that dtor completes tasks on encoding task runner via DestroyOnEncodingTaskRunner(). BUG=701030 TEST=WebRtcMediaRecorderTest.*FiresErrorEvent tests consistently pass on local Mac builds. Review-Url: https://codereview.chromium.org/2750993002 Cr-Commit-Position: refs/heads/master@{#457101} Committed: https://chromium.googlesource.com/chromium/src/+/caa1c995a9a8dc99a4d6999d78f796aa4dbdae0f

Patch Set 1 : #

Total comments: 6

Patch Set 2 : #

Patch Set 3 : #

Messages

Total messages: 33 (25 generated)
mcasas
lgtm https://codereview.chromium.org/2750993002/diff/20001/content/renderer/media_recorder/video_track_recorder.cc File content/renderer/media_recorder/video_track_recorder.cc (right): https://codereview.chromium.org/2750993002/diff/20001/content/renderer/media_recorder/video_track_recorder.cc#newcode590 content/renderer/media_recorder/video_track_recorder.cc:590: DVLOG(3) << __func__; nit: remove the DVLOG()s https://codereview.chromium.org/2750993002/diff/20001/content/renderer/media_recorder/video_track_recorder.cc#newcode596 ...
3 months, 1 week ago (2017-03-15 01:14:18 UTC) #9
emircan
https://codereview.chromium.org/2750993002/diff/20001/content/renderer/media_recorder/video_track_recorder.cc File content/renderer/media_recorder/video_track_recorder.cc (right): https://codereview.chromium.org/2750993002/diff/20001/content/renderer/media_recorder/video_track_recorder.cc#newcode590 content/renderer/media_recorder/video_track_recorder.cc:590: DVLOG(3) << __func__; On 2017/03/15 01:14:17, mcasas wrote: > ...
3 months, 1 week ago (2017-03-15 01:36:18 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2750993002/40001
3 months, 1 week ago (2017-03-15 01:53:08 UTC) #16
commit-bot: I haz the power
Failed to apply patch for content/renderer/media_recorder/video_track_recorder.cc: While running git apply --index -p1; error: patch failed: ...
3 months, 1 week ago (2017-03-15 03:00:16 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2750993002/40001
3 months, 1 week ago (2017-03-15 04:00:12 UTC) #20
commit-bot: I haz the power
Failed to apply patch for content/renderer/media_recorder/video_track_recorder.cc: While running git apply --index -p1; error: patch failed: ...
3 months, 1 week ago (2017-03-15 04:04:44 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2750993002/60001
3 months, 1 week ago (2017-03-15 16:20:50 UTC) #29
commit-bot: I haz the power
3 months, 1 week ago (2017-03-15 16:30:52 UTC) #33
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/caa1c995a9a8dc99a4d6999d78f7...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cb946e318