Create VTVideoDecodeAccelerator

Create VTVideoDecodeAccelerator.

This is a stub for what will eventually become a video decode accelerator for Mac.

BUG=133828
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276290

[scherkus (not reviewing), 2014-06-09 16:02:07]

mostly nits 'n q's

https://codereview.chromium.org/322433004/diff/40001/chrome/browser/about_fla...
File chrome/browser/about_flags.cc (left):

https://codereview.chromium.org/322433004/diff/40001/chrome/browser/about_fla...
chrome/browser/about_flags.cc:430: #if defined(OS_CHROMEOS)
bad merge? looks like you have a bunch of related changes

https://codereview.chromium.org/322433004/diff/40001/chrome/browser/about_fla...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/322433004/diff/40001/chrome/browser/about_fla...
chrome/browser/about_flags.cc:1031: kOsMac | kOsWin | kOsCrOS,
is this change needed or does --ignore-gpu-blacklist suffice for now

I believe the blacklist is the main mechanism that disables GPU decode on mac
today:
https://code.google.com/p/chromium/codesearch#chromium/src/gpu/config/softwar...
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/gp...

https://codereview.chromium.org/322433004/diff/40001/content/common/gpu/media...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (left):

https://codereview.chromium.org/322433004/diff/40001/content/common/gpu/media...
content/common/gpu/media/gpu_video_decode_accelerator.cc:462:
video_decode_accelerator_.reset();
ditto

https://codereview.chromium.org/322433004/diff/40001/content/common/gpu/media...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/322433004/diff/40001/content/common/gpu/media...
content/common/gpu/media/gpu_video_decode_accelerator.cc:363: texture_target_ ==
GL_TEXTURE_RECTANGLE) {
do we need this change right now?

in general I prefer to avoid making changes like this until absolutely necessary
(e.g., there is some code inside of VTVDA that needs this)

https://codereview.chromium.org/322433004/diff/40001/content/common/gpu/media...
File content/common/gpu/media/vt_video_decode_accelerator.cc (right):

https://codereview.chromium.org/322433004/diff/40001/content/common/gpu/media...
content/common/gpu/media/vt_video_decode_accelerator.cc:18: client_ = NULL;
paranoid NULL setting? :)

(it's fine to leave it in if you want, but it's not really doing us much)

https://codereview.chromium.org/322433004/diff/40001/content/common/gpu/media...
content/common/gpu/media/vt_video_decode_accelerator.cc:31: // Prevent anyone
from using VTVideoDecoder for now.
link to bug would be nice

https://codereview.chromium.org/322433004/diff/40001/content/common/gpu/media...
File content/common/gpu/media/vt_video_decode_accelerator.h (right):

https://codereview.chromium.org/322433004/diff/40001/content/common/gpu/media...
content/common/gpu/media/vt_video_decode_accelerator.h:8: #include <stdint.h>
what's this used for?

https://codereview.chromium.org/322433004/diff/40001/content/common/gpu/media...
content/common/gpu/media/vt_video_decode_accelerator.h:20: class CONTENT_EXPORT
VTVideoDecodeAccelerator
do you need CONTENT_EXPORT yet? usually it's needed when the class is
instantiated outside of content library (e.g., for unit tests or use outside of
content)

https://codereview.chromium.org/322433004/diff/40001/content/common/gpu/media...
content/common/gpu/media/vt_video_decode_accelerator.h:20: class CONTENT_EXPORT
VTVideoDecodeAccelerator
docs for the class

https://codereview.chromium.org/322433004/diff/40001/content/common/gpu/media...
content/common/gpu/media/vt_video_decode_accelerator.h:40:
base::WeakPtrFactory<VTVideoDecodeAccelerator> weak_this_factory_;
this should be the last member - feel free to copy the comment as well

see
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/serial_...
for an example

see https://code.google.com/p/chromium/issues/detail?id=351048 for reasoning

[sandersd (OOO until July 31), 2014-06-10 01:02:45]

https://codereview.chromium.org/322433004/diff/40001/chrome/browser/about_fla...
File chrome/browser/about_flags.cc (left):

https://codereview.chromium.org/322433004/diff/40001/chrome/browser/about_fla...
chrome/browser/about_flags.cc:430: #if defined(OS_CHROMEOS)
On 2014/06/09 16:02:06, scherkus (OOO June 6-13) wrote:
> bad merge? looks like you have a bunch of related changes

Ugh, that's no good. Probably happened when I transferred the branch from my
workstation to the laptop.

https://codereview.chromium.org/322433004/diff/40001/chrome/browser/about_fla...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/322433004/diff/40001/chrome/browser/about_fla...
chrome/browser/about_flags.cc:1031: kOsMac | kOsWin | kOsCrOS,
On 2014/06/09 16:02:06, scherkus (OOO June 6-13) wrote:
> is this change needed or does --ignore-gpu-blacklist suffice for now
> 
> I believe the blacklist is the main mechanism that disables GPU decode on mac
> today:
>
https://code.google.com/p/chromium/codesearch#chromium/src/gpu/config/softwar...
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/gp...

Done.

https://codereview.chromium.org/322433004/diff/40001/content/common/gpu/media...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (left):

