Fix errors in destruction sequence of GpuVideoEncodeAccelerator::OnWillDestroyStub()

Fix errors in destruction sequence of GpuVideoEncodeAccelerator::OnWillDestroyStub()

On Windows, we can use GPU IO thread to filter some messages for GPUVEA.
In that case, while GpuVideoEncodeAccelerator::OnWillDestroyStub() is running
on main thread, GpuVideoEncodeAccelerator::OnEncodeFrameCreated() can run 
on IO thread. If |encoder_| is reset on main thread and accessed on IO 
thread, we see the crashes listed on the bug. 
This CL solves the issue by:
- Checking |filter_removed_| which is signaled on IO thread before deciding
to continue with encode GpuVideoEncodeAccelerator::OnEncodeFrameCreated() on
IO thread.
- Making sure that |encoder_worker_thread_| tasks are completed before reset.

BUG=715759
TEST=Tested AppRTC loopback with H264.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2849443003
Cr-Commit-Position: refs/heads/master@{#468457}
Committed: https://chromium.googlesource.com/chromium/src/+/a561de6d59f43481c46e61a34508fa15c5fd2abd

[emircan, 2017-04-27 19:58:37]

Description was changed from

==========
fix threading

BUG=
==========

to

==========
fix threading

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[emircan, 2017-04-27 19:58:44]

The CQ bit was checked by emircan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-27 19:59:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-27 20:12:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-27 20:12:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[emircan, 2017-04-27 20:12:27]

The CQ bit was checked by emircan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-27 20:13:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-27 20:48:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-27 20:48:38]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[emircan, 2017-04-27 21:18:46]

Patchset #1 (id:1) has been deleted

[emircan, 2017-04-28 00:26:21]

Patchset #1 (id:20001) has been deleted

[emircan, 2017-04-28 00:28:31]

Description was changed from

==========
fix threading

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Fix errors in destruction sequence of
GpuVideoEncodeAccelerator::OnWillDestroyStub()

On Windows, we can use GPU IO thread to filter some messages for GPUVEA.
In that case, while GpuVideoEncodeAccelerator::OnWillDestroyStub() is running
on main thread, GpuVideoEncodeAccelerator::OnEncodeFrameCreated() can run 
on IO thread. If |encoder_| is reset on main thread and accessed on IO 
thread, we see the crashes listed on the bug. 
This CL solves the issue by:
- Checking |filter_removed_| which is signaled on IO thread before deciding
to continue with encode GpuVideoEncodeAccelerator::OnEncodeFrameCreated() on
IO thread.
- Making sure that |encoder_worker_thread_| tasks are completed before reset.

BUG=715759
TEST=Tested AppRTC loopback with H264.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[emircan, 2017-04-28 00:28:32]

emircan@chromium.org changed reviewers:
+ posciak@chromium.org, sandersd@chromium.org

[emircan, 2017-04-28 00:28:45]

PTAL.

[sandersd (OOO until July 31), 2017-04-28 00:47:44]

https://codereview.chromium.org/2849443003/diff/40001/media/gpu/ipc/service/g...
File media/gpu/ipc/service/gpu_video_encode_accelerator.cc (right):

https://codereview.chromium.org/2849443003/diff/40001/media/gpu/ipc/service/g...
media/gpu/ipc/service/gpu_video_encode_accelerator.cc:328:
weak_this_factory_for_encoder_worker_.GetWeakPtr()));
This WeakPtrFactory should probably not be vending outside of the worker thread.
It seems that it should be possible to create a weak pointer at worker thread
creation time and then just use copies of that?

I'm not entirely sure what's going on with |weak_this_factory_|. It is used for
posting to the main task runner but then is invalidated on the IO task runner?

[emircan, 2017-04-28 01:11:10]

https://codereview.chromium.org/2849443003/diff/40001/media/gpu/ipc/service/g...
File media/gpu/ipc/service/gpu_video_encode_accelerator.cc (right):

https://codereview.chromium.org/2849443003/diff/40001/media/gpu/ipc/service/g...
media/gpu/ipc/service/gpu_video_encode_accelerator.cc:328:
weak_this_factory_for_encoder_worker_.GetWeakPtr()));
On 2017/04/28 00:47:44, sandersd wrote:
> This WeakPtrFactory should probably not be vending outside of the worker
thread.
> It seems that it should be possible to create a weak pointer at worker thread
> creation time and then just use copies of that?

Sure. I changed it such that we use copies of the same WeakPtr.

> 
> I'm not entirely sure what's going on with |weak_this_factory_|. It is used
for
> posting to the main task runner but then is invalidated on the IO task runner?

There is actually 2 WeakPtrFactories here:
- |weak_this_factory_for_encoder_worker_| to post on |encoder_worker_thread_|.
It is also invalidated there.
- |weak_this_factory_| to post either main or io thread, based on |filter_|. The
choice is then stored as |encode_task_runner_| where this is posted. It is
invalidated on io if there is |filter_|, or on main(dtor) otherwise.

[emircan, 2017-04-28 01:16:43]

Patchset #2 (id:60001) has been deleted

[sandersd (OOO until July 31), 2017-04-28 18:22:45]

lgtm

https://codereview.chromium.org/2849443003/diff/40001/media/gpu/ipc/service/g...
File media/gpu/ipc/service/gpu_video_encode_accelerator.cc (right):

