Begin refactor of AVDA to support zero-copy.

Begin refactor of AVDA to support zero-copy.

Refactor AndroidVideoDecodeAccelerator into a class that manages the MediaCodec buffers, but delegates handling the SurfaceTexture to BackingStrategy implementations.  The AndroidCopyingBackingStrategy class implements the texture copy into the picture buffer, as it did in previous versions.

BUG=507834
Committed: https://crrev.com/c765882aa08a8dd38847400b05312c0b76aa2334
Cr-Commit-Position: refs/heads/master@{#348774}

[liberato (no reviews please), 2015-08-27 18:08:45]

liberato@chromium.org changed reviewers:
+ qinmin@chromium.org, sandersd@chromium.org, timav@chromium.org,
watk@chromium.org

[liberato (no reviews please), 2015-08-27 20:46:42]

liberato@chromium.org changed reviewers:
- qinmin@chromium.org, timav@chromium.org

[watk, 2015-08-28 21:52:05]

I only had minor comments

https://codereview.chromium.org/1313913003/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/android_video_decode_accelerator.h (right):

https://codereview.chromium.org/1313913003/diff/60001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.h:16: #include
"base/timer/timer.h"
Doesn't look like we need timer.h. Looks like there are a couple of other unused
headers too.

https://codereview.chromium.org/1313913003/diff/60001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.h:31: // Does not take
ownership of |client| which must outlive |*this|.
Comment is out of date

https://codereview.chromium.org/1313913003/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/android_video_decode_accelerator_base.cc (right):

https://codereview.chromium.org/1313913003/diff/60001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator_base.cc:1: //
Copyright (c) 2013 The Chromium Authors. All rights reserved.
s/2013/2015

https://codereview.chromium.org/1313913003/diff/60001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator_base.cc:306:
media_codec_->ReleaseOutputBuffer(buf_index, false);
AFAIK, this is ok and we won't get a valid frame with EOS.

https://codereview.chromium.org/1313913003/diff/60001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator_base.cc:334: int32
codec_buf_index,
Slight naming inconsistency with the header: codec_buffer_index vs
codec_buf_index.

https://codereview.chromium.org/1313913003/diff/60001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator_base.cc:356:
ILLEGAL_STATE);
Should this check be done inside AssignCurrent? That it uses the gl decoder
seems like an implementation detail of AssignCurrent.

https://codereview.chromium.org/1313913003/diff/60001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator_base.cc:548:
AndroidVideoDecodeAcceleratorBase::GetClient() const {
This and the following aren't in declaration order. The style guide suggests the
definition and declaration order should match.

https://codereview.chromium.org/1313913003/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/android_video_decode_accelerator_base.h (right):

https://codereview.chromium.org/1313913003/diff/60001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator_base.h:1: // Copyright
(c) 2013 The Chromium Authors. All rights reserved.
s/2013/2015

https://codereview.chromium.org/1313913003/diff/60001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator_base.h:29: // class.
http://developer.android.com/reference/android/media/MediaCodec.html
Update this comment?

https://codereview.chromium.org/1313913003/diff/60001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator_base.h:33: // Does not
take ownership of |client| which must outlive |*this|.
Comment needs to go down a couple of lines

[liberato (no reviews please), 2015-09-04 17:13:52]

liberato@chromium.org changed reviewers:
+ qinmin@chromium.org, timav@chromium.org

[liberato (no reviews please), 2015-09-04 17:25:39]

Patchset #6 (id:100001) has been deleted

[liberato (no reviews please), 2015-09-04 17:59:46]

liberato@chromium.org changed reviewers:
- qinmin@chromium.org, timav@chromium.org

[liberato (no reviews please), 2015-09-04 17:59:47]

timav, qinmin: comments always welcome, but also feel free to ignore as fyi.

after some conversation with sandersd@, this version delegates rather than
derives the work of assigning decoded frames to PictureBuffers.  now that i'm
looking at both versions, i think that i slightly prefer the original, given the
simplicity of it and the expected lifetime of this code.

https://codereview.chromium.org/1313913003/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/android_video_decode_accelerator.h (right):

https://codereview.chromium.org/1313913003/diff/60001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.h:16: #include
"base/timer/timer.h"
On 2015/08/28 21:52:05, watk wrote:
> Doesn't look like we need timer.h. Looks like there are a couple of other
unused
> headers too.

Done.

https://codereview.chromium.org/1313913003/diff/60001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.h:31: // Does not take
ownership of |client| which must outlive |*this|.
On 2015/08/28 21:52:04, watk wrote:
> Comment is out of date

Done.

https://codereview.chromium.org/1313913003/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/android_video_decode_accelerator_base.cc (right):

https://codereview.chromium.org/1313913003/diff/60001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator_base.cc:1: //
Copyright (c) 2013 The Chromium Authors. All rights reserved.
On 2015/08/28 21:52:05, watk wrote:
> s/2013/2015

Done.

https://codereview.chromium.org/1313913003/diff/60001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator_base.cc:306:
media_codec_->ReleaseOutputBuffer(buf_index, false);
On 2015/08/28 21:52:05, watk wrote:
> AFAIK, this is ok and we won't get a valid frame with EOS.

yeah, i this so too.  i'll leave the TODO here until qinmin@ and timav@ take a
look.

https://codereview.chromium.org/1313913003/diff/60001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator_base.cc:334: int32
codec_buf_index,
On 2015/08/28 21:52:05, watk wrote:
> Slight naming inconsistency with the header: codec_buffer_index vs
> codec_buf_index.

thanks.

https://codereview.chromium.org/1313913003/diff/60001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator_base.cc:356:
ILLEGAL_STATE);
On 2015/08/28 21:52:05, watk wrote:
> Should this check be done inside AssignCurrent? That it uses the gl decoder
> seems like an implementation detail of AssignCurrent.

