Introduce GpuVideoDecodeAcceleratorFactory.

Introduce GpuVideoDecodeAcceleratorFactory.

- Move platform-specific code from GpuVideoDecodeAccelerator to
GpuVideoDecodeAcceleratorFactory.

- Make GVDAFactory a content/public interface, to provide the ability to
instantiate VDAs from outside content/.

- Unify how we obtain access to various GL functionality/classes from VDAs
by introducing a set of callbacks provided by the client.

- Replace VDA::CanDecodeOnIOThread() with
VDA::TryInitializeDecodeOnSeparateThread(). This allows us to remove
additional client/taskrunner arguments from VDA constructors, and give client
the option to use a separate thread to decode, instead of having to make this
decision in the factory, and enforcing these arguments in the constructors.

- Deduplicate VDA creation code across users (currently GVDA and vdaunittest).

BUG=b/27687678
TEST=compile/run various VDA impls
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/6977e5243786901a766a38c2c291464875dffbd6
Cr-Commit-Position: refs/heads/master@{#383256}

[Pawel Osciak, 2016-02-29 01:51:32]

Description was changed from

==========
Introduce GpuVideoDecodeAcceleratorFactory.

- Move platform-specific code from GpuVideoDecodeAccelerator to
GpuVideoDecodeAcceleratorFactory.

- Make GVDAFactory a content/public interface, to provide the ability to
instantiate VDAs from outside content/.

- Unify how we obtain access to various GL functionality/classes from VDAs
by introducing a set of callbacks provided by the client.

- Replace VDA::CanDecodeOnIOThread() with
VDA::TryInitializeDecodeOnSeparateThread(). This allows us to remove
additional client/taskrunner arguments from VDA constructors, and give client
the option to use a separate thread to decode, instead of having to make this
decision in the factory, and enforcing these arguments in the constructors.

- Deduplicate VDA creation code across users (currently GVDA and vdaunittest).

BUG=None
TEST=compile/run various VDA impls
==========

to

==========
Introduce GpuVideoDecodeAcceleratorFactory.

- Move platform-specific code from GpuVideoDecodeAccelerator to
GpuVideoDecodeAcceleratorFactory.

- Make GVDAFactory a content/public interface, to provide the ability to
instantiate VDAs from outside content/.

- Unify how we obtain access to various GL functionality/classes from VDAs
by introducing a set of callbacks provided by the client.

- Replace VDA::CanDecodeOnIOThread() with
VDA::TryInitializeDecodeOnSeparateThread(). This allows us to remove
additional client/taskrunner arguments from VDA constructors, and give client
the option to use a separate thread to decode, instead of having to make this
decision in the factory, and enforcing these arguments in the constructors.

- Deduplicate VDA creation code across users (currently GVDA and vdaunittest).

BUG=None
TEST=compile/run various VDA impls
==========

[Pawel Osciak, 2016-02-29 01:51:32]

posciak@chromium.org changed reviewers:
+ kcwu@chromium.org, owenlin@chromium.org, wuchengli@chromium.org

[kcwu, 2016-03-01 10:25:07]

https://codereview.chromium.org/1745903002/diff/1/content/common/gpu/media/an...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1745903002/diff/1/content/common/gpu/media/an...
content/common/gpu/media/android_video_decode_accelerator.cc:250: auto
gles_decoder = get_gles2_decoder_cb_.Run();
However about moving those callbacks invoking to Initialize() for this and other
VDAs? Just in order to decouple the client and the implementation, so they don't
need to sync about what callbacks are required.

https://codereview.chromium.org/1745903002/diff/1/content/common/gpu/media/vi...
File content/common/gpu/media/video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/1745903002/diff/1/content/common/gpu/media/vi...
content/common/gpu/media/video_decode_accelerator_unittest.cc:363:
scoped_ptr<media::VideoDecodeAccelerator> CreateFakeVDA();
Unused

https://codereview.chromium.org/1745903002/diff/1/content/public/common/gpu_v...
File content/public/common/gpu_video_decode_accelerator_factory.h (right):

https://codereview.chromium.org/1745903002/diff/1/content/public/common/gpu_v...
content/public/common/gpu_video_decode_accelerator_factory.h:8: #include
"base/memory/weak_ptr.h"
unused

https://codereview.chromium.org/1745903002/diff/1/content/public/common/gpu_v...
content/public/common/gpu_video_decode_accelerator_factory.h:51: using
CreateVDAFp = scoped_ptr<media::VideoDecodeAccelerator> (
How about move this type into .cc file? This type is only used in CreateVDA(),
no need to be exposed in .h?

https://codereview.chromium.org/1745903002/diff/1/content/public/common/gpu_v...
File content/public/common/gpu_video_decode_accelerator_helpers.h (right):

https://codereview.chromium.org/1745903002/diff/1/content/public/common/gpu_v...
content/public/common/gpu_video_decode_accelerator_helpers.h:27: // process.
Which callbacks are required depends on the implementation.
However, the client is still tightly coupled with the implementation and must
know what callbacks are required. What if the client passed a dummy callback but
the implementation did call it? See my comments in
android_video_decode_accelerator.cc for example.

https://codereview.chromium.org/1745903002/diff/1/content/public/common/gpu_v...
content/public/common/gpu_video_decode_accelerator_helpers.h:28: struct
gpu_vda_helpers {
Add comments that what threads can call these callbacks?

And mention these callbacks will be called more than once. (According to
callback.h, callbacks running more than once cannot use with base::Passed).

[wuchengli, 2016-03-01 10:48:28]

wuchengli@chromium.org changed reviewers:
- wuchengli@chromium.org

[wuchengli, 2016-03-01 10:49:13]

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

[wuchengli, 2016-03-01 10:49:14]

Kuang-che has reviewed this. I'm removing myself from the reviewer.

[wuchengli, 2016-03-01 10:49:28]

wuchengli@chromium.org changed reviewers:
- wuchengli@chromium.org

[Pawel Osciak, 2016-03-02 04:34:51]

Patchset #3 (id:40001) has been deleted

[Pawel Osciak, 2016-03-02 04:46:15]

Patchset #3 (id:60001) has been deleted

[Pawel Osciak, 2016-03-02 07:17:12]

ptal

https://codereview.chromium.org/1745903002/diff/1/content/common/gpu/media/an...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1745903002/diff/1/content/common/gpu/media/an...
content/common/gpu/media/android_video_decode_accelerator.cc:250: auto
gles_decoder = get_gles2_decoder_cb_.Run();
On 2016/03/01 10:25:06, kcwu wrote:
> However about moving those callbacks invoking to Initialize() for this and
other
> VDAs? Just in order to decouple the client and the implementation, so they
don't
> need to sync about what callbacks are required.

Done.

https://codereview.chromium.org/1745903002/diff/1/content/common/gpu/media/vi...
File content/common/gpu/media/video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/1745903002/diff/1/content/common/gpu/media/vi...
content/common/gpu/media/video_decode_accelerator_unittest.cc:363:
scoped_ptr<media::VideoDecodeAccelerator> CreateFakeVDA();
On 2016/03/01 10:25:06, kcwu wrote:
> Unused

Done.

https://codereview.chromium.org/1745903002/diff/1/content/public/common/gpu_v...
File content/public/common/gpu_video_decode_accelerator_factory.h (right):

https://codereview.chromium.org/1745903002/diff/1/content/public/common/gpu_v...
content/public/common/gpu_video_decode_accelerator_factory.h:8: #include
"base/memory/weak_ptr.h"
On 2016/03/01 10:25:06, kcwu wrote:
> unused

Done.

https://codereview.chromium.org/1745903002/diff/1/content/public/common/gpu_v...
content/public/common/gpu_video_decode_accelerator_factory.h:51: using
CreateVDAFp = scoped_ptr<media::VideoDecodeAccelerator> (
On 2016/03/01 10:25:06, kcwu wrote:
> How about move this type into .cc file? This type is only used in CreateVDA(),
> no need to be exposed in .h?

Done.

https://codereview.chromium.org/1745903002/diff/1/content/public/common/gpu_v...
File content/public/common/gpu_video_decode_accelerator_helpers.h (right):

https://codereview.chromium.org/1745903002/diff/1/content/public/common/gpu_v...
content/public/common/gpu_video_decode_accelerator_helpers.h:27: // process.
Which callbacks are required depends on the implementation.
On 2016/03/01 10:25:06, kcwu wrote:
> However, the client is still tightly coupled with the implementation and must
> know what callbacks are required. What if the client passed a dummy callback
but
> the implementation did call it? See my comments in
> android_video_decode_accelerator.cc for example.

As per offline discussion, moved callback checks to Initialize().

https://codereview.chromium.org/1745903002/diff/1/content/public/common/gpu_v...
content/public/common/gpu_video_decode_accelerator_helpers.h:28: struct
gpu_vda_helpers {
On 2016/03/01 10:25:06, kcwu wrote:
> Add comments that what threads can call these callbacks?
> 
> And mention these callbacks will be called more than once. (According to
> callback.h, callbacks running more than once cannot use with base::Passed). 

Great comment, thanks.

[kcwu, 2016-03-02 07:52:46]

nits

https://codereview.chromium.org/1745903002/diff/100001/content/common/gpu/med...
File content/common/gpu/media/video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/1745903002/diff/100001/content/common/gpu/med...
content/common/gpu/media/video_decode_accelerator_unittest.cc:363:
scoped_ptr<media::VideoDecodeAccelerator> CreateDXVAVDA();
This and following are not used as well.

https://codereview.chromium.org/1745903002/diff/100001/content/common/gpu/med...
File content/common/gpu/media/vt_video_decode_accelerator_mac.cc (right):

https://codereview.chromium.org/1745903002/diff/100001/content/common/gpu/med...
content/common/gpu/media/vt_video_decode_accelerator_mac.cc:313:
DCHECK(!make_context_current_cb_.is_null());
Remove this? It is checked in Initialize().

https://codereview.chromium.org/1745903002/diff/100001/content/public/common/...
File content/public/common/gpu_video_decode_accelerator_factory.h (right):

https://codereview.chromium.org/1745903002/diff/100001/content/public/common/...
content/public/common/gpu_video_decode_accelerator_factory.h:6: #define
CONTENT_PUBLIC_GPU_GPU_VIDEO_DECODE_ACCELERATOR_FACTORY_H_
CONTENT_PUBLIC_COMMON

https://codereview.chromium.org/1745903002/diff/100001/content/public/common/...
File content/public/common/gpu_video_decode_accelerator_helpers.h (right):

https://codereview.chromium.org/1745903002/diff/100001/content/public/common/...
content/public/common/gpu_video_decode_accelerator_helpers.h:5: #define
CONTENT_PUBLIC_GPU_GPU_VIDEO_DECODE_ACCELERATOR_HELPERS_H_
CONTENT_PUBLIC_COMMON

[Pawel Osciak, 2016-03-02 08:16:52]

https://codereview.chromium.org/1745903002/diff/100001/content/common/gpu/med...
File content/common/gpu/media/video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/1745903002/diff/100001/content/common/gpu/med...
content/common/gpu/media/video_decode_accelerator_unittest.cc:363:
scoped_ptr<media::VideoDecodeAccelerator> CreateDXVAVDA();
On 2016/03/02 07:52:45, kcwu wrote:
> This and following are not used as well.

Done.

https://codereview.chromium.org/1745903002/diff/100001/content/common/gpu/med...
File content/common/gpu/media/vt_video_decode_accelerator_mac.cc (right):

https://codereview.chromium.org/1745903002/diff/100001/content/common/gpu/med...
content/common/gpu/media/vt_video_decode_accelerator_mac.cc:313:
DCHECK(!make_context_current_cb_.is_null());
On 2016/03/02 07:52:45, kcwu wrote:
> Remove this? It is checked in Initialize().

Done.

https://codereview.chromium.org/1745903002/diff/100001/content/public/common/...
File content/public/common/gpu_video_decode_accelerator_factory.h (right):

https://codereview.chromium.org/1745903002/diff/100001/content/public/common/...
content/public/common/gpu_video_decode_accelerator_factory.h:6: #define
CONTENT_PUBLIC_GPU_GPU_VIDEO_DECODE_ACCELERATOR_FACTORY_H_
On 2016/03/02 07:52:45, kcwu wrote:
> CONTENT_PUBLIC_COMMON

Done.

https://codereview.chromium.org/1745903002/diff/100001/content/public/common/...
File content/public/common/gpu_video_decode_accelerator_helpers.h (right):

https://codereview.chromium.org/1745903002/diff/100001/content/public/common/...
content/public/common/gpu_video_decode_accelerator_helpers.h:5: #define
CONTENT_PUBLIC_GPU_GPU_VIDEO_DECODE_ACCELERATOR_HELPERS_H_
On 2016/03/02 07:52:45, kcwu wrote:
> CONTENT_PUBLIC_COMMON

Done.

[Pawel Osciak, 2016-03-02 08:48:59]

posciak@chromium.org changed reviewers:
+ ananta@chromium.org, jam@chromium.org, liberato@chromium.org,
sandersd@chromium.org

[Pawel Osciak, 2016-03-02 08:49:00]

+liberato@ for AVDA please
+ananta@ for DXVDA please
+sandersd@ for VTVDA and media/ please

+jam@ for content/public please

[kcwu, 2016-03-02 09:00:12]

https://codereview.chromium.org/1745903002/diff/120001/content/common/gpu/med...
File content/common/gpu/media/fake_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1745903002/diff/120001/content/common/gpu/med...
content/common/gpu/media/fake_video_decode_accelerator.cc:165: return false;
FakeVideoDecodeAccelerator::CanDecodeOnIOThread() was true. Is it intended to be
changed to false?

https://codereview.chromium.org/1745903002/diff/120001/content/common/gpu/med...
File content/common/gpu/media/video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/1745903002/diff/120001/content/common/gpu/med...
content/common/gpu/media/video_decode_accelerator_unittest.cc:520: frame_size_,
base::Bind(&DoNothingReturnTrue)));
decoder_->Initialize()

https://codereview.chromium.org/1745903002/diff/120001/media/video/video_deco...
File media/video/video_decode_accelerator.h (right):

https://codereview.chromium.org/1745903002/diff/120001/media/video/video_deco...
media/video/video_decode_accelerator.h:260: virtual bool
TryInitializeDecodeOnSeparateThread(
When to call this method? From the your implementation, looks like this method
could be either called before or after Initialize(). Would you like to document
this?

Furthermore, "TryInitialize", the name seems suggest this method can suppress
Initialize(), which is false. Do you feel this is an issue?

[Pawel Osciak, 2016-03-02 10:16:50]

https://chromiumcodereview.appspot.com/1745903002/diff/120001/content/common/...
File content/common/gpu/media/fake_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/1745903002/diff/120001/content/common/...
content/common/gpu/media/fake_video_decode_accelerator.cc:165: return false;
On 2016/03/02 09:00:12, kcwu wrote:
> FakeVideoDecodeAccelerator::CanDecodeOnIOThread() was true. Is it intended to
be
> changed to false?

Yes. This class wasn't accepting an additional client and task runner, even
though it was reporting true.

https://chromiumcodereview.appspot.com/1745903002/diff/120001/content/common/...
File content/common/gpu/media/video_decode_accelerator_unittest.cc (right):

https://chromiumcodereview.appspot.com/1745903002/diff/120001/content/common/...
content/common/gpu/media/video_decode_accelerator_unittest.cc:520: frame_size_,
base::Bind(&DoNothingReturnTrue)));
On 2016/03/02 09:00:12, kcwu wrote:
> decoder_->Initialize()

Done.

https://chromiumcodereview.appspot.com/1745903002/diff/120001/media/video/vid...
File media/video/video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/1745903002/diff/120001/media/video/vid...
media/video/video_decode_accelerator.h:260: virtual bool
TryInitializeDecodeOnSeparateThread(
On 2016/03/02 09:00:12, kcwu wrote:
> When to call this method? From the your implementation, looks like this method
> could be either called before or after Initialize(). Would you like to
document
> this?
> 
> Furthermore, "TryInitialize", the name seems suggest this method can suppress
> Initialize(), which is false. Do you feel this is an issue?

Done.

[kcwu, 2016-03-02 10:23:03]

lgtm

[sandersd (OOO until July 31), 2016-03-02 18:33:54]

VTVDA LGTM (I'll check the rest too, but you don't have to wait for me on that.)

[liberato (no reviews please), 2016-03-02 18:42:46]

avda lgtm.  nice.

[jam, 2016-03-02 19:16:48]

the cl description says

"- Make GVDAFactory a content/public interface, to provide the ability to
instantiate VDAs from outside content/.
"

which just explains what the code does. as a reviewer without much background, I
can see that already. but i don't know what's motivating this. i'm surprised
this work is being done with no background bug as well, since it seems like a
multi-patch effort.

please fill me in

[Fady Samuel, 2016-03-02 19:21:27]

On 2016/03/02 19:16:48, jam wrote:
> the cl description says
> 
> "- Make GVDAFactory a content/public interface, to provide the ability to
> instantiate VDAs from outside content/.
> "
> 
> which just explains what the code does. as a reviewer without much background,
I
> can see that already. but i don't know what's motivating this. i'm surprised
> this work is being done with no background bug as well, since it seems like a
> multi-patch effort.
> 
> please fill me in

Also, I'm working on componentizing gpu and media, so putting this in
content/public may not be necessary soon. I'm not saying we should block this
CL, but I'd like to sync up on staging. Thanks!

[Fady Samuel, 2016-03-04 01:36:54]

On 2016/03/02 19:21:27, Fady Samuel wrote:
> On 2016/03/02 19:16:48, jam wrote:
> > the cl description says
> > 
> > "- Make GVDAFactory a content/public interface, to provide the ability to
> > instantiate VDAs from outside content/.
> > "
> > 
> > which just explains what the code does. as a reviewer without much
background,
> I
> > can see that already. but i don't know what's motivating this. i'm surprised
> > this work is being done with no background bug as well, since it seems like
a
> > multi-patch effort.
> > 
> > please fill me in
> 
> Also, I'm working on componentizing gpu and media, so putting this in
> content/public may not be necessary soon. I'm not saying we should block this
> CL, but I'd like to sync up on staging. Thanks!

Non-OWNER lgtm. I don't believe our refactors will conflict in the short term.
Long term this content API will not be necessary.

[Pawel Osciak, 2016-03-04 08:20:26]

posciak@chromium.org changed reviewers:
+ jochen@chromium.org
- jam@chromium.org

[Pawel Osciak, 2016-03-04 08:21:12]

-jam@ +jochen@ for content/public please

[jochen (gone - plz use gerrit), 2016-03-04 12:59:55]

jochen@chromium.org changed reviewers:
+ fsamuel@chromium.org

[jochen (gone - plz use gerrit), 2016-03-04 12:59:57]

https://codereview.chromium.org/1745903002/diff/140001/content/public/common/...
File content/public/common/gpu_video_decode_accelerator_helpers.h (right):

https://codereview.chromium.org/1745903002/diff/140001/content/public/common/...
content/public/common/gpu_video_decode_accelerator_helpers.h:34: struct
gpu_vda_helpers {
why is that not just a namespace?

https://codereview.chromium.org/1745903002/diff/140001/content/public/common/...
content/public/common/gpu_video_decode_accelerator_helpers.h:36: using
GetGLContextCb = base::Callback<gfx::GLContext*(void)>;
Callback instead of Cb please

[Pawel Osciak, 2016-03-07 02:02:02]

Thanks. Comments addressed. PTAL.

https://chromiumcodereview.appspot.com/1745903002/diff/140001/content/public/...
File content/public/common/gpu_video_decode_accelerator_helpers.h (right):

https://chromiumcodereview.appspot.com/1745903002/diff/140001/content/public/...
content/public/common/gpu_video_decode_accelerator_helpers.h:34: struct
gpu_vda_helpers {
On 2016/03/04 12:59:56, jochen wrote:
> why is that not just a namespace?

Done.

https://chromiumcodereview.appspot.com/1745903002/diff/140001/content/public/...
content/public/common/gpu_video_decode_accelerator_helpers.h:36: using
GetGLContextCb = base::Callback<gfx::GLContext*(void)>;
On 2016/03/04 12:59:56, jochen wrote:
> Callback instead of Cb please

Done.

[jochen (gone - plz use gerrit), 2016-03-07 12:02:57]

lgtm

[Pawel Osciak, 2016-03-09 07:42:03]

The CQ bit was checked by posciak@chromium.org

[Pawel Osciak, 2016-03-09 07:42:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
sandersd@chromium.org, liberato@chromium.org, kcwu@chromium.org,
fsamuel@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1745903002/#ps160001 (title: " ")

[commit-bot: I haz the power, 2016-03-09 07:42:18]

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

[commit-bot: I haz the power, 2016-03-09 07:44:39]

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

[commit-bot: I haz the power, 2016-03-09 07:44:40]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Pawel Osciak, 2016-03-11 06:53:08]

Patchset #10 (id:220001) has been deleted

[jam, 2016-03-14 15:58:28]

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

[jam, 2016-03-14 15:58:29]

not lgtm yet

my comment about motivation and tracking bug is still not addressed

https://codereview.chromium.org/1745903002/diff/260001/content/public/common/...
File content/public/common/gpu_video_decode_accelerator_factory.h (right):

https://codereview.chromium.org/1745903002/diff/260001/content/public/common/...
content/public/common/gpu_video_decode_accelerator_factory.h:18: //
configuration. To be used in GPU process only.
then this should be in content/public/gpu

https://codereview.chromium.org/1745903002/diff/260001/content/public/common/...
content/public/common/gpu_video_decode_accelerator_factory.h:45:
GpuVideoDecodeAcceleratorFactory(
this section in private is against the content api guidelines:
https://www.chromium.org/developers/content-module/content-api

[jam, 2016-03-15 18:25:38]

ok after looking at this some more. i think the helper file isn't needed.

my justification is that the implementation of the vda stuff needs to remain in
content/common as it's used in both browser (for android) and gpu processes (for
rest). however the new public api is only used in the gpu process for chromeos.
it would be confusing to put this in content/public/common, as we don't want
embedders calling it in non-gpu processes.

working backwards, since the factory header should be in content/public/gpu,
then it can't include a typedef file that is also included by content/common
(since common can't include gpu). a lot of the changes in this cl is changing
classes to use these typedefs. let's undo that, and it's ok if the public file
in content/public/gpu has these callbacks written again. then the implementation
of that header could be in content/gpu/.

https://codereview.chromium.org/1745903002/diff/260001/content/public/common/...
File content/public/common/gpu_video_decode_accelerator_helpers.h (right):

https://codereview.chromium.org/1745903002/diff/260001/content/public/common/...
content/public/common/gpu_video_decode_accelerator_helpers.h:34: namespace
gpu_vda {
remove gpu_vda namespace. all code in content is supposed to be just in
"content" namespace only.

https://codereview.chromium.org/1745903002/diff/260001/content/public/common/...
content/public/common/gpu_video_decode_accelerator_helpers.h:35: // Return
current GLContext.
this and the other comments below are confusing. i'd expect these comments on
the methods that take them as a parameter. in isolation seeing the callback,
they don't make much sense.

usually we put these callback typedefs right above the method that takes them as
a parameter. in this case, it would be in gpu_video_decode_accelerator_factory.h

[Pawel Osciak, 2016-03-16 05:12:47]

Description was changed from

==========
Introduce GpuVideoDecodeAcceleratorFactory.

- Move platform-specific code from GpuVideoDecodeAccelerator to
GpuVideoDecodeAcceleratorFactory.

- Make GVDAFactory a content/public interface, to provide the ability to
instantiate VDAs from outside content/.

- Unify how we obtain access to various GL functionality/classes from VDAs
by introducing a set of callbacks provided by the client.

- Replace VDA::CanDecodeOnIOThread() with
VDA::TryInitializeDecodeOnSeparateThread(). This allows us to remove
additional client/taskrunner arguments from VDA constructors, and give client
the option to use a separate thread to decode, instead of having to make this
decision in the factory, and enforcing these arguments in the constructors.

- Deduplicate VDA creation code across users (currently GVDA and vdaunittest).

BUG=None
TEST=compile/run various VDA impls
==========

to

==========
Introduce GpuVideoDecodeAcceleratorFactory.

- Move platform-specific code from GpuVideoDecodeAccelerator to
GpuVideoDecodeAcceleratorFactory.

- Make GVDAFactory a content/public interface, to provide the ability to
instantiate VDAs from outside content/.

- Unify how we obtain access to various GL functionality/classes from VDAs
by introducing a set of callbacks provided by the client.

- Replace VDA::CanDecodeOnIOThread() with
VDA::TryInitializeDecodeOnSeparateThread(). This allows us to remove
additional client/taskrunner arguments from VDA constructors, and give client
the option to use a separate thread to decode, instead of having to make this
decision in the factory, and enforcing these arguments in the constructors.

- Deduplicate VDA creation code across users (currently GVDA and vdaunittest).

BUG=b/27687678
TEST=compile/run various VDA impls
==========

[Pawel Osciak, 2016-03-16 05:56:07]

Patchset #12 (id:280001) has been deleted

[Pawel Osciak, 2016-03-16 06:03:30]

PTAL, removed gpu_vda namespace, moved factory header to content/public/gpu and
helpers header to content/common/gpu/media.

[kcwu, 2016-03-16 06:26:37]

nits

https://chromiumcodereview.appspot.com/1745903002/diff/300001/content/common/...
File content/common/gpu/media/gpu_video_decode_accelerator_helpers.h (right):

https://chromiumcodereview.appspot.com/1745903002/diff/300001/content/common/...
content/common/gpu/media/gpu_video_decode_accelerator_helpers.h:4: #ifndef
CONTENT_COMMON_GPU_MEDIA_GPU_VIDEO_DECODE_ACCELERATOR_HELPERS_H_
add blank line before #ifndef

https://chromiumcodereview.appspot.com/1745903002/diff/300001/content/public/...
File content/public/gpu/gpu_video_decode_accelerator_factory.h (right):

https://chromiumcodereview.appspot.com/1745903002/diff/300001/content/public/...
content/public/gpu/gpu_video_decode_accelerator_factory.h:6: #define
CONTENT_PUBLIC_COMMON_GPU_VIDEO_DECODE_ACCELERATOR_FACTORY_H_
s/COMMON/GPU/

https://chromiumcodereview.appspot.com/1745903002/diff/300001/content/public/...
content/public/gpu/gpu_video_decode_accelerator_factory.h:104: #endif  //
CONTENT_PUBLIC_COMMON_GPU_VIDEO_DECODE_ACCELERATOR_FACTORY_H_
s/COMMON/GPU/

[Pawel Osciak, 2016-03-16 09:50:10]

Now passing GpuPreferences to the factory after rebase. PTAL.

https://chromiumcodereview.appspot.com/1745903002/diff/300001/content/common/...
File content/common/gpu/media/gpu_video_decode_accelerator_helpers.h (right):

https://chromiumcodereview.appspot.com/1745903002/diff/300001/content/common/...
content/common/gpu/media/gpu_video_decode_accelerator_helpers.h:4: #ifndef
CONTENT_COMMON_GPU_MEDIA_GPU_VIDEO_DECODE_ACCELERATOR_HELPERS_H_
On 2016/03/16 06:26:37, kcwu wrote:
> add blank line before #ifndef

Done.

https://chromiumcodereview.appspot.com/1745903002/diff/300001/content/public/...
File content/public/gpu/gpu_video_decode_accelerator_factory.h (right):

https://chromiumcodereview.appspot.com/1745903002/diff/300001/content/public/...
content/public/gpu/gpu_video_decode_accelerator_factory.h:6: #define
CONTENT_PUBLIC_COMMON_GPU_VIDEO_DECODE_ACCELERATOR_FACTORY_H_
On 2016/03/16 06:26:37, kcwu wrote:
> s/COMMON/GPU/

Done.

[Pawel Osciak, 2016-03-16 10:01:33]

Patchset #13 (id:320001) has been deleted

[jam, 2016-03-16 20:56:43]

https://codereview.chromium.org/1745903002/diff/260001/content/public/common/...
File content/public/common/gpu_video_decode_accelerator_factory.h (right):

https://codereview.chromium.org/1745903002/diff/260001/content/public/common/...
content/public/common/gpu_video_decode_accelerator_factory.h:45:
GpuVideoDecodeAcceleratorFactory(
On 2016/03/14 15:58:29, jam wrote:
> this section in private is against the content api guidelines:
> https://www.chromium.org/developers/content-module/content-api

this comment is still not addressed.

why is there a private section instead of putting these in an annonymous
namespace in the implementation (for the methods) and using lazy instance for
these members?

https://codereview.chromium.org/1745903002/diff/340001/content/common/gpu/med...
File content/common/gpu/media/gpu_video_decode_accelerator_factory.cc (right):

https://codereview.chromium.org/1745903002/diff/340001/content/common/gpu/med...
content/common/gpu/media/gpu_video_decode_accelerator_factory.cc:1: // Copyright
2016 The Chromium Authors. All rights reserved.
this file should be in content/gpu to match the location of the public header

https://codereview.chromium.org/1745903002/diff/340001/content/public/gpu/DEPS
File content/public/gpu/DEPS (right):

https://codereview.chromium.org/1745903002/diff/340001/content/public/gpu/DEP...
content/public/gpu/DEPS:1: specific_include_rules = {
you could also just make this +media

[Pawel Osciak, 2016-03-17 11:15:16]

Thanks for comments. PTAL.

https://chromiumcodereview.appspot.com/1745903002/diff/260001/content/public/...
File content/public/common/gpu_video_decode_accelerator_factory.h (right):

https://chromiumcodereview.appspot.com/1745903002/diff/260001/content/public/...
content/public/common/gpu_video_decode_accelerator_factory.h:45:
GpuVideoDecodeAcceleratorFactory(
On 2016/03/16 20:56:43, jam wrote:
> On 2016/03/14 15:58:29, jam wrote:
> > this section in private is against the content api guidelines:
> > https://www.chromium.org/developers/content-module/content-api
> 
> this comment is still not addressed.
> 
> why is there a private section instead of putting these in an annonymous
> namespace in the implementation (for the methods) and using lazy instance for
> these members?

Sorry somehow missed this. Created a GpuVideoDecodeAcceleratorFactoryImpl, would
that work?

https://chromiumcodereview.appspot.com/1745903002/diff/340001/content/common/...
File content/common/gpu/media/gpu_video_decode_accelerator_factory.cc (right):

https://chromiumcodereview.appspot.com/1745903002/diff/340001/content/common/...
content/common/gpu/media/gpu_video_decode_accelerator_factory.cc:1: // Copyright
2016 The Chromium Authors. All rights reserved.
On 2016/03/16 20:56:43, jam wrote:
> this file should be in content/gpu to match the location of the public header

content/gpu is very generic, perhaps the public header should be moved instead?
For example create content/public/gpu/media ? Could we keep this file here in
that case?

https://chromiumcodereview.appspot.com/1745903002/diff/340001/content/public/...
File content/public/gpu/DEPS (right):

https://chromiumcodereview.appspot.com/1745903002/diff/340001/content/public/...
content/public/gpu/DEPS:1: specific_include_rules = {
On 2016/03/16 20:56:43, jam wrote:
> you could also just make this +media

Do you mean leave everything as is, but
s/"+media\/video\/video_decode_accelerator.h"/"+media"/ ?

[jam, 2016-03-17 15:43:26]

https://chromiumcodereview.appspot.com/1745903002/diff/260001/content/public/...
File content/public/common/gpu_video_decode_accelerator_factory.h (right):

https://chromiumcodereview.appspot.com/1745903002/diff/260001/content/public/...
content/public/common/gpu_video_decode_accelerator_factory.h:45:
GpuVideoDecodeAcceleratorFactory(
On 2016/03/17 11:15:16, Pawel Osciak wrote:
> On 2016/03/16 20:56:43, jam wrote:
> > On 2016/03/14 15:58:29, jam wrote:
> > > this section in private is against the content api guidelines:
> > > https://www.chromium.org/developers/content-module/content-api
> > 
> > this comment is still not addressed.
> > 
> > why is there a private section instead of putting these in an annonymous
> > namespace in the implementation (for the methods) and using lazy instance
for
> > these members?
> 
> Sorry somehow missed this. Created a GpuVideoDecodeAcceleratorFactoryImpl,
would
> that work?

I'm not sure I understand the point of having both
GpuVideoDecodeAcceleratorFactory and GpuVideoDecodeAcceleratorFactoryImpl that
each contain static methods? i.e. the impl file just duplicates the 4 static
methods and adds private members that are only used in impl.cc. those static
members could instead be in the cc file containing the implementation of the
public static methods.

normally we have "impl" files because we have a Foo interface and FooImpl
implementation hidden behind content. in this case with static methods, the
usual pattern for that is to declare them in content/public/bar/foo.h. and then
implement them in content/bar/foo.cc.

https://chromiumcodereview.appspot.com/1745903002/diff/340001/content/common/...
File content/common/gpu/media/gpu_video_decode_accelerator_factory.cc (right):

https://chromiumcodereview.appspot.com/1745903002/diff/340001/content/common/...
content/common/gpu/media/gpu_video_decode_accelerator_factory.cc:1: // Copyright
2016 The Chromium Authors. All rights reserved.
On 2016/03/17 11:15:16, Pawel Osciak wrote:
> On 2016/03/16 20:56:43, jam wrote:
> > this file should be in content/gpu to match the location of the public
header
> 
> content/gpu is very generic, perhaps the public header should be moved
instead?
> For example create content/public/gpu/media ? Could we keep this file here in
> that case?

not sure what "generic" has to do with it?

in general for content/public/foo, we avoid subdirectories.

please just put this in content/gpu/gpu_video_decode_accelerator_factory.cc

https://chromiumcodereview.appspot.com/1745903002/diff/340001/content/public/...
File content/public/gpu/DEPS (right):

https://chromiumcodereview.appspot.com/1745903002/diff/340001/content/public/...
content/public/gpu/DEPS:1: specific_include_rules = {
On 2016/03/17 11:15:16, Pawel Osciak wrote:
> On 2016/03/16 20:56:43, jam wrote:
> > you could also just make this +media
> 
> Do you mean leave everything as is, but
> s/"+media\/video\/video_decode_accelerator.h"/"+media"/ ?

correct

[jam, 2016-03-18 01:18:56]

lgtm

https://chromiumcodereview.appspot.com/1745903002/diff/260001/content/public/...
File content/public/common/gpu_video_decode_accelerator_factory.h (right):

https://chromiumcodereview.appspot.com/1745903002/diff/260001/content/public/...
content/public/common/gpu_video_decode_accelerator_factory.h:45:
GpuVideoDecodeAcceleratorFactory(
On 2016/03/17 15:43:26, jam wrote:
> On 2016/03/17 11:15:16, Pawel Osciak wrote:
> > On 2016/03/16 20:56:43, jam wrote:
> > > On 2016/03/14 15:58:29, jam wrote:
> > > > this section in private is against the content api guidelines:
> > > > https://www.chromium.org/developers/content-module/content-api
> > > 
> > > this comment is still not addressed.
> > > 
> > > why is there a private section instead of putting these in an annonymous
> > > namespace in the implementation (for the methods) and using lazy instance
> for
> > > these members?
> > 
> > Sorry somehow missed this. Created a GpuVideoDecodeAcceleratorFactoryImpl,
> would
> > that work?
> 
> I'm not sure I understand the point of having both
> GpuVideoDecodeAcceleratorFactory and GpuVideoDecodeAcceleratorFactoryImpl that
> each contain static methods? i.e. the impl file just duplicates the 4 static
> methods and adds private members that are only used in impl.cc. those static
> members could instead be in the cc file containing the implementation of the
> public static methods.
> 
> normally we have "impl" files because we have a Foo interface and FooImpl
> implementation hidden behind content. in this case with static methods, the
> usual pattern for that is to declare them in content/public/bar/foo.h. and
then
> implement them in content/bar/foo.cc.

ignore this comment, and thanks for the explanation over IM
I totally missed the non-static method, sorry

https://chromiumcodereview.appspot.com/1745903002/diff/340001/content/common/...
File content/common/gpu/media/gpu_video_decode_accelerator_factory.cc (right):

https://chromiumcodereview.appspot.com/1745903002/diff/340001/content/common/...
content/common/gpu/media/gpu_video_decode_accelerator_factory.cc:1: // Copyright
2016 The Chromium Authors. All rights reserved.
On 2016/03/17 15:43:26, jam wrote:
> On 2016/03/17 11:15:16, Pawel Osciak wrote:
> > On 2016/03/16 20:56:43, jam wrote:
> > > this file should be in content/gpu to match the location of the public
> header
> > 
> > content/gpu is very generic, perhaps the public header should be moved
> instead?
> > For example create content/public/gpu/media ? Could we keep this file here
in
> > that case?
> 
> not sure what "generic" has to do with it?
> 
> in general for content/public/foo, we avoid subdirectories.
> 
> please just put this in content/gpu/gpu_video_decode_accelerator_factory.cc

ditto, ignore this comment now. the current files in patchset 14 is fine

[Pawel Osciak, 2016-03-18 05:02:02]

Patchset #17 (id:420001) has been deleted

[jam, 2016-03-21 14:40:00]

https://codereview.chromium.org/1745903002/diff/440001/content/DEPS
File content/DEPS (right):

https://codereview.chromium.org/1745903002/diff/440001/content/DEPS#newcode12
content/DEPS:12: "+content/public/gpu",
sorry just saw this. don't add it.

code in any directory in content can include content/public/common and
content/common as that is code which can be run in any process. however only gpu
process can include content/gpu or content/public/gpu

i believe this is leftover code from before

https://codereview.chromium.org/1745903002/diff/440001/content/public/gpu/DEPS
File content/public/gpu/DEPS (right):

https://codereview.chromium.org/1745903002/diff/440001/content/public/gpu/DEP...
content/public/gpu/DEPS:3: "+content/common",
i don't think this is needed, it should already apply to all directories in
content (from content/DEPS)

[Pawel Osciak, 2016-03-23 07:12:42]

https://chromiumcodereview.appspot.com/1745903002/diff/440001/content/DEPS
File content/DEPS (right):

https://chromiumcodereview.appspot.com/1745903002/diff/440001/content/DEPS#ne...
content/DEPS:12: "+content/public/gpu",
On 2016/03/21 14:39:59, jam wrote:
> sorry just saw this. don't add it.
> 
> code in any directory in content can include content/public/common and
> content/common as that is code which can be run in any process. however only
gpu
> process can include content/gpu or content/public/gpu
> 
> i believe this is leftover code from before

It's needed, but for now as discussed I'm decoupling the Impl in content/common
from the interface in content/public and submitting crbug.com/597150. We should
have VDAs moved out of content/common soon, I will make Impl derive from the
public interface again then.

https://chromiumcodereview.appspot.com/1745903002/diff/440001/content/public/...
File content/public/gpu/DEPS (right):

https://chromiumcodereview.appspot.com/1745903002/diff/440001/content/public/...
content/public/gpu/DEPS:3: "+content/common",
On 2016/03/21 14:39:59, jam wrote:
> i don't think this is needed, it should already apply to all directories in
> content (from content/DEPS)

This is needed to be able to include
content/common/gpu/media/gpu_video_decode_accelerator_factory_impl.h from
content/public/gpu/gpu_video_decode_accelerator_factory.h.

[Pawel Osciak, 2016-03-23 09:43:34]

Patchset #19 (id:480001) has been deleted

[jam, 2016-03-24 00:21:50]

still lgtm

[Pawel Osciak, 2016-03-24 05:19:43]

Description was changed from

==========
Introduce GpuVideoDecodeAcceleratorFactory.

- Move platform-specific code from GpuVideoDecodeAccelerator to
GpuVideoDecodeAcceleratorFactory.

- Make GVDAFactory a content/public interface, to provide the ability to
instantiate VDAs from outside content/.

- Unify how we obtain access to various GL functionality/classes from VDAs
by introducing a set of callbacks provided by the client.

- Replace VDA::CanDecodeOnIOThread() with
VDA::TryInitializeDecodeOnSeparateThread(). This allows us to remove
additional client/taskrunner arguments from VDA constructors, and give client
the option to use a separate thread to decode, instead of having to make this
decision in the factory, and enforcing these arguments in the constructors.

- Deduplicate VDA creation code across users (currently GVDA and vdaunittest).

BUG=b/27687678
TEST=compile/run various VDA impls
==========

to

==========
Introduce GpuVideoDecodeAcceleratorFactory.

- Move platform-specific code from GpuVideoDecodeAccelerator to
GpuVideoDecodeAcceleratorFactory.

- Make GVDAFactory a content/public interface, to provide the ability to
instantiate VDAs from outside content/.

- Unify how we obtain access to various GL functionality/classes from VDAs
by introducing a set of callbacks provided by the client.

- Replace VDA::CanDecodeOnIOThread() with
VDA::TryInitializeDecodeOnSeparateThread(). This allows us to remove
additional client/taskrunner arguments from VDA constructors, and give client
the option to use a separate thread to decode, instead of having to make this
decision in the factory, and enforcing these arguments in the constructors.

- Deduplicate VDA creation code across users (currently GVDA and vdaunittest).

BUG=b/27687678
TEST=compile/run various VDA impls
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Pawel Osciak, 2016-03-24 07:41:34]

Patchset #22 (id:560001) has been deleted

[Pawel Osciak, 2016-03-24 07:42:24]

Patchset #21 (id:540001) has been deleted

[Pawel Osciak, 2016-03-24 09:16:33]

PTAL at the additional changes in BUILD.gn and DEPS files in latest PS. Thank
you!

[jam, 2016-03-24 21:57:20]

On 2016/03/24 09:16:33, Pawel Osciak wrote:
> PTAL at the additional changes in BUILD.gn and DEPS files in latest PS. Thank
> you!

still lgtm

[Pawel Osciak, 2016-03-25 00:38:23]

The CQ bit was checked by posciak@chromium.org

[Pawel Osciak, 2016-03-25 00:38:24]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
sandersd@chromium.org, liberato@chromium.org, kcwu@chromium.org,
fsamuel@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1745903002/#ps580001 (title: "")

[commit-bot: I haz the power, 2016-03-25 00:38:54]

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

[commit-bot: I haz the power, 2016-03-25 00:46:24]

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

[commit-bot: I haz the power, 2016-03-25 00:46:26]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Pawel Osciak, 2016-03-25 01:22:16]

The CQ bit was checked by posciak@chromium.org

[Pawel Osciak, 2016-03-25 01:22:16]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
sandersd@chromium.org, jam@chromium.org, liberato@chromium.org,
kcwu@chromium.org, fsamuel@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1745903002/#ps600001 (title: " ")

[commit-bot: I haz the power, 2016-03-25 01:22:32]

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

[commit-bot: I haz the power, 2016-03-25 04:45:51]

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

[commit-bot: I haz the power, 2016-03-25 04:45:53]

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

[Pawel Osciak, 2016-03-25 04:46:53]

The CQ bit was checked by posciak@chromium.org

[commit-bot: I haz the power, 2016-03-25 04:47:03]

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

[commit-bot: I haz the power, 2016-03-25 06:02:33]

Committed patchset #22 (id:600001)

[commit-bot: I haz the power, 2016-03-25 06:04:02]

Description was changed from

==========
Introduce GpuVideoDecodeAcceleratorFactory.

- Move platform-specific code from GpuVideoDecodeAccelerator to
GpuVideoDecodeAcceleratorFactory.

- Make GVDAFactory a content/public interface, to provide the ability to
instantiate VDAs from outside content/.

- Unify how we obtain access to various GL functionality/classes from VDAs
by introducing a set of callbacks provided by the client.

- Replace VDA::CanDecodeOnIOThread() with
VDA::TryInitializeDecodeOnSeparateThread(). This allows us to remove
additional client/taskrunner arguments from VDA constructors, and give client
the option to use a separate thread to decode, instead of having to make this
decision in the factory, and enforcing these arguments in the constructors.

- Deduplicate VDA creation code across users (currently GVDA and vdaunittest).

BUG=b/27687678
TEST=compile/run various VDA impls
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Introduce GpuVideoDecodeAcceleratorFactory.

- Move platform-specific code from GpuVideoDecodeAccelerator to
GpuVideoDecodeAcceleratorFactory.

- Make GVDAFactory a content/public interface, to provide the ability to
instantiate VDAs from outside content/.

- Unify how we obtain access to various GL functionality/classes from VDAs
by introducing a set of callbacks provided by the client.

- Replace VDA::CanDecodeOnIOThread() with
VDA::TryInitializeDecodeOnSeparateThread(). This allows us to remove
additional client/taskrunner arguments from VDA constructors, and give client
the option to use a separate thread to decode, instead of having to make this
decision in the factory, and enforcing these arguments in the constructors.

- Deduplicate VDA creation code across users (currently GVDA and vdaunittest).

BUG=b/27687678
TEST=compile/run various VDA impls
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/6977e5243786901a766a38c2c291464875dffbd6
Cr-Commit-Position: refs/heads/master@{#383256}
==========

[commit-bot: I haz the power, 2016-03-25 06:04:03]

Patchset 22 (id:??) landed as
https://crrev.com/6977e5243786901a766a38c2c291464875dffbd6
Cr-Commit-Position: refs/heads/master@{#383256}

[boliu, 2016-03-25 16:36:08]

On 2016/03/25 06:04:03, commit-bot: I haz the power wrote:
> Patchset 22 (id:??) landed as
> https://crrev.com/6977e5243786901a766a38c2c291464875dffbd6
> Cr-Commit-Position: refs/heads/master@{#383256}

This CL apparently broke texture sharing on android webview, ie all rendering.
filed crbug.com/597958

[boliu, 2016-03-25 17:49:06]

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

[boliu, 2016-03-25 17:49:08]

https://codereview.chromium.org/1745903002/diff/600001/content/public/gpu/BUI...
File content/public/gpu/BUILD.gn (right):

https://codereview.chromium.org/1745903002/diff/600001/content/public/gpu/BUI...
content/public/gpu/BUILD.gn:33: "//gpu/config:config_sources",
The problem was gpu/command_buffer/service and gpu/config are being built twice
in gn component build. Once into the content shared lib, and once into the gpu
shared lib.

I think these should not have been deps on the source of the targets, but the
targets themselves? Fix here: https://codereview.chromium.org/1838493002

[brettw, 2016-03-25 17:49:37]

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

[brettw, 2016-03-25 17:49:39]

https://codereview.chromium.org/1745903002/diff/600001/gpu/config/BUILD.gn
File gpu/config/BUILD.gn (right):

https://codereview.chromium.org/1745903002/diff/600001/gpu/config/BUILD.gn#ne...
gpu/config/BUILD.gn:28: "//content/public/gpu/*",
For author and reviewers: this change was incorrect. The visibility here is to
prevent you from depending on this target, you should go through the "config"
target immediately above this or this code will get duplicated into multiple
modules in the component build.

I will add a better comment here, but adding things to visibility in other
modules should immediately set of alarm bells.

[brettw, 2016-03-25 17:52:47]

I'm going to revert, I think there are too many problems.

[brettw, 2016-03-25 17:53:57]

A revert of this CL (patchset #22 id:600001) has been created in
https://codereview.chromium.org/1832123002/ by brettw@chromium.org.

The reason for reverting is: Too many problems: Android, Win64 linking,
component linking (duplicate code die to incorrect visibility).