Group AVDA output surface into AVDASurfaceBundle.

Group AVDA output surface into AVDASurfaceBundle.

AVDASurfaceBundle holds the surface and/or SurfaceTexture that a
MediaCodec instance is using.  A reference to it is kept with the
codec, so that we don't destroy the output surface before the codec.

Keeping a reference to the surface bundle replaces the explicit post
of the SurfaceTexture during AVDA::ActualDestroy.  All calls to
ReleaseCodec send a reference to the SurfaceBundle along with the
codec.  The bundle also is replaced in UpdateSurface (the only time
that AVDA can change surfaces), so that failed calls to SetSurface
are easier to recover from.

Note that it other things (e.g., shared state) may still hold a
reference to the SurfaceTexture itself, so that the GL texture will
also outlast the GLImages.

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

Review-Url: https://codereview.chromium.org/2707703002
Cr-Commit-Position: refs/heads/master@{#455511}
Committed: https://chromium.googlesource.com/chromium/src/+/5e0962e514794c20d7c5fb614f7b7d0589177fb7

[liberato (no reviews please), 2017-02-18 20:49:06]

Description was changed from

==========
started removing st post

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

to

==========
started removing st post

BUG=
CQ_INCLUDE_TRYBOTS=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
==========

[liberato (no reviews please), 2017-02-18 20:49:48]

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

[commit-bot: I haz the power, 2017-02-18 20:49:53]

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-02-18 22:34:39]

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

[commit-bot: I haz the power, 2017-02-18 22:34:39]

Dry run: This issue passed the CQ dry run.

[liberato (no reviews please), 2017-02-19 06:58:54]

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

[commit-bot: I haz the power, 2017-02-19 06:58:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[liberato (no reviews please), 2017-02-19 07:07:03]

Description was changed from

==========
started removing st post

BUG=
CQ_INCLUDE_TRYBOTS=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

==========
Group AVDA output surface into AVDASurfaceBundle.

AVDASurfaceBundle holds the surface and/or SurfaceTexture that a
MediaCodec instance is using.  A reference to it is kept with the
codec, so that we don't destroy the output surface before the codec.

Keeping a reference to the surface bundle replaces the explicit post
of the SurfaceTexture during AVDA::ActualDestroy.  All calls to
ReleaseCodec send a reference to the SurfaceBundle along with the
codec.  The bundle also is replaced in UpdateSurface (the only time
that AVDA can change surfaces), so that failed calls to SetSurface
are easier to recover from.

Note that it other things (e.g., shared state) may still hold a
reference to the SurfaceTexture itself, so that the GL texture will
also outlast the GLImages.

BUG=
CQ_INCLUDE_TRYBOTS=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
==========

[liberato (no reviews please), 2017-02-19 07:44:30]

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

[commit-bot: I haz the power, 2017-02-19 07:44:39]

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-02-19 09:15:47]

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

[commit-bot: I haz the power, 2017-02-19 09:15:48]

Dry run: This issue passed the CQ dry run.

[liberato (no reviews please), 2017-02-21 06:24:30]

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

[commit-bot: I haz the power, 2017-02-21 06:24:46]

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-02-21 07:47:04]

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

[commit-bot: I haz the power, 2017-02-21 07:47:04]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[liberato (no reviews please), 2017-02-22 01:23:30]

Description was changed from

==========
Group AVDA output surface into AVDASurfaceBundle.

AVDASurfaceBundle holds the surface and/or SurfaceTexture that a
MediaCodec instance is using.  A reference to it is kept with the
codec, so that we don't destroy the output surface before the codec.

Keeping a reference to the surface bundle replaces the explicit post
of the SurfaceTexture during AVDA::ActualDestroy.  All calls to
ReleaseCodec send a reference to the SurfaceBundle along with the
codec.  The bundle also is replaced in UpdateSurface (the only time
that AVDA can change surfaces), so that failed calls to SetSurface
are easier to recover from.

Note that it other things (e.g., shared state) may still hold a
reference to the SurfaceTexture itself, so that the GL texture will
also outlast the GLImages.

BUG=
CQ_INCLUDE_TRYBOTS=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

==========
Group AVDA output surface into AVDASurfaceBundle.

AVDASurfaceBundle holds the surface and/or SurfaceTexture that a
MediaCodec instance is using.  A reference to it is kept with the
codec, so that we don't destroy the output surface before the codec.

