Reland: Introduce GpuVideoDecodeAcceleratorFactory.

Reland: Introduce GpuVideoDecodeAcceleratorFactory.

Original CL: https://codereview.chromium.org/1745903002.

This includes fixes from boliu@ for gn build.

TBR=kcwu@chromium.org,owenlin@chromium.org,sandersd@chromium.org,liberato@chromium.org,fsamuel@chromium.org,jochen@chromium.org,jam@chromium.org

Original message:

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

Committed: https://crrev.com/41c590cdfef6ce63694776f99a1b85584b485478
Cr-Commit-Position: refs/heads/master@{#385343}

[Pawel Osciak, 2016-03-30 11:19:11]

Patchset #1 (id:1) has been deleted

[Pawel Osciak, 2016-03-30 11:50:06]

posciak@chromium.org changed reviewers:
+ boliu@chromium.org, brettw@chromium.org

[Pawel Osciak, 2016-03-30 11:50:07]

boliu@, brettw@: PTAL at corrected BUILD files (based on
https://codereview.chromium.org/1838493002/).

[jam, 2016-03-30 15:55:22]

slgtm

[jam, 2016-03-30 15:55:38]

On 2016/03/30 15:55:22, jam wrote:
> slgtm

(still lgtm)

[liberato (no reviews please), 2016-03-30 16:18:01]

lgtm % one nit i missed earlier.

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

https://codereview.chromium.org/1839193003/diff/20001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.cc:1003:
strategy_->Cleanup(have_context, output_picture_buffers_);
since this is no longer initialized in the constructor, please add a null  check
or dcheck on |strategy_|.  the custom deleter on VDA will call this if it's
assigned to a refptr.

i missed this last time around, sorry.

[boliu, 2016-03-30 16:22:21]

I'll let brettw@ review the gn things. But I can confirm this doesn't break
webview rendering in component build :)

[Pawel Osciak, 2016-04-05 10:01:09]

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

https://codereview.chromium.org/1839193003/diff/20001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.cc:1003:
strategy_->Cleanup(have_context, output_picture_buffers_);
On 2016/03/30 16:18:01, liberato wrote:
> since this is no longer initialized in the constructor, please add a null 
check
> or dcheck on |strategy_|.  the custom deleter on VDA will call this if it's
> assigned to a refptr.
> 
> i missed this last time around, sorry.

Done.

[Pawel Osciak, 2016-04-06 00:14:26]

The CQ bit was checked by posciak@chromium.org

[Pawel Osciak, 2016-04-06 00:14:27]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
liberato@chromium.org
Link to the patchset: https://codereview.chromium.org/1839193003/#ps40001
(title: " ")

[commit-bot: I haz the power, 2016-04-06 00:14:57]

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

[commit-bot: I haz the power, 2016-04-06 00:21:20]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-04-06 00:23:42]

Description was changed from

==========
Reland: Introduce GpuVideoDecodeAcceleratorFactory.

Original CL: https://codereview.chromium.org/1745903002.

This includes fixes from boliu@ for gn build.

TBR=kcwu@chromium.org,owenlin@chromium.org,sandersd@chromium.org,liberato@chr...

Original message:

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

==========
Reland: Introduce GpuVideoDecodeAcceleratorFactory.

Original CL: https://codereview.chromium.org/1745903002.

This includes fixes from boliu@ for gn build.

TBR=kcwu@chromium.org,owenlin@chromium.org,sandersd@chromium.org,liberato@chr...

Original message:

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

