Replace DecodeStatus with Status in DecodeCB.

This is the first of several CLs to replace DecodeStatus with
media::Status.  This one adjusts DecodeCB to return a Status, and
alters the consumers to expect it.  It also adds the DecodeStatus
enum values to StatusCode, so that old code continues to work
without modification.  DecodeStatus is aliased to StatusCode.

DecodeCB implementations have been modified to check for "not
ok, not aborted" where they used to check for
`DecodeStatus::DECODE_ERROR`.  While `DECODE_ERROR` is still defined
in the enum, and decoders continue to return it, the consumers
should now work even if individual decoders start sending more
detailed status codes.  `DECODE_ERROR` will be removed once all the
decoders have been updated.

For tests, there is now a `IsDecodeErrorStatus()` matcher that does
the same thing.

Future CLs will update other uses of DecodeStatus to do it more
properly, and not rely on this somewhat hacky enum-merging.  It can
be done incrementally with these changes, which is the real goal.

Change-Id: I77dd3ee9e3be5b17ca070e602f2b173ba98d4694
Bug: 1129662
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2392950
Reviewed-by: Sergey Volk <servolk@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Bill Budge <bbudge@chromium.org>
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Reviewed-by: Xiaohan Wang <xhwang@chromium.org>
Commit-Queue: Frank Liberato <liberato@chromium.org>
Cr-Commit-Position: refs/heads/master@{#810257}

chromecast/media/cma/decoder/cast_audio_decoder.cc

@@ -184,15 +184,15 @@ class CastAudioDecoderImpl : public CastAudioDecoder {
   }
 
   void OnDecodeStatus(base::TimeDelta buffer_timestamp,
-                      ::media::DecodeStatus status) {
+                      ::media::Status status) {
     DCHECK(pending_decode_callback_);
 
     Status result_status = kDecodeOk;
     scoped_refptr<media::DecoderBufferBase> decoded;
-    if (status == ::media::DecodeStatus::OK && !decoded_chunks_.empty()) {
+    if (status.is_ok() && !decoded_chunks_.empty()) {
       decoded = ConvertDecoded();
     } else {
-      if (status != ::media::DecodeStatus::OK)
+      if (!status.is_ok())
         result_status = kDecodeError;
       decoded = base::MakeRefCounted<media::DecoderBufferAdapter>(
           output_config_.id, base::MakeRefCounted<::media::DecoderBuffer>(0));

content/renderer/pepper/video_decoder_shim.cc

@@ -120,7 +120,7 @@ class VideoDecoderShim::DecoderImpl {
  private:
   void OnInitDone(media::Status status);
   void DoDecode();
-  void OnDecodeComplete(media::DecodeStatus status);
+  void OnDecodeComplete(media::Status status);
   void OnOutputComplete(scoped_refptr<media::VideoFrame> frame);
   void OnResetComplete();
 
@@ -252,18 +252,17 @@ void VideoDecoderShim::DecoderImpl::DoDecode() {
   pending_decodes_.pop();
 }
 
-void VideoDecoderShim::DecoderImpl::OnDecodeComplete(
-    media::DecodeStatus status) {
+void VideoDecoderShim::DecoderImpl::OnDecodeComplete(media::Status status) {
   DCHECK(awaiting_decoder_);
   awaiting_decoder_ = false;
 
   int32_t result;
-  switch (status) {
-    case media::DecodeStatus::OK:
-    case media::DecodeStatus::ABORTED:
+  switch (status.code()) {
+    case media::StatusCode::kOk:
+    case media::StatusCode::kAborted:
       result = PP_OK;
       break;
-    case media::DecodeStatus::DECODE_ERROR:
+    default:
       result = PP_ERROR_RESOURCE_FAILED;
       break;
   }

media/base/audio_decoder.h

@@ -27,16 +27,20 @@ class CdmContext;
 
 class MEDIA_EXPORT AudioDecoder : public Decoder {
  public:
-  // Callback for AudioDecoder initialization.
+  // Callback for Decoder initialization.
   using InitCB = base::OnceCallback<void(Status)>;
 
   // Callback for AudioDecoder to return a decoded frame whenever it becomes
   // available. Only non-EOS frames should be returned via this callback.
   using OutputCB = base::RepeatingCallback<void(scoped_refptr<AudioBuffer>)>;
 
-  // Callback for Decode(). Called after the decoder has accepted corresponding
-  // DecoderBuffer, indicating that the pipeline can send next buffer to decode.
-  using DecodeCB = base::OnceCallback<void(DecodeStatus)>;
+  // Callback type for Decode(). Called after the decoder has completed decoding
+  // corresponding DecoderBuffer, indicating that it's ready to accept another
+  // buffer to decode.  |kOk| implies success, |kAborted| implies that the
+  // decode was aborted, which does not necessarily indicate an error.  For
+  // example, a Reset() can trigger this.  Any other status code indicates that
+  // the decoder encountered an error, and must be reset.
+  using DecodeCB = base::OnceCallback<void(Status)>;
 
   AudioDecoder();
 

media/base/decode_status.cc

@@ -7,6 +7,7 @@
 #include <ostream>
 
 #include "base/trace_event/trace_event.h"
+#include "media/base/status.h"
 
 namespace media {
 
@@ -18,17 +19,16 @@ const char* GetDecodeStatusString(DecodeStatus status) {
       return "DecodeStatus::ABORTED";
     case DecodeStatus::DECODE_ERROR:
       return "DecodeStatus::DECODE_ERROR";
+    default:
+      // TODO(liberato): Temporary while converting to media::Status.  This
+      // fn should go away.
+      return "DecodeStatus::UNKNOWN_ERROR";
   }
 
   NOTREACHED();
   return "";
 }
 
-std::ostream& operator<<(std::ostream& os, const DecodeStatus& status) {
-  os << GetDecodeStatusString(status);
-  return os;
-}
-
 // static
 bool ScopedDecodeTrace::IsEnabled() {
   bool enable_decode_traces = false;
@@ -59,11 +59,11 @@ ScopedDecodeTrace::~ScopedDecodeTrace() {
     EndTrace(DecodeStatus::ABORTED);
 }
 
-void ScopedDecodeTrace::EndTrace(DecodeStatus status) {
+void ScopedDecodeTrace::EndTrace(const Status& status) {
   DCHECK(!closed_);
   closed_ = true;
   TRACE_EVENT_ASYNC_END1("media", trace_name_, this, "status",
-                         GetDecodeStatusString(status));
+                         GetDecodeStatusString(status.code()));
 }
 
 }  // namespace media

media/base/decode_status.h

@@ -9,22 +9,17 @@
 
 #include "media/base/decoder_buffer.h"
 #include "media/base/media_export.h"
+#include "media/base/status_codes.h"
 
 namespace media {
 
-enum class DecodeStatus {
-  OK = 0,        // Everything went as planned.
-  ABORTED,       // Read aborted due to Reset() during pending read.
-  DECODE_ERROR,  // Decoder returned decode error. Note: Prefixed by DECODE_
-                 // since ERROR is a reserved name (special macro) on Windows.
-  DECODE_STATUS_MAX = DECODE_ERROR
-};
+class Status;
 
-MEDIA_EXPORT const char* GetDecodeStatusString(DecodeStatus status);
+// TODO(crbug.com/1129662): This is temporary, to allow DecodeStatus::OK to
+// work, while we replace DecodeStatus with actual status codes.
+using DecodeStatus = StatusCode;
 
-// Helper function so that DecodeStatus can be printed easily.
-MEDIA_EXPORT std::ostream& operator<<(std::ostream& os,
-                                      const DecodeStatus& status);
+MEDIA_EXPORT const char* GetDecodeStatusString(DecodeStatus status);
 
 // Helper class for ensuring that Decode() traces are properly unique and closed
 // if the Decode is aborted via a WeakPtr invalidation. We use the |this|
@@ -45,7 +40,7 @@ class MEDIA_EXPORT ScopedDecodeTrace {
   ~ScopedDecodeTrace();
 
   // Completes the Decode() trace with the given status.
-  void EndTrace(DecodeStatus status);
+  void EndTrace(const Status& status);
 
  private:
   const char* trace_name_;

media/base/decoder.h

@@ -7,8 +7,10 @@
 
 #include <string>
 
+#include "base/callback.h"
 #include "base/macros.h"
 #include "media/base/media_export.h"
+#include "media/base/status.h"
 
 namespace media {
 

media/base/ipc/media_param_traits_macros.h

@@ -75,9 +75,6 @@ IPC_ENUM_TRAITS_MAX_VALUE(media::CdmSessionType,
 
 IPC_ENUM_TRAITS_MAX_VALUE(media::ChannelLayout, media::CHANNEL_LAYOUT_MAX)
 
-IPC_ENUM_TRAITS_MAX_VALUE(media::DecodeStatus,
-                          media::DecodeStatus::DECODE_STATUS_MAX)
-
 IPC_ENUM_TRAITS_MAX_VALUE(media::Decryptor::Status,
                           media::Decryptor::Status::kStatusMax)
 

media/base/status_codes.h

@@ -27,6 +27,9 @@ using StatusCodeType = int32_t;
 enum class StatusCode : StatusCodeType {
   kOk = 0,
 
+  // General errors: 0x00
+  kAborted = 0x00000001,
+
   // Decoder Errors: 0x01
   kDecoderInitializeNeverCompleted = 0x00000101,
   kDecoderFailedDecode = 0x00000102,
@@ -44,6 +47,9 @@ enum class StatusCode : StatusCodeType {
   kInitializationUnspecifiedFailure = 0x0000010C,
   kDecoderVideoFrameConstructionFailed = 0x0000010D,
   kMakeContextCurrentFailed = 0x0000010E,
+  // This is a temporary error for use only by existing code during the
+  // DecodeStatus => Status conversion.
+  kDecodeErrorDoNotUse = 0x0000010F,
 
   // Windows Errors: 0x02
   kWindowsWrappedHresult = 0x00000201,
@@ -120,6 +126,19 @@ enum class StatusCode : StatusCodeType {
   kH264ParsingError = 0x00000801,
   kH264BufferTooSmall = 0x00000802,
 
+  // DecodeStatus temporary codes.  These names were chosen to match the
+  // DecodeStatus enum, so that un-converted code can DecodeStatus::OK/etc.
+  // Note that OK must result in Status::is_ok(), since converted code will
+  // check for it.  These will be removed when the conversion is complete.
+  //
+  // DO NOT ADD NEW USES OF OK/ABORTED/DECODE_ERROR.
+  OK = kOk,  // Everything went as planned.
+  // Read aborted due to Reset() during pending read.
+  ABORTED = kAborted,  // Read aborted due to Reset() during pending read.
+  // Decoder returned decode error. Note: Prefixed by DECODE_
+  // since ERROR is a reserved name (special macro) on Windows.
+  DECODE_ERROR = kDecodeErrorDoNotUse,
+
   // Special codes
   kGenericErrorPleaseRemove = 0x79999999,
   kCodeOnlyForTesting = std::numeric_limits<StatusCodeType>::max(),

media/base/test_helpers.h

@@ -224,7 +224,7 @@ MATCHER_P(SameStatusCode, status, "") {
   return arg.code() == status.code();
 }
 
-// Compares two an |arg| Status to a StatusCode provided
+// Compares an `arg` Status.code() to a test-supplied StatusCode.
 MATCHER_P(HasStatusCode, status_code, "") {
   return arg.code() == status_code;
 }
@@ -233,6 +233,12 @@ MATCHER(IsOkStatus, "") {
   return arg.is_ok();
 }
 
+// True if and only if the Status would be interpreted as an error from a decode
+// callback (not okay, not aborted).
+MATCHER(IsDecodeErrorStatus, "") {
+  return !arg.is_ok() && arg.code() != StatusCode::kAborted;
+}
+
 // Compares two {Audio|Video}DecoderConfigs
 MATCHER_P(DecoderConfigEq, config, "") {
   return arg.Matches(config);

media/base/video_decoder.h

@@ -7,14 +7,12 @@
 
 #include <string>
 
-#include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "media/base/decode_status.h"
 #include "media/base/decoder.h"
 #include "media/base/media_export.h"
 #include "media/base/pipeline_status.h"
-#include "media/base/status.h"
 #include "media/base/waiting.h"
 #include "ui/gfx/geometry/size.h"
 
@@ -27,17 +25,20 @@ class VideoFrame;
 
 class MEDIA_EXPORT VideoDecoder : public Decoder {
  public:
-  // Callback for VideoDecoder initialization.
-  using InitCB = base::OnceCallback<void(Status status)>;
+  // Callback for Decoder initialization.
+  using InitCB = base::OnceCallback<void(Status)>;
 
   // Callback for VideoDecoder to return a decoded frame whenever it becomes
   // available. Only non-EOS frames should be returned via this callback.
   using OutputCB = base::RepeatingCallback<void(scoped_refptr<VideoFrame>)>;
 
   // Callback type for Decode(). Called after the decoder has completed decoding
   // corresponding DecoderBuffer, indicating that it's ready to accept another
-  // buffer to decode.
-  using DecodeCB = base::OnceCallback<void(DecodeStatus)>;
+  // buffer to decode.  |kOk| implies success, |kAborted| implies that the
+  // decode was aborted, which does not necessarily indicate an error.  For
+  // example, a Reset() can trigger this.  Any other status code indicates that
+  // the decoder encountered an error, and must be reset.
+  using DecodeCB = base::OnceCallback<void(Status)>;
 
   VideoDecoder();
   ~VideoDecoder() override;

media/base/video_thumbnail_decoder.cc

@@ -50,8 +50,8 @@ void VideoThumbnailDecoder::OnVideoDecoderInitialized(Status status) {
                                   weak_factory_.GetWeakPtr()));
 }
 
-void VideoThumbnailDecoder::OnVideoBufferDecoded(DecodeStatus status) {
-  if (status != DecodeStatus::OK) {
+void VideoThumbnailDecoder::OnVideoBufferDecoded(Status status) {
+  if (!status.is_ok()) {
     NotifyComplete(nullptr);
     return;
   }
@@ -62,8 +62,8 @@ void VideoThumbnailDecoder::OnVideoBufferDecoded(DecodeStatus status) {
                                   weak_factory_.GetWeakPtr()));
 }
 
-void VideoThumbnailDecoder::OnEosBufferDecoded(DecodeStatus status) {
-  if (status != DecodeStatus::OK)
+void VideoThumbnailDecoder::OnEosBufferDecoded(Status status) {
+  if (!status.is_ok())
     NotifyComplete(nullptr);
 }
 

media/base/video_thumbnail_decoder.h

@@ -39,8 +39,8 @@ class MEDIA_EXPORT VideoThumbnailDecoder {
 
  private:
   void OnVideoDecoderInitialized(Status status);
-  void OnVideoBufferDecoded(DecodeStatus status);
-  void OnEosBufferDecoded(DecodeStatus status);
+  void OnVideoBufferDecoded(Status status);
+  void OnEosBufferDecoded(Status status);
 
   // Called when the output frame is generated.
   void OnVideoFrameDecoded(scoped_refptr<VideoFrame> frame);

media/cast/sender/h264_vt_encoder_unittest.cc

@@ -157,7 +157,7 @@ class EndToEndFrameChecker
     ++count_frames_checked_;
   }
 
-  void DecodeDone(DecodeStatus status) { EXPECT_EQ(DecodeStatus::OK, status); }
+  void DecodeDone(Status status) { EXPECT_TRUE(status.is_ok()); }
 
   int count_frames_checked() const { return count_frames_checked_; }
 

media/cdm/library_cdm/clear_key_cdm/cdm_video_decoder.cc

@@ -233,11 +233,12 @@ class VideoDecoderAdapter : public CdmVideoDecoder {
     auto decode_status = last_decode_status_.value();
     last_decode_status_.reset();
 
-    if (decode_status == DecodeStatus::DECODE_ERROR)
-      return cdm::kDecodeError;
+    // "kAborted" shouldn't happen during a sync decode, so treat it as an
+    // error.
+    DCHECK_NE(decode_status.code(), StatusCode::kAborted);
 
-    // "ABORTED" shouldn't happen during a sync decode, so treat it as an error.
-    DCHECK_EQ(decode_status, DecodeStatus::OK);
+    if (!decode_status.is_ok())
+      return cdm::kDecodeError;
 
     if (decoded_video_frames_.empty())
       return cdm::kNeedMoreData;
@@ -272,9 +273,9 @@ class VideoDecoderAdapter : public CdmVideoDecoder {
     std::move(quit_closure).Run();
   }
 
-  void OnDecoded(base::OnceClosure quit_closure, DecodeStatus decode_status) {
+  void OnDecoded(base::OnceClosure quit_closure, Status decode_status) {
     DCHECK(!last_decode_status_.has_value());
-    last_decode_status_ = decode_status;
+    last_decode_status_ = std::move(decode_status);
     std::move(quit_closure).Run();
   }
 
@@ -284,7 +285,7 @@ class VideoDecoderAdapter : public CdmVideoDecoder {
   // Results of |video_decoder_| operations. Set iff the callback of the
   // operation has been called.
   base::Optional<Status> last_init_result_;
-  base::Optional<DecodeStatus> last_decode_status_;
+  base::Optional<Status> last_decode_status_;
 
   // Queue of decoded video frames.
   using VideoFrameQueue = base::queue<scoped_refptr<VideoFrame>>;