Keeping a reference to the surface bundle replaces the explicit post
of the SurfaceTexture during AVDA::ActualDestroy.  All calls to
ReleaseCodec send a reference to the SurfaceBundle along with the
codec.  The bundle also is replaced in UpdateSurface (the only time
that AVDA can change surfaces), so that failed calls to SetSurface
are easier to recover from.

Note that it other things (e.g., shared state) may still hold a
reference to the SurfaceTexture itself, so that the GL texture will
also outlast the GLImages.

BUG=
CQ_INCLUDE_TRYBOTS=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
==========

[liberato (no reviews please), 2017-02-22 01:23:30]

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

[liberato (no reviews please), 2017-02-22 01:27:05]

i did a lot of manual testing.  handles updatesurface / no updatesurface.  made
avda NotifyError on every other call to "Configure...Async" with a 5 second
delay in the async path so that avda would be gone before it returned.  codec
was cleaned up on the right thread.

also made onSurfaceDestroyed create the ST / updateSurface even when WMPI
thought it needed a restart.  was accidental, but it did that correctly too. 
created a temporary ST, UpdateSurface, then tore it all down properly when wmpi
got around to restarting the decoder.

thanks
-fl

[watk, 2017-02-22 20:38:56]

Nice one, I mostly had nits. Also I should have done this when I fixed the
ReleaseST thing..

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/android_vide...
File media/gpu/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:442: state_ = NO_ERROR;
I don't think we should be changing state_ here?

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:1203: if (surface_id ==
this->surface_id()) {
remove this->

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:1239:
codec_allocator_->DeallocateSurface(this, surface_id());
If we have an incoming SurfaceBundle, we don't want to deallocate that surface
do we? We should deallocate the config one? Or at least both of them?

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/android_vide...
File media/gpu/android_video_decode_accelerator.h (right):

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.h:249: // the current surface bundle
if not.
Could you give me a mental model for why this is the right behavior? I believe
you that it is, but whenever I see surface_id() written I wonder which one it
is, and whether it matters which one it is.

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/avda_codec_a...
File media/gpu/avda_codec_allocator.cc (right):

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/avda_codec_a...
media/gpu/avda_codec_allocator.cc:290: // |surface_bundle| already.
This comment seems out of place. It seems to be justifying why OnCodecConfigured
doesn't accept the bundle as a param? Can be deleted IMO.

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/avda_surface...
File media/gpu/avda_surface_bundle.cc (right):

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/avda_surface...
media/gpu/avda_surface_bundle.cc:9: AVDASurfaceBundle::AVDASurfaceBundle(int id)
: surface_id(id) {}
s/id/surface_id/ to match header

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/avda_surface...
media/gpu/avda_surface_bundle.cc:12: // Explicitly free the surface first, just
to be sure.
Unclear what you want to be sure of here. That it's deleted before the ST?

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/avda_surface...
media/gpu/avda_surface_bundle.cc:16: surface_texture = nullptr;
Yes I think so. If you drop the bundle you're dropping the ability to render to
the surface, so there's no reason not to ReleaseST(). Codec images might outlive
AVDA for an arbitrary time.

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/avda_surface...
File media/gpu/avda_surface_bundle.h (right):

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/avda_surface...
media/gpu/avda_surface_bundle.h:16: // the output surface that one needs.  In
other words, it's the surface, any
First sentence a word.

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/avda_surface...
media/gpu/avda_surface_bundle.h:17: // SurfaceTexture that backs it, and / or,
eventually, the AndroidOverlay.
Can we delete the AndroidOverlay comment until that's added as a member? (The
"eventually" won't make sense when it's added anyway).

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/avda_surface...
media/gpu/avda_surface_bundle.h:23: // be destroyed on the main thread.
s/main/gpu main/ (ambiguous for in-process gpu).

[liberato (no reviews please), 2017-02-23 18:18:47]

thanks for the comments.
-fl

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/android_vide...
File media/gpu/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:442: state_ = NO_ERROR;
On 2017/02/22 20:38:55, watk wrote:
> I don't think we should be changing state_ here?

it can be WAITING_FOR_SURFACE if it came through OnSurfaceAvailable.  i'd
actually prefer it if it were always WAITING_FOR_SURFACE when calling InitPBM,
or even some new JUST_GOT_SURFACE state.  not today, though.  :)

