Implement Pepper PPB_VideoDecoder interface.

content/renderer/pepper/pepper_video_decoder_host.h

+// Copyright (c) 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CONTENT_RENDERER_PEPPER_PEPPER_VIDEO_DECODER_HOST_H_
+#define CONTENT_RENDERER_PEPPER_PEPPER_VIDEO_DECODER_HOST_H_
+
+#include <map>
+#include <vector>
+
+#include "base/basictypes.h"
+#include "base/containers/hash_tables.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/scoped_vector.h"
+#include "content/common/content_export.h"
+#include "gpu/command_buffer/common/mailbox.h"
+#include "media/video/video_decode_accelerator.h"
+#include "ppapi/c/pp_codecs.h"
+#include "ppapi/host/host_message_context.h"
+#include "ppapi/host/resource_host.h"
+#include "ppapi/proxy/resource_message_params.h"
+
+namespace base {
+class SharedMemory;
+}
+
+namespace content {
+
+class PPB_Graphics3D_Impl;
+class RendererPpapiHost;
+class RenderViewImpl;
+
+class CONTENT_EXPORT PepperVideoDecoderHost
+    : public ppapi::host::ResourceHost,
+      public media::VideoDecodeAccelerator::Client {
+ public:
+  PepperVideoDecoderHost(RendererPpapiHost* host,
+                         PP_Instance instance,
+                         PP_Resource resource);
+  virtual ~PepperVideoDecoderHost();
+
+ private:
+  struct PendingDecode {
+    PendingDecode(uint32_t shm_id,
+                  const ppapi::host::ReplyMessageContext& reply_context);
+    ~PendingDecode();
+
+    uint32_t shm_id;
+    ppapi::host::ReplyMessageContext reply_context;
+  };
+
+  virtual int32_t OnResourceMessageReceived(
+      const IPC::Message& msg,
+      ppapi::host::HostMessageContext* context) OVERRIDE;

[Ami GONE FROM CHROMIUM, 2014/05/28 20:50:56]

is nice to include for each batch of OVERRIDEs a comment saying where the
interface comes from (like you have at l.57 below).

Here in particular I'm surprised to see an OVERRIDE in a "private:" section. 
The base being overridden is protected: so giving it a different access class
here is strange to me.  Is there a reason to do so?

[dmichael (off chromium), 2014/05/28 21:00:02]

IMO, methods should always be protected at the highest level possible. It makes
it clearer that this method is never called on this class directly, and doing so
might be a mistake. This has come up on chromium-dev before, and while there's
not exactly consensus, a lot of folks fall on this side of the debate. darin@
expressed it well:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/CwzjcWd9cYM/TcUUl...

[bbudge, 2014/05/29 00:03:34]

Added comment.

+
+  // media::VideoDecodeAccelerator::Client implementation.
+  virtual void ProvidePictureBuffers(uint32 requested_num_of_buffers,
+                                     const gfx::Size& dimensions,
+                                     uint32 texture_target) OVERRIDE;
+  virtual void DismissPictureBuffer(int32 picture_buffer_id) OVERRIDE;
+  virtual void PictureReady(const media::Picture& picture) OVERRIDE;
+  virtual void NotifyError(media::VideoDecodeAccelerator::Error error) OVERRIDE;
+  virtual void NotifyFlushDone() OVERRIDE;
+  virtual void NotifyEndOfBitstreamBuffer(int32 bitstream_buffer_id) OVERRIDE;
+  virtual void NotifyResetDone() OVERRIDE;
+
+  int32_t OnHostMsgInitialize(ppapi::host::HostMessageContext* context,
+                              const ppapi::HostResource& graphics_context,
+                              PP_VideoProfile profile,
+                              bool allow_software_fallback);
+  int32_t OnHostMsgGetShm(ppapi::host::HostMessageContext* context,
+                          uint32_t shm_id,
+                          uint32_t shm_size);
+  int32_t OnHostMsgDecode(ppapi::host::HostMessageContext* context,
+                          uint32_t shm_id,
+                          uint32_t size);
+  int32_t OnHostMsgAssignTextures(ppapi::host::HostMessageContext* context,
+                                  const PP_Size& size,
+                                  const std::vector<uint32_t>& texture_ids);
+  int32_t OnHostMsgRecyclePicture(ppapi::host::HostMessageContext* context,
+                                  uint32_t picture_id);
+  int32_t OnHostMsgFlush(ppapi::host::HostMessageContext* context);
+  int32_t OnHostMsgReset(ppapi::host::HostMessageContext* context);
+
+  void DoDecode(ppapi::host::HostMessageContext* context,

[Ami GONE FROM CHROMIUM, 2014/05/28 20:50:56]

Short method, single caller, and no doco make me wonder: why is this a separate
function?

[bbudge, 2014/05/29 00:03:34]

Yeah, left over from an earlier CL. Removed.

+                uint32_t shm_id,
+                uint32_t size);
+  void RequestTextures(uint32 num_textures,

[Ami GONE FROM CHROMIUM, 2014/05/28 20:50:56]

ditto but more so.

[bbudge, 2014/05/29 00:03:34]

Done.

+                       const gfx::Size& dimensions,
+                       uint32 texture_target);
+
+  // Non-owning pointer.
+  RendererPpapiHost* renderer_ppapi_host_;
+
+  scoped_ptr<media::VideoDecodeAccelerator> decoder_;
+
+  scoped_refptr<PPB_Graphics3D_Impl> graphics3d_;
+  ScopedVector<base::SharedMemory> shm_buffers_;

[Ami GONE FROM CHROMIUM, 2014/05/28 20:50:56]

I think it's the case that the shm_id variable that shows up elsewhere is an
index into this vector; would be nice to doco if true.

[bbudge, 2014/05/29 00:03:34]

Done.

+
+  // Tracks buffers that are in use by the decoder.

[Ami GONE FROM CHROMIUM, 2014/05/28 20:50:56]

Not clear what the contents of the vector are (and whether index is same as
shm_id / same as shm_buffers_).

If this is a parallel vector to shm_buffers_ would it make more sense to make
them as single vector of pairs or a struct?  Even if not that, would be nice to
put them closer together visually.

[bbudge, 2014/05/29 00:03:34]

Yes, this is parallel to shm_buffers_. I'm trying to avoid having to define a
type that can fit in a vector and own SharedMemory.
I use uint8_t to avoid vector<bool>.
How about I make these adjacent and add a comment?

+  std::vector<uint8_t> shm_buffer_busy_;
+  // Number of Decode calls made so far, to generate a uid for each decode.
+  // This number will overflow after 414 days, assuming 60Hz video.

[Ami GONE FROM CHROMIUM, 2014/05/28 20:50:56]

I think this sentence is saying "This code will trigger undefined behavior after
414 days" which seems like a bad time.
Would you mind doing any of:
1) making it a uint32 so you could s/overflow/wrap-around/
2) CHECKing for overflow
3) changing to int64_t
4) implement wrap-around manually by testing the value before/after the ++.

[bbudge, 2014/05/29 00:03:34]

These pass through the decoder as int32_t's so that forces my hand. I pick 4,
wrapping, which allows us to play indefinitely.
I also added a comment that this counts in unison with the resource.
Done.

+  int32_t num_decodes_;
+  // Maps decode uid to PendingDecode info.
+  typedef base::hash_map<int32_t, PendingDecode> PendingDecodeMap;
+  PendingDecodeMap pending_decodes_;
+
+  ppapi::host::ReplyMessageContext flush_reply_context_;
+  ppapi::host::ReplyMessageContext reset_reply_context_;
+
+  bool initialized_;
+
+  DISALLOW_COPY_AND_ASSIGN(PepperVideoDecoderHost);
+};
+
+}  // namespace content
+
+#endif  // CONTENT_RENDERER_PEPPER_PEPPER_VIDEO_DECODER_HOST_H_