Break tight coupling of VideoCaptureDeviceClient to renderer_host

content/browser/renderer_host/media/video_capture_gpu_jpeg_decoder.h

 // Copyright 2015 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_BROWSER_RENDERER_HOST_MEDIA_VIDEO_CAPTURE_GPU_JPEG_DECODER_H_
 #define CONTENT_BROWSER_RENDERER_HOST_MEDIA_VIDEO_CAPTURE_GPU_JPEG_DECODER_H_
 
 #include <stddef.h>
 #include <stdint.h>
 
@@ skipping 13 matching lines @@
 namespace gpu {
 class GpuChannelHost;
 }
 
 namespace media {
 class VideoFrame;
 }
 
 namespace content {
 
-// Adapter to GpuJpegDecodeAccelerator for VideoCaptureDevice::Client. It takes
-// care of GpuJpegDecodeAccelerator creation, shared memory, and threading
-// issues.
-//
-// All public methods except JpegDecodeAccelerator::Client ones should be called
-// on the same thread. JpegDecodeAccelerator::Client methods should be called on
-// the IO thread.
-class CONTENT_EXPORT VideoCaptureGpuJpegDecoder
-    : public media::JpegDecodeAccelerator::Client,
-      public base::NonThreadSafe,
-      public base::SupportsWeakPtr<VideoCaptureGpuJpegDecoder> {
+class CONTENT_EXPORT VideoCaptureJpegDecoder {
  public:
   // Enumeration of decoder status. The enumeration is published for clients to
   // decide the behavior according to STATUS.
   enum STATUS {
     INIT_PENDING,  // Default value while waiting initialization finished.
     INIT_PASSED,   // Initialization succeed.
     FAILED,        // JPEG decode is not supported, initialization failed, or
                    // decode error.
   };
 
-  typedef base::Callback<void(
+  using DecodeDoneCB = base::Callback<void(
       std::unique_ptr<media::VideoCaptureDevice::Client::Buffer>,
-      const scoped_refptr<media::VideoFrame>&)>
-      DecodeDoneCB;
+      const scoped_refptr<media::VideoFrame>&)>;
 
+  virtual ~VideoCaptureJpegDecoder() {}
+
+  // Creates and intializes decoder asynchronously.
+  virtual void Initialize() = 0;
+
+  // Returns initialization status.
+  virtual STATUS GetStatus() = 0;
+
+  // Decodes a JPEG picture.
+  virtual void DecodeCapturedData(
+      const uint8_t* data,
+      size_t in_buffer_size,
+      const media::VideoCaptureFormat& frame_format,
+      base::TimeTicks reference_time,
+      base::TimeDelta timestamp,
+      std::unique_ptr<media::VideoCaptureDevice::Client::Buffer>
+          out_buffer) = 0;
+};
+
+class CONTENT_EXPORT VideoCaptureJpegDecoderFactory {
+ public:
+  virtual ~VideoCaptureJpegDecoderFactory() {}
+  virtual std::unique_ptr<VideoCaptureJpegDecoder> CreateJpegDecoder() = 0;
+};
+
+// Adapter to GpuJpegDecodeAccelerator for VideoCaptureDevice::Client. It takes
+// care of GpuJpegDecodeAccelerator creation, shared memory, and threading
+// issues.
+//
+// All public methods except JpegDecodeAccelerator::Client ones should be called
+// on the same thread. JpegDecodeAccelerator::Client methods should be called on
+// the IO thread.
+class CONTENT_EXPORT VideoCaptureGpuJpegDecoder

[mcasas, 2016/09/09 22:28:02]

Following my recommendation before, this class
should be called VideoCaptureJpegDecoderImpl.

[chfremer, 2016/09/12 18:31:22]

Despite this being done a lot in the Chromium code base, naming an
implementation of an interface just <InterfaceName>Impl has several downsides:
1.) It assumes that there is only ever going to be a single implementation.
2.) It does not express what it is that makes the implementation more concrete
than the interface.

I highly recommend naming implementations of an interface specification as
follows:
<WhatMakesThisImplementationDifferentFromOthers><InterfaceName>

In this example case, I think a good name would be GpuVideoCaptureJpegDecoder.
This nicely distinguishes it from a second potential implementation which would
be called SoftwareVideoCaptureJpegDecoder. 

Since the name of the existing class already was "VideoCaptureGpuJpegDecoder" I
thought it would be good to not touch it in order to not add to the amount of
change of the CL.

Please let me know if you still prefer to rename this to
VideoCaptureJpeDecoderImpl, and I'll go ahead and make the change.

+    : public VideoCaptureJpegDecoder,
+      public media::JpegDecodeAccelerator::Client,
+      public base::NonThreadSafe,
+      public base::SupportsWeakPtr<VideoCaptureGpuJpegDecoder> {
+ public:

[mcasas, 2016/09/09 22:28:02]

Also, if we're creating abstract base classes and Impls,
shouldn't we extract the base classes into their own
files, and rename the appropriate ones to _impl.{cc,h} ?
I'm more than OK with doing that in a subsequent, single, 
specific CL :-)

[chfremer, 2016/09/12 18:31:22]

Agreed. 
As stated in the CL description: "Interfaces will be moved to new files in a new

location in a follow-up CL.".

   // |decode_done_cb| is called on the IO thread when decode succeed. This can
   // be on any thread. |decode_done_cb| is never called after
   // VideoCaptureGpuJpegDecoder is destroyed.
   explicit VideoCaptureGpuJpegDecoder(const DecodeDoneCB& decode_done_cb);
   ~VideoCaptureGpuJpegDecoder() override;
 
-  // Creates and intializes decoder asynchronously.
-  void Initialize();
-
-  // Returns initialization status.
-  STATUS GetStatus() const;
-
-  // Decodes a JPEG picture.
+  // Implementation of VideoCaptureJpegDecoder:
+  void Initialize() override;
+  STATUS GetStatus() override;
   void DecodeCapturedData(
       const uint8_t* data,
       size_t in_buffer_size,
       const media::VideoCaptureFormat& frame_format,
       base::TimeTicks reference_time,
       base::TimeDelta timestamp,
-      std::unique_ptr<media::VideoCaptureDevice::Client::Buffer> out_buffer);
+      std::unique_ptr<media::VideoCaptureDevice::Client::Buffer> out_buffer)
+      override;
 
   // JpegDecodeAccelerator::Client implementation.
   // These will be called on IO thread.
   void VideoFrameReady(int32_t buffer_id) override;
   void NotifyError(int32_t buffer_id,
                    media::JpegDecodeAccelerator::Error error) override;
 
  private:
   // Initialization helper, to establish GPU channel.
   static void EstablishGpuChannelOnUIThread(
@@ skipping 39 matching lines @@
   std::unique_ptr<base::SharedMemory> in_shared_memory_;
 
   STATUS decoder_status_;
 
   DISALLOW_COPY_AND_ASSIGN(VideoCaptureGpuJpegDecoder);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_RENDERER_HOST_MEDIA_VIDEO_CAPTURE_GPU_JPEG_DECODER_H_