Implement PPAPI VideoDecode out-of-process support

Implement PPAPI VideoDecode out-of-process support

This CL implements the proxy necessary for out-of-process video decoding and
introduces a shared base class between the PPB_VideoDecoder_Impl and the proxy.

BUG=86106
TEST=gles2 plugin runs with or without --ppapi-out-of-process flag, no crashes


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

[vrk (LEFT CHROMIUM), 2011-08-01 19:41:08]

everyone: thoughts on how I implemented proxy + shared_impl
brettw: OWNERS
piman: modifications to HostDispatcher
fischman: everything!


As brettw suggested, I tried to follow PPB_Audio and PPB_Surface3D as examples.
There is still some redundancy between the VideoDecoderProxy and the
VideoDecoderImpl, but it is a bit tricky to fix... let me know if you want me to
elaborate.

[ddorwin, 2011-08-01 20:53:04]

Does the UI test pass now? If so, please remove the FAILS_ and comment:
http://codesearch.google.com/#OAMlx_jo-ck/src/chrome/test/ui/ppapi_uitest.cc&...

[piman, 2011-08-01 20:57:48]

A few initial thoughts, haven't finished the full review, but the overall
approach seems fine to me.
Also, as I asked on IM, could you extract the HostDispatcher change ?

http://codereview.chromium.org/7545014/diff/36/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

http://codereview.chromium.org/7545014/diff/36/ppapi/proxy/ppapi_messages.h#n...
ppapi/proxy/ppapi_messages.h:329: PP_Instance /* instance */,
Instance is already encoded in the HostResource, so you don't need to pass it
separately (here and all messages below)

http://codereview.chromium.org/7545014/diff/36/ppapi/proxy/ppapi_messages.h#n...
ppapi/proxy/ppapi_messages.h:954: std::vector<PP_Size> /* buffer sizes */)
How about a std::vector<PP_PictureBuffer_Dev> instead ? That way you don't need
extra work to verify consistency etc. Logically it's easier to understand too.
Since those are all POD, it should be trivial.

http://codereview.chromium.org/7545014/diff/36/ppapi/proxy/ppapi_messages.h#n...
ppapi/proxy/ppapi_messages.h:962:
IPC_SYNC_MESSAGE_ROUTED1_0(PpapiHostMsg_PPBVideoDecoder_Destroy,
Does this need to be sync ?

[vrk (LEFT CHROMIUM), 2011-08-01 23:40:04]

> Does the UI test pass now? If so, please remove the FAILS_ and comment
Yes and done. Thanks for reminding me, ddorwin!

> Also, as I asked on IM, could you extract the HostDispatcher change ?
Done here: http://codereview.chromium.org/7540032/


Thanks for the clean-up suggestions, piman! Made those areas quite a bit nicer.

http://codereview.chromium.org/7545014/diff/36/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

http://codereview.chromium.org/7545014/diff/36/ppapi/proxy/ppapi_messages.h#n...
ppapi/proxy/ppapi_messages.h:329: PP_Instance /* instance */,
On 2011/08/01 20:57:48, piman wrote:
> Instance is already encoded in the HostResource, so you don't need to pass it
> separately (here and all messages below)

Done.

http://codereview.chromium.org/7545014/diff/36/ppapi/proxy/ppapi_messages.h#n...
ppapi/proxy/ppapi_messages.h:954: std::vector<PP_Size> /* buffer sizes */)
On 2011/08/01 20:57:48, piman wrote:
> How about a std::vector<PP_PictureBuffer_Dev> instead ? That way you don't
need
> extra work to verify consistency etc. Logically it's easier to understand too.
> Since those are all POD, it should be trivial.

Done.

http://codereview.chromium.org/7545014/diff/36/ppapi/proxy/ppapi_messages.h#n...
ppapi/proxy/ppapi_messages.h:962:
IPC_SYNC_MESSAGE_ROUTED1_0(PpapiHostMsg_PPBVideoDecoder_Destroy,
On 2011/08/01 20:57:48, piman wrote:
> Does this need to be sync ?

Yes, the pepper -> renderer -> gpu path for this message must be synchronous.
The VideoDecoder API guarantees a synchronous Destroy such that after a call to
Destroy, it is safe to delete textures and resources and there will never be any
callbacks to the plugin again.

[piman, 2011-08-02 00:40:08]

Some more comments.

http://codereview.chromium.org/7545014/diff/3059/ppapi/proxy/ppb_video_decode...
File ppapi/proxy/ppb_video_decoder_proxy.cc (right):

http://codereview.chromium.org/7545014/diff/3059/ppapi/proxy/ppb_video_decode...
ppapi/proxy/ppb_video_decoder_proxy.cc:53: DCHECK(enter_context.succeeded());
So, we do this enter of the context, which is a map lookup, twice. Isn't there a
way to pass through the Context3D* directly instead of the resource id ?

http://codereview.chromium.org/7545014/diff/3059/ppapi/proxy/ppb_video_decode...
ppapi/proxy/ppb_video_decoder_proxy.cc:92: buffer_list.push_back(buffers[i]);
You can write all this as:
std::vector<PP_PictureBuffer_Dev> buffer_list(buffers, buffers+no_of_buffers);

http://codereview.chromium.org/7545014/diff/3059/ppapi/proxy/ppb_video_decode...
ppapi/proxy/ppb_video_decoder_proxy.cc:253: { id, buffer.host_resource(), size
};
nit: Can't this fit on one line ? If yes, please remove spaces after { and
before }.
If not, the correct style is:
PP_VideoBitstreamBuffer_Dev bitstream = {
  A,
  B,
  C
};

