Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2750993002:
Move destruction of VEAEncoder to encoding task runner (Closed)
|
Created:
3 years, 9 months ago by emircan Modified:
3 years, 9 months 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. |
DescriptionMove 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)
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== flakes BUG= ========== to ========== 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 |encoding_task_runner_| is completed before moving on. This CL solves the issues by making sure that BUG=701030 TEST=WebRtcMediaRecorderTest.*FiresErrorEvent tests consistently pass on local Mac builds. ==========
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== 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 |encoding_task_runner_| is completed before moving on. This CL solves the issues by making sure that BUG=701030 TEST=WebRtcMediaRecorderTest.*FiresErrorEvent tests consistently pass on local Mac builds. ========== to ========== 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. ==========
emircan@chromium.org changed reviewers: + mcasas@chromium.org
Patchset #1 (id:1) has been deleted
lgtm https://codereview.chromium.org/2750993002/diff/20001/content/renderer/media_... File content/renderer/media_recorder/video_track_recorder.cc (right): https://codereview.chromium.org/2750993002/diff/20001/content/renderer/media_... 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_... content/renderer/media_recorder/video_track_recorder.cc:596: // |release_waiter| is signaled. I (still) think we need a comment here saying that it's unsafe to just do a encoding_task_runner_->DeleteSoon(FROM_HERE, video_encoder_.release()); because |video_encoder_| holds a naked pointer to us in |encoding_task_runner_|. TODO+BUG point this situation out or not, I'll leave it to you :-) If VEA is staying alive in strange ways because of IPC, mojo and its clear lifetime semantics can be of help #justsayin' https://codereview.chromium.org/2750993002/diff/20001/content/renderer/media_... content/renderer/media_recorder/video_track_recorder.cc:778: DCHECK(encoding_task_runner_->BelongsToCurrentThread()); This one as well.
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2750993002/diff/20001/content/renderer/media_... File content/renderer/media_recorder/video_track_recorder.cc (right): https://codereview.chromium.org/2750993002/diff/20001/content/renderer/media_... content/renderer/media_recorder/video_track_recorder.cc:590: DVLOG(3) << __func__; On 2017/03/15 01:14:17, mcasas wrote: > nit: remove the DVLOG()s Done. https://codereview.chromium.org/2750993002/diff/20001/content/renderer/media_... content/renderer/media_recorder/video_track_recorder.cc:596: // |release_waiter| is signaled. On 2017/03/15 01:14:17, mcasas wrote: > I (still) think we need a comment here saying that > it's unsafe to just do a > encoding_task_runner_->DeleteSoon(FROM_HERE, video_encoder_.release()); > because |video_encoder_| holds a naked pointer to > us in |encoding_task_runner_|. > > TODO+BUG point this situation out or not, I'll leave it > to you :-) > If VEA is staying alive in strange ways because of IPC, > mojo and its clear lifetime semantics can be of help > #justsayin' Done. https://codereview.chromium.org/2750993002/diff/20001/content/renderer/media_... content/renderer/media_recorder/video_track_recorder.cc:778: DCHECK(encoding_task_runner_->BelongsToCurrentThread()); On 2017/03/15 01:14:17, mcasas wrote: > This one as well. Done.
The CQ bit was unchecked by emircan@chromium.org
The CQ bit was checked by emircan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org Link to the patchset: https://codereview.chromium.org/2750993002/#ps40001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
content/renderer/media_recorder/video_track_recorder.cc:
While running git apply --index -p1;
error: patch failed:
content/renderer/media_recorder/video_track_recorder.cc:449
error: content/renderer/media_recorder/video_track_recorder.cc: patch does not
apply
Patch: content/renderer/media_recorder/video_track_recorder.cc
Index: content/renderer/media_recorder/video_track_recorder.cc
diff --git a/content/renderer/media_recorder/video_track_recorder.cc
b/content/renderer/media_recorder/video_track_recorder.cc
index
d77d300d8da957f3b0675927fff963acca09c093..30e33f51aa9803b026c3cb82e12da2fca5e1d69a
100644
--- a/content/renderer/media_recorder/video_track_recorder.cc
+++ b/content/renderer/media_recorder/video_track_recorder.cc
@@ -449,6 +449,8 @@ class VEAEncoder final : public VideoTrackRecorder::Encoder,
base::TimeTicks capture_timestamp) override;
void ConfigureEncoderOnEncodingTaskRunner(const gfx::Size& size) override;
+ void DestroyOnEncodingTaskRunner(base::WaitableEvent* async_waiter);
+
media::GpuVideoAcceleratorFactories* const gpu_factories_;
const media::VideoCodecProfile codec_;
@@ -585,9 +587,20 @@ VEAEncoder::VEAEncoder(
}
VEAEncoder::~VEAEncoder() {
+ base::WaitableEvent release_waiter(
+ base::WaitableEvent::ResetPolicy::MANUAL,
+ base::WaitableEvent::InitialState::NOT_SIGNALED);
+ // base::Unretained is safe because the class will be alive until
+ // |release_waiter| is signaled.
+ // TODO(emircan): Consider refactoring media::VideoEncodeAccelerator to avoid
+ // using naked pointers and using DeleteSoon() here, see
+ // http://crbug.com/701627.
+ // It is currently unsafe because |video_encoder_| might be in use on another
+ // function on |encoding_task_runner_|, see http://crbug.com/701030.
encoding_task_runner_->PostTask(
- FROM_HERE, base::Bind(&media::VideoEncodeAccelerator::Destroy,
- base::Unretained(video_encoder_.release())));
+ FROM_HERE, base::Bind(&VEAEncoder::DestroyOnEncodingTaskRunner,
+ base::Unretained(this), &release_waiter));
+ release_waiter.Wait();
}
void VEAEncoder::RequireBitstreamBuffers(unsigned int /*input_count*/,
@@ -744,10 +757,7 @@ void VEAEncoder::EncodeOnEncodingTaskRunner(
frames_in_encode_.push(std::make_pair(
media::WebmMuxer::VideoParameters(frame), capture_timestamp));
- encoding_task_runner_->PostTask(
- FROM_HERE,
- base::Bind(&media::VideoEncodeAccelerator::Encode,
- base::Unretained(video_encoder_.get()), video_frame, false));
+ video_encoder_->Encode(video_frame, false);
}
void VEAEncoder::ConfigureEncoderOnEncodingTaskRunner(const gfx::Size& size) {
@@ -765,6 +775,13 @@ void VEAEncoder::ConfigureEncoderOnEncodingTaskRunner(const
gfx::Size& size) {
}
}
+void VEAEncoder::DestroyOnEncodingTaskRunner(
+ base::WaitableEvent* async_waiter) {
+ DCHECK(encoding_task_runner_->BelongsToCurrentThread());
+ video_encoder_.reset();
+ async_waiter->Signal();
+}
+
// static
void VpxEncoder::ShutdownEncoder(std::unique_ptr<base::Thread> encoding_thread,
ScopedVpxCodecCtxPtr encoder) {
The CQ bit was checked by emircan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
content/renderer/media_recorder/video_track_recorder.cc:
While running git apply --index -p1;
error: patch failed:
content/renderer/media_recorder/video_track_recorder.cc:449
error: content/renderer/media_recorder/video_track_recorder.cc: patch does not
apply
Patch: content/renderer/media_recorder/video_track_recorder.cc
Index: content/renderer/media_recorder/video_track_recorder.cc
diff --git a/content/renderer/media_recorder/video_track_recorder.cc
b/content/renderer/media_recorder/video_track_recorder.cc
index
d77d300d8da957f3b0675927fff963acca09c093..30e33f51aa9803b026c3cb82e12da2fca5e1d69a
100644
--- a/content/renderer/media_recorder/video_track_recorder.cc
+++ b/content/renderer/media_recorder/video_track_recorder.cc
@@ -449,6 +449,8 @@ class VEAEncoder final : public VideoTrackRecorder::Encoder,
base::TimeTicks capture_timestamp) override;
void ConfigureEncoderOnEncodingTaskRunner(const gfx::Size& size) override;
+ void DestroyOnEncodingTaskRunner(base::WaitableEvent* async_waiter);
+
media::GpuVideoAcceleratorFactories* const gpu_factories_;
const media::VideoCodecProfile codec_;
@@ -585,9 +587,20 @@ VEAEncoder::VEAEncoder(
}
VEAEncoder::~VEAEncoder() {
+ base::WaitableEvent release_waiter(
+ base::WaitableEvent::ResetPolicy::MANUAL,
+ base::WaitableEvent::InitialState::NOT_SIGNALED);
+ // base::Unretained is safe because the class will be alive until
+ // |release_waiter| is signaled.
+ // TODO(emircan): Consider refactoring media::VideoEncodeAccelerator to avoid
+ // using naked pointers and using DeleteSoon() here, see
+ // http://crbug.com/701627.
+ // It is currently unsafe because |video_encoder_| might be in use on another
+ // function on |encoding_task_runner_|, see http://crbug.com/701030.
encoding_task_runner_->PostTask(
- FROM_HERE, base::Bind(&media::VideoEncodeAccelerator::Destroy,
- base::Unretained(video_encoder_.release())));
+ FROM_HERE, base::Bind(&VEAEncoder::DestroyOnEncodingTaskRunner,
+ base::Unretained(this), &release_waiter));
+ release_waiter.Wait();
}
void VEAEncoder::RequireBitstreamBuffers(unsigned int /*input_count*/,
@@ -744,10 +757,7 @@ void VEAEncoder::EncodeOnEncodingTaskRunner(
frames_in_encode_.push(std::make_pair(
media::WebmMuxer::VideoParameters(frame), capture_timestamp));
- encoding_task_runner_->PostTask(
- FROM_HERE,
- base::Bind(&media::VideoEncodeAccelerator::Encode,
- base::Unretained(video_encoder_.get()), video_frame, false));
+ video_encoder_->Encode(video_frame, false);
}
void VEAEncoder::ConfigureEncoderOnEncodingTaskRunner(const gfx::Size& size) {
@@ -765,6 +775,13 @@ void VEAEncoder::ConfigureEncoderOnEncodingTaskRunner(const
gfx::Size& size) {
}
}
+void VEAEncoder::DestroyOnEncodingTaskRunner(
+ base::WaitableEvent* async_waiter) {
+ DCHECK(encoding_task_runner_->BelongsToCurrentThread());
+ video_encoder_.reset();
+ async_waiter->Signal();
+}
+
// static
void VpxEncoder::ShutdownEncoder(std::unique_ptr<base::Thread> encoding_thread,
ScopedVpxCodecCtxPtr encoder) {
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by emircan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org Link to the patchset: https://codereview.chromium.org/2750993002/#ps60001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489594819710240,
"parent_rev": "3998fbc50bb215733a081780b286804aa18225c3", "commit_rev":
"e8acd88950a74706654ce51f2613a96d0e46d298"}
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489594819710240,
"parent_rev": "2407c2d02fa072311a4421da6bb5f6a0ae628c3b", "commit_rev":
"caa1c995a9a8dc99a4d6999d78f796aa4dbdae0f"}
Message was sent while issue was closed.
Description was changed from ========== 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. ========== to ========== 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/+/caa1c995a9a8dc99a4d6999d78f7... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/caa1c995a9a8dc99a4d6999d78f7... |