https://codereview.chromium.org/322433004/diff/40001/content/common/gpu/media...
content/common/gpu/media/gpu_video_decode_accelerator.cc:462:
video_decode_accelerator_.reset();
On 2014/06/09 16:02:06, scherkus (OOO June 6-13) wrote:
> ditto

More bad merging :-(

https://codereview.chromium.org/322433004/diff/40001/content/common/gpu/media...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/322433004/diff/40001/content/common/gpu/media...
content/common/gpu/media/gpu_video_decode_accelerator.cc:363: texture_target_ ==
GL_TEXTURE_RECTANGLE) {
On 2014/06/09 16:02:06, scherkus (OOO June 6-13) wrote:
> do we need this change right now?
> 
> in general I prefer to avoid making changes like this until absolutely
necessary
> (e.g., there is some code inside of VTVDA that needs this)

Done.

https://codereview.chromium.org/322433004/diff/40001/content/common/gpu/media...
File content/common/gpu/media/vt_video_decode_accelerator.cc (right):

https://codereview.chromium.org/322433004/diff/40001/content/common/gpu/media...
content/common/gpu/media/vt_video_decode_accelerator.cc:18: client_ = NULL;
On 2014/06/09 16:02:07, scherkus (OOO June 6-13) wrote:
> paranoid NULL setting? :)
> 
> (it's fine to leave it in if you want, but it's not really doing us much)

Done.

https://codereview.chromium.org/322433004/diff/40001/content/common/gpu/media...
content/common/gpu/media/vt_video_decode_accelerator.cc:31: // Prevent anyone
from using VTVideoDecoder for now.
On 2014/06/09 16:02:06, scherkus (OOO June 6-13) wrote:
> link to bug would be nice

Done.

https://codereview.chromium.org/322433004/diff/40001/content/common/gpu/media...
File content/common/gpu/media/vt_video_decode_accelerator.h (right):

https://codereview.chromium.org/322433004/diff/40001/content/common/gpu/media...
content/common/gpu/media/vt_video_decode_accelerator.h:8: #include <stdint.h>
On 2014/06/09 16:02:07, scherkus (OOO June 6-13) wrote:
> what's this used for?

int32_t, but the include is implied by the fact that it is an interface
implementation.

I'm not sure of the IWYU status of Chromium, it sounds like the idea is
encouraged but the tool is broken.

https://codereview.chromium.org/322433004/diff/40001/content/common/gpu/media...
content/common/gpu/media/vt_video_decode_accelerator.h:20: class CONTENT_EXPORT
VTVideoDecodeAccelerator
On 2014/06/09 16:02:07, scherkus (OOO June 6-13) wrote:
> do you need CONTENT_EXPORT yet? usually it's needed when the class is
> instantiated outside of content library (e.g., for unit tests or use outside
of
> content)

Done.

https://codereview.chromium.org/322433004/diff/40001/content/common/gpu/media...
content/common/gpu/media/vt_video_decode_accelerator.h:20: class CONTENT_EXPORT
VTVideoDecodeAccelerator
On 2014/06/09 16:02:07, scherkus (OOO June 6-13) wrote:
> docs for the class

Done.

https://codereview.chromium.org/322433004/diff/40001/content/common/gpu/media...
content/common/gpu/media/vt_video_decode_accelerator.h:40:
base::WeakPtrFactory<VTVideoDecodeAccelerator> weak_this_factory_;
On 2014/06/09 16:02:07, scherkus (OOO June 6-13) wrote:
> this should be the last member - feel free to copy the comment as well
> 
> see
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/serial_...
> for an example
> 
> see https://code.google.com/p/chromium/issues/detail?id=351048 for reasoning

Done.

[scherkus (not reviewing), 2014-06-10 16:58:59]

lgtm w/ nit

FYI I'll be busy most of this week so I'd recommend finding some other folk for
reviewing (maybe dalecurtis?)

https://codereview.chromium.org/322433004/diff/60001/content/common/gpu/media...
File content/common/gpu/media/vt_video_decode_accelerator.cc (right):

https://codereview.chromium.org/322433004/diff/60001/content/common/gpu/media...
content/common/gpu/media/vt_video_decode_accelerator.cc:23: DVLOG(2) <<
"VTVideoDecodeAccelerator::Initialize";
nit: we've been using __FUNCTION__ these days

it'll only print out "Initialize", but it's more future-proof, less typo prone
and takes less characters to print

[sandersd (OOO until July 31), 2014-06-10 20:40:13]

The CQ bit was checked by sandersd@chromium.org

[commit-bot: I haz the power, 2014-06-10 20:43:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sandersd@chromium.org/322433004/80001

[sandersd (OOO until July 31), 2014-06-10 21:26:52]

The CQ bit was unchecked by sandersd@chromium.org

[sandersd (OOO until July 31), 2014-06-10 21:54:05]

The CQ bit was checked by sandersd@chromium.org

[commit-bot: I haz the power, 2014-06-10 21:54:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sandersd@chromium.org/322433004/90001

[commit-bot: I haz the power, 2014-06-11 07:22:00]

Change committed as 276290