Parse codecs on VideoEncoder::Configure()

This CL front-loads the configuration parsing and codec support
verification into the Configure() method. This means that and invalid
or unsupported config should immediately throw an exception, rather than
being reported in the error callback.

This CL also allows encoders to be re-configured.

Bug: 1116783, 1094166
Change-Id: I88df452cfa2a1b0fcfeb4da6c279665661d7380f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2363711
Commit-Queue: Thomas Guilbert <tguilbert@chromium.org>
Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>
Cr-Commit-Position: refs/heads/master@{#800277}

third_party/blink/renderer/modules/webcodecs/video_encoder.cc

@@ -10,6 +10,7 @@
 #include "base/callback.h"
 #include "base/logging.h"
 #include "base/macros.h"
+#include "media/base/mime_util.h"
 #include "media/base/video_codecs.h"
 #include "media/base/video_color_space.h"
 #include "media/base/video_encoder.h"
@@ -119,25 +120,133 @@ int32_t VideoEncoder::encodeQueueSize() {
   return requested_encodes_;
 }
 
+std::unique_ptr<VideoEncoder::ParsedConfig> VideoEncoder::ParseConfig(
+    const VideoEncoderConfig* config,
+    ExceptionState& exception_state) {
+  auto parsed = std::make_unique<ParsedConfig>();
+
+  parsed->options.height = config->height();
+  if (parsed->options.height == 0) {
+    exception_state.ThrowTypeError("Invalid height.");
+    return nullptr;
+  }
+
+  parsed->options.width = config->width();
+  if (parsed->options.width == 0) {
+    exception_state.ThrowTypeError("Invalid width.");
+    return nullptr;
+  }
+
+  parsed->options.framerate = config->framerate();
+
+  if (config->hasBitrate())
+    parsed->options.bitrate = config->bitrate();
+
+  // The IDL defines a default value of "allow".
+  DCHECK(config->hasAcceleration());
+
+  std::string preference = IDLEnumAsString(config->acceleration()).Utf8();
+  if (preference == "allow") {
+    parsed->acc_pref = AccelerationPreference::kAllow;
+  } else if (preference == "require") {
+    parsed->acc_pref = AccelerationPreference::kRequire;
+  } else if (preference == "deny") {
+    parsed->acc_pref = AccelerationPreference::kDeny;
+  } else {
+    NOTREACHED();
+  }
+
+  bool is_codec_ambiguous = true;
+  parsed->codec = media::kUnknownVideoCodec;
+  parsed->profile = media::VIDEO_CODEC_PROFILE_UNKNOWN;
+  parsed->color_space = media::VideoColorSpace::REC709();
+  parsed->level = 0;
+
+  bool parse_succeeded = media::ParseVideoCodecString(
+      "", config->codec().Utf8(), &is_codec_ambiguous, &parsed->codec,
+      &parsed->profile, &parsed->level, &parsed->color_space);
+
+  if (!parse_succeeded) {
+    exception_state.ThrowTypeError("Invalid codec string.");
+    return nullptr;
+  }
+
+  if (is_codec_ambiguous) {
+    exception_state.ThrowTypeError("Ambiguous codec string.");
+    return nullptr;
+  }
+
+  return parsed;
+}
+
+bool VideoEncoder::VerifyCodecSupport(ParsedConfig* config,
+                                      ExceptionState& exception_state) {
+  switch (config->codec) {
+    case media::kCodecVP8:
+      if (config->acc_pref == AccelerationPreference::kRequire) {
+        exception_state.ThrowDOMException(DOMExceptionCode::kNotSupportedError,
+                                          "Accelerated vp8 is not supported");
+        return false;
+      }
+      break;
+
+    case media::kCodecVP9:
+      if (config->acc_pref == AccelerationPreference::kRequire) {
+        exception_state.ThrowDOMException(DOMExceptionCode::kNotSupportedError,
+                                          "Accelerated vp9 is not supported");
+        return false;
+      }
+
+      // TODO(https://crbug.com/1119636): Implement / call a proper method for
+      // detecting support of encoder configs.
+      if (config->profile == media::VideoCodecProfile::VP9PROFILE_PROFILE1 ||
+          config->profile == media::VideoCodecProfile::VP9PROFILE_PROFILE3) {
+        exception_state.ThrowDOMException(DOMExceptionCode::kNotSupportedError,
+                                          "Unsupported vp9 profile.");
+        return false;
+      }
+
+      break;
+
+    case media::kCodecH264:
+      if (config->acc_pref == AccelerationPreference::kDeny) {
+        exception_state.ThrowDOMException(DOMExceptionCode::kNotSupportedError,
+                                          "Software h264 is not supported yet");
+        return false;
+      }
+      break;
+
+    default:
+      exception_state.ThrowDOMException(DOMExceptionCode::kNotSupportedError,
+                                        "Unsupported codec type.");
+      return false;
+  }
+
+  return true;
+}
+
 void VideoEncoder::configure(const VideoEncoderConfig* config,
                              ExceptionState& exception_state) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  if (config->height() == 0) {
-    exception_state.ThrowDOMException(DOMExceptionCode::kInvalidStateError,
-                                      "Invalid height.");
+  auto parsed_config = ParseConfig(config, exception_state);
+
+  if (!parsed_config) {
+    DCHECK(exception_state.HadException());
     return;
   }
 
-  if (config->width() == 0) {
-    exception_state.ThrowDOMException(DOMExceptionCode::kInvalidStateError,
-                                      "Invalid width.");
+  if (!VerifyCodecSupport(parsed_config.get(), exception_state)) {
+    DCHECK(exception_state.HadException());
     return;
   }
 
+  // TODO(https://crbug.com/1119892): flush |media_encoder_| if it already
+  // exists, otherwise might could lose frames in flight.
+
   Request* request = MakeGarbageCollected<Request>();
   request->type = Request::Type::kConfigure;
-  request->config = config;
+  request->config = std::move(parsed_config);
   EnqueueRequest(request);
 }
 
@@ -304,86 +413,27 @@ void VideoEncoder::ProcessConfigure(Request* request) {
   DCHECK(request->config);
   DCHECK_EQ(request->type, Request::Type::kConfigure);
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  auto* config = request->config.Get();
-  AccelerationPreference acc_pref = AccelerationPreference::kAllow;
-
-  // TODO(https://crbug.com/1116783): Delete this, allow reconfiguration.
-  if (media_encoder_) {
-    HandleError(DOMExceptionCode::kOperationError,
-                "Encoder has already been congfigured");
-    return;
-  }
 
-  if (config->hasAcceleration()) {
-    std::string preference = IDLEnumAsString(config->acceleration()).Utf8();
-    if (preference == "deny") {
-      acc_pref = AccelerationPreference::kDeny;
-    } else if (preference == "require") {
-      acc_pref = AccelerationPreference::kRequire;
-    } else if (preference == "allow") {
-      acc_pref = AccelerationPreference::kAllow;
-    } else {
-      HandleError(DOMExceptionCode::kNotFoundError,
-                  "Unknown acceleration type");
-      return;
-    }
-  }
+  auto config = std::move(request->config);
 
-  std::string codec_str = config->codec().Utf8();
-  std::string profile_str;
-  if (config->hasProfile())
-    profile_str = config->profile().Utf8();
-  auto codec_type = media::StringToVideoCodec(codec_str);
-  if (codec_type == media::kUnknownVideoCodec) {
-    HandleError(DOMExceptionCode::kNotFoundError, "Unknown codec type");
-    return;
-  }
+  switch (config->codec) {
+    case media::kCodecVP8:
+    case media::kCodecVP9:
+      media_encoder_ = CreateVpxVideoEncoder();
+      break;
 
-  // TODO(ezemtsov): Put the encoder creation logic below in a separate class or
-  // method.
-  media::VideoCodecProfile profile = media::VIDEO_CODEC_PROFILE_UNKNOWN;
-  if (codec_type == media::kCodecVP8) {
-    if (acc_pref == AccelerationPreference::kRequire) {
-      HandleError(DOMExceptionCode::kNotFoundError,
-                  "Accelerated vp8 is not supported");
-      return;
-    }
-    media_encoder_ = CreateVpxVideoEncoder();
-    profile = media::VP8PROFILE_ANY;
-  } else if (codec_type == media::kCodecVP9) {
-    uint8_t level = 0;
-    media::VideoColorSpace color_space;
-    if (!ParseNewStyleVp9CodecID(profile_str, &profile, &level, &color_space)) {
-      HandleError(DOMExceptionCode::kNotFoundError, "Invalid vp9 codec string");
-      return;
-    }
-    if (acc_pref == AccelerationPreference::kRequire) {
-      HandleError(DOMExceptionCode::kNotFoundError,
-                  "Accelerated vp9 is not supported");
-      return;
-    }
-    media_encoder_ = CreateVpxVideoEncoder();
-  } else if (codec_type == media::kCodecH264) {
-    codec_type = media::kCodecH264;
-    uint8_t level = 0;
-    if (!ParseAVCCodecId(profile_str, &profile, &level)) {
-      HandleError(DOMExceptionCode::kNotFoundError, "Invalid AVC profile");
-      return;
-    }
-    if (acc_pref == AccelerationPreference::kDeny) {
-      HandleError(DOMExceptionCode::kNotFoundError,
-                  "Software h264 is not supported yet");
-      return;
-    }
-    media_encoder_ = CreateAcceleratedVideoEncoder();
-  }
+    case media::kCodecH264:
+      media_encoder_ = CreateAcceleratedVideoEncoder();
+      break;
 
-  if (!media_encoder_) {
-    HandleError(DOMExceptionCode::kNotFoundError, "Unsupported codec type");
-    return;
+    default:
+      // This should already have been caught in ParseConfig() and
+      // VerifyCodecSupport().
+      NOTREACHED();
+      break;
   }
 
-  frame_size_ = gfx::Size(config->width(), config->height());
+  frame_size_ = gfx::Size(config->options.width, config->options.height);
 
   auto output_cb = WTF::BindRepeating(&VideoEncoder::MediaEncoderOutputCallback,
                                       WrapWeakPersistent(this));
@@ -401,23 +451,12 @@ void VideoEncoder::ProcessConfigure(Request* request) {
     self->ProcessRequests();
   };
 
-  media::VideoEncoder::Options options;
-
-  // Required configuration.
-  options.height = frame_size_.height();
-  options.width = frame_size_.width();
-  options.framerate = config->framerate();
-
-  // Optional configuration.
-  if (config->hasBitrate())
-    options.bitrate = config->bitrate();
-
   // TODO(https://crbug.com/1116771): Let the encoder figure out its thread
   // count (it knows better).
-  options.threads = 1;
+  config->options.threads = 1;
 
   stall_request_processing_ = true;
-  media_encoder_->Initialize(profile, options, output_cb,
+  media_encoder_->Initialize(config->profile, config->options, output_cb,
                              WTF::Bind(done_callback, WrapWeakPersistent(this),
                                        WrapPersistent(request)));
 }  // namespace blink
@@ -481,10 +520,8 @@ void VideoEncoder::Trace(Visitor* visitor) const {
 }
 
 void VideoEncoder::Request::Trace(Visitor* visitor) const {
-  visitor->Trace(config);
   visitor->Trace(frame);
   visitor->Trace(encodeOpts);
   visitor->Trace(resolver);
 }
-
 }  // namespace blink

third_party/blink/renderer/modules/webcodecs/video_encoder.h

@@ -9,6 +9,9 @@
 
 #include "base/optional.h"
 #include "media/base/status.h"
+#include "media/base/video_codecs.h"
+#include "media/base/video_color_space.h"
+#include "media/base/video_encoder.h"
 #include "third_party/blink/renderer/bindings/core/v8/script_promise_resolver.h"
 #include "third_party/blink/renderer/bindings/modules/v8/v8_video_encoder_output_callback.h"
 #include "third_party/blink/renderer/bindings/modules/v8/v8_web_codecs_error_callback.h"
@@ -59,7 +62,23 @@ class MODULES_EXPORT VideoEncoder final : public ScriptWrappable {
   void Trace(Visitor*) const override;
 
  private:
-  struct Request : public GarbageCollected<Request> {
+  enum class AccelerationPreference { kAllow, kDeny, kRequire };
+
+  // TODO(ezemtsov): Replace this with a {Audio|Video}EncoderConfig.
+  struct ParsedConfig final {
+    void Trace(Visitor*) const;
+
+    media::VideoCodec codec;
+    media::VideoCodecProfile profile;
+    uint8_t level;
+    media::VideoColorSpace color_space;
+
+    AccelerationPreference acc_pref;
+
+    media::VideoEncoder::Options options;
+  };
+
+  struct Request final : public GarbageCollected<Request> {
     enum class Type {
       kConfigure,
       kEncode,
@@ -69,14 +88,12 @@ class MODULES_EXPORT VideoEncoder final : public ScriptWrappable {
     void Trace(Visitor*) const;
 
     Type type;
-    Member<const VideoEncoderConfig> config;             // used by kConfigure
+    std::unique_ptr<ParsedConfig> config;                // used by kConfigure
     Member<VideoFrame> frame;                            // used by kEncode
     Member<const VideoEncoderEncodeOptions> encodeOpts;  // used by kEncode
     Member<ScriptPromiseResolver> resolver;              // used by kFlush
   };
 
-  enum class AccelerationPreference { kAllow, kDeny, kRequire };
-
   void CallOutputCallback(EncodedVideoChunk* chunk);
   void HandleError(DOMException* ex);
   void HandleError(DOMExceptionCode code, const String& message);
@@ -88,6 +105,10 @@ class MODULES_EXPORT VideoEncoder final : public ScriptWrappable {
 
   void MediaEncoderOutputCallback(media::VideoEncoderOutput output);
 
+  std::unique_ptr<ParsedConfig> ParseConfig(const VideoEncoderConfig*,
+                                            ExceptionState&);
+  bool VerifyCodecSupport(ParsedConfig*, ExceptionState&);
+
   gfx::Size frame_size_;
   std::unique_ptr<media::VideoEncoder> media_encoder_;
 

third_party/blink/renderer/modules/webcodecs/video_encoder_config.idl

@@ -13,7 +13,6 @@ enum VideoEncoderAccelerationPreference {
 
 dictionary VideoEncoderConfig {
   required DOMString codec;
-  DOMString profile;
   VideoEncoderAccelerationPreference acceleration = "allow";
 
   unsigned long long bitrate;

third_party/blink/web_tests/external/wpt/webcodecs/video-encoder.html

@@ -31,8 +31,8 @@ async_test(async (t) => {
 
   // VideoEncoderInit has required fields.
   let encoder = new VideoEncoder({
-    output(chunk) { assert_unreached("Unexpected output");  },
-    error(error) { assert_unreached("Unexpected error:" + error); },
+    output(chunk) { t.unreached_func("Unexpected output").call(); },
+    error(error) { t.unreached_func("Unexpected error:" + error).call(); },
   });
   encoder.close();
 
@@ -41,8 +41,8 @@ async_test(async (t) => {
 
 async_test(async (t) => {
   let encoder = new VideoEncoder({
-    output(chunk) { assert_unreached("Unexpected output");  },
-    error(error) { assert_unreached("Unexpected error:" + error); },
+    output(chunk) { t.unreached_func("Unexpected output").call(); },
+    error(error) { t.unreached_func("Unexpected error:" + error).call(); },
   });
 
   const requiredConfigPairs = {
@@ -54,7 +54,7 @@ async_test(async (t) => {
   let incrementalConfig = {};
 
   for (let key in requiredConfigPairs) {
-    // Configure  should fail while required keys are missing.
+    // Configure should fail while required keys are missing.
     assert_throws_js(TypeError, () => { encoder.configure(incrementalConfig); });
     incrementalConfig[key] = requiredConfigPairs[key];
   }
@@ -62,6 +62,27 @@ async_test(async (t) => {
   // Configure should pass once incrementalConfig meets all requirements.
   encoder.configure(incrementalConfig);
 
+  // We should be able to reconfigure the encoder.
+  encoder.configure(incrementalConfig);
+
+  let config = incrementalConfig;
+
+  // Bogus codec rejected.
+  config.codec = 'bogus';
+  assert_throws_js(TypeError, () => { encoder.configure(config); });
+
+  // Audio codec rejected.
+  config.codec = 'vorbis';
+  assert_throws_js(TypeError, () => { encoder.configure(config); });
+
+  // Ambiguous codec rejected.
+  config.codec = 'vp9';
+  assert_throws_js(TypeError, () => { encoder.configure(config); });
+
+  // Codec with mime type rejected.
+  config.codec = 'video/webm; codecs="vp9"';
+  assert_throws_js(TypeError, () => { encoder.configure(config); });
+
   encoder.close();
 
   asyncDone(t);
@@ -69,8 +90,8 @@ async_test(async (t) => {
 
 async_test(async (t) => {
   let encoder = new VideoEncoder({
-    output(chunk) { assert_unreached("Unexpected output"); },
-    error(error) { assert_unreached("Unexpected error:" + error); },
+    output(chunk) { t.unreached_func("Unexpected output").call(); },
+    error(error) { t.unreached_func("Unexpected error:" + error).call(); },
   });
 
   let videoFrame = await createVideoFrame(640, 480, 0);
@@ -93,7 +114,7 @@ async_test(async (t) => {
   let output_chunks = [];
   let encoder = new VideoEncoder({
     output(chunk) { output_chunks.push(chunk); },
-    error(error) { assert_unreached("Unexpected error:" + error); },
+    error(error) { t.unreached_func("Unexpected error:" + error).call(); },
   });
 
   // No encodes yet.
@@ -133,5 +154,75 @@ async_test(async (t) => {
 
   asyncDone(t);
 }, 'Test successful configure(), encode(), and flush()');
+
+async_test(async (t) => {
+  let output_chunks = [];
+  let encoder = new VideoEncoder({
+    output(chunk) { output_chunks.push(chunk); },
+    error(error) { t.unreached_func("Unexpected error:" + error).call(); },
+  });
+
+  // No encodes yet.
+  assert_equals(encoder.encodeQueueSize, 0);
+
+  const config = {
+    codec: 'vp8',
+    framerate: 25,
+    width: 640,
+    height: 480
+  };
+  encoder.configure(config);
+
+  let frame1 = await createVideoFrame(640, 480, 0);
+  let frame2 = await createVideoFrame(640, 480, 33333);
+
+  encoder.encode(frame1);
+  encoder.configure(config);
+
+  encoder.encode(frame2);
+
+  await encoder.flush();
+
+  // We can guarantee that all encodes are processed after a flush.
+  assert_equals(encoder.encodeQueueSize, 0);
+
+  // The first frame may have been dropped when reconfiguring.
+  // This shouldn't happen, and should be fixed/called out in the spec, but
+  // this is preptively added to prevent flakiness.
+  // TODO: Remove these checks when implementations handle this correctly.
+  assert_true(output_chunks.length == 1 || output_chunks.length == 2);
+
+  if (output_chunks.length == 1) {
+    // If we only have one chunk frame, make sure we droped the frame that was
+    // in flight when we reconfigured.
+    assert_equals(output_chunks[0].timestamp, frame2.timestamp);
+  } else {
+    assert_equals(output_chunks[0].timestamp, frame1.timestamp);
+    assert_equals(output_chunks[1].timestamp, frame2.timestamp);
+  }
+
+  output_chunks = [];
+
+  let frame3 = await createVideoFrame(640, 480, 66666);
+  let frame4 = await createVideoFrame(640, 480, 100000);
+
+  encoder.encode(frame3);
+
+  // Verify that a failed call to configure does not change the encoder's state.
+  config.codec = 'bogus';
+  assert_throws_js(TypeError, () => encoder.configure(config));
+
+  encoder.encode(frame4);
+
+  await encoder.flush();
+
+  assert_equals(output_chunks[0].timestamp, frame3.timestamp);
+  assert_equals(output_chunks[1].timestamp, frame4.timestamp);
+
+  encoder.close();
+
+  asyncDone(t);
+}, 'Test successful encode() after re-configure().');
+
 </script>
 </html>

third_party/blink/web_tests/webcodecs/basic_video_encoding.html

@@ -27,7 +27,7 @@ async function createFrame(width, height, ts) {
   return new VideoFrame(imageBitmap, { timestamp: ts });
 }
 
-async function encode_test(codec, profile, acc) {
+async function encode_test(codec, acc) {
   let w = 640;
   let h = 480;
   let next_ts = 0
@@ -49,7 +49,6 @@ async function encode_test(codec, profile, acc) {
   };
   const params = {
     codec : codec,
-    profile : profile,
     acceleration: acc,
     width : w,
     height : h,
@@ -68,13 +67,13 @@ async function encode_test(codec, profile, acc) {
   assert_equals(frames_processed, frames_to_encode);
 }
 
-promise_test(encode_test.bind(null, "vp9", "vp09.00.10.08", "allow"),
+promise_test(encode_test.bind(null, "vp09.00.10.08", "allow"),
     "encoding vp9 profile0");
 
-promise_test(encode_test.bind(null, "vp9", "vp09.02.10.10", "allow"),
+promise_test(encode_test.bind(null, "vp09.02.10.10", "allow"),
     "encoding vp9 profile2");
 
-promise_test(encode_test.bind(null, "vp8", null, "allow"),
+promise_test(encode_test.bind(null, "vp8", "allow"),
     "encoding vp8");
 
 

third_party/blink/web_tests/webcodecs/videoframe-imagebitmap.html

@@ -164,8 +164,7 @@
         };
 
         const videoEncoderConfig = {
-          codec: "vp9",
-          profile: "vp09.00.10.08",
+          codec: "vp09.00.10.08",
           width: width,
           height: height,
           bitrate: 10e6,