Update TestVideoRenderer with video decoding capabilities

remoting/test/test_video_renderer.h

Index: remoting/test/test_video_renderer.h
diff --git a/remoting/test/test_video_renderer.h b/remoting/test/test_video_renderer.h
index fcf122abcd9aa29bddcf086d522aae0cd08eb5bc..af5ff603969cef8600535bf8a7da0f257ae21f35 100644
--- a/remoting/test/test_video_renderer.h
+++ b/remoting/test/test_video_renderer.h
@@ -5,11 +5,23 @@
 #ifndef REMOTING_TEST_TEST_VIDEO_RENDERER_H_
 #define REMOTING_TEST_TEST_VIDEO_RENDERER_H_
 
-#include "base/callback.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "base/threading/thread_checker.h"
+#include "media/base/video_frame.h"
 #include "remoting/client/video_renderer.h"
+#include "remoting/protocol/session_config.h"
 #include "remoting/protocol/video_stub.h"
 
+namespace base {
+class Thread;
+class SingleThreadTaskRunner;
+}
+
+namespace webrtc {
+class DesktopFrame;
+}
+
 namespace remoting {
 namespace test {
 
@@ -24,13 +36,41 @@ class TestVideoRenderer : public VideoRenderer, public protocol::VideoStub {
   ChromotingStats* GetStats() override;
   protocol::VideoStub* GetVideoStub() override;
 
- private:
   // protocol::VideoStub interface.
   void ProcessVideoPacket(scoped_ptr<VideoPacket> video_packet,
                           const base::Closure& done) override;
 
-  // Track the number of populated video frames which have been received.
-  int video_frames_processed_;
+  // Initialize a decoder to decode video packets.
+  void SetDecoder(const protocol::ChannelConfig::Codec codec);

[joedow, 2015/06/30 17:52:27]

Should this be renamed "SetCodecForDecoding"?

[liaoyuke, 2015/06/30 19:34:59]

Done.

+
+  // Runs a RunLoop until the video packet is processed.

[joedow, 2015/06/30 17:52:27]

Interface comments shouldn't document internal implementations.  Can you update
this comment such that it describes what the method does (instead of how)?

Actually, now that I think about it, this method may not be necessary.  A better
mechanism would be for ProcessVideoPacket to pass the done closure to the
decoding thread and then have the decoding thread (once it has done its work)
post the done callback back to the main thread.  (Check out how this is done in
SoftwareVideoRenderer.cc (lines 349, 165)

[liaoyuke, 2015/06/30 19:35:00]

Done.

+  void WaitForPacketDone(const base::Closure& done);
+
+  // Returns a copy of the current buffer.
+  scoped_ptr<webrtc::DesktopFrame> GetBufferForTest() const;
+
+ private:
+  // The actual implementation resides in Core class.
+  class Core;
+  scoped_ptr<Core> core_;
+
+  // Signals when is packet is processed.
+  void OnPacketDone();
+
+  // Used to quit the RunLoop of main thread.
+  base::Closure main_closure_;

[joedow, 2015/06/30 17:52:27]

I think we should go with the approach I mention above, then you can get rid of
special case members like this which are only used for testing.

[liaoyuke, 2015/06/30 19:34:59]

Done.

+
+  // Used to ensure TestVideoRenderer methods are called on the same thread.
+  base::ThreadChecker thread_checker_;
+
+  // Used to decode and process video packets.
+  scoped_ptr<base::Thread>  video_decode_thread_;
+
+  // Used to post tasks to video decode thread.
+  scoped_refptr<base::SingleThreadTaskRunner> video_decode_task_runner_;
+
+  // Used to weakly bind |this| to |OnPacketDone| method.
+  base::WeakPtrFactory<TestVideoRenderer> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(TestVideoRenderer);
 };