Implement video renderer based on VideoDecode API.

remoting/client/plugin/pepper_video_renderer_3d.h

+// Copyright 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 REMOTING_CLIENT_PLUGIN_PEPPER_VIDEO_RENDERER_3D_H_
+#define REMOTING_CLIENT_PLUGIN_PEPPER_VIDEO_RENDERER_3D_H_
+
+#include <deque>
+#include <string>
+
+#include "base/basictypes.h"
+#include "base/callback.h"
+#include "base/memory/scoped_ptr.h"
+#include "ppapi/cpp/graphics_3d.h"
+#include "ppapi/cpp/instance_handle.h"
+#include "ppapi/cpp/video_decoder.h"
+#include "ppapi/lib/gl/include/GLES2/gl2.h"

[Wez, 2014/12/24 17:11:15]

nit: It's really unfortunate to have to pollute the calling code's namespace
with a load of PPAPI stuff, especially GL. Easiest way to avoid that would be to
hide the implementation details of the 3D renderer behind a Create() function,
so that all these includes end up in the cc file. WDYT?

[Sergey Ulanov, 2015/01/06 01:16:33]

This header was necessary only for GL[u]int types. Removed this #include and
replaced GL types with unsigned int and int.

+#include "ppapi/utility/completion_callback_factory.h"
+#include "remoting/client/chromoting_stats.h"
+#include "remoting/client/plugin/pepper_video_renderer.h"
+#include "third_party/webrtc/modules/desktop_capture/desktop_geometry.h"
+#include "third_party/webrtc/modules/desktop_capture/desktop_region.h"
+
+struct PPB_OpenGLES2;
+
+namespace remoting {
+
+// PepperVideoRenderer that uses PPB_VideoDecoder interface for video decoding

[Wez, 2014/12/24 17:11:15]

nit: "... uses the PPB_VideoDecoder interface for ..." or "... uses
pp:VideoDecode for ..."

[Sergey Ulanov, 2015/01/06 01:16:33]

Done.

+// and Graphics3D for rendering.
+class PepperVideoRenderer3D : public PepperVideoRenderer {
+ public:
+  explicit PepperVideoRenderer3D();

[Wez, 2014/12/24 17:11:15]

nit: No need for explicit here.

[Sergey Ulanov, 2015/01/06 01:16:33]

Done.

+  ~PepperVideoRenderer3D() override;
+
+  // PepperVideoRenderer interface.
+  bool Initialize(pp::Instance* instance,
+                  const ClientContext& context,
+                  EventHandler* event_handler) override;
+  void OnViewChanged(const pp::View& view) override;
+  void OnSessionConfig(const protocol::SessionConfig& config) override;
+  ChromotingStats* GetStats() override;
+  void ProcessVideoPacket(scoped_ptr<VideoPacket> packet,
+                          const base::Closure& done) override;
+
+ private:
+  class PendingPacket;
+
+  struct FrameDecodeTimer {

[Wez, 2014/12/24 17:11:15]

nit: This isn't a timer in the base::Timer sense, but rather a record of the
decode start time, so perhaps call it FrameDecodeTimestamp?

[Sergey Ulanov, 2015/01/06 01:16:34]

Done.

+    FrameDecodeTimer(uint32_t frame_id, base::TimeTicks decode_started_time);
+    uint32_t frame_id;
+    base::TimeTicks decode_started_time;
+  };
+
+  void OnInitialized(int32_t result);
+  void DoDecode();
+  void OnDecodeDone(int32_t result);
+  void DoGetPicture();
+  void OnPictureReady(int32_t result, PP_VideoPicture picture);
+  void RecyclePictures(bool keep_first);
+  void DoPaint();
+  void OnPaintDone(int32_t result);

[Wez, 2014/12/24 17:11:16]

nit: Break the members above into groups of logically related functions, e.g.
those involved in receiving a picture & painting it, those involved in doing
decoding, etc, with one-liner comments on each block?

It's not clear from the names what DoDecode(), DoGetPicture() or DoPaint()do, in
particular why they need "Do" in front - why is DoDecode() not DecodePacket(),
DoGetPicture() not simply Get[Next]Picture(), etc?

[Sergey Ulanov, 2015/01/06 01:16:34]

Done.
Renamed all Do*() methods.

+
+  void EnsureProgramForTexture(uint32_t texture_target);
+  void CreateProgram(const char* vertex_shader, const char* fragment_shader);
+  void CreateShaderProgram(int type, const char* source);
+  void CheckGLError();

[Wez, 2014/12/24 17:11:16]

nit: Suggest one-line comments be added for each of these methods, e.g. to
clarify that CreateProgram() creates a render program in |shader_program_|, that
includes the necessary shader programs.

[Sergey Ulanov, 2015/01/06 01:16:33]

Done.

+
+  EventHandler* event_handler_;
+
+  pp::Graphics3D graphics_;
+  const PPB_OpenGLES2* gles2_if_;
+  pp::VideoDecoder video_decoder_;
+
+  webrtc::DesktopSize frame_size_;
+  webrtc::DesktopVector frame_dpi_;
+  webrtc::DesktopRegion desktop_shape_;
+
+  int view_width_;
+  int view_height_;

[Wez, 2014/12/24 17:11:16]

nit: Why not store these in a DesktopSize, for consistency?

[Sergey Ulanov, 2015/01/06 01:16:33]

Done.

+
+  ChromotingStats stats_;
+  int64 latest_sequence_number_;
+
+  bool initialized_;

[Wez, 2014/12/24 17:11:15]

nit: Could you derive |initialized_| from something higher level like
|!graphics_.is_null()| and have an initialized() getter rather than a member
variable?

[Sergey Ulanov, 2015/01/06 01:16:34]

No. VideoDecoder initialization is asynchronous and we want to hold off calling
Decode() until initialization is finished - see DoDecode(). Renamed to
initialization_finished_

+  bool decode_pending_;
+  bool get_picture_pending_;
+  bool paint_pending_;
+
+  uint32_t last_frame_id_;

[Wez, 2014/12/24 17:11:15]

nit: Be consistent "last" here versus "latest" above.

Why do we need both a sequence number _and_ a frame-Id? Could we get away with
just one of the two?

[Sergey Ulanov, 2015/01/06 01:16:34]

client_sequence_number is a misnomer - it's actually an input event timestamp we
send to the host with input events. It may not change between consecutive
frames. frame_id is used to identify the frames we pass to pp::VideoDecoder, and
we want it to be unique for each frame.

Renamed latest_sequence_number_to last_input_event_timestamp_ in this CL. Will
rename it on the protocol level in a separate CL.

[Wez, 2015/01/06 22:17:10]

nit: Prefer "latest" to "last" since it's more obvious that it means "most
recent" versus "final".

[Sergey Ulanov, 2015/01/07 19:37:30]

Done.

+
+  // Queue of packets that that have been received, but haven't been decoded
+  // yet.
+  std::deque<PendingPacket*> pending_packets_;
+
+  // List of per-frame structs that are used to time decoding time for each
+  // frame.
+  std::deque<FrameDecodeTimer> frames_decode_timers_;

[Wez, 2014/12/24 17:11:16]

nit: frame_decode_timers_

Or, since the things being decoded are packets, not frames, consider
packet_decode_timers_.

Given that you can only have one decode in-progress at any given time, I wonder
if you can dispense with the queue and instead embed the decode start-time as a
callback parameter, e.g. use NewCallback(...::OnDecodeDone, start_time) etc -
then you just store the decode time with the PP_VideoPicture when you see
OnPictureReady (see comments elsewhere re dispensing with the picture queue in
favour of a pair of PP_VideoPicture members).

[Sergey Ulanov, 2015/01/06 01:16:34]

Decoding a frame is a two-step process. First we call Decode() and then
GetPicture() to receive the result. Decoder is allowed to decode multiple frames
in parallel, so there may be multiple frames in the queue and we want to keep
timestamp for each of them.

[Wez, 2015/01/06 22:17:10]

Right, I was thinking we could embed the timestamp in the completion callbacks
to the decode and get-picture processes, but of without the queue we can't tally
the received picture with the decoded packet, so I suppose that won't really
work. Seems like it should since they're 1:1 but nevermind...

+
+  // Queue of video frames to be painted. May contain up to 2 frames. The last
+  // frame is always to kept in this vector, in case it needs to be repainted.

[Wez, 2014/12/24 17:11:15]

Why might it contain two frames, rather than containing exactly one?

[Sergey Ulanov, 2015/01/06 01:16:34]

If a previous frame is still being rendered we want to keep it until rendering
is finished even if another one has been received. Replaced it with two
variables |current_frame_| and |next_frame_|

+  std::vector<PP_VideoPicture> pending_pictures_;
+
+  // Set to true if the screen needs to be repainted.

[Wez, 2014/12/24 17:11:15]

nit: Clarify why that would be the case, e.g. due to View resize?

[Sergey Ulanov, 2015/01/06 01:16:33]

Done.

+  bool need_repaint_;
+
+  // Time the last paint operation was started.
+  base::TimeTicks last_paint_started_time_;

[Wez, 2014/12/24 17:11:15]

nit: See last/latest comment, above.

[Sergey Ulanov, 2015/01/06 01:16:33]

Done.

+
+  uint32_t current_shader_program_texture_target_;
+  GLuint shader_program_;
+  GLint shader_texcoord_scale_location_;
+
+  pp::CompletionCallbackFactory<PepperVideoRenderer3D> callback_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(PepperVideoRenderer3D);
+};
+
+}  // namespace remoting
+
+#endif  // REMOTING_CLIENT_PLUGIN_PEPPER_VIDEO_RENDERER_3D_H_