That's surprising. Is there a second MediaCodec active at that time? Or is only one active? I'd guess only one since on exit the other should have been released?

Why are we calling configure() in this situation? Are we recreating MediaCodec?
Then, do we delete the first before creating the second?

Yeah, there should only be one active. I'll verify.

timav: fullscreen transitions result in destructing and recreating a new AVDA (and MediaCodec) to switch the output surface. And we do delete the first before creating the second.

Thank you. I thought with Spitzer we are using the same SurfaceTexture for both regular and fullscreen.

Luckily, it looks like this isn't actually the problem I thought it was. It's not when we're creating the new AVDA, but while we're shutting down the first one. AVDA::Reset gets called, and on JB, it tries to create a new MediaCodec. Since the fullscreen surface is no longer valid, it fails to create it.

I think the fix is to lazily instantiate the replacement MediaCodec after a Reset.

if there happens to be a decode (for example) that arrives after we lose the surface, which causes us to re-init the codec, might we not hit the same problem?

once we lose the surface, perhaps we shouldn't try to get the codec back at all until a restart.  alternatively, we should forget that we ever had a surface once we lose it, but that's harder to get right since the strategy has to be involved.

I think decodes after we lose the surface will be okay if we defer the error notification. Reset doesn't defer right now.

We still haven't plumbed the surfaceDestroyed callback though, so we don't know that the surface is lost on the VDA side. (I'll create a bug for this). 

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

commit 9edddb9ded49023460a204675ee4e9bb763b9da7
Author: watk <watk@chromium.org>
Date: Mon Mar 21 22:21:10 2016

media: Stop flushing the renderer for pipeline suspend

Previously when performing a pipeline suspend we would flush the
renderer before destructing it. This CL removes the flush because
it's no longer necessary and has issues:
1) it adds ~100-150ms to suspend on a Galaxy Nexus (which makes
   a difference for fullscreen transitions), and
2) on <API level 18 devices it will cause AVDA to go into an
   error state when it handles the flush by creating a new
   MediaCodec without a valid output surface.

BUG= 596641 , 595545 
TEST=media_unittests

Review URL: https://codereview.chromium.org/1815423002

Cr-Commit-Position: refs/heads/master@{#382417}

[modify] https://crrev.com/9edddb9ded49023460a204675ee4e9bb763b9da7/media/base/pipeline_impl.cc
[modify] https://crrev.com/9edddb9ded49023460a204675ee4e9bb763b9da7/media/base/pipeline_impl_unittest.cc

Your change meets the bar and is auto-approved for M50 (branch: 2661)

[Bulk Edit]

This bug has been approved for merge to M50, but according to our records, the merge has not been processed yet.  If this bug has been merged, please remove the Merge-Approved-50 label.  If it has not, please merge ASAP.

I reverted this change and won't be trying to resubmit it for M50. We may go a different route to solve this problem though. If so, I'll request another merge.