Plumb a Browser->GPU surface destruction message for Android VDAs

Plumb a surface destruction callback for AVDAs on Android

This plumbs the SurfaceHolder.Callback#surfaceDestroyed() callback,
that comes from the Android framework when our video SurfaceView surface
is being destroyed, from the Browser UI thread to the GPU main thread.
The code to handle this callback in AVDA will be added in a future CL.

It's not ideal that we have to do this, but we don't really have a choice.
We don't control the lifetime of SurfaceView surfaces directly, so we have
to be prepared to handle their destruction.

Currently, AVDA handles the loss of surface by not immediately reporting
an error when its MediaCodec starts throwing IllegalStateException. In
some cases, the ISE is due to losing the surface, and in others it's a
real error. By deferring reporting the error to the VDA client, the client
has a window of time where it can Destroy the VDA (which it does on
fullscreen transitions) without seeing the error.

However, on JB devices it's not enough to defer errors. Letting the
surface be destroyed while the MediaCodec is still active can cause it
to hit an internal CHECK, which kills the GPU process. So this CL
introduces a path for AVDAs to get a synchronous callback in which they
can tear down their MediaCodecs before the surface is destroyed (which
is what the surfaceDestroyed() callback is for).

BUG=598408
Committed: https://crrev.com/ad8743d5832349034e92739052be79e913f6b06e
Cr-Commit-Position: refs/heads/master@{#390167}

[watk, 2016-04-12 01:33:46]

watk@chromium.org changed reviewers:
+ sievers@chromium.org

[watk, 2016-04-12 01:33:47]

Hey Sievers, WDYT about this approach for plumbing the onSurfaceDestroyed
callback for AVDA? I hate to add a new friend to ScopedAllowWait, but I don't
think we have a choice. BTW, I haven't talked to jam or brettw about the
ScopedAllowWait yet (as the comment says to do).

[watk, 2016-04-18 18:50:54]

watk@chromium.org changed reviewers:
+ jam@chromium.org

[watk, 2016-04-18 18:50:54]

Friendly ping sievers.

jam@: PTAL at this usage of ScopedAllowWait.

We have a similar sync path when the ui compositor surface is being destroyed
(in response to the same callback from Android), but that goes through the
command buffer instead of being plumbed explicitly like this.

[jam, 2016-04-18 22:48:57]

On 2016/04/18 18:50:54, watk wrote:
> Friendly ping sievers.
> 
> jam@: PTAL at this usage of ScopedAllowWait.
> 
> We have a similar sync path when the ui compositor surface is being destroyed
> (in response to the same callback from Android), but that goes through the
> command buffer instead of being plumbed explicitly like this.

if there's absolutely no other way to do this, which it sounds like it, then I
defer to you folks and I'm fine with this new addition.

[no sievers, 2016-04-18 23:41:25]

On 2016/04/18 22:48:57, jam wrote:
> if there's absolutely no other way to do this, which it sounds like it, then I
> defer to you folks and I'm fine with this new addition.


I think it's ok (though unfortunate that we have to do this) because like Chris
pointed out it's similar what we do in other cases.
Or even to the notion of a synchronous GL call in general (since it's UI->GPU).

But that being said, if you just make it a sync IPC then you don't need to do
all this whitelisting and it's not really different from for example
CreateCommandBuffer.

https://codereview.chromium.org/1878103002/diff/20001/content/common/gpu/medi...
File content/common/gpu/media/avda_surface_tracker.h (right):

https://codereview.chromium.org/1878103002/diff/20001/content/common/gpu/medi...
content/common/gpu/media/avda_surface_tracker.h:35: void
AddSurfaceDestructionObserver(SurfaceDestructionObserver* observer);
I don't see any call sites to this.

I'm asking also to figure out if this message should be routed to the stub
instead. I thought the decoders were associated with a stub already.

[watk, 2016-04-19 21:14:47]

I tried making the message sync, but it didn't work. I believe because the child
process channel is not a sync channel:
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/chi....
When I tried sending a sync message I  got this: 
[FATAL:gpu_process_host_ui_shim.cc(227)] Check failed: false. Invalid message
with type = 4294967280
I'm assuming that's the sync reply not being handled by anything but I haven't
dug into it.

I took this idea with the async message and waitable event from
CreateGpuMemoryBuffer which also has a path to do a sync allocation from the UI
thread. (The difference is that it's handled on the IO thread service side)
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/gp...

[watk, 2016-04-19 21:15:08]

https://codereview.chromium.org/1878103002/diff/20001/content/common/gpu/medi...
File content/common/gpu/media/avda_surface_tracker.h (right):

https://codereview.chromium.org/1878103002/diff/20001/content/common/gpu/medi...
content/common/gpu/media/avda_surface_tracker.h:35: void
AddSurfaceDestructionObserver(SurfaceDestructionObserver* observer);
On 2016/04/18 23:41:25, sievers wrote:
> I don't see any call sites to this.
> 
> I'm asking also to figure out if this message should be routed to the stub
> instead. I thought the decoders were associated with a stub already.

I forgot to mention that I was going to do the AVDA changes in a follow up. But
AVDA instances will register themselves as observers whenever they look up a
Surface with GpuSurfaceLookup.

VDAs are given a pointer to a stub, but stubs don't keep a pointer to VDAs.
They're constructed here
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu....
GpuVideoDecodeAccelerators observe stub destruction to delete themselves.

I hadn't considered routing the message directly to the stub (or
GpuVideoDecodeAccelerator is probably the place). Would that mean passing the
Browser->Gpu channel down to where GVDA is constructed so that it could install
a filter on it? There might be multiple GVDAs, and we don't know which one owns
the surface, so is it possible to set it up so that they all see the message,
but none of them say they handled it?

That's probably not very clear if you haven't looked at the VDA infrastructure
in a while.. Let me know if I can clarify.

[watk, 2016-04-19 21:52:46]

On 2016/04/19 21:15:08, watk wrote:
>
https://codereview.chromium.org/1878103002/diff/20001/content/common/gpu/medi...
> File content/common/gpu/media/avda_surface_tracker.h (right):
> 
>
https://codereview.chromium.org/1878103002/diff/20001/content/common/gpu/medi...
> content/common/gpu/media/avda_surface_tracker.h:35: void
> AddSurfaceDestructionObserver(SurfaceDestructionObserver* observer);
> On 2016/04/18 23:41:25, sievers wrote:
> > I don't see any call sites to this.
> > 
> > I'm asking also to figure out if this message should be routed to the stub
> > instead. I thought the decoders were associated with a stub already.
> 
> I forgot to mention that I was going to do the AVDA changes in a follow up.
But
> AVDA instances will register themselves as observers whenever they look up a
> Surface with GpuSurfaceLookup.
> 
> VDAs are given a pointer to a stub, but stubs don't keep a pointer to VDAs.
> They're constructed here
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu....
> GpuVideoDecodeAccelerators observe stub destruction to delete themselves.
> 
> I hadn't considered routing the message directly to the stub (or
> GpuVideoDecodeAccelerator is probably the place). Would that mean passing the
> Browser->Gpu channel down to where GVDA is constructed so that it could
install
> a filter on it? There might be multiple GVDAs, and we don't know which one
owns
> the surface, so is it possible to set it up so that they all see the message,
> but none of them say they handled it?> 
> That's probably not very clear if you haven't looked at the VDA infrastructure
> in a while.. Let me know if I can clarify.

I was also thinking about doing this over binder. i.e., wrap the Surface in a
binder object that we can send destroy notifications to. Then when we get
onSurfaceDestroyed we look up the Surface binder and send a sync binder call.
When an AVDA looks up a surface it could get the binder wrapper and register for
a callback on destruction.

[no sievers, 2016-04-19 23:30:18]

ok let's just do as you suggested then.

https://codereview.chromium.org/1878103002/diff/20001/content/browser/gpu/gpu...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/1878103002/diff/20001/content/browser/gpu/gpu...
content/browser/gpu/gpu_process_host.cc:798: TRACE_EVENT0("gpu",
"GpuProcessHost::DestroyingVideoSurface");
DCHECK(destroying_video_surface_request_.is_null());
DCHECK(!done_cb.is_null());

https://codereview.chromium.org/1878103002/diff/20001/content/browser/media/a...
File content/browser/media/android/browser_surface_view_manager.cc (right):

https://codereview.chromium.org/1878103002/diff/20001/content/browser/media/a...
content/browser/media/android/browser_surface_view_manager.cc:95: // XXX: How do
we loop over all instances?
Can you move the thread bouncing code from GpuProcessHostUIShim into here, and
then just call the following to get to GpuProcessHost?

GpuProcessHost::Get(GpuProcessHost::GPU_PROCESS_KIND_SANDBOXED,                 
               CAUSE_FOR_GPU_LAUNCH_NO_LAUNCH);

^^ and you can use NO_LAUNCH since if it's not there, then it won't know about
this Surface anyhow.

https://codereview.chromium.org/1878103002/diff/20001/content/common/gpu/medi...
File content/common/gpu/media/avda_surface_tracker.cc (right):

https://codereview.chromium.org/1878103002/diff/20001/content/common/gpu/medi...
content/common/gpu/media/avda_surface_tracker.cc:16: static
base::LazyInstance<AVDASurfaceTracker>::Leaky g_lazy_instance =
nit: doesn't have to be Leaky

https://codereview.chromium.org/1878103002/diff/20001/content/common/gpu/medi...
File content/common/gpu/media/avda_surface_tracker.h (right):

https://codereview.chromium.org/1878103002/diff/20001/content/common/gpu/medi...
content/common/gpu/media/avda_surface_tracker.h:13: class
SurfaceDestructionObserver {
nit: This only has one method so could be a base::Callback.

https://codereview.chromium.org/1878103002/diff/20001/content/common/gpu_host...
File content/common/gpu_host_messages.h (right):

https://codereview.chromium.org/1878103002/diff/20001/content/common/gpu_host...
content/common/gpu_host_messages.h:198: IPC_MESSAGE_CONTROL0(GpuMsg_WakeUpGpu)
nit: newline

https://codereview.chromium.org/1878103002/diff/20001/content/common/gpu_host...
content/common/gpu_host_messages.h:275:
IPC_MESSAGE_CONTROL1(GpuHostMsg_DestroyingVideoSurfaceHandled,
nit: how about DestroyingVideoSurfaceAck? We use that naming in other places for
simple replies.

[watk, 2016-04-22 02:41:47]

Patchset #3 (id:40001) has been deleted

[watk, 2016-04-22 02:48:56]

Thanks! PTAL

https://codereview.chromium.org/1878103002/diff/20001/content/browser/gpu/gpu...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/1878103002/diff/20001/content/browser/gpu/gpu...
content/browser/gpu/gpu_process_host.cc:798: TRACE_EVENT0("gpu",
"GpuProcessHost::DestroyingVideoSurface");
On 2016/04/19 23:30:17, sievers wrote:
> DCHECK(destroying_video_surface_request_.is_null());
> DCHECK(!done_cb.is_null());

Done.

https://codereview.chromium.org/1878103002/diff/20001/content/browser/media/a...
File content/browser/media/android/browser_surface_view_manager.cc (right):

https://codereview.chromium.org/1878103002/diff/20001/content/browser/media/a...
content/browser/media/android/browser_surface_view_manager.cc:95: // XXX: How do
we loop over all instances?
On 2016/04/19 23:30:17, sievers wrote:
> Can you move the thread bouncing code from GpuProcessHostUIShim into here, and
> then just call the following to get to GpuProcessHost?
> 
> GpuProcessHost::Get(GpuProcessHost::GPU_PROCESS_KIND_SANDBOXED,               
 
>                CAUSE_FOR_GPU_LAUNCH_NO_LAUNCH);
> 
> ^^ and you can use NO_LAUNCH since if it's not there, then it won't know about
> this Surface anyhow.
> 

Done. This is much better

https://codereview.chromium.org/1878103002/diff/20001/content/common/gpu/medi...
File content/common/gpu/media/avda_surface_tracker.cc (right):

https://codereview.chromium.org/1878103002/diff/20001/content/common/gpu/medi...
content/common/gpu/media/avda_surface_tracker.cc:16: static
base::LazyInstance<AVDASurfaceTracker>::Leaky g_lazy_instance =
On 2016/04/19 23:30:17, sievers wrote:
> nit: doesn't have to be Leaky

Done.

https://codereview.chromium.org/1878103002/diff/20001/content/common/gpu/medi...
File content/common/gpu/media/avda_surface_tracker.h (right):

https://codereview.chromium.org/1878103002/diff/20001/content/common/gpu/medi...
content/common/gpu/media/avda_surface_tracker.h:13: class
SurfaceDestructionObserver {
On 2016/04/19 23:30:17, sievers wrote:
> nit: This only has one method so could be a base::Callback.

Done.

https://codereview.chromium.org/1878103002/diff/20001/content/common/gpu_host...
File content/common/gpu_host_messages.h (right):

https://codereview.chromium.org/1878103002/diff/20001/content/common/gpu_host...
content/common/gpu_host_messages.h:198: IPC_MESSAGE_CONTROL0(GpuMsg_WakeUpGpu)
On 2016/04/19 23:30:17, sievers wrote:
> nit: newline

Done.

https://codereview.chromium.org/1878103002/diff/20001/content/common/gpu_host...
content/common/gpu_host_messages.h:275:
IPC_MESSAGE_CONTROL1(GpuHostMsg_DestroyingVideoSurfaceHandled,
On 2016/04/19 23:30:17, sievers wrote:
> nit: how about DestroyingVideoSurfaceAck? We use that naming in other places
for
> simple replies.

Done.

[DaleCurtis, 2016-04-22 18:17:13]

dalecurtis@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[DaleCurtis, 2016-04-22 18:17:14]

This doesn't seem to hook up the call to AVDA right now?

https://codereview.chromium.org/1878103002/diff/80001/content/common/gpu/medi...
File content/common/gpu/media/avda_surface_tracker.h (right):

https://codereview.chromium.org/1878103002/diff/80001/content/common/gpu/medi...
content/common/gpu/media/avda_surface_tracker.h:14: // AVDASurfaceTracker
provides AndroidVideoDecodeAccelerators a way to register
Kind of feels like we should merge this and the AVDATimerManager into a single
AVDAManager class :)

[watk, 2016-04-22 18:29:57]

On 2016/04/22 18:17:14, DaleCurtis wrote:
> This doesn't seem to hook up the call to AVDA right now?

Yeah, I was going to keep them separate to keep the reviews focused. But now I
see my commit description isn't clear about that. I'll update the description.

>
https://codereview.chromium.org/1878103002/diff/80001/content/common/gpu/medi...
> File content/common/gpu/media/avda_surface_tracker.h (right):
> 
>
https://codereview.chromium.org/1878103002/diff/80001/content/common/gpu/medi...
> content/common/gpu/media/avda_surface_tracker.h:14: // AVDASurfaceTracker
> provides AndroidVideoDecodeAccelerators a way to register
> Kind of feels like we should merge this and the AVDATimerManager into a single
> AVDAManager class :)

That sounds like a good idea. At least then we'll only have one singleton :P
I'll try it.

[liberato (no reviews please), 2016-04-22 20:31:29]

liberato@chromium.org changed reviewers:
+ liberato@chromium.org

[liberato (no reviews please), 2016-04-22 20:31:30]

https://codereview.chromium.org/1878103002/diff/80001/content/common/gpu/medi...
File content/common/gpu/media/avda_surface_tracker.h (right):

https://codereview.chromium.org/1878103002/diff/80001/content/common/gpu/medi...
content/common/gpu/media/avda_surface_tracker.h:14: // AVDASurfaceTracker
provides AndroidVideoDecodeAccelerators a way to register
On 2016/04/22 18:17:14, DaleCurtis wrote:
> Kind of feels like we should merge this and the AVDATimerManager into a single
> AVDAManager class :)

might want to hold off on that until MediaCodecLoop is finished.  i suspect that
they'll go their own way (or any other Fleetwood Mac song).

[watk, 2016-04-22 21:19:58]

Description was changed from

==========
Notify Android decoders when their output surface is being destroyed

Previously, the SurfaceView surface that an AndroidVideoDecodeAccelerator
was decoding to (in fullscreen) got destroyed without the decoder being
notified. The MediaCodec decoder goes into an IllegalState, but we handled
that by deferring the reporting of that error to the VDA client until we're
sure that it's a real error and not a normal surface destruction due to a
fullscreen transition.

On JB devices it's not enough to defer errors. Letting the surface be
destroyed while the MediaCodec is still active can cause us to hit a
CHECK inside MediaCodec, which kills the GPU process. This CL adds a sync
notification from the Browser UI thread (where the onSurfaceDestroyed()
callback is called by the framework) to the GPU main thread (where AVDAs
run). Inside this we can release the MediaCodec that's about to lose its
surface to avoid crashing.

BUG=598408
==========

to

==========
Plumb a surface destruction callback for AVDAs on Android

This plumbs the SurfaceHolder.Callback#surfaceDestroyed() callback,
that comes from the Android framework when our video SurfaceView surface
is being destroyed, from the Browser UI thread to the GPU main thread.
The code to handle this callback in AVDA will be added in a future CL.

It's not ideal that we have to do this, but we don't really have a choice.
We don't control the lifetime of SurfaceView surfaces directly, so we have
to be prepared to handle their destruction.

Currently, AVDA handles the loss of surface by not immediately reporting
an error when its MediaCodec starts throwing IllegalStateException. In
some cases, the ISE is due to losing the surface, and in others it's a
real error. By deferring reporting the error to the VDA client, the client
has a window of time where it can Destroy the VDA (which it does on
fullscreen transitions) without seeing the error.

However, on JB devices it's not enough to defer errors. Letting the
surface be destroyed while the MediaCodec is still active can cause it
to hit an internal CHECK, which kills the GPU process. So this CL
introduces a path for AVDAs to get a synchronous callback in which they
can tear down their MediaCodecs before the surface is destroyed (which
is what the surfaceDestroyed() callback is for).

BUG=598408
==========

[watk, 2016-04-22 21:20:53]

Patchset #5 (id:100001) has been deleted

[watk, 2016-04-22 21:39:51]

On 2016/04/22 20:31:30, liberato wrote:
>
https://codereview.chromium.org/1878103002/diff/80001/content/common/gpu/medi...
> File content/common/gpu/media/avda_surface_tracker.h (right):
> 
>
https://codereview.chromium.org/1878103002/diff/80001/content/common/gpu/medi...
> content/common/gpu/media/avda_surface_tracker.h:14: // AVDASurfaceTracker
> provides AndroidVideoDecodeAccelerators a way to register
> On 2016/04/22 18:17:14, DaleCurtis wrote:
> > Kind of feels like we should merge this and the AVDATimerManager into a
single
> > AVDAManager class :)
> 
> might want to hold off on that until MediaCodecLoop is finished.  i suspect
that
> they'll go their own way (or any other Fleetwood Mac song).

Okay, I'll leave it as is for now then, thanks.

Description is updated. This is ready to review again sievers@.

[no sievers, 2016-04-22 21:50:11]

lgtm

[DaleCurtis, 2016-04-22 22:19:59]

lgtm

[watk, 2016-04-22 22:41:00]

watk@chromium.org changed reviewers:
+ wfh@chromium.org

[watk, 2016-04-22 22:41:01]

wfh@chromium.org: Please review changes in gpu_host_messages

[Will Harris, 2016-04-22 23:31:10]

lgtm % nit/suggestion.

https://codereview.chromium.org/1878103002/diff/140001/content/common/gpu/med...
File content/common/gpu/media/avda_surface_tracker.h (right):

https://codereview.chromium.org/1878103002/diff/140001/content/common/gpu/med...
content/common/gpu/media/avda_surface_tracker.h:22: // This is not thread safe.
All access should be on the gpu main thread.
nit: if this is not thread safe it would be nice to see some DCHECK_CURRENTLY_ON
the right thread in the methods, just in case.

[watk, 2016-04-23 00:48:58]

Thanks

https://codereview.chromium.org/1878103002/diff/140001/content/common/gpu/med...
File content/common/gpu/media/avda_surface_tracker.h (right):

https://codereview.chromium.org/1878103002/diff/140001/content/common/gpu/med...
content/common/gpu/media/avda_surface_tracker.h:22: // This is not thread safe.
All access should be on the gpu main thread.
On 2016/04/22 23:31:09, Will Harris wrote:
> nit: if this is not thread safe it would be nice to see some
DCHECK_CURRENTLY_ON
> the right thread in the methods, just in case.

Sounds good. Done with a ThreadChecker.

[watk, 2016-04-23 00:49:06]

The CQ bit was checked by watk@chromium.org

[watk, 2016-04-23 00:49:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org, sievers@chromium.org, wfh@chromium.org
Link to the patchset: https://codereview.chromium.org/1878103002/#ps160001
(title: "Add a threadchecker")

[commit-bot: I haz the power, 2016-04-23 00:49:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1878103002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1878103002/160001

[commit-bot: I haz the power, 2016-04-23 00:57:35]

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

[commit-bot: I haz the power, 2016-04-23 00:57:36]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[watk, 2016-04-23 01:02:24]

On 2016/04/23 00:57:36, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

jam@, could I get a lgtm for thread_restrictions please.

[watk, 2016-04-26 18:54:26]

On 2016/04/23 01:02:24, watk wrote:
> On 2016/04/23 00:57:36, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> 
> jam@, could I get a lgtm for thread_restrictions please.

friendly ping jam@

[jam, 2016-04-27 19:10:53]

lgtm

[watk, 2016-04-27 19:19:29]

The CQ bit was checked by watk@chromium.org

[commit-bot: I haz the power, 2016-04-27 19:19:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1878103002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1878103002/160001

[commit-bot: I haz the power, 2016-04-27 20:27:58]

Description was changed from

==========
Plumb a surface destruction callback for AVDAs on Android

This plumbs the SurfaceHolder.Callback#surfaceDestroyed() callback,
that comes from the Android framework when our video SurfaceView surface
is being destroyed, from the Browser UI thread to the GPU main thread.
The code to handle this callback in AVDA will be added in a future CL.

It's not ideal that we have to do this, but we don't really have a choice.
We don't control the lifetime of SurfaceView surfaces directly, so we have
to be prepared to handle their destruction.

Currently, AVDA handles the loss of surface by not immediately reporting
an error when its MediaCodec starts throwing IllegalStateException. In
some cases, the ISE is due to losing the surface, and in others it's a
real error. By deferring reporting the error to the VDA client, the client
has a window of time where it can Destroy the VDA (which it does on
fullscreen transitions) without seeing the error.

However, on JB devices it's not enough to defer errors. Letting the
surface be destroyed while the MediaCodec is still active can cause it
to hit an internal CHECK, which kills the GPU process. So this CL
introduces a path for AVDAs to get a synchronous callback in which they
can tear down their MediaCodecs before the surface is destroyed (which
is what the surfaceDestroyed() callback is for).

BUG=598408
==========

to

==========
Plumb a surface destruction callback for AVDAs on Android

This plumbs the SurfaceHolder.Callback#surfaceDestroyed() callback,
that comes from the Android framework when our video SurfaceView surface
is being destroyed, from the Browser UI thread to the GPU main thread.
The code to handle this callback in AVDA will be added in a future CL.

It's not ideal that we have to do this, but we don't really have a choice.
We don't control the lifetime of SurfaceView surfaces directly, so we have
to be prepared to handle their destruction.

Currently, AVDA handles the loss of surface by not immediately reporting
an error when its MediaCodec starts throwing IllegalStateException. In
some cases, the ISE is due to losing the surface, and in others it's a
real error. By deferring reporting the error to the VDA client, the client
has a window of time where it can Destroy the VDA (which it does on
fullscreen transitions) without seeing the error.

However, on JB devices it's not enough to defer errors. Letting the
surface be destroyed while the MediaCodec is still active can cause it
to hit an internal CHECK, which kills the GPU process. So this CL
introduces a path for AVDAs to get a synchronous callback in which they
can tear down their MediaCodecs before the surface is destroyed (which
is what the surfaceDestroyed() callback is for).

BUG=598408
==========

[commit-bot: I haz the power, 2016-04-27 20:28:00]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2016-04-30 17:12:35]

Patchset 7 (id:??) landed as
https://crrev.com/ad8743d5832349034e92739052be79e913f6b06e
Cr-Commit-Position: refs/heads/master@{#390167}

[commit-bot: I haz the power, 2016-04-30 17:51:36]

Description was changed from

==========
Plumb a surface destruction callback for AVDAs on Android

This plumbs the SurfaceHolder.Callback#surfaceDestroyed() callback,
that comes from the Android framework when our video SurfaceView surface
is being destroyed, from the Browser UI thread to the GPU main thread.
The code to handle this callback in AVDA will be added in a future CL.

It's not ideal that we have to do this, but we don't really have a choice.
We don't control the lifetime of SurfaceView surfaces directly, so we have
to be prepared to handle their destruction.

Currently, AVDA handles the loss of surface by not immediately reporting
an error when its MediaCodec starts throwing IllegalStateException. In
some cases, the ISE is due to losing the surface, and in others it's a
real error. By deferring reporting the error to the VDA client, the client
has a window of time where it can Destroy the VDA (which it does on
fullscreen transitions) without seeing the error.

However, on JB devices it's not enough to defer errors. Letting the
surface be destroyed while the MediaCodec is still active can cause it
to hit an internal CHECK, which kills the GPU process. So this CL
introduces a path for AVDAs to get a synchronous callback in which they
can tear down their MediaCodecs before the surface is destroyed (which
is what the surfaceDestroyed() callback is for).

BUG=598408
==========

to

==========
Plumb a surface destruction callback for AVDAs on Android

This plumbs the SurfaceHolder.Callback#surfaceDestroyed() callback,
that comes from the Android framework when our video SurfaceView surface
is being destroyed, from the Browser UI thread to the GPU main thread.
The code to handle this callback in AVDA will be added in a future CL.

It's not ideal that we have to do this, but we don't really have a choice.
We don't control the lifetime of SurfaceView surfaces directly, so we have
to be prepared to handle their destruction.

Currently, AVDA handles the loss of surface by not immediately reporting
an error when its MediaCodec starts throwing IllegalStateException. In
some cases, the ISE is due to losing the surface, and in others it's a
real error. By deferring reporting the error to the VDA client, the client
has a window of time where it can Destroy the VDA (which it does on
fullscreen transitions) without seeing the error.

However, on JB devices it's not enough to defer errors. Letting the
surface be destroyed while the MediaCodec is still active can cause it
to hit an internal CHECK, which kills the GPU process. So this CL
introduces a path for AVDAs to get a synchronous callback in which they
can tear down their MediaCodecs before the surface is destroyed (which
is what the surfaceDestroyed() callback is for).

BUG=598408

Committed: https://crrev.com/ad8743d5832349034e92739052be79e913f6b06e
Cr-Commit-Position: refs/heads/master@{#390167}
==========