http://codereview.chromium.org/7545014/diff/3059/ppapi/proxy/ppb_video_decode...
ppapi/proxy/ppb_video_decoder_proxy.cc:289: CompletionCallback callback =
callback_factory_.NewOptionalCallback(
Make this a NewRequiredCallback...

http://codereview.chromium.org/7545014/diff/3059/ppapi/proxy/ppb_video_decode...
ppapi/proxy/ppb_video_decoder_proxy.cc:295: result =
enter.object()->Flush(callback.pp_completion_callback());
... Call the host interface directly through target_interface() [you may want to
implement a ppb_video_decoder_target() like in other proxies] ...

http://codereview.chromium.org/7545014/diff/3059/ppapi/proxy/ppb_video_decode...
ppapi/proxy/ppb_video_decoder_proxy.cc:296: if (result !=
PP_OK_COMPLETIONPENDING)
... and you can skip this part !

This whole function can be as simple as:
CompletionCallback callback =
callback_factory_.NewRequiredCallback(&PPB_VideoDecoder_Proxy::SendMsgFlushACKToPlugin,
decoder);
ppb_video_decoder_target()->Flush(decoder.host_resource(),
callback.pp_completion_callback());

Same with several other functions here...

http://codereview.chromium.org/7545014/diff/3059/ppapi/proxy/ppp_video_decode...
File ppapi/proxy/ppp_video_decoder_proxy.h (right):

http://codereview.chromium.org/7545014/diff/3059/ppapi/proxy/ppp_video_decode...
ppapi/proxy/ppp_video_decoder_proxy.h:26: return reinterpret_cast<const
PPP_VideoDecoder_Dev*>(target_interface());
reinterpret_cast->static_cast.

http://codereview.chromium.org/7545014/diff/3059/ppapi/shared_impl/DEPS
File ppapi/shared_impl/DEPS (right):

http://codereview.chromium.org/7545014/diff/3059/ppapi/shared_impl/DEPS#newcode6
ppapi/shared_impl/DEPS:6: "+webkit/plugins/ppapi",
That seems wrong, why do you need this ?

http://codereview.chromium.org/7545014/diff/3059/ppapi/shared_impl/video_deco...
File ppapi/shared_impl/video_decoder_impl.cc (right):

http://codereview.chromium.org/7545014/diff/3059/ppapi/shared_impl/video_deco...
ppapi/shared_impl/video_decoder_impl.cc:64:
DCHECK(bitstream_buffer_callbacks_.insert(
DCHECK(side effect) ??
Very bad idea... The side effect won't execute in release...
Instead: bool result = bitstream_buffer_callbacks_.insert(...).second;
DCHECK(result).

http://codereview.chromium.org/7545014/diff/3059/ppapi/shared_impl/video_deco...
ppapi/shared_impl/video_decoder_impl.cc:98: // merge PP_VideoAttributeDictionary
and VideoAttributeKey.
You can add COMPILE_ASSERT for those.

http://codereview.chromium.org/7545014/diff/3059/ppapi/shared_impl/video_deco...
ppapi/shared_impl/video_decoder_impl.cc:104:
out_configs->push_back(static_cast<int32>(*current));
Is the static_cast needed ? enums should cast implicitly to ints.

http://codereview.chromium.org/7545014/diff/3059/ppapi/shared_impl/video_deco...
ppapi/shared_impl/video_decoder_impl.cc:121:
::pp::proxy::PluginResourceTracker::GetInstance()->AddRefResource(resource);
That sort of logic is kinda bad. Why not pass the resource tracker in parameter
to the VideoDecoderImpl instead of in_renderer_process ?

[Ami GONE FROM CHROMIUM, 2011-08-02 00:49:08]

Despite the number of nits, I like this change!

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/host_dispatcher.cc
File ppapi/proxy/host_dispatcher.cc (right):

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/host_dispatcher....
ppapi/proxy/host_dispatcher.cc:216: allow_plugin_reentrancy_ = true;
Revert this file

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/plugin_resource.h
File ppapi/proxy/plugin_resource.h (right):

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/plugin_resource....
ppapi/proxy/plugin_resource.h:36: F(URLResponseInfo)\
extra space before backslash here (and on previous line).
(otherwise you're just getting lucky that the next line has leading spaces :))

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppapi_messages.h...
ppapi/proxy/ppapi_messages.h:324:
IPC_MESSAGE_ROUTED2(PpapiMsg_PPBVideoDecoder_EndOfBitstreamACK,
s/ACK/Callback/g ?
I guess ACK is also used elsewhere in this file, but it seems like a strange
convention to me.  I defer to ppapi folks' judgement.

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppapi_messages.h...
ppapi/proxy/ppapi_messages.h:326: int32_t /* pp_error */)
pp_error doesn't name a type.
The only thing I can think of to use here is
/* value from pp_errors.h */

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppapi_messages.h...
ppapi/proxy/ppapi_messages.h:338: PP_Size /* dimensions of buffer */)
s/buffer/buffers/

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppapi_messages.h...
ppapi/proxy/ppapi_messages.h:341: int32_t /* picture id */)
s/picture/picture buffer/
(here and below)

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppapi_messages.h...
ppapi/proxy/ppapi_messages.h:342:
IPC_MESSAGE_ROUTED3(PpapiMsg_PPPVideoDecoder_PictureReady,
Any reason not to pass a PP_Picture_Dev?

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppapi_messages.h...
ppapi/proxy/ppapi_messages.h:344: int32_t /* picture id */,
ditto

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppapi_messages.h...
ppapi/proxy/ppapi_messages.h:345: int32_t /* bitstream id */)
s/bitstream/bitstream buffer/

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppapi_messages.h...
ppapi/proxy/ppapi_messages.h:350: uint32_t /* error id */)
PP_VideoDecodeError_Dev ?

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppapi_messages.h...
ppapi/proxy/ppapi_messages.h:945: std::vector<int32_t> /* config */,
PP_VideoConfigElement ?

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppapi_messages.h...
ppapi/proxy/ppapi_messages.h:947:
IPC_MESSAGE_ROUTED4(PpapiHostMsg_PPBVideoDecoder_Decode,
Pass PP_VideoBitstreamBuffer_Dev instead?

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppb_buffer_proxy.cc
File ppapi/proxy/ppb_buffer_proxy.cc (right):

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppb_buffer_proxy...
ppapi/proxy/ppb_buffer_proxy.cc:17: #include "ppapi/proxy/plugin_resource.h"
drop?

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppb_video_decode...
File ppapi/proxy/ppb_video_decoder_proxy.cc (right):

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppb_video_decode...
ppapi/proxy/ppb_video_decoder_proxy.cc:53: DCHECK(enter_context.succeeded());
ditto to comment in ppb_impl

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppb_video_decode...
ppapi/proxy/ppb_video_decoder_proxy.cc:61: HostResource host_context =
ppb_context->host_resource();
this doesn't seem to go anywhere... ;)

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppb_video_decode...
ppapi/proxy/ppb_video_decoder_proxy.cc:92: buffer_list.push_back(buffers[i]);
Can you not use the iterator-based vector ctor here?
(vector<T>::iterator is spec-guaranteed to be T* so you can just do
std::vector<PP_PictureBuffer_Dev> buffer_list(buffers, buffers + no_of_buffers);

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppb_video_decode...
ppapi/proxy/ppb_video_decoder_proxy.cc:192: // FIXME(brettw) handle bad
messages!
DCHECK instead of FIXME?

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppb_video_decode...
ppapi/proxy/ppb_video_decoder_proxy.cc:200: if (!dispatcher)
When can this fail?

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppb_video_decode...
ppapi/proxy/ppb_video_decoder_proxy.cc:270: if (!buffers.empty())
DCHECK instead?

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppb_video_decode...
File ppapi/proxy/ppb_video_decoder_proxy.h (right):

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppb_video_decode...
ppapi/proxy/ppb_video_decoder_proxy.h:41: void FlushACK();
Does proxy stuff not believe in "friend"ship?
(i.e. are these only public so that PPB_VD_P can see them?  if so does making it
a friend help?)

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppb_video_decode...
ppapi/proxy/ppb_video_decoder_proxy.h:45: virtual bool Init(
s/public/protected/

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppb_video_decode...
ppapi/proxy/ppb_video_decoder_proxy.h:53: class PPB_VideoDecoder_Proxy : public
InterfaceProxy {
None of the other .h files in this dir declare two classes.  Do you have an
opinion on unifying these two vs. not?

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppp_video_decode...
File ppapi/proxy/ppp_video_decoder_proxy.cc (right):

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppp_video_decode...
ppapi/proxy/ppp_video_decoder_proxy.cc:125: return handled;
DCHECK(handled)?

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppp_video_decode...
ppapi/proxy/ppp_video_decoder_proxy.cc:130: if (ppp_video_decoder_target()) {
here and below instead of having the whole body indented, you can reverse the
test and early-return if it fails.

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppp_video_decode...
ppapi/proxy/ppp_video_decoder_proxy.cc:130: if (ppp_video_decoder_target()) {
When will this fail?

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppp_video_decode...
ppapi/proxy/ppp_video_decoder_proxy.cc:166:
static_cast<PP_VideoDecodeError_Dev>(error));
hopefully can lose this cast by moving to more explicit types in the messages.

http://codereview.chromium.org/7545014/diff/4035/ppapi/shared_impl/video_deco...
File ppapi/shared_impl/video_decoder_impl.cc (right):

http://codereview.chromium.org/7545014/diff/4035/ppapi/shared_impl/video_deco...
ppapi/shared_impl/video_decoder_impl.cc:64:
DCHECK(bitstream_buffer_callbacks_.insert(
Did you intentionally s/CHECK/DCHECK/ in the move here?

http://codereview.chromium.org/7545014/diff/4035/ppapi/shared_impl/video_deco...
ppapi/shared_impl/video_decoder_impl.cc:69: if (flush_callback_.func != NULL)
Can this be a DCHECK instead?

http://codereview.chromium.org/7545014/diff/4035/ppapi/shared_impl/video_deco...
ppapi/shared_impl/video_decoder_impl.cc:74: if (reset_callback_.func != NULL)
ditto

http://codereview.chromium.org/7545014/diff/4035/ppapi/shared_impl/video_deco...
ppapi/shared_impl/video_decoder_impl.cc:101: if (!(current + 1))
I think you're missing a * here ;)

http://codereview.chromium.org/7545014/diff/4035/ppapi/shared_impl/video_deco...
ppapi/shared_impl/video_decoder_impl.cc:104:
out_configs->push_back(static_cast<int32>(*current));
Hopefully you can lose these casts if you switch to using vector<PP_VCE>
everywhere.

http://codereview.chromium.org/7545014/diff/4035/ppapi/shared_impl/video_deco...
ppapi/shared_impl/video_decoder_impl.cc:111: void
VideoDecoderImpl::SetGLES2Impl(
Given this has a single caller, I'd simply in-line it there (where a single 
DCHECK(!gles2_impl_ && !context3d_id_);
guarantees Init() is called no more than once).

http://codereview.chromium.org/7545014/diff/4035/ppapi/shared_impl/video_deco...
ppapi/shared_impl/video_decoder_impl.cc:119:
::webkit::ppapi::ResourceTracker::Get()->AddRefResource(resource);
Oh my goodness.
/me weeps

http://codereview.chromium.org/7545014/diff/4035/ppapi/shared_impl/video_deco...
File ppapi/shared_impl/video_decoder_impl.h (right):

http://codereview.chromium.org/7545014/diff/4035/ppapi/shared_impl/video_deco...
ppapi/shared_impl/video_decoder_impl.h:39: void
SetFlushCallback(PP_CompletionCallback callback);
Can these all be protected instead of public?

http://codereview.chromium.org/7545014/diff/4035/ppapi/shared_impl/video_deco...
ppapi/shared_impl/video_decoder_impl.h:41: void AddBitstreamBufferCallback(
Since the id is unique, I think Set is better than Add here.

http://codereview.chromium.org/7545014/diff/4035/webkit/plugins/ppapi/ppb_vid...
File webkit/plugins/ppapi/ppb_video_decoder_impl.cc (right):

http://codereview.chromium.org/7545014/diff/4035/webkit/plugins/ppapi/ppb_vid...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:63: if(!instance() ||
!::ppapi::VideoDecoderImpl::Init(context3d_id, config))
How can instance() be NULL, do you know?

http://codereview.chromium.org/7545014/diff/4035/webkit/plugins/ppapi/ppb_vid...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:67:
DCHECK(enter_context.succeeded());
I'm not entirely sure about this; it's only correct b/c the super-class Init()
would have returned false if its copy of this failed, but that seems brittle
without at least a comment documenting the dependency.

http://codereview.chromium.org/7545014/diff/4035/webkit/plugins/ppapi/ppb_vid...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:143: // TODO(fischman,vrk):
consider implications of already-outstanding callback.
This TODO (and its sibling below) don't belong here anymore.  If anywhere, they
should probably go in the shared impl's header above the declaration of the
Set*Callback() methods.

http://codereview.chromium.org/7545014/diff/4035/webkit/plugins/ppapi/ppb_vid...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:173:
::ppapi::VideoDecoderImpl::Destroy();
Is it obvious why this belongs after the 2 =NULL's above, as opposed to before
them?

http://codereview.chromium.org/7545014/diff/4035/webkit/plugins/ppapi/ppb_vid...
File webkit/plugins/ppapi/ppb_video_decoder_impl.h (right):

http://codereview.chromium.org/7545014/diff/4035/webkit/plugins/ppapi/ppb_vid...
webkit/plugins/ppapi/ppb_video_decoder_impl.h:60: virtual void Destroy()
OVERRIDE;
The section-heading comment is now out of date, since this is actually
overriding VDI, not PPB_VD_A.  Not sure if it's worth clarifying.

http://codereview.chromium.org/7545014/diff/4035/webkit/plugins/ppapi/ppb_vid...
webkit/plugins/ppapi/ppb_video_decoder_impl.h:75: // Initialize the underlying
decoder and return success status.
s/public/protected/?

http://codereview.chromium.org/7545014/diff/4035/webkit/plugins/ppapi/ppb_vid...
webkit/plugins/ppapi/ppb_video_decoder_impl.h:77: const PP_VideoConfigElement*
dec_config);
OVERRIDE

[vrk (LEFT CHROMIUM), 2011-08-03 19:04:29]

Phew, addressed most every comment.

fischman asked some questions that I have to look into still (will reply with a
follow-up email and/or CL).

PTAL!

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/host_dispatcher.cc
File ppapi/proxy/host_dispatcher.cc (right):

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/host_dispatcher....
ppapi/proxy/host_dispatcher.cc:216: allow_plugin_reentrancy_ = true;
On 2011/08/02 00:49:08, Ami Fischman wrote:
> Revert this file

Done.

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/plugin_resource.h
File ppapi/proxy/plugin_resource.h (right):

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/plugin_resource....
ppapi/proxy/plugin_resource.h:36: F(URLResponseInfo)\
On 2011/08/02 00:49:08, Ami Fischman wrote:
> extra space before backslash here (and on previous line).
> (otherwise you're just getting lucky that the next line has leading spaces :))

Done.

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppapi_messages.h...
ppapi/proxy/ppapi_messages.h:324:
IPC_MESSAGE_ROUTED2(PpapiMsg_PPBVideoDecoder_EndOfBitstreamACK,
On 2011/08/02 00:49:08, Ami Fischman wrote:
> s/ACK/Callback/g ?
> I guess ACK is also used elsewhere in this file, but it seems like a strange
> convention to me.  I defer to ppapi folks' judgement.

Yeah, I'm not a fan of ACK, either. 

brettw/piman: opinions?

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppapi_messages.h...
ppapi/proxy/ppapi_messages.h:326: int32_t /* pp_error */)
On 2011/08/02 00:49:08, Ami Fischman wrote:
> pp_error doesn't name a type.
> The only thing I can think of to use here is
> /* value from pp_errors.h */

I changed it to "error result for callback," a description rather than a type.
(I also plumbed it through the rest of the methods properly; previously I was
dropping the error value and replacing it with PP_OK.)

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppapi_messages.h...
ppapi/proxy/ppapi_messages.h:338: PP_Size /* dimensions of buffer */)
On 2011/08/02 00:49:08, Ami Fischman wrote:
> s/buffer/buffers/

Done.

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppapi_messages.h...
ppapi/proxy/ppapi_messages.h:341: int32_t /* picture id */)
On 2011/08/02 00:49:08, Ami Fischman wrote:
> s/picture/picture buffer/
> (here and below)

Done.

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppapi_messages.h...
ppapi/proxy/ppapi_messages.h:342:
IPC_MESSAGE_ROUTED3(PpapiMsg_PPPVideoDecoder_PictureReady,
On 2011/08/02 00:49:08, Ami Fischman wrote:
> Any reason not to pass a PP_Picture_Dev?

No. Done!

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppapi_messages.h...
ppapi/proxy/ppapi_messages.h:344: int32_t /* picture id */,
On 2011/08/02 00:49:08, Ami Fischman wrote:
> ditto

n/a w/ PP_Picture_Dev.

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppapi_messages.h...
ppapi/proxy/ppapi_messages.h:345: int32_t /* bitstream id */)
On 2011/08/02 00:49:08, Ami Fischman wrote:
> s/bitstream/bitstream buffer/

n/a w/ PP_Picture_Dev.

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppapi_messages.h...
ppapi/proxy/ppapi_messages.h:350: uint32_t /* error id */)
On 2011/08/02 00:49:08, Ami Fischman wrote:
> PP_VideoDecodeError_Dev ?

Done.

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppapi_messages.h...
ppapi/proxy/ppapi_messages.h:945: std::vector<int32_t> /* config */,
On 2011/08/02 00:49:08, Ami Fischman wrote:
> PP_VideoConfigElement ?

Done.

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppapi_messages.h...
ppapi/proxy/ppapi_messages.h:947:
IPC_MESSAGE_ROUTED4(PpapiHostMsg_PPBVideoDecoder_Decode,
On 2011/08/02 00:49:08, Ami Fischman wrote:
> Pass PP_VideoBitstreamBuffer_Dev instead?

I tried to do this, and I think it makes the plugin crash :) I'm guessing this
isn't quite as simple as the others because PP_VideoBitstreamBuffer_Dev has a
PP_Resource for one of its fields (representing the PP_Buffer). I'm not sure
though. I could try harder.

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppb_buffer_proxy.cc
File ppapi/proxy/ppb_buffer_proxy.cc (right):

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppb_buffer_proxy...
ppapi/proxy/ppb_buffer_proxy.cc:17: #include "ppapi/proxy/plugin_resource.h"
On 2011/08/02 00:49:08, Ami Fischman wrote:
> drop?

Done.

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppb_video_decode...
File ppapi/proxy/ppb_video_decoder_proxy.cc (right):

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppb_video_decode...
ppapi/proxy/ppb_video_decoder_proxy.cc:53: DCHECK(enter_context.succeeded());
On 2011/08/02 00:49:08, Ami Fischman wrote:
> ditto to comment in ppb_impl

n/a method gone!

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppb_video_decode...
ppapi/proxy/ppb_video_decoder_proxy.cc:61: HostResource host_context =
ppb_context->host_resource();
On 2011/08/02 00:49:08, Ami Fischman wrote:
> this doesn't seem to go anywhere... ;)

HAHA! Yeahhhhh whoops... there is no point in overriding this method. Deleted! 

(in the past there was a point for this, back when this was overriding the
Initialize method, and these values were going to be passed over IPC...)

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppb_video_decode...
ppapi/proxy/ppb_video_decoder_proxy.cc:92: buffer_list.push_back(buffers[i]);
On 2011/08/02 00:49:08, Ami Fischman wrote:
> Can you not use the iterator-based vector ctor here?
> (vector<T>::iterator is spec-guaranteed to be T* so you can just do
> std::vector<PP_PictureBuffer_Dev> buffer_list(buffers, buffers +
no_of_buffers);

Ahh yes, that is superior. Done!

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppb_video_decode...
ppapi/proxy/ppb_video_decoder_proxy.cc:192: // FIXME(brettw) handle bad
messages!
On 2011/08/02 00:49:08, Ami Fischman wrote:
> DCHECK instead of FIXME?

Done.

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppb_video_decode...
ppapi/proxy/ppb_video_decoder_proxy.cc:270: if (!buffers.empty())
On 2011/08/02 00:49:08, Ami Fischman wrote:
> DCHECK instead?

Done.

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppb_video_decode...
File ppapi/proxy/ppb_video_decoder_proxy.h (right):

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppb_video_decode...
ppapi/proxy/ppb_video_decoder_proxy.h:41: void FlushACK();
On 2011/08/02 00:49:08, Ami Fischman wrote:
> (i.e. are these only public so that PPB_VD_P can see them?  
Yes.
> if so does making it a friend help?)
Yes!
> Does proxy stuff not believe in "friend"ship?
I don't know! Since I don't know of any rule against this, I have made
PPB_VideoDecoder_Proxy a friend of VideoDecoder and made these methods private.

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppb_video_decode...
ppapi/proxy/ppb_video_decoder_proxy.h:45: virtual bool Init(
On 2011/08/02 00:49:08, Ami Fischman wrote:
> s/public/protected/

n/a removed the overrided method.

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppb_video_decode...
ppapi/proxy/ppb_video_decoder_proxy.h:53: class PPB_VideoDecoder_Proxy : public
InterfaceProxy {
On 2011/08/02 00:49:08, Ami Fischman wrote:
> None of the other .h files in this dir declare two classes.  Do you have an
> opinion on unifying these two vs. not?

Talked in person. Not unified but class def moved to .cc file.

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppp_video_decode...
File ppapi/proxy/ppp_video_decoder_proxy.cc (right):

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppp_video_decode...
ppapi/proxy/ppp_video_decoder_proxy.cc:125: return handled;
On 2011/08/02 00:49:08, Ami Fischman wrote:
> DCHECK(handled)?

Done.

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppp_video_decode...
ppapi/proxy/ppp_video_decoder_proxy.cc:130: if (ppp_video_decoder_target()) {
On 2011/08/02 00:49:08, Ami Fischman wrote:
> here and below instead of having the whole body indented, you can reverse the
> test and early-return if it fails.

Done.

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppp_video_decode...
ppapi/proxy/ppp_video_decoder_proxy.cc:166:
static_cast<PP_VideoDecodeError_Dev>(error));
On 2011/08/02 00:49:08, Ami Fischman wrote:
> hopefully can lose this cast by moving to more explicit types in the messages.

Done.

http://codereview.chromium.org/7545014/diff/4035/ppapi/shared_impl/video_deco...
File ppapi/shared_impl/video_decoder_impl.cc (right):

http://codereview.chromium.org/7545014/diff/4035/ppapi/shared_impl/video_deco...
ppapi/shared_impl/video_decoder_impl.cc:64:
DCHECK(bitstream_buffer_callbacks_.insert(
On 2011/08/02 00:49:08, Ami Fischman wrote:
> Did you intentionally s/CHECK/DCHECK/ in the move here?

noooooooooooooooooooooooooooo

CRAP and this is why my Release build wasn't working! Done and yeah, now Release
is working :P

http://codereview.chromium.org/7545014/diff/4035/ppapi/shared_impl/video_deco...
ppapi/shared_impl/video_decoder_impl.cc:69: if (flush_callback_.func != NULL)
On 2011/08/02 00:49:08, Ami Fischman wrote:
> Can this be a DCHECK instead?

Yes, done.

http://codereview.chromium.org/7545014/diff/4035/ppapi/shared_impl/video_deco...
ppapi/shared_impl/video_decoder_impl.cc:74: if (reset_callback_.func != NULL)
On 2011/08/02 00:49:08, Ami Fischman wrote:
> ditto

Done.

http://codereview.chromium.org/7545014/diff/4035/ppapi/shared_impl/video_deco...
ppapi/shared_impl/video_decoder_impl.cc:101: if (!(current + 1))
On 2011/08/02 00:49:08, Ami Fischman wrote:
> I think you're missing a * here ;)

No, sadly, I am just AN IDIOT! (omg sometimes I think dumb things, I BLAME JAVA)

Fixed to make more sense, though still not that fixed. Comment to add real
fixes.

http://codereview.chromium.org/7545014/diff/4035/ppapi/shared_impl/video_deco...
ppapi/shared_impl/video_decoder_impl.cc:104:
out_configs->push_back(static_cast<int32>(*current));
On 2011/08/02 00:49:08, Ami Fischman wrote:
> Hopefully you can lose these casts if you switch to using vector<PP_VCE>
> everywhere.

Done.

http://codereview.chromium.org/7545014/diff/4035/ppapi/shared_impl/video_deco...
ppapi/shared_impl/video_decoder_impl.cc:111: void
VideoDecoderImpl::SetGLES2Impl(
On 2011/08/02 00:49:08, Ami Fischman wrote:
> Given this has a single caller, I'd simply in-line it there (where a single 
> DCHECK(!gles2_impl_ && !context3d_id_);
> guarantees Init() is called no more than once).

Done.

http://codereview.chromium.org/7545014/diff/4035/ppapi/shared_impl/video_deco...
ppapi/shared_impl/video_decoder_impl.cc:119:
::webkit::ppapi::ResourceTracker::Get()->AddRefResource(resource);
On 2011/08/02 00:49:08, Ami Fischman wrote:
> Oh my goodness.
> /me weeps

I knowwwwwwwwwwwwwwwwww

But the ugliness is expected to go away when brettw DOes his TODO in
tracker_base.h to merge these classes! I added a TODO for us in tracker_base.h
to fix VideoDecoderImpl when that time comes.

http://codereview.chromium.org/7545014/diff/4035/ppapi/shared_impl/video_deco...
File ppapi/shared_impl/video_decoder_impl.h (right):

http://codereview.chromium.org/7545014/diff/4035/ppapi/shared_impl/video_deco...
ppapi/shared_impl/video_decoder_impl.h:39: void
SetFlushCallback(PP_CompletionCallback callback);
On 2011/08/02 00:49:08, Ami Fischman wrote:
> Can these all be protected instead of public?

Yes, for everything but CopyConfigsToVector. Done.

http://codereview.chromium.org/7545014/diff/4035/ppapi/shared_impl/video_deco...
ppapi/shared_impl/video_decoder_impl.h:41: void AddBitstreamBufferCallback(
On 2011/08/02 00:49:08, Ami Fischman wrote:
> Since the id is unique, I think Set is better than Add here.

Done.

http://codereview.chromium.org/7545014/diff/4035/webkit/plugins/ppapi/ppb_vid...
File webkit/plugins/ppapi/ppb_video_decoder_impl.cc (right):

http://codereview.chromium.org/7545014/diff/4035/webkit/plugins/ppapi/ppb_vid...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:67:
DCHECK(enter_context.succeeded());
On 2011/08/02 00:49:08, Ami Fischman wrote:
> I'm not entirely sure about this; it's only correct b/c the super-class Init()
> would have returned false if its copy of this failed, but that seems brittle
> without at least a comment documenting the dependency.

Agreed. To also address piman's concern w/ the double map lookup, I changed Init
to take a PPB_Context3D_API* in addition to PP_Resource (can't drop the
PP_Resource so that we keep resource tracking). Let me know what you think!

http://codereview.chromium.org/7545014/diff/4035/webkit/plugins/ppapi/ppb_vid...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:143: // TODO(fischman,vrk):
consider implications of already-outstanding callback.
On 2011/08/02 00:49:08, Ami Fischman wrote:
> This TODO (and its sibling below) don't belong here anymore.  If anywhere,
they
> should probably go in the shared impl's header above the declaration of the
> Set*Callback() methods.

Done.

http://codereview.chromium.org/7545014/diff/4035/webkit/plugins/ppapi/ppb_vid...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:173:
::ppapi::VideoDecoderImpl::Destroy();
On 2011/08/02 00:49:08, Ami Fischman wrote:
> Is it obvious why this belongs after the 2 =NULL's above, as opposed to before
> them?

Talked in person, done.

http://codereview.chromium.org/7545014/diff/4035/webkit/plugins/ppapi/ppb_vid...
File webkit/plugins/ppapi/ppb_video_decoder_impl.h (right):

http://codereview.chromium.org/7545014/diff/4035/webkit/plugins/ppapi/ppb_vid...
webkit/plugins/ppapi/ppb_video_decoder_impl.h:60: virtual void Destroy()
OVERRIDE;
On 2011/08/02 00:49:08, Ami Fischman wrote:
> The section-heading comment is now out of date, since this is actually
> overriding VDI, not PPB_VD_A.  Not sure if it's worth clarifying.

Kept it the same for now because I think a reference to PPB_VideoDecoder_API is
more helpful. Let me know if you disagree.

http://codereview.chromium.org/7545014/diff/4035/webkit/plugins/ppapi/ppb_vid...
webkit/plugins/ppapi/ppb_video_decoder_impl.h:75: // Initialize the underlying
decoder and return success status.
On 2011/08/02 00:49:08, Ami Fischman wrote:
> s/public/protected/?

Done.

http://codereview.chromium.org/7545014/diff/4035/webkit/plugins/ppapi/ppb_vid...
webkit/plugins/ppapi/ppb_video_decoder_impl.h:77: const PP_VideoConfigElement*
dec_config);
On 2011/08/02 00:49:08, Ami Fischman wrote:
> OVERRIDE

Done.

http://codereview.chromium.org/7545014/diff/3059/ppapi/proxy/ppb_video_decode...
File ppapi/proxy/ppb_video_decoder_proxy.cc (right):

http://codereview.chromium.org/7545014/diff/3059/ppapi/proxy/ppb_video_decode...
ppapi/proxy/ppb_video_decoder_proxy.cc:53: DCHECK(enter_context.succeeded());
On 2011/08/02 00:40:08, piman wrote:
> So, we do this enter of the context, which is a map lookup, twice. Isn't there
a
> way to pass through the Context3D* directly instead of the resource id ?

OK, I changed the Init function to take a PPB_Context3D_API* in addition to
PP_Resource. (The PP_Resource is still needed for resource tracking.)

http://codereview.chromium.org/7545014/diff/3059/ppapi/proxy/ppb_video_decode...
ppapi/proxy/ppb_video_decoder_proxy.cc:92: buffer_list.push_back(buffers[i]);
On 2011/08/02 00:40:08, piman wrote:
> You can write all this as:
> std::vector<PP_PictureBuffer_Dev> buffer_list(buffers, buffers+no_of_buffers);

Done.

http://codereview.chromium.org/7545014/diff/3059/ppapi/proxy/ppb_video_decode...
ppapi/proxy/ppb_video_decoder_proxy.cc:253: { id, buffer.host_resource(), size
};
On 2011/08/02 00:40:08, piman wrote:
> nit: Can't this fit on one line ? If yes, please remove spaces after { and
> before }.
> If not, the correct style is:
> PP_VideoBitstreamBuffer_Dev bitstream = {
>   A,
>   B,
>   C
> };

Done.

http://codereview.chromium.org/7545014/diff/3059/ppapi/proxy/ppb_video_decode...
ppapi/proxy/ppb_video_decoder_proxy.cc:289: CompletionCallback callback =
callback_factory_.NewOptionalCallback(
On 2011/08/02 00:40:08, piman wrote:
> Make this a NewRequiredCallback...

Done.

http://codereview.chromium.org/7545014/diff/3059/ppapi/proxy/ppb_video_decode...
ppapi/proxy/ppb_video_decoder_proxy.cc:295: result =
enter.object()->Flush(callback.pp_completion_callback());
On 2011/08/02 00:40:08, piman wrote:
> ... Call the host interface directly through target_interface() [you may want
to
> implement a ppb_video_decoder_target() like in other proxies] ...

Done.

http://codereview.chromium.org/7545014/diff/3059/ppapi/proxy/ppb_video_decode...
ppapi/proxy/ppb_video_decoder_proxy.cc:296: if (result !=
PP_OK_COMPLETIONPENDING)
On 2011/08/02 00:40:08, piman wrote:
> ... and you can skip this part !
> 
> This whole function can be as simple as:
> CompletionCallback callback =
>
callback_factory_.NewRequiredCallback(&PPB_VideoDecoder_Proxy::SendMsgFlushACKToPlugin,
> decoder);
> ppb_video_decoder_target()->Flush(decoder.host_resource(),
> callback.pp_completion_callback());
> 
> Same with several other functions here...

Neato! Done w/ all, thanks!

http://codereview.chromium.org/7545014/diff/3059/ppapi/proxy/ppp_video_decode...
File ppapi/proxy/ppp_video_decoder_proxy.h (right):

http://codereview.chromium.org/7545014/diff/3059/ppapi/proxy/ppp_video_decode...
ppapi/proxy/ppp_video_decoder_proxy.h:26: return reinterpret_cast<const
PPP_VideoDecoder_Dev*>(target_interface());
On 2011/08/02 00:40:08, piman wrote:
> reinterpret_cast->static_cast.

Done.

http://codereview.chromium.org/7545014/diff/3059/ppapi/shared_impl/DEPS
File ppapi/shared_impl/DEPS (right):

http://codereview.chromium.org/7545014/diff/3059/ppapi/shared_impl/DEPS#newcode6
ppapi/shared_impl/DEPS:6: "+webkit/plugins/ppapi",
On 2011/08/02 00:40:08, piman wrote:
> That seems wrong, why do you need this ?

Unfortunately needed for VideoDecoderImpl to be able to use
webkit::ppapi::ResourceTracker. Again, this dependency is no longer needed when
brettw merges the resource tracker objects into one.

http://codereview.chromium.org/7545014/diff/3059/ppapi/shared_impl/video_deco...
File ppapi/shared_impl/video_decoder_impl.cc (right):

http://codereview.chromium.org/7545014/diff/3059/ppapi/shared_impl/video_deco...
ppapi/shared_impl/video_decoder_impl.cc:64:
DCHECK(bitstream_buffer_callbacks_.insert(
On 2011/08/02 00:40:08, piman wrote:
> DCHECK(side effect) ??
> Very bad idea... The side effect won't execute in release...
> Instead: bool result = bitstream_buffer_callbacks_.insert(...).second;
> DCHECK(result).

Yeaaaaah, was supposed to be a CHECK, not a DCHECK! Really not a good thing to
mix up in this case. Fixed, thank you!!

(If you don't mind, I put it as a CHECK rather than DCHECK on result because if
this isn't happening then most likely the browser hangs in Release mode... as it
was doing for gles2. Working now though! :) )

http://codereview.chromium.org/7545014/diff/3059/ppapi/shared_impl/video_deco...
ppapi/shared_impl/video_decoder_impl.cc:98: // merge PP_VideoAttributeDictionary
and VideoAttributeKey.
On 2011/08/02 00:40:08, piman wrote:
> You can add COMPILE_ASSERT for those.

Moved this comment to PPB_VideoDecoder_Impl::Create where it is actually
relevant (here the values are copied with the same types now).

This TODO has been around for a long time; mind if I fix it in a different CL?

http://codereview.chromium.org/7545014/diff/3059/ppapi/shared_impl/video_deco...
ppapi/shared_impl/video_decoder_impl.cc:104:
out_configs->push_back(static_cast<int32>(*current));
On 2011/08/02 00:40:08, piman wrote:
> Is the static_cast needed ? enums should cast implicitly to ints.

Done.

http://codereview.chromium.org/7545014/diff/3059/ppapi/shared_impl/video_deco...
ppapi/shared_impl/video_decoder_impl.cc:121:
::pp::proxy::PluginResourceTracker::GetInstance()->AddRefResource(resource);
On 2011/08/02 00:40:08, piman wrote:
> That sort of logic is kinda bad. Why not pass the resource tracker in
parameter
> to the VideoDecoderImpl instead of in_renderer_process ?

Originally that is what I intended to do, but then I realized that the
AddRefResource/UnrefResource are not part of the ppapi::TrackerBase interface.
Even worse, the function name for releasing a ref is different in either case
(UnrefResource vs ReleaseResource) so adding this to the TrackerAPI would be a
big change... so I did this lamer thing instead.

What do you think is a better route to go?

[piman, 2011-08-03 19:12:44]

The only thing that bugs me is the craziness with
VideoDecoderImpl::Ref/UnrefResource
Brett do you have an opinion about how to do this more cleanly ?
The rest LGTM.

[brettw, 2011-08-03 19:19:57]

http://codereview.chromium.org/7545014/diff/11008/ppapi/shared_impl/video_dec...
File ppapi/shared_impl/video_decoder_impl.cc (right):

http://codereview.chromium.org/7545014/diff/11008/ppapi/shared_impl/video_dec...
ppapi/shared_impl/video_decoder_impl.cc:13: #include
"webkit/plugins/ppapi/resource_tracker.h"
The point of the shared_impl directory is that it's used by the proxy in the
plugin process, and the webkit implementation in the renderer process. In future
iterations, the webkit code won't exist in the plugin process at all, so your
code won't link.

I've wanted to unify resource tracking so we don't have to have separate code
here, but for now you need to have separate code. In other places, we've had a
pure virtual function implemented in the proper way in the _proxy and _impl
objects appropriate for the current process.

Be sure to also revert the DEPS change.

[Ami GONE FROM CHROMIUM, 2011-08-03 20:25:53]

LGTM modulo:
- question about when the *_instance() and *_target() methods can fail; can wait
till post-commit
- nits below

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppapi_messages.h...
ppapi/proxy/ppapi_messages.h:947:
IPC_MESSAGE_ROUTED4(PpapiHostMsg_PPBVideoDecoder_Decode,
On 2011/08/03 19:04:30, Victoria Kirst wrote:
> On 2011/08/02 00:49:08, Ami Fischman wrote:
> > Pass PP_VideoBitstreamBuffer_Dev instead?
> 
> I tried to do this, and I think it makes the plugin crash :) I'm guessing this
> isn't quite as simple as the others because PP_VideoBitstreamBuffer_Dev has a
> PP_Resource for one of its fields (representing the PP_Buffer). I'm not sure
> though. I could try harder.

I think it's worth trying harder not so much because it's a so-much-sexier API,
but because it'd be good to know what the crasher is.

http://codereview.chromium.org/7545014/diff/11008/ppapi/proxy/ppapi_messages....
ppapi/proxy/ppapi_messages.h:333: 
But these aren't errors most of the time (they're PP_OK).
If anything s/error result/status/, but I still like "value from pp_errors.h"
best.  Your call.

http://codereview.chromium.org/7545014/diff/11008/ppapi/proxy/ppb_video_decod...
File ppapi/proxy/ppb_video_decoder_proxy.h (right):

http://codereview.chromium.org/7545014/diff/11008/ppapi/proxy/ppb_video_decod...
ppapi/proxy/ppb_video_decoder_proxy.h:68: int32_t id, int32_t pp_error);
elsewhere these are called "result" and I think that's better.
pp_error only works for me for when you know you have a not-PP_OK value in hand.

http://codereview.chromium.org/7545014/diff/11008/ppapi/shared_impl/video_dec...
File ppapi/shared_impl/video_decoder_impl.cc (right):

http://codereview.chromium.org/7545014/diff/11008/ppapi/shared_impl/video_dec...
ppapi/shared_impl/video_decoder_impl.cc:52: // TODO(fischman,vrk): consider
implications of already-outstanding callback.
Instead of future-considering it would you accept just DCHECKing it's not
already set?
(here and below)

http://codereview.chromium.org/7545014/diff/11008/ppapi/shared_impl/video_dec...
ppapi/shared_impl/video_decoder_impl.cc:101: while (*current && *current !=
PP_VIDEOATTR_DICTIONARY_TERMINATOR) {
The first *current isn't useful.  You want either the bare "current" or nothing.

http://codereview.chromium.org/7545014/diff/11008/ppapi/shared_impl/video_dec...
ppapi/shared_impl/video_decoder_impl.cc:102: if (*(current + 1) ==
PP_VIDEOATTR_DICTIONARY_TERMINATOR)
No, I think you want to drop this test entirely.

http://codereview.chromium.org/7545014/diff/11008/webkit/plugins/ppapi/ppb_vi...
File webkit/plugins/ppapi/ppb_video_decoder_impl.cc (right):

http://codereview.chromium.org/7545014/diff/11008/webkit/plugins/ppapi/ppb_vi...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:71:
!::ppapi::VideoDecoderImpl::Init(context3d_id, context3d, config))
need braces now that if test is 2 lines.

[brettw, 2011-08-03 20:33:13]

http://codereview.chromium.org/7545014/diff/11008/ppapi/proxy/ppp_video_decod...
File ppapi/proxy/ppp_video_decoder_proxy.cc (right):

http://codereview.chromium.org/7545014/diff/11008/ppapi/proxy/ppp_video_decod...
ppapi/proxy/ppp_video_decoder_proxy.cc:129: if (!ppp_video_decoder_target())
You don't need to check this, the proxy won't create your object if there's no
target for this API. So I'm just remove all of them.

I don't know what Ami is talking about with "_instance() functions", can you be
more specific?

[Ami GONE FROM CHROMIUM, 2011-08-03 20:37:36]

http://codereview.chromium.org/7545014/diff/11008/ppapi/proxy/ppp_video_decod...
File ppapi/proxy/ppp_video_decoder_proxy.cc (right):

http://codereview.chromium.org/7545014/diff/11008/ppapi/proxy/ppp_video_decod...
ppapi/proxy/ppp_video_decoder_proxy.cc:129: if (!ppp_video_decoder_target())
On 2011/08/03 20:33:14, brettw wrote:
> I don't know what Ami is talking about with "_instance() functions", can you
be
> more specific?

I meant dispatcher-for-instance, not instance.  Specifically,
ppb_video_decoder_proxy.cc contains:
 220   PluginDispatcher* dispatcher =
PluginDispatcher::GetForInstance(instance);
 221   if (!dispatcher)
 222     return 0;

When might that (!dispatcher) evaluate to true?

[brettw, 2011-08-03 20:39:03]

http://codereview.chromium.org/7545014/diff/11008/ppapi/proxy/ppp_video_decod...
File ppapi/proxy/ppp_video_decoder_proxy.cc (right):

http://codereview.chromium.org/7545014/diff/11008/ppapi/proxy/ppp_video_decod...
ppapi/proxy/ppp_video_decoder_proxy.cc:129: if (!ppp_video_decoder_target())
Oh yeah, we do need to check those cases. The user might pass an invalid
instance.

[vrk (LEFT CHROMIUM), 2011-08-03 22:05:22]

Thanks everyone for the reviews!

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppb_video_decode...
File ppapi/proxy/ppb_video_decoder_proxy.cc (right):

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppb_video_decode...
ppapi/proxy/ppb_video_decoder_proxy.cc:200: if (!dispatcher)
On 2011/08/02 00:49:08, Ami Fischman wrote:
> When can this fail?

brettw addressed; I added comment. Done.

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppp_video_decode...
File ppapi/proxy/ppp_video_decoder_proxy.cc (right):

http://codereview.chromium.org/7545014/diff/4035/ppapi/proxy/ppp_video_decode...
ppapi/proxy/ppp_video_decoder_proxy.cc:130: if (ppp_video_decoder_target()) {
On 2011/08/02 00:49:08, Ami Fischman wrote:
> When will this fail?

Won't! Deleted here & everywhere.

http://codereview.chromium.org/7545014/diff/4035/webkit/plugins/ppapi/ppb_vid...
File webkit/plugins/ppapi/ppb_video_decoder_impl.cc (right):

http://codereview.chromium.org/7545014/diff/4035/webkit/plugins/ppapi/ppb_vid...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:63: if(!instance() ||
!::ppapi::VideoDecoderImpl::Init(context3d_id, config))
On 2011/08/02 00:49:08, Ami Fischman wrote:
> How can instance() be NULL, do you know?

It appears it can't be -- when ResourceCreationImpl calls
PPB_VideoDecoder_Impl::Create, it passes as an instance its own instance_ field,
ignoring what is passed into it? And that instance_ field has been vetted via
another method in ResourceCreationImpl. Tentatively removing the check...

brettw: does this sound right?

http://codereview.chromium.org/7545014/diff/11008/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

http://codereview.chromium.org/7545014/diff/11008/ppapi/proxy/ppapi_messages....
ppapi/proxy/ppapi_messages.h:333: int32_t /* error result for callback */)
On 2011/08/03 20:25:53, Ami Fischman wrote:
> But these aren't errors most of the time (they're PP_OK).
> If anything s/error result/status/, but I still like "value from pp_errors.h"
> best.  Your call.

Talked in person. Done.

http://codereview.chromium.org/7545014/diff/11008/ppapi/proxy/ppb_video_decod...
File ppapi/proxy/ppb_video_decoder_proxy.h (right):

http://codereview.chromium.org/7545014/diff/11008/ppapi/proxy/ppb_video_decod...
ppapi/proxy/ppb_video_decoder_proxy.h:68: int32_t id, int32_t pp_error);
On 2011/08/03 20:25:53, Ami Fischman wrote:
> elsewhere these are called "result" and I think that's better.
> pp_error only works for me for when you know you have a not-PP_OK value in
hand.

Done.

http://codereview.chromium.org/7545014/diff/11008/ppapi/proxy/ppp_video_decod...
File ppapi/proxy/ppp_video_decoder_proxy.cc (right):

http://codereview.chromium.org/7545014/diff/11008/ppapi/proxy/ppp_video_decod...
ppapi/proxy/ppp_video_decoder_proxy.cc:129: if (!ppp_video_decoder_target())
On 2011/08/03 20:33:14, brettw wrote:
> You don't need to check this, the proxy won't create your object if there's no
> target for this API. So I'm just remove all of them.
> 
> I don't know what Ami is talking about with "_instance() functions", can you
be
> more specific?

Hooray! Done for this and in the pp*b* version.

http://codereview.chromium.org/7545014/diff/11008/ppapi/proxy/ppp_video_decod...
ppapi/proxy/ppp_video_decoder_proxy.cc:129: if (!ppp_video_decoder_target())
On 2011/08/03 20:39:03, brettw wrote:
> Oh yeah, we do need to check those cases. The user might pass an invalid
> instance.

SGTM, though I think in PPB_VideoDecoder_Impl::Create
(ppb_video_decoder_impl.cc) the instance() check is superfluous? Can you take a
look at that case?

http://codereview.chromium.org/7545014/diff/11008/ppapi/shared_impl/video_dec...
File ppapi/shared_impl/video_decoder_impl.cc (right):

http://codereview.chromium.org/7545014/diff/11008/ppapi/shared_impl/video_dec...
ppapi/shared_impl/video_decoder_impl.cc:13: #include
"webkit/plugins/ppapi/resource_tracker.h"
On 2011/08/03 19:19:57, brettw wrote:
> The point of the shared_impl directory is that it's used by the proxy in the
> plugin process, and the webkit implementation in the renderer process. In
future
> iterations, the webkit code won't exist in the plugin process at all, so your
> code won't link.
> 
> I've wanted to unify resource tracking so we don't have to have separate code
> here, but for now you need to have separate code. In other places, we've had a
> pure virtual function implemented in the proper way in the _proxy and _impl
> objects appropriate for the current process.
> 
> Be sure to also revert the DEPS change.

Using the pure virtual methods instead and DEPS reverted. Thanks!

http://codereview.chromium.org/7545014/diff/11008/ppapi/shared_impl/video_dec...
ppapi/shared_impl/video_decoder_impl.cc:52: // TODO(fischman,vrk): consider
implications of already-outstanding callback.
On 2011/08/03 20:25:53, Ami Fischman wrote:
> Instead of future-considering it would you accept just DCHECKing it's not
> already set?
> (here and below)

SGTM. Done!

http://codereview.chromium.org/7545014/diff/11008/ppapi/shared_impl/video_dec...
ppapi/shared_impl/video_decoder_impl.cc:101: while (*current && *current !=
PP_VIDEOATTR_DICTIONARY_TERMINATOR) {
On 2011/08/03 20:25:53, Ami Fischman wrote:
> The first *current isn't useful.  You want either the bare "current" or
nothing.

Dang it now I'm adding stars to everything!! Must have added that star when
trying to add the star below, reverted!

http://codereview.chromium.org/7545014/diff/11008/ppapi/shared_impl/video_dec...
ppapi/shared_impl/video_decoder_impl.cc:102: if (*(current + 1) ==
PP_VIDEOATTR_DICTIONARY_TERMINATOR)
On 2011/08/03 20:25:53, Ami Fischman wrote:
> No, I think you want to drop this test entirely.

Hahaha yeah. Removed.

http://codereview.chromium.org/7545014/diff/11008/webkit/plugins/ppapi/ppb_vi...
File webkit/plugins/ppapi/ppb_video_decoder_impl.cc (right):

http://codereview.chromium.org/7545014/diff/11008/webkit/plugins/ppapi/ppb_vi...
webkit/plugins/ppapi/ppb_video_decoder_impl.cc:71:
!::ppapi::VideoDecoderImpl::Init(context3d_id, context3d, config))
On 2011/08/03 20:25:53, Ami Fischman wrote:
> need braces now that if test is 2 lines.

Removed !instance() check, back to one-liner.

[piman, 2011-08-03 23:27:34]

LGTM

[brettw, 2011-08-04 19:47:30]

http://codereview.chromium.org/7545014/diff/10017/ppapi/proxy/ppb_video_decod...
File ppapi/proxy/ppb_video_decoder_proxy.cc (right):

http://codereview.chromium.org/7545014/diff/10017/ppapi/proxy/ppb_video_decod...
ppapi/proxy/ppb_video_decoder_proxy.cc:119: new
PpapiHostMsg_PPBVideoDecoder_AssignPictureBuffers(
Wrapped lines should be indented 4 spaces.

http://codereview.chromium.org/7545014/diff/10017/ppapi/proxy/ppp_video_decod...
File ppapi/proxy/ppp_video_decoder_proxy.cc (right):

http://codereview.chromium.org/7545014/diff/10017/ppapi/proxy/ppp_video_decod...
ppapi/proxy/ppp_video_decoder_proxy.cc:130: decoder.instance(),
decoder.host_resource(), req_num_of_bufs, dimensions);
The use of host_resource() in these functions is wrong. A resource ID in the
host (renderer) is not the same as a resource ID in the plugin. At the proxy
layer you have to convert. Most of this is done for you already. The thunk layer
calls into your object (which does the plugin PP_Resource lookup), and then you
call host_resource which gives you the renderer's ID.

In this case, you're passing a host resource to the plugin code, which will be
garbage. You need to call
PluginResourceTracker::GetInstance()->PluginResourceForHostResource

(This is the reason the HostResource stuff may seem a bit cumbersome to use.
It's deliberately this way so you have to be verbose and say "host" to get a
PP_Resource out, which should hopefully make you think about whether the guy
using that value is expecting a PP_Resource in the context of the host or for
the plugin.)

[vrk (LEFT CHROMIUM), 2011-08-04 21:17:09]

brettw: PTAL!

http://codereview.chromium.org/7545014/diff/10017/ppapi/proxy/ppb_video_decod...
File ppapi/proxy/ppb_video_decoder_proxy.cc (right):

http://codereview.chromium.org/7545014/diff/10017/ppapi/proxy/ppb_video_decod...
ppapi/proxy/ppb_video_decoder_proxy.cc:119: new
PpapiHostMsg_PPBVideoDecoder_AssignPictureBuffers(
On 2011/08/04 19:47:30, brettw wrote:
> Wrapped lines should be indented 4 spaces.

Done.

http://codereview.chromium.org/7545014/diff/10017/ppapi/proxy/ppp_video_decod...
File ppapi/proxy/ppp_video_decoder_proxy.cc (right):

http://codereview.chromium.org/7545014/diff/10017/ppapi/proxy/ppp_video_decod...
ppapi/proxy/ppp_video_decoder_proxy.cc:130: decoder.instance(),
decoder.host_resource(), req_num_of_bufs, dimensions);
On 2011/08/04 19:47:30, brettw wrote:
> The use of host_resource() in these functions is wrong. A resource ID in the
> host (renderer) is not the same as a resource ID in the plugin. At the proxy
> layer you have to convert. Most of this is done for you already. The thunk
layer
> calls into your object (which does the plugin PP_Resource lookup), and then
you
> call host_resource which gives you the renderer's ID.
> 
> In this case, you're passing a host resource to the plugin code, which will be
> garbage. You need to call
> PluginResourceTracker::GetInstance()->PluginResourceForHostResource
> 
> (This is the reason the HostResource stuff may seem a bit cumbersome to use.
> It's deliberately this way so you have to be verbose and say "host" to get a
> PP_Resource out, which should hopefully make you think about whether the guy
> using that value is expecting a PP_Resource in the context of the host or for
> the plugin.)

Ahhh good catch!! Fixed and thanks for pointing that out!

The gles2 plugin didn't catch that because we were ignoring the passed in
PP_Resource... so I also modified gles2.cc to assert that the PP_Resource given
to us from the callbacks matches the one for the resource we're using. In the
future it will catch that mistake!

[brettw, 2011-08-05 23:05:14]

LGTM, sorry for the delay.

[commit-bot: I haz the power, 2011-08-06 03:24:01]

Change committed as 95724