media: Remove the unused NdkMediaCodecBridge

media/base/android/media_codec_bridge.h

Index: media/base/android/media_codec_bridge.h
diff --git a/media/base/android/media_codec_bridge.h b/media/base/android/media_codec_bridge.h
index dd315917779db1dd689f6a57c62f0744318e2e69..306c32e938c69584535b63a26be67229395e3d61 100644
--- a/media/base/android/media_codec_bridge.h
+++ b/media/base/android/media_codec_bridge.h
@@ -8,15 +8,17 @@
 #include <stddef.h>
 #include <stdint.h>
 
-#include <set>
 #include <string>
-#include <vector>
 
+#include "base/android/scoped_java_ref.h"
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/time/time.h"
+#include "media/base/android/media_codec_bridge.h"
 #include "media/base/android/media_codec_direction.h"
+#include "media/base/audio_decoder_config.h"
 #include "media/base/media_export.h"
+#include "media/base/video_decoder_config.h"
 #include "ui/gfx/geometry/size.h"
 
 namespace media {
@@ -32,31 +34,25 @@ enum MediaCodecStatus {
   MEDIA_CODEC_DEQUEUE_OUTPUT_AGAIN_LATER,
   MEDIA_CODEC_OUTPUT_BUFFERS_CHANGED,
   MEDIA_CODEC_OUTPUT_FORMAT_CHANGED,
-  MEDIA_CODEC_INPUT_END_OF_STREAM,
-  MEDIA_CODEC_OUTPUT_END_OF_STREAM,
   MEDIA_CODEC_NO_KEY,
-  MEDIA_CODEC_ABORT,
   MEDIA_CODEC_ERROR
 };
 
-// Interface for wrapping different Android MediaCodec implementations. For
-// more information on Android MediaCodec, check
-// http://developer.android.com/reference/android/media/MediaCodec.html
-// Note: MediaCodec is only available on JB and greater.
+// A wrapper around a Java MediaCodecBridge.
 class MEDIA_EXPORT MediaCodecBridge {
  public:
   virtual ~MediaCodecBridge();
 
   // Calls start() against the media codec instance. Returns whether media
   // codec was successfully started.
-  virtual bool Start() = 0;
+  virtual bool Start();
 
   // Finishes the decode/encode session. The instance remains active
   // and ready to be StartAudio/Video()ed again. HOWEVER, due to the buggy
   // vendor's implementation , b/8125974, Stop() -> StartAudio/Video() may not
   // work on some devices. For reliability, Stop() -> delete and recreate new
   // instance -> StartAudio/Video() is recommended.
-  virtual void Stop() = 0;
+  virtual void Stop();
 
   // Calls flush() on the MediaCodec. All indices previously returned in calls
   // to DequeueInputBuffer() and DequeueOutputBuffer() become invalid. Please
@@ -64,38 +60,35 @@ class MEDIA_EXPORT MediaCodecBridge {
   // there will be no outputs until new input is provided. Returns
   // MEDIA_CODEC_ERROR if an unexpected error happens, or MEDIA_CODEC_OK
   // otherwise.
-  virtual MediaCodecStatus Flush() = 0;
+  virtual MediaCodecStatus Flush();
 
   // Used for getting the output size. This is valid after DequeueInputBuffer()
   // returns a format change by returning INFO_OUTPUT_FORMAT_CHANGED.
   // Returns MEDIA_CODEC_ERROR if an error occurs, or MEDIA_CODEC_OK otherwise.
-  virtual MediaCodecStatus GetOutputSize(gfx::Size* size) = 0;
+  virtual MediaCodecStatus GetOutputSize(gfx::Size* size);
 
   // Used for checking for new sampling rate after DequeueInputBuffer() returns
   // INFO_OUTPUT_FORMAT_CHANGED
   // Returns MEDIA_CODEC_ERROR if an error occurs, or MEDIA_CODEC_OK otherwise.
-  virtual MediaCodecStatus GetOutputSamplingRate(int* sampling_rate) = 0;
+  virtual MediaCodecStatus GetOutputSamplingRate(int* sampling_rate);
 
   // Fills |channel_count| with the number of audio channels. Useful after
   // INFO_OUTPUT_FORMAT_CHANGED.
   // Returns MEDIA_CODEC_ERROR if an error occurs, or MEDIA_CODEC_OK otherwise.
-  virtual MediaCodecStatus GetOutputChannelCount(int* channel_count) = 0;
+  virtual MediaCodecStatus GetOutputChannelCount(int* channel_count);
 
   // Submits a byte array to the given input buffer. Call this after getting an
   // available buffer from DequeueInputBuffer(). If |data| is NULL, assume the
   // input buffer has already been populated (but still obey |size|).
   // |data_size| must be less than kint32max (because Java).
-  virtual MediaCodecStatus QueueInputBuffer(
-      int index,
-      const uint8_t* data,
-      size_t data_size,
-      base::TimeDelta presentation_time) = 0;
+  virtual MediaCodecStatus QueueInputBuffer(int index,
+                                            const uint8_t* data,
+                                            size_t data_size,
+                                            base::TimeDelta presentation_time);
 
-  // Similar to the above call, but submits a buffer that is encrypted.  Note:
-  // NULL |subsamples| indicates the whole buffer is encrypted.  If |data| is
-  // NULL, assume the input buffer has already been populated (but still obey
-  // |data_size|).  |data_size| must be less than kint32max (because Java).
-  MediaCodecStatus QueueSecureInputBuffer(
+  // As above but for encrypted buffers. NULL |subsamples| indicates the
+  // whole buffer is encrypted.
+  virtual MediaCodecStatus QueueSecureInputBuffer(
       int index,
       const uint8_t* data,
       size_t data_size,
@@ -105,42 +98,21 @@ class MEDIA_EXPORT MediaCodecBridge {
       const EncryptionScheme& encryption_scheme,
       base::TimeDelta presentation_time);
 
-  // Same QueueSecureInputBuffer overriden for the use with
-  // AndroidVideoDecodeAccelerator and MediaCodecAudioDecoder. TODO(timav):
-  // remove this method and keep only the one above after we switch to the
-  // Spitzer pipeline.
-  virtual MediaCodecStatus QueueSecureInputBuffer(
-      int index,
-      const uint8_t* data,
-      size_t data_size,
-      const std::vector<char>& key_id,
-      const std::vector<char>& iv,
-      const SubsampleEntry* subsamples,
-      int subsamples_size,
-      const EncryptionScheme& encryption_scheme,
-      base::TimeDelta presentation_time) = 0;
-
-  // Submits an empty buffer with a EOS (END OF STREAM) flag.
-  virtual void QueueEOS(int input_buffer_index) = 0;
+  // Submits an empty buffer with the END_OF_STREAM flag set.
+  virtual void QueueEOS(int input_buffer_index);
 
   // Returns:
   // MEDIA_CODEC_OK if an input buffer is ready to be filled with valid data,
   // MEDIA_CODEC_ENQUEUE_INPUT_AGAIN_LATER if no such buffer is available, or
   // MEDIA_CODEC_ERROR if unexpected error happens.
-  // Note: Never use infinite timeout as this would block the decoder thread and
-  // prevent the decoder job from being released.
   virtual MediaCodecStatus DequeueInputBuffer(base::TimeDelta timeout,
-                                              int* index) = 0;
+                                              int* index);
 
-  // Dequeues an output buffer, block at most timeout_us microseconds.
+  // Dequeues an output buffer, block for up to |timeout|.
   // Returns the status of this operation. If OK is returned, the output
   // parameters should be populated. Otherwise, the values of output parameters
   // should not be used.  Output parameters other than index/offset/size are
   // optional and only set if not NULL.
-  // Note: Never use infinite timeout as this would block the decoder thread and
-  // prevent the decoder job from being released.
-  // TODO(xhwang): Can we drop |end_of_stream| and return
-  // MEDIA_CODEC_OUTPUT_END_OF_STREAM?
   virtual MediaCodecStatus DequeueOutputBuffer(
       base::TimeDelta timeout,
       int* index,
@@ -148,25 +120,16 @@ class MEDIA_EXPORT MediaCodecBridge {
       size_t* size,
       base::TimeDelta* presentation_time,
       bool* end_of_stream,
-      bool* key_frame) = 0;
+      bool* key_frame);
 
   // Returns the buffer to the codec. If you previously specified a surface when
   // configuring this video decoder you can optionally render the buffer.
-  virtual void ReleaseOutputBuffer(int index, bool render) = 0;
+  virtual void ReleaseOutputBuffer(int index, bool render);
 
   // Returns an input buffer's base pointer and capacity.
   virtual MediaCodecStatus GetInputBuffer(int input_buffer_index,
                                           uint8_t** data,
-                                          size_t* capacity) = 0;
-
-  // Gives the access to buffer's data which is referenced by |index| and
-  // |offset|. The size of available data for reading is written to |*capacity|
-  // and the address is written to |*addr|.
-  // Returns MEDIA_CODEC_ERROR if a error occurs, or MEDIA_CODEC_OK otherwise.
-  virtual MediaCodecStatus GetOutputBufferAddress(int index,
-                                                  size_t offset,
-                                                  const uint8_t** addr,
-                                                  size_t* capacity) = 0;
+                                          size_t* capacity);
 
   // Copies |num| bytes from output buffer |index|'s |offset| into the memory
   // region pointed to by |dst|. To avoid overflows, the size of both source
@@ -178,20 +141,137 @@ class MEDIA_EXPORT MediaCodecBridge {
                                         size_t num);
 
   // Gets the component name. Before API level 18 this returns an empty string.
-  virtual std::string GetName() = 0;
+  virtual std::string GetName();
 
  protected:
+  // This constructor is only for making this class mockable.

[liberato (no reviews please), 2017/02/07 06:19:43]

personally, i think that this should still be an interface.  it's okay if the
interface now includes java-specific types things that cause it not to compile
as android_anywhere.

however, i don't think that it adds any clarity merging the sdk impl and the
interface.  having an SdkMediaCodecBridge isn't by itself bad.

i believe that i misunderstood our offline discussion, and i apologize.  i
thought that removing ndk caused it to became convenient to move methods with
non-portable signatures (e.g. setOutputSurface) into MCB, breaking
android_anywhere.

[DaleCurtis, 2017/02/07 20:08:01]

Chrome is taking an active stance to reduce layers these days (see onion soup),
interfaces that only serve one implementation are unnecessary overhead and
should be removed.

A better argument would be that keeping the interface allows us to mock out
badly behaving MediaCodec implementations to simulate their badness and thus
write better tests for problematic devices.

[liberato (no reviews please), 2017/02/07 20:37:30]

won't we need virtual functions anyway to mock this thing (badly behaving codecs
or an (elusive!) correctly behaving codec)?  mocking is my primary concern,
though i didn't write that anywhere in the comment.  :)

[watk, 2017/02/08 02:53:57]

I was intending to make it mockable either way (that's one, far removed,
motivation for this CL). Talked and researched offline and decided that a
separate pure virtual interface is preferable for mocking (as opposed to having
the mock inherit from the impl) so I've done that now.

The interface is called MediaCodecBridge and the impl is called
MediaCodecBridgeImpl. I don't want to call it SdkMCB because that's no longer an
interesting distinction and Impl is conventional for a single impl.

   MediaCodecBridge();
+  MediaCodecBridge(const std::string& mime,
+                   bool is_secure,
+                   MediaCodecDirection direction,
+                   bool require_software_codec);
 
-  // Fills a particular input buffer; returns false if |data_size| exceeds the
+  jobject media_codec() { return j_media_codec_.obj(); }
+
+  MediaCodecDirection direction_;
+
+ private:
+  // Fills the given input buffer. Returns false if |data_size| exceeds the
   // input buffer's capacity (and doesn't touch the input buffer in that case).
   bool FillInputBuffer(int index,
                        const uint8_t* data,
                        size_t data_size) WARN_UNUSED_RESULT;
 
+  // Gets the address of the data in the given output buffer given by |index|
+  // and |offset|. The number of bytes available to read is written to
+  // |*capacity| and the address is written to |*addr|. Returns
+  // MEDIA_CODEC_ERROR if an error occurs, or MEDIA_CODEC_OK otherwise.
+  MediaCodecStatus GetOutputBufferAddress(int index,
+                                          size_t offset,
+                                          const uint8_t** addr,
+                                          size_t* capacity);
+
+  // The Java MediaCodecBridge instance.
+  base::android::ScopedJavaGlobalRef<jobject> j_media_codec_;
+
   DISALLOW_COPY_AND_ASSIGN(MediaCodecBridge);
 };
 
+// A MediaCodec wrapper for audio decoding.
+class MEDIA_EXPORT AudioCodecBridge : public MediaCodecBridge {

[liberato (no reviews please), 2017/02/07 06:19:43]

this and VideoCodecBridge should be in their own headers / impls.

they are wrong in ToT, too, in my opinion.  but if you're moving them, then
might as well split them up.

[watk, 2017/02/08 02:53:58]

Will do in a follow up.

+ public:
+  // See MediaCodecUtil::IsKnownUnaccelerated().
+  static bool IsKnownUnaccelerated(const AudioCodec& codec);
+
+  // Returns an AudioCodecBridge instance if |codec| is supported, or a NULL
+  // pointer otherwise.
+  static AudioCodecBridge* Create(const AudioCodec& codec);
+
+  // Starts the audio codec bridge.
+  bool ConfigureAndStart(const AudioDecoderConfig& config,
+                         jobject media_crypto) WARN_UNUSED_RESULT;
+
+  bool ConfigureAndStart(const AudioCodec& codec,
+                         int sample_rate,
+                         int channel_count,
+                         const uint8_t* extra_data,
+                         size_t extra_data_size,
+                         int64_t codec_delay_ns,
+                         int64_t seek_preroll_ns,
+                         jobject media_crypto) WARN_UNUSED_RESULT;
+
+ private:
+  explicit AudioCodecBridge(const std::string& mime);
+
+  // Configure the java MediaFormat object with the extra codec data passed in.
+  bool ConfigureMediaFormat(jobject j_format,
+                            const AudioCodec& codec,
+                            const uint8_t* extra_data,
+                            size_t extra_data_size,
+                            int64_t codec_delay_ns,
+                            int64_t seek_preroll_ns);
+};
+
+// A MediaCodecBridge with for video encoding and decoding.
+class MEDIA_EXPORT VideoCodecBridge : public MediaCodecBridge {

[liberato (no reviews please), 2017/02/07 06:19:43]

i keep going back and forth on these.  on one hand, it's kind of nice to have a
VideoCodecBridge instead of a MediaCodecBridge that has some handy helper
functions.

however, it's also handy if they aren't concrete implementations.  how to get
both of those together is a good question.  :)

the constructors can eventually be moved into factories, as can the other
statics.  i.e., one could get a VideoCodecBridgeFactory that can answer (without
statics) IsKnownUnaccelerated.  probably not part of this cl, but as part of the
larger picture of enabling better testing.

one might also argue that, since the underlying MediaCodec object doesn't have
video and audio variants, then neither should we.  it seems kinda arbitrary that
we have VideoCodecBridge but not VideoDecoderBridge / VideoEncoderBridge.  one
could still keep the factories separate to control the arguments to
construction, which is really where most of the differences are.  the other
differences are just extra functions that are just as easy not to call if one
doesn't need them.

i'm sure that it's all out of scope for this CL, but i'm curious about your
thoughts on it.

[watk, 2017/02/08 02:53:58]

Yeah I don't like these. I have long felt that they don't serve a purpose and
are only awkward. Especially because they each require their own mock. I'll plan
to refactor in a follow up.

+ public:
+  // See MediaCodecUtil::IsKnownUnaccelerated().
+  static bool IsKnownUnaccelerated(const VideoCodec& codec,
+                                   MediaCodecDirection direction);
+
+  // Create, start, and return a VideoCodecBridge decoder or NULL on failure.
+  static VideoCodecBridge* CreateDecoder(

[liberato (no reviews please), 2017/02/07 06:19:43]

if you're aiming to make these testable, then these should be factories.  once
you remove the sdk / ndk "pick an include" method of inheritance, we don't need
these static creation functions either.

maybe it's outside the scope of this CL, but it's worth considering if you
haven't already.

[watk, 2017/02/08 02:53:57]

Sounds good. For my immediate testing needs, I think it's enough to think of
AVDACodecAllocator as the factory, but I'll rethink this when I remove the
video/audio bridges.

+      const VideoCodec& codec,
+      bool is_secure,         // Will be used with encrypted content.
+      const gfx::Size& size,  // Output frame size.
+      jobject surface,        // Output surface, optional.
+      jobject media_crypto,   // MediaCrypto object, optional.
+      // Codec specific data. See MediaCodec docs.
+      const std::vector<uint8_t>& csd0,
+      const std::vector<uint8_t>& csd1,
+      // Should adaptive playback be allowed if supported.
+      bool allow_adaptive_playback = true,
+      bool require_software_codec = false);
+
+  // Create, start, and return a VideoCodecBridge encoder or NULL on failure.
+  static VideoCodecBridge* CreateEncoder(
+      const VideoCodec& codec,  // e.g. media::kCodecVP8
+      const gfx::Size& size,    // input frame size
+      int bit_rate,             // bits/second
+      int frame_rate,           // frames/second
+      int i_frame_interval,     // count
+      int color_format);        // MediaCodecInfo.CodecCapabilities.
+
+  void SetVideoBitrate(int bps, int frame_rate);
+  void RequestKeyFrameSoon();
+
+  // Returns whether adaptive playback is supported for this object given
+  // the new size.
+  bool IsAdaptivePlaybackSupported(int width, int height);
+
+  // Changes the output surface for the MediaCodec. May only be used on API
+  // level 23 and higher (Marshmallow).
+  bool SetSurface(jobject surface);
+
+  // Test-only method to set the return value of IsAdaptivePlaybackSupported().
+  // Without this function, the return value of that function will be device
+  // dependent. If |adaptive_playback_supported| is equal to 0, the return value
+  // will be false. If |adaptive_playback_supported| is larger than 0, the
+  // return value will be true.
+  void set_adaptive_playback_supported_for_testing(
+      int adaptive_playback_supported) {
+    adaptive_playback_supported_for_testing_ = adaptive_playback_supported;
+  }
+
+ private:
+  VideoCodecBridge(const std::string& mime,
+                   bool is_secure,
+                   MediaCodecDirection direction,
+                   bool require_software_codec);
+
+  int adaptive_playback_supported_for_testing_;
+};
+
 }  // namespace media
 
 #endif  // MEDIA_BASE_ANDROID_MEDIA_CODEC_BRIDGE_H_