https://codereview.chromium.org/2849443003/diff/40001/media/gpu/ipc/service/g...
media/gpu/ipc/service/gpu_video_encode_accelerator.cc:328:
weak_this_factory_for_encoder_worker_.GetWeakPtr()));
> - |weak_this_factory_| to post either main or io thread, based on |filter_|.
The
> choice is then stored as |encode_task_runner_| where this is posted. It is
> invalidated on io if there is |filter_|, or on main(dtor) otherwise.

Please document this in the header file, the current comment about VideoFrames
is not as clear.

https://codereview.chromium.org/2849443003/diff/80001/media/gpu/ipc/service/g...
File media/gpu/ipc/service/gpu_video_encode_accelerator.cc (right):

https://codereview.chromium.org/2849443003/diff/80001/media/gpu/ipc/service/g...
media/gpu/ipc/service/gpu_video_encode_accelerator.cc:174: weak
weak_this_factory_for_encoder_worker_(this),
Presumably does not compile.

[emircan, 2017-04-28 18:28:26]

The CQ bit was checked by emircan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-28 18:29:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 20:07:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-28 20:07:54]

Dry run: This issue passed the CQ dry run.

[emircan, 2017-04-29 00:10:04]

The CQ bit was checked by emircan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-29 00:10:28]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emircan, 2017-04-29 00:10:47]

https://codereview.chromium.org/2849443003/diff/40001/media/gpu/ipc/service/g...
File media/gpu/ipc/service/gpu_video_encode_accelerator.cc (right):

https://codereview.chromium.org/2849443003/diff/40001/media/gpu/ipc/service/g...
media/gpu/ipc/service/gpu_video_encode_accelerator.cc:328:
weak_this_factory_for_encoder_worker_.GetWeakPtr()));
On 2017/04/28 18:22:44, sandersd wrote:
> > - |weak_this_factory_| to post either main or io thread, based on |filter_|.
> The
> > choice is then stored as |encode_task_runner_| where this is posted. It is
> > invalidated on io if there is |filter_|, or on main(dtor) otherwise.
> 
> Please document this in the header file, the current comment about VideoFrames
> is not as clear.

Done.

https://codereview.chromium.org/2849443003/diff/80001/media/gpu/ipc/service/g...
File media/gpu/ipc/service/gpu_video_encode_accelerator.cc (right):

https://codereview.chromium.org/2849443003/diff/80001/media/gpu/ipc/service/g...
media/gpu/ipc/service/gpu_video_encode_accelerator.cc:174: weak
weak_this_factory_for_encoder_worker_(this),
On 2017/04/28 18:22:44, sandersd wrote:
> Presumably does not compile.

Done.

[commit-bot: I haz the power, 2017-04-29 01:44:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-29 01:44:22]

Dry run: This issue passed the CQ dry run.

[emircan, 2017-05-01 17:14:27]

The CQ bit was checked by emircan@chromium.org

[emircan, 2017-05-01 17:14:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
sandersd@chromium.org
Link to the patchset: https://codereview.chromium.org/2849443003/#ps120001
(title: " ")

[commit-bot: I haz the power, 2017-05-01 17:14:50]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-01 22:28:25]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1493658867198150,
"parent_rev": "221e92f4f397ace9c60dd7361c306e7f7a1c4851", "commit_rev":
"a561de6d59f43481c46e61a34508fa15c5fd2abd"}

[commit-bot: I haz the power, 2017-05-01 22:28:35]

Description was changed from

==========
Fix errors in destruction sequence of
GpuVideoEncodeAccelerator::OnWillDestroyStub()

On Windows, we can use GPU IO thread to filter some messages for GPUVEA.
In that case, while GpuVideoEncodeAccelerator::OnWillDestroyStub() is running
on main thread, GpuVideoEncodeAccelerator::OnEncodeFrameCreated() can run 
on IO thread. If |encoder_| is reset on main thread and accessed on IO 
thread, we see the crashes listed on the bug. 
This CL solves the issue by:
- Checking |filter_removed_| which is signaled on IO thread before deciding
to continue with encode GpuVideoEncodeAccelerator::OnEncodeFrameCreated() on
IO thread.
- Making sure that |encoder_worker_thread_| tasks are completed before reset.

BUG=715759
TEST=Tested AppRTC loopback with H264.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Fix errors in destruction sequence of
GpuVideoEncodeAccelerator::OnWillDestroyStub()

On Windows, we can use GPU IO thread to filter some messages for GPUVEA.
In that case, while GpuVideoEncodeAccelerator::OnWillDestroyStub() is running
on main thread, GpuVideoEncodeAccelerator::OnEncodeFrameCreated() can run 
on IO thread. If |encoder_| is reset on main thread and accessed on IO 
thread, we see the crashes listed on the bug. 
This CL solves the issue by:
- Checking |filter_removed_| which is signaled on IO thread before deciding
to continue with encode GpuVideoEncodeAccelerator::OnEncodeFrameCreated() on
IO thread.
- Making sure that |encoder_worker_thread_| tasks are completed before reset.

BUG=715759
TEST=Tested AppRTC loopback with H264.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2849443003
Cr-Commit-Position: refs/heads/master@{#468457}
Committed:
https://chromium.googlesource.com/chromium/src/+/a561de6d59f43481c46e61a34508...
==========

[commit-bot: I haz the power, 2017-05-01 22:28:36]

Committed patchset #4 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/a561de6d59f43481c46e61a34508...