Implementation for Pepper C++ Video Decoder API (wrapper on top of C API).

Implementation for Pepper C++ Video Decoder API (wrapper on top of C API).

BUG=
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=87831

[Ville-Mikko Rautio, 2011-05-30 12:07:36]

Uploaded simple C++ wrapper implementation for Pepper C API as separate CL. Hope
to land quick.

[Ville-Mikko Rautio, 2011-05-31 11:55:44]

One more small change and added Brett for OWNERS approval.

[brettw, 2011-05-31 12:46:44]

http://codereview.chromium.org/7085030/diff/8001/ppapi/cpp/dev/video_decoder_...
File ppapi/cpp/dev/video_decoder_dev.cc (right):

http://codereview.chromium.org/7085030/diff/8001/ppapi/cpp/dev/video_decoder_...
ppapi/cpp/dev/video_decoder_dev.cc:37: std::copy(config.begin(), config.end(),
c_config);
I don't really understand what you're doing here. The config is in a vector but
you're still requiring the terminator at the end, and it will probably crash if
you forget that.

It frankly seems safer from a caller's perspective to just take the pointer to
the config element array, since then it's more clear that "you need to do
something to tell it about the end of the array".

Also, the copying here seems totally unnecessary.

http://codereview.chromium.org/7085030/diff/8001/ppapi/cpp/dev/video_decoder_...
ppapi/cpp/dev/video_decoder_dev.cc:40: delete [] c_config;
Normally I think we don't put a space in delete[].