i'll add a comment.

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:1203: if (surface_id ==
this->surface_id()) {
On 2017/02/22 20:38:55, watk wrote:
> remove this->

can't.  |surface_id| shadows the method.

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.cc:1239:
codec_allocator_->DeallocateSurface(this, surface_id());
On 2017/02/22 20:38:55, watk wrote:
> If we have an incoming SurfaceBundle, we don't want to deallocate that surface
> do we? We should deallocate the config one? Or at least both of them?

having |incoming_bundle_| implies that there is no allocated surface in
|current_config_|, either because there's no bundle in |current_config_| at all
(i.e., during init), or because UpdateSurface already deallocated the outgoing
surface.

this is tricky and weird.  better would be for it to be handled by the
SurfaceBundle, but that's not straightforward either.  SurfaceBundle would have
to (a) become the allocator client, and (b) WP back to AVDA to prevent callbacks
after ActualDestroy.  even then, i'm not sure that there aren't weird issues
with unexpected callbacks on a pending-destruction SurfaceBundle.

i'm leaving that for future work.

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/android_vide...
File media/gpu/android_video_decode_accelerator.h (right):

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/android_vide...
media/gpu/android_video_decode_accelerator.h:249: // the current surface bundle
if not.
On 2017/02/22 20:38:55, watk wrote:
> Could you give me a mental model for why this is the right behavior? I believe
> you that it is, but whenever I see surface_id() written I wonder which one it
> is, and whether it matters which one it is.

after yet more thought after our offline discussion, i think that it's right as
written after all.  i'll update the comment.

TL;DR: only one of |incoming| and |current_config_->surface_bundle| has an
allocated (or is a waiter for) surface.  UpdateSurface ensures it.

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/avda_codec_a...
File media/gpu/avda_codec_allocator.cc (right):

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/avda_codec_a...
media/gpu/avda_codec_allocator.cc:290: // |surface_bundle| already.
On 2017/02/22 20:38:56, watk wrote:
> This comment seems out of place. It seems to be justifying why
OnCodecConfigured
> doesn't accept the bundle as a param? Can be deleted IMO.

Done.

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/avda_surface...
File media/gpu/avda_surface_bundle.cc (right):

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/avda_surface...
media/gpu/avda_surface_bundle.cc:9: AVDASurfaceBundle::AVDASurfaceBundle(int id)
: surface_id(id) {}
On 2017/02/22 20:38:56, watk wrote:
> s/id/surface_id/ to match header

Done.

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/avda_surface...
media/gpu/avda_surface_bundle.cc:12: // Explicitly free the surface first, just
to be sure.
On 2017/02/22 20:38:56, watk wrote:
> Unclear what you want to be sure of here. That it's deleted before the ST?

yes, done.

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/avda_surface...
media/gpu/avda_surface_bundle.cc:16: surface_texture = nullptr;
On 2017/02/22 20:38:56, watk wrote:
> Yes I think so. If you drop the bundle you're dropping the ability to render
to
> the surface, so there's no reason not to ReleaseST(). Codec images might
outlive
> AVDA for an arbitrary time.

i'm not sure that it's safe, the more that i think about it.  the consumer might
have a back buffer that it wants to swap.  i'll update the comment.

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/avda_surface...
File media/gpu/avda_surface_bundle.h (right):

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/avda_surface...
media/gpu/avda_surface_bundle.h:16: // the output surface that one needs.  In
other words, it's the surface, any
On 2017/02/22 20:38:56, watk wrote:
> First sentence a word.

parse i could not.

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/avda_surface...
media/gpu/avda_surface_bundle.h:17: // SurfaceTexture that backs it, and / or,
eventually, the AndroidOverlay.
On 2017/02/22 20:38:56, watk wrote:
> Can we delete the AndroidOverlay comment until that's added as a member? (The
> "eventually" won't make sense when it's added anyway).

Done.

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/avda_surface...
media/gpu/avda_surface_bundle.h:23: // be destroyed on the main thread.
On 2017/02/22 20:38:56, watk wrote:
> s/main/gpu main/ (ambiguous for in-process gpu).

Done.

[watk, 2017-02-23 20:03:47]

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/avda_surface...
File media/gpu/avda_surface_bundle.cc (right):

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/avda_surface...
media/gpu/avda_surface_bundle.cc:16: surface_texture = nullptr;
On 2017/02/23 18:18:46, liberato wrote:
> On 2017/02/22 20:38:56, watk wrote:
> > Yes I think so. If you drop the bundle you're dropping the ability to render
> to
> > the surface, so there's no reason not to ReleaseST(). Codec images might
> outlive
> > AVDA for an arbitrary time.
> 
> i'm not sure that it's safe, the more that i think about it.  the consumer
might
> have a back buffer that it wants to swap.  i'll update the comment.