yeah, i went back and forth on this one.  i kept it as-is because moving the
RETURN_ON_FAILURE logic around turned out to make it ugly.

i'll see if i can figure out a way to do this better.

... i refactored RETURN_ON_FAILURE a bit and moved it into its own .h file.  now
_base and the subclasses can use it.  see if you like this better.

https://codereview.chromium.org/1313913003/diff/60001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator_base.cc:548:
AndroidVideoDecodeAcceleratorBase::GetClient() const {
On 2015/08/28 21:52:05, watk wrote:
> This and the following aren't in declaration order. The style guide suggests
the
> definition and declaration order should match.

i think that several functions were out of order before, too.  i'll move these
ones closer to where they should be, but i don't want to re-order existing ones
as part of this, in order to keep the (hopefully lack of) diff clearer.

https://codereview.chromium.org/1313913003/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/android_video_decode_accelerator_base.h (right):

https://codereview.chromium.org/1313913003/diff/60001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator_base.h:1: // Copyright
(c) 2013 The Chromium Authors. All rights reserved.
On 2015/08/28 21:52:05, watk wrote:
> s/2013/2015

Done.

https://codereview.chromium.org/1313913003/diff/60001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator_base.h:29: // class.
http://developer.android.com/reference/android/media/MediaCodec.html
On 2015/08/28 21:52:05, watk wrote:
> Update this comment?

whoops!  thanks!

https://codereview.chromium.org/1313913003/diff/60001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator_base.h:33: // Does not
take ownership of |client| which must outlive |*this|.
On 2015/08/28 21:52:05, watk wrote:
> Comment needs to go down a couple of lines

Done.

https://codereview.chromium.org/1313913003/diff/80001/content/content_common....
File content/content_common.gypi (right):

https://codereview.chromium.org/1313913003/diff/80001/content/content_common....
content/content_common.gypi:799:
'common/gpu/media/android_video_decode_accelerator_base.cc',
forgot to add avda_return_on_failure.h here.  will do so.

[sandersd (OOO until July 31), 2015-09-04 22:12:53]

I like this better than the inheritance, but not as much as just forking the
whole implementation. The good news is that this structure is easy to integrate
back into a single class when we eventually delete the copying version.

https://codereview.chromium.org/1313913003/diff/120001/content/common/gpu/med...
File content/common/gpu/media/android_video_decode_accelerator_impl.h (right):

https://codereview.chromium.org/1313913003/diff/120001/content/common/gpu/med...
content/common/gpu/media/android_video_decode_accelerator_impl.h:33: class
CONTENT_EXPORT AndroidVideoDecodeAcceleratorImpl
It seems like this would be better as just AndroidVideoDecodeAccelerator; the
interface isn't really used anymore.

https://codereview.chromium.org/1313913003/diff/180001/content/common/gpu/med...
File content/common/gpu/media/android_video_decode_accelerator.h (right):

https://codereview.chromium.org/1313913003/diff/180001/content/common/gpu/med...
content/common/gpu/media/android_video_decode_accelerator.h:58: class
BackingStrategy : public base::RefCounted<BackingStrategy> {
Why do backingstrategies need to be refcounted?

[watk, 2015-09-04 23:15:16]

https://codereview.chromium.org/1313913003/diff/180001/content/common/gpu/med...
File content/common/gpu/media/android_video_decode_accelerator.h (right):

https://codereview.chromium.org/1313913003/diff/180001/content/common/gpu/med...
content/common/gpu/media/android_video_decode_accelerator.h:38: class
StateProvider {
I think this is mostly subjective, so take or leave this thought. I wonder if it
would be simpler to inject every dependency (or most dependencies) as parameters
of AssignCurrentSurfaceToPictureBuffer, and delete StateProvider.

* GetSize: pass it in (even if you keep StateProvider, I think this should be
passed in, because it's a per frame thing).
* ThreadChecker: seems like the strategy could create its own
* GetSurfaceTexture: pass a pointer/ref
* GetSurfaceTextureId: pass it in
* GetGlDecoder: could pass it in the constructor of the strategy
* GetMediaCodec: pass a pointer/ref
* PostError: LOG(ERROR) the error and return a media::VDA::Error from
AssignCurrentSurfaceToPictureBuffer

Then you'd also get to remove SetStateProvider, and avda_return_on_failure.h

The parameter list would be quite long, which I feel like is OK. Also, the call
site should be (fairly) readable because the arguments have good names.

[liberato (no reviews please), 2015-09-08 19:50:00]

i think this covers all the feedback.  impl class is no longer needed.  things
are generally simpler.  as an added bonus, webrtc still works.

-fl

https://codereview.chromium.org/1313913003/diff/120001/content/common/gpu/med...
File content/common/gpu/media/android_video_decode_accelerator_impl.h (right):

https://codereview.chromium.org/1313913003/diff/120001/content/common/gpu/med...
content/common/gpu/media/android_video_decode_accelerator_impl.h:33: class
CONTENT_EXPORT AndroidVideoDecodeAcceleratorImpl
On 2015/09/04 22:12:52, sandersd wrote:
> It seems like this would be better as just AndroidVideoDecodeAccelerator; the
> interface isn't really used anymore.

it was a forward reference management strategy.  in other words, i used it like
a namespace :)

i'll move StateProvider to a separate class, and get rid of Create() too.  it'll
probably be clearer.

https://codereview.chromium.org/1313913003/diff/180001/content/common/gpu/med...
File content/common/gpu/media/android_video_decode_accelerator.h (right):

https://codereview.chromium.org/1313913003/diff/180001/content/common/gpu/med...
content/common/gpu/media/android_video_decode_accelerator.h:38: class
StateProvider {
On 2015/09/04 23:15:16, watk wrote:
> I think this is mostly subjective, so take or leave this thought. I wonder if
it
> would be simpler to inject every dependency (or most dependencies) as
parameters
> of AssignCurrentSurfaceToPictureBuffer, and delete StateProvider.
> 
> * GetSize: pass it in (even if you keep StateProvider, I think this should be
> passed in, because it's a per frame thing).
> * ThreadChecker: seems like the strategy could create its own
> * GetSurfaceTexture: pass a pointer/ref
> * GetSurfaceTextureId: pass it in
> * GetGlDecoder: could pass it in the constructor of the strategy
> * GetMediaCodec: pass a pointer/ref
> * PostError: LOG(ERROR) the error and return a media::VDA::Error from
> AssignCurrentSurfaceToPictureBuffer
> 
> Then you'd also get to remove SetStateProvider, and avda_return_on_failure.h
> 
> The parameter list would be quite long, which I feel like is OK. Also, the
call
> site should be (fairly) readable because the arguments have good names.

i had a similar this argument with myself.  i decided to wait until the zero
copy version was in place to make a decision.  i originally went with the
explicit setter now because the strategy does implicitly depend on the
StateProvider staying constant.

for the copying case, it turned out to look rather like there's little stateful
connection between the AVDA and the strategy.  for the zero copy case, we'll
care about frame numbers and outstanding codec buffers and such.  depending on
where that state is held, it'll make it obvious what to do here.

https://codereview.chromium.org/1313913003/diff/180001/content/common/gpu/med...
content/common/gpu/media/android_video_decode_accelerator.h:58: class
BackingStrategy : public base::RefCounted<BackingStrategy> {
On 2015/09/04 22:12:53, sandersd wrote:
> Why do backingstrategies need to be refcounted?

Done.

[watk, 2015-09-09 00:36:01]

Bunch of formatting comments, sorry! I stopped half way because it's probably
easier for you to run clang format than address them manually.

I recommend setting up clang format with your editor like this
(https://www.chromium.org/developers/sublime-text#TOC-Format-selection-or-area...).
Super handy for running clang-format on just the lines you change. I never think
about formatting because I run that habitually.

https://codereview.chromium.org/1313913003/diff/220001/content/common/gpu/med...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1313913003/diff/220001/content/common/gpu/med...
content/common/gpu/media/android_video_decode_accelerator.cc:91: Client* client)
{
Unnecessary formatting change?

https://codereview.chromium.org/1313913003/diff/220001/content/common/gpu/med...
content/common/gpu/media/android_video_decode_accelerator.cc:263:
base::Bind(&AndroidVideoDecodeAccelerator::
Unnecessary formatting change?

https://codereview.chromium.org/1313913003/diff/220001/content/common/gpu/med...
content/common/gpu/media/android_video_decode_accelerator.cc:354: i->second);
Formatting seems weird

https://codereview.chromium.org/1313913003/diff/220001/content/common/gpu/med...
content/common/gpu/media/android_video_decode_accelerator.cc:363:
gfx::Rect(size_), false)));
Unnecessary formatting change?

https://codereview.chromium.org/1313913003/diff/220001/content/common/gpu/med...
content/common/gpu/media/android_video_decode_accelerator.cc:387: size_,
strategy_->GetTextureTarget());
Formatting seems weird, indented too far

https://codereview.chromium.org/1313913003/diff/220001/content/common/gpu/med...
File content/common/gpu/media/android_video_decode_accelerator_state_provider.h
(right):

https://codereview.chromium.org/1313913003/diff/220001/content/common/gpu/med...
content/common/gpu/media/android_video_decode_accelerator_state_provider.h:32:
virtual ~AndroidVideoDecodeAcceleratorStateProvider() {}
indentation should be two spaces

https://codereview.chromium.org/1313913003/diff/220001/content/common/gpu/med...
content/common/gpu/media/android_video_decode_accelerator_state_provider.h:42:
// Helper function to report an error condition and stop decodnig.
s/decodnig/decoding/

https://codereview.chromium.org/1313913003/diff/220001/content/common/gpu/med...
File content/common/gpu/media/android_video_decode_accelerator_unittest.cc
(right):

https://codereview.chromium.org/1313913003/diff/220001/content/common/gpu/med...
content/common/gpu/media/android_video_decode_accelerator_unittest.cc:69: new
AndroidCopyingBackingStrategy())));
You can use make_scoped_ptr() to make this less noisy