http://codereview.chromium.org/7085030/diff/8001/ppapi/cpp/dev/video_decoder_...
ppapi/cpp/dev/video_decoder_dev.cc:47: const vector<uint32_t>& prototype_config)
{
You're not very consistent about std:: vs. not. I'd just use std:: everywhere
and delete the using at the top of this file.

http://codereview.chromium.org/7085030/diff/8001/ppapi/cpp/dev/video_decoder_...
ppapi/cpp/dev/video_decoder_dev.cc:58: std::copy(prototype_config.begin(),
prototype_config.end(), c_proto);
Is this copy necessary? Why not just pass &prototype_config[0] if there's an
element (being careful to pass NULL if there's no element). I also don't see why
these vectors are all uint32_t instead of the PP_VideoConfigElement typedef.

http://codereview.chromium.org/7085030/diff/8001/ppapi/cpp/dev/video_decoder_...
ppapi/cpp/dev/video_decoder_dev.cc:69: new
PP_VideoConfigElement[num_of_matching_configs];
You can also copy straight into the vector and avoid this.

http://codereview.chromium.org/7085030/diff/8001/ppapi/cpp/dev/video_decoder_...
ppapi/cpp/dev/video_decoder_dev.cc:86: if (!gles_buffers) {
We don't normally check for OOM (and neither do you above) unless it's something
in particular that may fail. I'd just remove these checks (next function as
well).

http://codereview.chromium.org/7085030/diff/8001/ppapi/cpp/dev/video_decoder_...
ppapi/cpp/dev/video_decoder_dev.cc:90: std::copy(buffers.begin(), buffers.end(),
gles_buffers);
I think you can avoid this copy.

http://codereview.chromium.org/7085030/diff/8001/ppapi/cpp/dev/video_decoder_...
File ppapi/cpp/dev/video_decoder_dev.h (right):

http://codereview.chromium.org/7085030/diff/8001/ppapi/cpp/dev/video_decoder_...
ppapi/cpp/dev/video_decoder_dev.h:73: void AssignGLESBuffers(const
std::vector<PP_GLESBuffer_Dev> buffers);
The arguments to these two functions should be references.

[Ville-Mikko Rautio, 2011-05-31 13:33:14]

Brett, thanks for quick review! Here's the updated CL. It now trusts on
allocations to simply to work and std::vector implementation to be C-array. It
really made code a lot prettier! Also one nasty bug was fixed!

http://codereview.chromium.org/7085030/diff/8001/ppapi/cpp/dev/video_decoder_...
File ppapi/cpp/dev/video_decoder_dev.cc (right):

http://codereview.chromium.org/7085030/diff/8001/ppapi/cpp/dev/video_decoder_...
ppapi/cpp/dev/video_decoder_dev.cc:37: std::copy(config.begin(), config.end(),
c_config);
On 2011/05/31 12:46:44, brettw wrote:
> I don't really understand what you're doing here. The config is in a vector
but
> you're still requiring the terminator at the end, and it will probably crash
if
> you forget that.
> 
> It frankly seems safer from a caller's perspective to just take the pointer to
> the config element array, since then it's more clear that "you need to do
> something to tell it about the end of the array".
> 
> Also, the copying here seems totally unnecessary.

Done.

http://codereview.chromium.org/7085030/diff/8001/ppapi/cpp/dev/video_decoder_...
ppapi/cpp/dev/video_decoder_dev.cc:40: delete [] c_config;
On 2011/05/31 12:46:44, brettw wrote:
> Normally I think we don't put a space in delete[].

Done.

http://codereview.chromium.org/7085030/diff/8001/ppapi/cpp/dev/video_decoder_...
ppapi/cpp/dev/video_decoder_dev.cc:47: const vector<uint32_t>& prototype_config)
{
On 2011/05/31 12:46:44, brettw wrote:
> You're not very consistent about std:: vs. not. I'd just use std:: everywhere
> and delete the using at the top of this file.

Done.

http://codereview.chromium.org/7085030/diff/8001/ppapi/cpp/dev/video_decoder_...
ppapi/cpp/dev/video_decoder_dev.cc:58: std::copy(prototype_config.begin(),
prototype_config.end(), c_proto);
On 2011/05/31 12:46:44, brettw wrote:
> Is this copy necessary? Why not just pass &prototype_config[0] if there's an
> element (being careful to pass NULL if there's no element).

I changed this as well. From now on, we are absolutely trust that vector
implementation is on all targets implemented as C-array, right? 

> I also don't see why
> these vectors are all uint32_t instead of the PP_VideoConfigElement typedef.

Done.

http://codereview.chromium.org/7085030/diff/8001/ppapi/cpp/dev/video_decoder_...
ppapi/cpp/dev/video_decoder_dev.cc:69: new
PP_VideoConfigElement[num_of_matching_configs];
On 2011/05/31 12:46:44, brettw wrote:
> You can also copy straight into the vector and avoid this.

Done.

http://codereview.chromium.org/7085030/diff/8001/ppapi/cpp/dev/video_decoder_...
ppapi/cpp/dev/video_decoder_dev.cc:86: if (!gles_buffers) {
On 2011/05/31 12:46:44, brettw wrote:
> We don't normally check for OOM (and neither do you above) unless it's
something
> in particular that may fail. I'd just remove these checks (next function as
> well).

Done.

http://codereview.chromium.org/7085030/diff/8001/ppapi/cpp/dev/video_decoder_...
ppapi/cpp/dev/video_decoder_dev.cc:90: std::copy(buffers.begin(), buffers.end(),
gles_buffers);
On 2011/05/31 12:46:44, brettw wrote:
> I think you can avoid this copy.

Done.

http://codereview.chromium.org/7085030/diff/8001/ppapi/cpp/dev/video_decoder_...
File ppapi/cpp/dev/video_decoder_dev.h (right):

http://codereview.chromium.org/7085030/diff/8001/ppapi/cpp/dev/video_decoder_...
ppapi/cpp/dev/video_decoder_dev.h:73: void AssignGLESBuffers(const
std::vector<PP_GLESBuffer_Dev> buffers);
On 2011/05/31 12:46:44, brettw wrote:
> The arguments to these two functions should be references.

Done.

[Ami GONE FROM CHROMIUM, 2011-05-31 15:01:56]

Change LGTM, but const_cast<>'s leave me sad.
I think the C API should be changed to take const*'s instead of *'s for its
(unmodified) arrays.
Wanna add a TODO in this CL to the .cc file to make that change and remove the
const_cast<>'s from this file?  You can assign it to me.

[Ville-Mikko Rautio, 2011-05-31 15:10:11]

On 2011/05/31 15:01:56, Ami Fischman wrote:
> Change LGTM, but const_cast<>'s leave me sad.
> I think the C API should be changed to take const*'s instead of *'s for its
> (unmodified) arrays.
> Wanna add a TODO in this CL to the .cc file to make that change and remove the
> const_cast<>'s from this file?  You can assign it to me.

If you want I can add the TODOs, but I can't remove const casts as they will
cause the build to break. For example:

ppapi/cpp/dev/video_decoder_dev.cc:34: error: invalid conversion from ‘const
int*’ to ‘PP_VideoConfigElement*’

Of course both have underlying type of int*, but it seems the compiler gets
really picky about the conversion for some reason that has probably to do with
the deferencing of the first element in the const vector (which is of type
PP_VideoConfigElement).

[Ami GONE FROM CHROMIUM, 2011-05-31 15:18:21]

I think you mis-parsed my question:
I said: Wanna add a TODO in this CL to the .cc file to make that change and
remove the const_cast<>'s from this file?
You read: Wanna (add a TODO in this CL to the .cc file to make that change)
and (remove the const_cast<>'s from this file)?
I meant: Wanna add a TODO in this CL to the .cc file to (make that change
and remove the const_cast<>'s from this file)?

FWIW: the reason your attempt breaks the build is that
std::vector::operator[] has two overloads: one that is marked "const" and
returns a const-ref, and the other that is non-const and returns a
non-const-ref.  When operating on a const& vector, only the former
operator[] can be applied, so the returned ref is const, so its address is a
const*.  The C API declares the parameter to be a non-const pointer, so
giving it a const pointer is illegal.  My point about the TODO is that the C
API doesn't actually need a non-const pointer, so adding a const to its
declaration makes using it easier.

[vmr, 2011-05-31 15:20:57]

So I did. :) I'll make the change.

On Tue, May 31, 2011 at 6:18 PM, Ami Fischman <fischman@chromium.org> wrote:

> I think you mis-parsed my question:
> I said: Wanna add a TODO in this CL to the .cc file to make that change and
> remove the const_cast<>'s from this file?
> You read: Wanna (add a TODO in this CL to the .cc file to make that change)
> and (remove the const_cast<>'s from this file)?
> I meant: Wanna add a TODO in this CL to the .cc file to (make that change
> and remove the const_cast<>'s from this file)?
>
> FWIW: the reason your attempt breaks the build is that
> std::vector::operator[] has two overloads: one that is marked "const" and
> returns a const-ref, and the other that is non-const and returns a
> non-const-ref.  When operating on a const& vector, only the former
> operator[] can be applied, so the returned ref is const, so its address is a
> const*.  The C API declares the parameter to be a non-const pointer, so
> giving it a const pointer is illegal.  My point about the TODO is that the C
> API doesn't actually need a non-const pointer, so adding a const to its
> declaration makes using it easier.
>

[Ville-Mikko Rautio, 2011-05-31 15:26:37]

Now the CL has the TODO asked by fischman.

[Ami GONE FROM CHROMIUM, 2011-05-31 15:27:53]

LGTM

[brettw, 2011-06-01 01:21:44]

Why not just fix the const in this patch? It seems like a pretty trivial change.

[Ami GONE FROM CHROMIUM, 2011-06-01 02:50:31]

>
> Why not just fix the const in this patch? It seems like a pretty trivial
> change.


I was trying to avoid the extra RTT (vmr@ is in Finland).  Obviously, fine
by me if the fix is part of this CL, too.  I guess if you don't LGTM by the
time vmr@ gets back in the office in (his) morning, he'll do this, too.

-a

[brettw, 2011-06-01 07:57:03]

http://codereview.chromium.org/7085030/diff/20001/ppapi/cpp/dev/video_decoder...
File ppapi/cpp/dev/video_decoder_dev.h (right):

http://codereview.chromium.org/7085030/diff/20001/ppapi/cpp/dev/video_decoder...
ppapi/cpp/dev/video_decoder_dev.h:61: const std::vector<PP_VideoConfigElement>&
config,
I still have concerns about using a vector here, are you sure this is an
improvement over a C string?

As I mentioned in the previous review, the user must add the terminator in the
list even though the vector has a specific size and you're not used to having to
have some "ending marker" on a vector. This seems surprising and will crash.

How do we expect people to typically call it? For some things like this you have
a hardcoded list, and the vector makes this less convenient (since you'd just
make an array in C declaring the things you want, and pass it in). If we expect
this is the common usage, it seems like a regular C pointer like the C interface
is better.

If we expect callers to have to dynamically build it, the vector seems better,
but it seems like we should have some checks that it has the terminator at the
end. In this case, I'd make the constructor assert that it has the terminator,
and just return early (leaving the object uninitialized) if it's not there.

[Ville-Mikko Rautio, 2011-06-01 09:45:26]

Builds ok locally, waiting for tryserver results. After that once Brett gives
OWNERS approval, this one should be ready to land. If I am OOO at that time,
anyone with committer access, feel free to check the commit box.

http://codereview.chromium.org/7085030/diff/20001/ppapi/cpp/dev/video_decoder...
File ppapi/cpp/dev/video_decoder_dev.h (right):

http://codereview.chromium.org/7085030/diff/20001/ppapi/cpp/dev/video_decoder...
ppapi/cpp/dev/video_decoder_dev.h:61: const std::vector<PP_VideoConfigElement>&
config,
On 2011/06/01 07:57:03, brettw wrote:
> I still have concerns about using a vector here, are you sure this is an
> improvement over a C string?
> 
> As I mentioned in the previous review, the user must add the terminator in the
> list even though the vector has a specific size and you're not used to having
to
> have some "ending marker" on a vector. This seems surprising and will crash.
> 
> How do we expect people to typically call it? For some things like this you
have
> a hardcoded list, and the vector makes this less convenient (since you'd just
> make an array in C declaring the things you want, and pass it in). If we
expect
> this is the common usage, it seems like a regular C pointer like the C
interface
> is better.
> 
> If we expect callers to have to dynamically build it, the vector seems better,
> but it seems like we should have some checks that it has the terminator at the
> end. In this case, I'd make the constructor assert that it has the terminator,
> and just return early (leaving the object uninitialized) if it's not there.

Agreed. C-array style is better from user perspective. Changed it back.

[brettw, 2011-06-01 14:25:39]

LGTM

[Ami GONE FROM CHROMIUM, 2011-06-01 18:26:28]

http://codereview.chromium.org/7085030/diff/2002/ppapi/cpp/dev/video_decoder_...
File ppapi/cpp/dev/video_decoder_dev.cc (right):

http://codereview.chromium.org/7085030/diff/2002/ppapi/cpp/dev/video_decoder_...
ppapi/cpp/dev/video_decoder_dev.cc:26: PP_VideoConfigElement* config,
const

http://codereview.chromium.org/7085030/diff/2002/ppapi/cpp/dev/video_decoder_...
File ppapi/cpp/dev/video_decoder_dev.h (right):

http://codereview.chromium.org/7085030/diff/2002/ppapi/cpp/dev/video_decoder_...
ppapi/cpp/dev/video_decoder_dev.h:61: PP_VideoConfigElement* config,
should be const*

http://codereview.chromium.org/7085030/diff/2002/ppapi/cpp/dev/video_decoder_...
ppapi/cpp/dev/video_decoder_dev.h:71: uint32_t* num_of_matching_configs);
I've been confused about this API for the last 24h or so, probably because of
the pluralization of the names.

prototype_config is a single array of different config options, so they are
presumably all orthogonal to each other.

matching_configs is a plural, which makes me think that possibly more than one
matching sets of configurations will be returned.  But the type is PP_VCE*, not
PP_VCE** or some other double-array mechanism.  Is the expectation that
*matching_configs will be laid out with the different config sets back-to-back,
and *num_of_matching_configs is used to figure out whether a TERMINATOR is the
last config element or whether another configset follows?

[vjain, 2011-06-02 01:03:05]

Hi Ville-Mikko,
I do not see any mechanism to send EOS to GPU video decoder from app/plugin. Can
you please update, how to send EOS to Video decoder?

[Ville-Mikko Rautio, 2011-06-03 09:26:05]

http://codereview.chromium.org/7085030/diff/2002/ppapi/cpp/dev/video_decoder_...
File ppapi/cpp/dev/video_decoder_dev.cc (right):

http://codereview.chromium.org/7085030/diff/2002/ppapi/cpp/dev/video_decoder_...
ppapi/cpp/dev/video_decoder_dev.cc:26: PP_VideoConfigElement* config,
On 2011/06/01 18:26:28, Ami Fischman wrote:
> const

Done.

http://codereview.chromium.org/7085030/diff/2002/ppapi/cpp/dev/video_decoder_...
File ppapi/cpp/dev/video_decoder_dev.h (right):

http://codereview.chromium.org/7085030/diff/2002/ppapi/cpp/dev/video_decoder_...
ppapi/cpp/dev/video_decoder_dev.h:61: PP_VideoConfigElement* config,
On 2011/06/01 18:26:28, Ami Fischman wrote:
> should be const*

Done.

http://codereview.chromium.org/7085030/diff/2002/ppapi/cpp/dev/video_decoder_...
ppapi/cpp/dev/video_decoder_dev.h:71: uint32_t* num_of_matching_configs);
On 2011/06/01 18:26:28, Ami Fischman wrote:
> I've been confused about this API for the last 24h or so, probably because of
> the pluralization of the names.
> 
> prototype_config is a single array of different config options, so they are
> presumably all orthogonal to each other.

Yes, that is the idea.

> matching_configs is a plural, which makes me think that possibly more than one
> matching sets of configurations will be returned.  But the type is PP_VCE*,
not
> PP_VCE** or some other double-array mechanism.  Is the expectation that
> *matching_configs will be laid out with the different config sets
back-to-back,
> and *num_of_matching_configs is used to figure out whether a TERMINATOR is the
> last config element or whether another configset follows?

Exactly like this.

[Ville-Mikko Rautio, 2011-06-03 09:28:40]

On 2011/06/02 01:03:05, vjain wrote:
> Hi Ville-Mikko,
> I do not see any mechanism to send EOS to GPU video decoder from app/plugin.
Can
> you please update, how to send EOS to Video decoder?

Simply call Flush(), wait for completion and call Abort and wait for completion.
Once Abort is completed, your session has finished.

[Ville-Mikko Rautio, 2011-06-03 13:08:24]

I think this CL could be landed now.

Mac trybot fails on completely unrelated Skia unittest at the following
location:

https://cs.corp.google.com/#chrome/trunk/src/skia/ext/image_operations_unitte...

[commit-bot: I haz the power, 2011-06-03 18:01:11]

Presubmit check for 7085030-29001 failed and returned exit status 1.

Running presubmit commit checks ...
Traceback (most recent call last):
  File "/mnt/data/b/commit-queue/verification/presubmit_shim.py", line 34, in
<module>
    sys.exit(presubmit_support.Main(None))
  File "/b/depot_tools/presubmit_support.py", line 1235, in Main
    None)
  File "/b/depot_tools/presubmit_support.py", line 1091, in DoPresubmitChecks
    results += executer.ExecPresubmitScript(presubmit_script, filename)
  File "/b/depot_tools/presubmit_support.py", line 1006, in ExecPresubmitScript
    result = eval(function_name + '(*__args)', context)
  File "<string>", line 1, in <module>
  File "<string>", line 128, in CheckChangeOnCommit
  File "<string>", line 45, in _CommonChecks
  File "/b/depot_tools/presubmit_canned_checks.py", line 890, in
PanProjectChecks
    input_api, output_api, source_file_filter=sources))
  File "/b/depot_tools/presubmit_canned_checks.py", line 743, in CheckOwners
    owners_db.email_regexp)
  File "/b/depot_tools/presubmit_canned_checks.py", line 767, in