But this is what we've always done right? The PostTask you deleted from
ActualDestroy().

We want to release the back-buffer memory so we don't have a single frame
keeping a full ST alive. It doesn't really matter at that point if there's one
image with its buffer in the back buffer; we only drop at most one frame.

We could potentially have the images call ReleaseSurfaceTexture() after the last
one calls updateTexImage(). But that's probably not worth the complication.

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/avda_surface...
File media/gpu/avda_surface_bundle.h (right):

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/avda_surface...
media/gpu/avda_surface_bundle.h:16: // the output surface that one needs.  In
other words, it's the surface, any
On 2017/02/23 18:18:46, liberato wrote:
> On 2017/02/22 20:38:56, watk wrote:
> > First sentence a word.
> 
> parse i could not.

Sorry :) I was trying to say that I couldn't parse the first sentence. Delete
the second "that one" maybe?

https://codereview.chromium.org/2707703002/diff/260001/media/gpu/avda_surface...
File media/gpu/avda_surface_bundle.cc (right):

https://codereview.chromium.org/2707703002/diff/260001/media/gpu/avda_surface...
media/gpu/avda_surface_bundle.cc:18: // ithere since the producer side is shut
down.
s/ithere/it there/

[liberato (no reviews please), 2017-03-06 22:59:36]

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/avda_surface...
File media/gpu/avda_surface_bundle.cc (right):

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/avda_surface...
media/gpu/avda_surface_bundle.cc:16: surface_texture = nullptr;
On 2017/02/23 20:03:47, watk wrote:
> On 2017/02/23 18:18:46, liberato wrote:
> > On 2017/02/22 20:38:56, watk wrote:
> > > Yes I think so. If you drop the bundle you're dropping the ability to
render
> > to
> > > the surface, so there's no reason not to ReleaseST(). Codec images might
> > outlive
> > > AVDA for an arbitrary time.
> > 
> > i'm not sure that it's safe, the more that i think about it.  the consumer
> might
> > have a back buffer that it wants to swap.  i'll update the comment.
> 
> But this is what we've always done right? The PostTask you deleted from
> ActualDestroy().
> 
> We want to release the back-buffer memory so we don't have a single frame
> keeping a full ST alive. It doesn't really matter at that point if there's one
> image with its buffer in the back buffer; we only drop at most one frame.
> 
> We could potentially have the images call ReleaseSurfaceTexture() after the
last
> one calls updateTexImage(). But that's probably not worth the complication.

ah, really good catch!  done.

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/avda_surface...
File media/gpu/avda_surface_bundle.h (right):

https://codereview.chromium.org/2707703002/diff/240001/media/gpu/avda_surface...
media/gpu/avda_surface_bundle.h:16: // the output surface that one needs.  In
other words, it's the surface, any
On 2017/02/23 20:03:47, watk wrote:
> On 2017/02/23 18:18:46, liberato wrote:
> > On 2017/02/22 20:38:56, watk wrote:
> > > First sentence a word.
> > 
> > parse i could not.
> 
> Sorry :) I was trying to say that I couldn't parse the first sentence. Delete
> the second "that one" maybe?

Done.

https://codereview.chromium.org/2707703002/diff/260001/media/gpu/avda_surface...
File media/gpu/avda_surface_bundle.cc (right):

https://codereview.chromium.org/2707703002/diff/260001/media/gpu/avda_surface...
media/gpu/avda_surface_bundle.cc:18: // ithere since the producer side is shut
down.
On 2017/02/23 20:03:47, watk wrote:
> s/ithere/it there/

Done.

[watk, 2017-03-06 23:03:44]

I forgot about this. My last review said it looked mostly good though, so I'll
trust past me: lgtm

[liberato (no reviews please), 2017-03-06 23:07:27]

On 2017/03/06 23:03:44, watk wrote:
> I forgot about this. My last review said it looked mostly good though, so I'll
> trust past me: lgtm

don't worry, i'll test it again before landing.  i don't remember either.... :)

[liberato (no reviews please), 2017-03-06 23:08:40]

Description was changed from

==========
Group AVDA output surface into AVDASurfaceBundle.