https://codereview.chromium.org/1313913003/diff/220001/content/common/gpu/med...
File content/common/gpu/media/avda_return_on_failure.h (right):

https://codereview.chromium.org/1313913003/diff/220001/content/common/gpu/med...
content/common/gpu/media/avda_return_on_failure.h:14: // various subclasses. 
|provider| must support PostError.
Comment is out of date already :P

[liberato (no reviews please), 2015-09-09 18:20:44]

Patchset #12 (id:240001) has been deleted

[liberato (no reviews please), 2015-09-09 18:28:17]

sorry, i entirely forgot to run clang-format.

didn't reply per-comment this time.  i updated two instances of make_scoped_ptr
(thanks, neat), and fixed up two comments.

thanks
-fl

[watk, 2015-09-09 18:49:49]

On 2015/09/09 18:28:17, liberato wrote:
> sorry, i entirely forgot to run clang-format.
> 
> didn't reply per-comment this time.  i updated two instances of
make_scoped_ptr
> (thanks, neat), and fixed up two comments.
> 
> thanks
> -fl

np, lgtm

[liberato (no reviews please), 2015-09-09 21:01:32]

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

[liberato (no reviews please), 2015-09-09 21:03:18]

@sievers: can you provide an owner lgtm for BUILD.gn? any other comments on the
refactor are, of course, welcome.