Committed: https://crrev.com/41c590cdfef6ce63694776f99a1b85584b485478
Cr-Commit-Position: refs/heads/master@{#385343}
==========

[commit-bot: I haz the power, 2016-04-06 00:23:43]

Patchset 2 (id:??) landed as
https://crrev.com/41c590cdfef6ce63694776f99a1b85584b485478
Cr-Commit-Position: refs/heads/master@{#385343}

[hans, 2016-04-08 01:29:42]

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

[hans, 2016-04-08 01:29:43]

I'm getting link errors for video_decode_accelerator_unittest.exe in
component=shared_library builds with Clang on Windows. I expect it's due to this
change, but I haven't been able to confirm because the build seems broken on
HEAD for other reasons.

Can you investigate?

https://codereview.chromium.org/1839193003/diff/40001/content/common/gpu/medi...
File content/common/gpu/media/video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/1839193003/diff/40001/content/common/gpu/medi...
content/common/gpu/media/video_decode_accelerator_unittest.cc:53: #include
"gpu/command_buffer/service/gpu_preferences.h"
I think you need a dependency on gpu.gyp:command_buffer_service for this..

[hans, 2016-04-08 01:30:35]

Error message (from 
https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%28dbg%29/build...)

video_decode_accelerator_unittest.video_decode_accelerator_unittest.obj : error
LNK2019: unresolved external symbol "public: __thiscall
content::GpuVideoDecodeAcceleratorFactoryImpl::~GpuVideoDecodeAcceleratorFactoryImpl(void)"
(??1GpuVideoDecodeAcceleratorFactoryImpl@content@@QAE@XZ) referenced in function
"public: void __thiscall std::default_delete<class
content::GpuVideoDecodeAcceleratorFactoryImpl>::operator()(class
content::GpuVideoDecodeAcceleratorFactoryImpl *)const "
(??R?$default_delete@VGpuVideoDecodeAcceleratorFactoryImpl@content@@@std@@QBEXPAVGpuVideoDecodeAcceleratorFactoryImpl@content@@@Z)

video_decode_accelerator_unittest.video_decode_accelerator_unittest.obj : error
LNK2019: unresolved external symbol "public: static class std::unique_ptr<class
content::GpuVideoDecodeAcceleratorFactoryImpl,struct std::default_delete<class
content::GpuVideoDecodeAcceleratorFactoryImpl> > __cdecl
content::GpuVideoDecodeAcceleratorFactoryImpl::Create(class base::Callback<class
gfx::GLContext * __cdecl(void),1> const &,class base::Callback<bool
__cdecl(void),1> const &,class base::Callback<bool __cdecl(unsigned int,unsigned
int,class scoped_refptr<class gl::GLImage> const &,bool),1> const &)"
(?Create@GpuVideoDecodeAcceleratorFactoryImpl@content@@SA?AV?$unique_ptr@VGpuVideoDecodeAcceleratorFactoryImpl@content@@U?$default_delete@VGpuVideoDecodeAcceleratorFactoryImpl@content@@@std@@@std@@ABV?$Callback@$$A6APAVGLContext@gfx@@XZ$00@base@@ABV?$Callback@$$A6A_NXZ$00@6@ABV?$Callback@$$A6A_NIIABV?$scoped_refptr@VGLImage@gl@@@@_N@Z$00@6@@Z)
referenced in function "public: void __thiscall content::`anonymous
namespace'::GLRenderingVDAClient::CreateAndStartDecoder(void)"
(?CreateAndStartDecoder@GLRenderingVDAClient@?A@content@@QAEXXZ)

video_decode_accelerator_unittest.video_decode_accelerator_unittest.obj : error
LNK2019: unresolved external symbol "__declspec(dllimport) public: __thiscall
gpu::GpuPreferences::GpuPreferences(void)" (__imp_??0GpuPreferences@gpu@@QAE@XZ)
referenced in function "public: void __thiscall content::`anonymous
namespace'::GLRenderingVDAClient::CreateAndStartDecoder(void)"
(?CreateAndStartDecoder@GLRenderingVDAClient@?A@content@@QAEXXZ)

video_decode_accelerator_unittest.video_decode_accelerator_unittest.obj : error
LNK2019: unresolved external symbol "public: class std::unique_ptr<class
media::VideoDecodeAccelerator,struct std::default_delete<class
media::VideoDecodeAccelerator> > __thiscall
content::GpuVideoDecodeAcceleratorFactoryImpl::CreateVDA(class
media::VideoDecodeAccelerator::Client *,struct
media::VideoDecodeAccelerator::Config const &,struct gpu::GpuPreferences const
&)"
(?CreateVDA@GpuVideoDecodeAcceleratorFactoryImpl@content@@QAE?AV?$unique_ptr@VVideoDecodeAccelerator@media@@U?$default_delete@VVideoDecodeAccelerator@media@@@std@@@std@@PAVClient@VideoDecodeAccelerator@media@@ABUConfig@67@ABUGpuPreferences@gpu@@@Z)
referenced in function "public: void __thiscall content::`anonymous
namespace'::GLRenderingVDAClient::CreateAndStartDecoder(void)"
(?CreateAndStartDecoder@GLRenderingVDAClient@?A@content@@QAEXXZ)

video_decode_accelerator_unittest.video_decode_accelerator_unittest.obj : error
LNK2019: unresolved external symbol "__declspec(dllimport) public: __thiscall
gpu::GpuPreferences::~GpuPreferences(void)"
(__imp_??1GpuPreferences@gpu@@QAE@XZ) referenced in function "public: void
__thiscall content::`anonymous
namespace'::GLRenderingVDAClient::CreateAndStartDecoder(void)"
(?CreateAndStartDecoder@GLRenderingVDAClient@?A@content@@QAEXXZ)

video_decode_accelerator_unittest.exe : fatal error LNK1120: 5 unresolved
externals

[Pawel Osciak, 2016-04-08 01:34:11]

On 2016/04/08 01:29:43, hans wrote:
> I'm getting link errors for video_decode_accelerator_unittest.exe in
> component=shared_library builds with Clang on Windows. I expect it's due to
this
> change, but I haven't been able to confirm because the build seems broken on
> HEAD for other reasons.
> 
> Can you investigate?
> 
>
https://codereview.chromium.org/1839193003/diff/40001/content/common/gpu/medi...
> File content/common/gpu/media/video_decode_accelerator_unittest.cc (right):
> 
>
https://codereview.chromium.org/1839193003/diff/40001/content/common/gpu/medi...
> content/common/gpu/media/video_decode_accelerator_unittest.cc:53: #include
> "gpu/command_buffer/service/gpu_preferences.h"
> I think you need a dependency on gpu.gyp:command_buffer_service for this..


Thanks! Unfortunately I don't have a Windows build environment. Would you
perhaps be able to submit a CL with a fix for this please? Thanks!

[hans, 2016-04-08 04:42:45]

On 2016/04/08 01:34:11, Pawel Osciak wrote:
> On 2016/04/08 01:29:43, hans wrote:
> > I'm getting link errors for video_decode_accelerator_unittest.exe in
> > component=shared_library builds with Clang on Windows. I expect it's due to
> this
> > change, but I haven't been able to confirm because the build seems broken on
> > HEAD for other reasons.
> > 
> > Can you investigate?
> > 
> >
>
https://codereview.chromium.org/1839193003/diff/40001/content/common/gpu/medi...
> > File content/common/gpu/media/video_decode_accelerator_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/1839193003/diff/40001/content/common/gpu/medi...
> > content/common/gpu/media/video_decode_accelerator_unittest.cc:53: #include
> > "gpu/command_buffer/service/gpu_preferences.h"
> > I think you need a dependency on gpu.gyp:command_buffer_service for this..
> 
> 
> Thanks! Unfortunately I don't have a Windows build environment. Would you
> perhaps be able to submit a CL with a fix for this please? Thanks!

That one only fixes the errors referring to GpuPreferences.

The others are trickier.

Like the error about GpuVideoDecodeAcceleratorFactoryImpl destructor. Your test
is using this, but not depending on any target that provides it. I tried adding
that source file as a dependency, but then got more link errors, etc.

I've confirmed that the MSVC Windows build is also broken by this, so it's not a
Clang problem.

[hans, 2016-04-08 05:09:08]

I've tried to revert this, but there are a ton of patches landed on top making
it hard.

[Pawel Osciak, 2016-04-08 07:41:01]

On 2016/04/08 05:09:08, hans wrote:
> I've tried to revert this, but there are a ton of patches landed on top making
> it hard.


Given that revert is not straightforward I think it would be great to try to
just fix the dependency with a small followup CL. I believe
https://chromiumcodereview.appspot.com/1859403002/ should remove the dependency
altogether.

[hans, 2016-04-08 16:54:07]

On 2016/04/08 07:41:01, Pawel Osciak wrote:
> On 2016/04/08 05:09:08, hans wrote:
> > I've tried to revert this, but there are a ton of patches landed on top
making
> > it hard.
> 
> 
> Given that revert is not straightforward I think it would be great to try to
> just fix the dependency with a small followup CL. I believe
> https://chromiumcodereview.appspot.com/1859403002/ should remove the
dependency
> altogether.

It seems https://codereview.chromium.org/1845563005 fixed the build. Let's hope
it sticks.