_RietveldOwnerAndApprovers
    issue_props = input_api.rietveld.get_issue_properties(
AttributeError: 'NoneType' object has no attribute 'get_issue_properties'
Sending crash report ...
  exception: 'NoneType' object has no attribute 'get_issue_prop
  args: ['/mnt/data/b/commit-queue/verification/presubmit_shim.py', '--commit',
'--author', 'vmr@chromium.org', '--issue', '7085030', '--patchset', '29001',
'--name', '7085030-29001', '--description', 'Implementation for Pepper C++ Video
Decoder API (wrapper on top of C API).\n\nBUG=\nTEST=\n',
'ppapi/cpp/dev/video_decoder_dev.h',
'webkit/plugins/ppapi/ppb_video_decoder_impl.cc',
'webkit/plugins/ppapi/ppb_video_decoder_impl.h',
'ppapi/c/dev/ppb_video_decoder_dev.h', 'ppapi/cpp/dev/video_decoder_dev.cc']
  stack:   File "/mnt/data/b/commit-queue/verification/pres
  host: vm105-m4.golo.chromium.org
  version: 2.6.5 (r265:79063, Apr 16 2010, 13:57:41) 
[GCC 4.
  user: chrome-bot
  cwd: /mnt/data/b/commit-queue/workdir/chromium
A stack trace has been sent to the maintainers.

[Ami GONE FROM CHROMIUM, 2011-06-03 18:01:15]

On 2011/06/03 13:08:24, vmr1 wrote:
> I think this CL could be landed now.
> 
> Mac trybot fails on completely unrelated Skia unittest at the following
> location:
> 
>
https://cs.corp.google.com/#chrome/trunk/src/skia/ext/image_operations_unitte...

Commit-box checked.

[commit-bot: I haz the power, 2011-06-03 19:33:41]

Change committed as 87831