thanks
-fl

[sandersd (OOO until July 31), 2015-09-10 20:28:20]

I agree with watk@ that some of these could be parameters (constructor or
otherwise). We can wait until the followup CL to decide the best path there.

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
File content/common/gpu/media/android_copying_backing_strategy.cc (right):

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
content/common/gpu/media/android_copying_backing_strategy.cc:25: // have actual
use case.
We should probably update all of the copies of this comment because the
referenced bug is both closed and unclear as to the root issue. There is a
specific check for available picture buffers during preroll and we enter
low-delay mode if that happens, so even if the root issue is real I don't think
it can deadlock decoding.

A more current explanation would be more like: We need at least one
PictureBuffer for each buffered frame, plus one for the current frame.
Additional PictureBuffers can only slightly reduce latency while significantly
increasing memory.

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
File content/common/gpu/media/android_copying_backing_strategy.h (right):

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
content/common/gpu/media/android_copying_backing_strategy.h:1: // Copyright (c)
2015 The Chromium Authors. All rights reserved.
http://www.chromium.org/developers/coding-style#TOC-File-headers

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
content/common/gpu/media/android_copying_backing_strategy.h:8: #include <list>
Many of these headers are not used in this file.

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
File content/common/gpu/media/android_video_decode_accelerator_state_provider.h
(right):

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
content/common/gpu/media/android_video_decode_accelerator_state_provider.h:8:
#include <list>
Again several of these are unused. Perhaps you should do a sweep through all the
includes in this CL.

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
File content/common/gpu/media/android_video_decode_accelerator_unittest.cc
(right):

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
content/common/gpu/media/android_video_decode_accelerator_unittest.cc:69: //
scoped_ptr<AndroidVideoDecodeAccelerator::BackingStrategy>(
Remove commented code.

[no sievers, 2015-09-11 00:20:39]

just a few nits, otherwise lgtm

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
File content/common/gpu/media/android_video_decode_accelerator.h (right):

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
content/common/gpu/media/android_video_decode_accelerator.h:82: //
AndroidVideoDecodeAccelerator::StateProvider
nit: AndroidVideoDecodeAcceleratorStateProvider

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
File content/common/gpu/media/android_video_decode_accelerator_state_provider.h
(right):

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
content/common/gpu/media/android_video_decode_accelerator_state_provider.h:1: //
Copyright (c) 2015 The Chromium Authors. All rights reserved.
nit: drop (c)

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
content/common/gpu/media/android_video_decode_accelerator_state_provider.h:8:
#include <list>
On 2015/09/10 20:28:20, sandersd wrote:
> Again several of these are unused. Perhaps you should do a sweep through all
the
> includes in this CL.

Actually, all of these are unused in this header.

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
content/common/gpu/media/android_video_decode_accelerator_state_provider.h:18:
#include "gpu/command_buffer/service/gles2_cmd_decoder.h"
And most of these are could be forward-declared.

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
File content/common/gpu/media/avda_return_on_failure.h (right):

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
content/common/gpu/media/avda_return_on_failure.h:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
nit: drop (c)

[qinmin, 2015-09-11 17:29:50]

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

[qinmin, 2015-09-11 17:29:51]

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
File content/common/gpu/media/android_copying_backing_strategy.cc (right):

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
content/common/gpu/media/android_copying_backing_strategy.cc:103: const static
GLfloat default_matrix[16] = {1.0f, 0.0f, 0.0f, 0.0f, 0.0f, 1.0f,
should this be using the transform_matrix instead?

[liberato (no reviews please), 2015-09-14 17:41:28]

copyrights are up to date, includes are reduced, misc comments are updated, bugs
are filed.

thanks
-fl

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
File content/common/gpu/media/android_copying_backing_strategy.cc (right):

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
content/common/gpu/media/android_copying_backing_strategy.cc:25: // have actual
use case.
On 2015/09/10 20:28:20, sandersd wrote:
> We should probably update all of the copies of this comment because the
> referenced bug is both closed and unclear as to the root issue. There is a
> specific check for available picture buffers during preroll and we enter
> low-delay mode if that happens, so even if the root issue is real I don't
think
> it can deadlock decoding.
> 
> A more current explanation would be more like: We need at least one
> PictureBuffer for each buffered frame, plus one for the current frame.
> Additional PictureBuffers can only slightly reduce latency while significantly
> increasing memory.

i saw weirdness when prototyping the zero copy code, with this set to 2.  looked
like a frame was being held outside of the VDA sometimes during playback,
causing a similar deadlock.

i opened a bug to sort it out, and updated the comment correspondingly.

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
content/common/gpu/media/android_copying_backing_strategy.cc:103: const static
GLfloat default_matrix[16] = {1.0f, 0.0f, 0.0f, 0.0f, 0.0f, 1.0f,
On 2015/09/11 17:29:51, qinmin wrote:
> should this be using the transform_matrix instead?

great question.  i want to preserve current behavior with this CL, so i've
opened crbug.com/530681 to track this.

possibly, nobody went back to update this code after crbug.com/226218 was
closed.  i'll see if any of my devices produce a non-identity transform that i
can test against.  if they do, it still has to be pretty close to identity given
that it looks okay.

do you know of any devices that are far enough from identity to be obviously
broken?

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
File content/common/gpu/media/android_copying_backing_strategy.h (right):

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
content/common/gpu/media/android_copying_backing_strategy.h:1: // Copyright (c)
2015 The Chromium Authors. All rights reserved.
On 2015/09/10 20:28:20, sandersd wrote:
> http://www.chromium.org/developers/coding-style#TOC-File-headers

Done, and elsewhere.

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
content/common/gpu/media/android_copying_backing_strategy.h:8: #include <list>
On 2015/09/10 20:28:20, sandersd wrote:
> Many of these headers are not used in this file.

Done.

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
File content/common/gpu/media/android_video_decode_accelerator.h (right):

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
content/common/gpu/media/android_video_decode_accelerator.h:82: //
AndroidVideoDecodeAccelerator::StateProvider
On 2015/09/11 00:20:39, sievers wrote:
> nit: AndroidVideoDecodeAcceleratorStateProvider

Done.

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
File content/common/gpu/media/android_video_decode_accelerator_state_provider.h
(right):

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
content/common/gpu/media/android_video_decode_accelerator_state_provider.h:1: //
Copyright (c) 2015 The Chromium Authors. All rights reserved.
On 2015/09/11 00:20:39, sievers wrote:
> nit: drop (c)

Done.

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
content/common/gpu/media/android_video_decode_accelerator_state_provider.h:8:
#include <list>
On 2015/09/10 20:28:20, sandersd wrote:
> Again several of these are unused. Perhaps you should do a sweep through all
the
> includes in this CL.

Done.

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
content/common/gpu/media/android_video_decode_accelerator_state_provider.h:8:
#include <list>
On 2015/09/11 00:20:39, sievers wrote:
> On 2015/09/10 20:28:20, sandersd wrote:
> > Again several of these are unused. Perhaps you should do a sweep through all
> the
> > includes in this CL.
> 
> Actually, all of these are unused in this header.

Done.

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
content/common/gpu/media/android_video_decode_accelerator_state_provider.h:18:
#include "gpu/command_buffer/service/gles2_cmd_decoder.h"
On 2015/09/11 00:20:39, sievers wrote:
> And most of these are could be forward-declared.

Done.

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
File content/common/gpu/media/android_video_decode_accelerator_unittest.cc
(right):

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
content/common/gpu/media/android_video_decode_accelerator_unittest.cc:69: //
scoped_ptr<AndroidVideoDecodeAccelerator::BackingStrategy>(
On 2015/09/10 20:28:20, sandersd wrote:
> Remove commented code.

whoops.  done.

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
File content/common/gpu/media/avda_return_on_failure.h (right):

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
content/common/gpu/media/avda_return_on_failure.h:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
On 2015/09/11 00:20:39, sievers wrote:
> nit: drop (c)

Done.

[watk, 2015-09-14 18:15:20]

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
File content/common/gpu/media/android_copying_backing_strategy.cc (right):

https://codereview.chromium.org/1313913003/diff/280001/content/common/gpu/med...
content/common/gpu/media/android_copying_backing_strategy.cc:103: const static
GLfloat default_matrix[16] = {1.0f, 0.0f, 0.0f, 0.0f, 0.0f, 1.0f,
On 2015/09/14 17:41:28, liberato wrote:
> On 2015/09/11 17:29:51, qinmin wrote:
> > should this be using the transform_matrix instead?
> 
> great question.  i want to preserve current behavior with this CL, so i've
> opened crbug.com/530681 to track this.
> 
> possibly, nobody went back to update this code after crbug.com/226218 was
> closed.  i'll see if any of my devices produce a non-identity transform that i
> can test against.  if they do, it still has to be pretty close to identity
given
> that it looks okay.
> 
> do you know of any devices that are far enough from identity to be obviously
> broken?

qinmin@ said the xoom has non identity, but I don't have one. I do have access
to a Galaxy S6 which isn't identity and clearly looks wrong, so I could test it.
I'll leave a note on the bug.

[DaleCurtis, 2015-09-14 18:46:20]

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

[DaleCurtis, 2015-09-14 18:46:21]

owner lgtm % nits.

https://codereview.chromium.org/1313913003/diff/300001/content/common/gpu/med...
File content/common/gpu/media/android_copying_backing_strategy.cc (right):

https://codereview.chromium.org/1313913003/diff/300001/content/common/gpu/med...
content/common/gpu/media/android_copying_backing_strategy.cc:109: const static
GLfloat default_matrix[16] = {1.0f, 0.0f, 0.0f, 0.0f, 0.0f, 1.0f,
static const? Rename to kDefaultMatrix?

https://codereview.chromium.org/1313913003/diff/300001/content/common/gpu/med...
File content/common/gpu/media/android_copying_backing_strategy.h (right):

https://codereview.chromium.org/1313913003/diff/300001/content/common/gpu/med...
content/common/gpu/media/android_copying_backing_strategy.h:8: //#include <list>
?

https://codereview.chromium.org/1313913003/diff/300001/content/common/gpu/med...
File content/common/gpu/media/android_video_decode_accelerator_state_provider.h
(right):

https://codereview.chromium.org/1313913003/diff/300001/content/common/gpu/med...
content/common/gpu/media/android_video_decode_accelerator_state_provider.h:8:
//#include <list>
?

[liberato (no reviews please), 2015-09-14 22:34:53]

https://codereview.chromium.org/1313913003/diff/300001/content/common/gpu/med...
File content/common/gpu/media/android_copying_backing_strategy.cc (right):

https://codereview.chromium.org/1313913003/diff/300001/content/common/gpu/med...
content/common/gpu/media/android_copying_backing_strategy.cc:109: const static
GLfloat default_matrix[16] = {1.0f, 0.0f, 0.0f, 0.0f, 0.0f, 1.0f,
On 2015/09/14 18:46:20, DaleCurtis wrote:
> static const? Rename to kDefaultMatrix?

done, as kIdentityMatrix.

https://codereview.chromium.org/1313913003/diff/300001/content/common/gpu/med...
File content/common/gpu/media/android_copying_backing_strategy.h (right):

https://codereview.chromium.org/1313913003/diff/300001/content/common/gpu/med...
content/common/gpu/media/android_copying_backing_strategy.h:8: //#include <list>
On 2015/09/14 18:46:21, DaleCurtis wrote:
> ?

no idea. done.

https://codereview.chromium.org/1313913003/diff/300001/content/common/gpu/med...
File content/common/gpu/media/android_video_decode_accelerator_state_provider.h
(right):

https://codereview.chromium.org/1313913003/diff/300001/content/common/gpu/med...
content/common/gpu/media/android_video_decode_accelerator_state_provider.h:8:
//#include <list>
On 2015/09/14 18:46:21, DaleCurtis wrote:
> ?

i'm sure i removed at least one set of those, and i thought both.  must have
done something unintentionally, or gotten interrupted.  thanks for catching.

[liberato (no reviews please), 2015-09-14 22:34:56]

The CQ bit was checked by liberato@chromium.org

[liberato (no reviews please), 2015-09-14 22:34:57]

The patchset sent to the CQ was uploaded after l-g-t-m from watk@chromium.org,
sievers@chromium.org, dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1313913003/#ps320001
(title: "cl feedback + rebased.")

[commit-bot: I haz the power, 2015-09-14 22:36:13]

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

[commit-bot: I haz the power, 2015-09-14 23:54:31]

Committed patchset #15 (id:320001)

[commit-bot: I haz the power, 2015-09-14 23:55:14]

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

[commit-bot: I haz the power, 2015-09-23 12:39:32]

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