AVDASurfaceBundle holds the surface and/or SurfaceTexture that a
MediaCodec instance is using.  A reference to it is kept with the
codec, so that we don't destroy the output surface before the codec.

Keeping a reference to the surface bundle replaces the explicit post
of the SurfaceTexture during AVDA::ActualDestroy.  All calls to
ReleaseCodec send a reference to the SurfaceBundle along with the
codec.  The bundle also is replaced in UpdateSurface (the only time
that AVDA can change surfaces), so that failed calls to SetSurface
are easier to recover from.

Note that it other things (e.g., shared state) may still hold a
reference to the SurfaceTexture itself, so that the GL texture will
also outlast the GLImages.

BUG=
CQ_INCLUDE_TRYBOTS=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

==========
Group AVDA output surface into AVDASurfaceBundle.

AVDASurfaceBundle holds the surface and/or SurfaceTexture that a
MediaCodec instance is using.  A reference to it is kept with the
codec, so that we don't destroy the output surface before the codec.

Keeping a reference to the surface bundle replaces the explicit post
of the SurfaceTexture during AVDA::ActualDestroy.  All calls to
ReleaseCodec send a reference to the SurfaceBundle along with the
codec.  The bundle also is replaced in UpdateSurface (the only time
that AVDA can change surfaces), so that failed calls to SetSurface
are easier to recover from.

Note that it other things (e.g., shared state) may still hold a
reference to the SurfaceTexture itself, so that the GL texture will
also outlast the GLImages.

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
==========

[liberato (no reviews please), 2017-03-07 21:35:07]

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

[commit-bot: I haz the power, 2017-03-07 21:35:56]

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-03-07 23:59:55]

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

[commit-bot: I haz the power, 2017-03-07 23:59:56]

Dry run: This issue passed the CQ dry run.

[liberato (no reviews please), 2017-03-08 19:15:00]

The CQ bit was checked by liberato@chromium.org

[liberato (no reviews please), 2017-03-08 19:15:01]

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

[commit-bot: I haz the power, 2017-03-08 19:15:33]

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-03-08 19:21:40]

CQ is committing da patch.

Bot data: {"patchset_id": 340001, "attempt_start_ts": 1489000500908410,
"parent_rev": "78d93eb4a4457b4be9efcc4e792e1970612b4ea9", "commit_rev":
"5e0962e514794c20d7c5fb614f7b7d0589177fb7"}

[commit-bot: I haz the power, 2017-03-08 19:22:19]

Description was changed from

==========
Group AVDA output surface into AVDASurfaceBundle.

AVDASurfaceBundle holds the surface and/or SurfaceTexture that a
MediaCodec instance is using.  A reference to it is kept with the
codec, so that we don't destroy the output surface before the codec.

Keeping a reference to the surface bundle replaces the explicit post
of the SurfaceTexture during AVDA::ActualDestroy.  All calls to
ReleaseCodec send a reference to the SurfaceBundle along with the
codec.  The bundle also is replaced in UpdateSurface (the only time
that AVDA can change surfaces), so that failed calls to SetSurface
are easier to recover from.

Note that it other things (e.g., shared state) may still hold a
reference to the SurfaceTexture itself, so that the GL texture will
also outlast the GLImages.

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

==========
Group AVDA output surface into AVDASurfaceBundle.

AVDASurfaceBundle holds the surface and/or SurfaceTexture that a
MediaCodec instance is using.  A reference to it is kept with the
codec, so that we don't destroy the output surface before the codec.

Keeping a reference to the surface bundle replaces the explicit post
of the SurfaceTexture during AVDA::ActualDestroy.  All calls to
ReleaseCodec send a reference to the SurfaceBundle along with the
codec.  The bundle also is replaced in UpdateSurface (the only time
that AVDA can change surfaces), so that failed calls to SetSurface
are easier to recover from.

Note that it other things (e.g., shared state) may still hold a
reference to the SurfaceTexture itself, so that the GL texture will
also outlast the GLImages.

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

Review-Url: https://codereview.chromium.org/2707703002
Cr-Commit-Position: refs/heads/master@{#455511}
Committed:
https://chromium.googlesource.com/chromium/src/+/5e0962e514794c20d7c5fb614f7b...
==========

[commit-bot: I haz the power, 2017-03-08 19:22:20]

Committed patchset #18 (id:340001) as
https://chromium.googlesource.com/chromium/src/+/5e0962e514794c20d7c5fb614f7b...