New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 760822 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug

Blocking:
issue 660942



Sign in to add a comment

Make AVDACodecAllocator safe to use from multiple threads

Project Member Reported by w...@chromium.org, Aug 31 2017

Issue description

MCVD and AVDA need to both be able to use this concurrently (AVDA from GPU thread for WebRTC, MCVD from media service thread).
 
Cc: -liber...@chromium.org w...@chromium.org
Owner: liber...@chromium.org

Comment 2 by w...@chromium.org, Sep 15 2017

Thanks! Let me do a quick brain dump of related things:

* MCVD doesn't support CVV. So no sync SurfaceDestroyed stuff to worry about. 

* I'm thinking we will just use AndroidSurfaceChooser and AndroidOverlayMojos only on the gpu main thread. Without sync surfacedestroyed it should be fine to host these on a different thread.

* Thinking that making AVDASurfaceBundle RefCountedDeleteOnSequence would be a good idea to handle some edge cases with codec creation when MCVD goes away (SurfaceTextureGLOwner is an example).
+1 RedCountedDeleteOnSequence.

some thoughts on AVDASurfaceChooser:

besides destroy, there's very little that we do with AndroidOverlayMojo in AVDA: get its java surface, and register a destruction callback is the whole list, i think.

i think it'll be the same in MCVD, and the last two can just be folded into surfacechooser to avoid multi-threading in AO entirely, i think.  as in, surfacechooser can be told during Initialize() what DestroyedCB the client would like to use.  it can then set the destruction callback on its own thread before notifying the client when the overlay is ready.  MCVD would provide a thread-hopping callback.

actually, it has no choice but to do it before providing the overlay, if UseOverlayCB hops threads to MCVD thread.  else the overlay could be destroyed before the MCVD thread registers the destruction callback.

for GetJavaSurface, we can just send it as an additional argument to UseOverlayCB.  not sure what the rules with JavaRef and threads are, but pretty sure there's a way to make it work.  this is somewhat hacky, i suppose.
i think that i have a plan for AVDACodecAllocator.  the use of WeakPtrs to ensure that threads are stopped correctly makes it harder.

i think i'm going to make all the calls post to the gpu main thread if they're called elsewhere.  this enforces proper ordering.  that's the only way i could see to ensure that stopping the codec threads can be done without a risk of something else posting to them.

will also make StartThread return success asynchronously.
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 4 2017

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

commit 04ed2188b440cc9d7fc6b4fa3808368ea7f5ab0e
Author: liberato@chromium.org <liberato@chromium.org>
Date: Wed Oct 04 18:46:28 2017

Make AVDACodecAllocator thread safe.

AVDACodecAllocator previously was designed to work with AVDA on the
GPU main thread.  This CL makes it work properly even if clients
call it from other threads, such as MCVD will do.

It does three main things:

 - Create/ReleaseCodec calls post to the thread on which the
   allocator was constructed.  This keeps everything ordered.
   Otherwise, it's very difficult to ensure that one client stopping
   threads won't happen concurrently with another posting tasks.
   Locks worked, but this prevents blocking the caller, and makes it
   easier to deal with the weakptr used to cancel StopThreadTask.
 - Responses to the client are on whatever thread it called on.
   This lets us continue to use client-provided WeakPtrs.
 - StartThread now sends its results asynchronously, since it posts
   to another thread sometimes.

Bug:  760822 
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
Change-Id: Ib1bca55602a2f43ea9e2ecfd55fa98c877fb6217
Reviewed-on: https://chromium-review.googlesource.com/669799
Commit-Queue: Frank Liberato <liberato@chromium.org>
Reviewed-by: Chris Watkins <watk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506463}
[modify] https://crrev.com/04ed2188b440cc9d7fc6b4fa3808368ea7f5ab0e/media/base/android/BUILD.gn
[modify] https://crrev.com/04ed2188b440cc9d7fc6b4fa3808368ea7f5ab0e/media/base/android/media_codec_bridge.h
[modify] https://crrev.com/04ed2188b440cc9d7fc6b4fa3808368ea7f5ab0e/media/base/android/media_codec_bridge_impl.h
[modify] https://crrev.com/04ed2188b440cc9d7fc6b4fa3808368ea7f5ab0e/media/base/android/mock_media_codec_bridge.cc
[modify] https://crrev.com/04ed2188b440cc9d7fc6b4fa3808368ea7f5ab0e/media/base/android/mock_media_codec_bridge.h
[modify] https://crrev.com/04ed2188b440cc9d7fc6b4fa3808368ea7f5ab0e/media/gpu/android/android_video_decode_accelerator.cc
[modify] https://crrev.com/04ed2188b440cc9d7fc6b4fa3808368ea7f5ab0e/media/gpu/android/android_video_decode_accelerator_unittest.cc
[modify] https://crrev.com/04ed2188b440cc9d7fc6b4fa3808368ea7f5ab0e/media/gpu/android/avda_codec_allocator.cc
[modify] https://crrev.com/04ed2188b440cc9d7fc6b4fa3808368ea7f5ab0e/media/gpu/android/avda_codec_allocator.h
[modify] https://crrev.com/04ed2188b440cc9d7fc6b4fa3808368ea7f5ab0e/media/gpu/android/avda_codec_allocator_unittest.cc
[modify] https://crrev.com/04ed2188b440cc9d7fc6b4fa3808368ea7f5ab0e/media/gpu/android/content_video_view_overlay_allocator.cc
[modify] https://crrev.com/04ed2188b440cc9d7fc6b4fa3808368ea7f5ab0e/media/gpu/android/content_video_view_overlay_allocator.h
[modify] https://crrev.com/04ed2188b440cc9d7fc6b4fa3808368ea7f5ab0e/media/gpu/android/content_video_view_overlay_allocator_unittest.cc
[modify] https://crrev.com/04ed2188b440cc9d7fc6b4fa3808368ea7f5ab0e/media/gpu/android/fake_codec_allocator.cc
[modify] https://crrev.com/04ed2188b440cc9d7fc6b4fa3808368ea7f5ab0e/media/gpu/android/fake_codec_allocator.h
[modify] https://crrev.com/04ed2188b440cc9d7fc6b4fa3808368ea7f5ab0e/media/gpu/android/media_codec_video_decoder.cc
[modify] https://crrev.com/04ed2188b440cc9d7fc6b4fa3808368ea7f5ab0e/media/gpu/android/media_codec_video_decoder_unittest.cc
[modify] https://crrev.com/04ed2188b440cc9d7fc6b4fa3808368ea7f5ab0e/media/gpu/gpu_video_decode_accelerator_factory.cc
[modify] https://crrev.com/04ed2188b440cc9d7fc6b4fa3808368ea7f5ab0e/media/mojo/services/gpu_mojo_media_client.cc

Status: Fixed (was: Assigned)

Sign in to add a comment