Copy and destroy frames on encode()

This CL changes the behavior of VideoEncoder to make an internal copy
of frames passed in through encode(), before destroying the passed
frame. This means that users don't need to call destroy() after passing
frames to encode(), which could result in a frame being invalidated
before we send it to the internal encoder. It also means we can destroy
the frame internally when we no longer need it without risking that it
might still be in use by the user.

Users that which to keep their frames alive after giving them to the
encoder can send a cloned copy instead.

Bug: 1108023
Change-Id: Ie5cd98d688a53314ac16a6a3ac7afe2a65d742f8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2363863
Commit-Queue: Thomas Guilbert <tguilbert@chromium.org>
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#800296}

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

@@ -259,17 +259,36 @@ void VideoEncoder::encode(VideoFrame* frame,
                                       "Encoder is not configured yet.");
     return;
   }
-  if (frame->cropWidth() != uint32_t{frame_size_.width()} ||
-      frame->cropHeight() != uint32_t{frame_size_.height()}) {
+
+  // This will fail if |frame| is already destroyed.
+  auto* internal_frame = frame->clone(exception_state);
+
+  if (!internal_frame) {
+    // Set a more helpful exception than the cloning error message.
+    exception_state.ClearException();
+    exception_state.ThrowDOMException(DOMExceptionCode::kOperationError,
+                                      "Cannot encode destroyed frame.");
+    return;
+  }
+
+  if (internal_frame->cropWidth() != uint32_t{frame_size_.width()} ||
+      internal_frame->cropHeight() != uint32_t{frame_size_.height()}) {
     exception_state.ThrowDOMException(
         DOMExceptionCode::kOperationError,
         "Frame size doesn't match initial encoder parameters.");
+
+    // Free the temporary clone.
+    internal_frame->destroy();
     return;
   }
 
+  // At this point, we have "consumed" the frame, and will destroy the clone in
+  // ProcessEncode().
+  frame->destroy();
+
   Request* request = MakeGarbageCollected<Request>();
   request->type = Request::Type::kEncode;
-  request->frame = frame;
+  request->frame = internal_frame;
   request->encodeOpts = opts;
   ++requested_encodes_;
   return EnqueueRequest(request);
@@ -407,6 +426,9 @@ void VideoEncoder::ProcessEncode(Request* request) {
   media_encoder_->Encode(frame, keyframe,
                          WTF::Bind(done_callback, WrapWeakPersistent(this),
                                    WrapPersistentIfNeeded(request)));
+
+  // We passed a copy of frame() above, so this should be safe to destroy here.
+  request->frame->destroy();
 }
 
 void VideoEncoder::ProcessConfigure(Request* request) {

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

@@ -134,8 +134,8 @@ async_test(async (t) => {
   let frame1 = await createVideoFrame(640, 480, 0);
   let frame2 = await createVideoFrame(640, 480, 33333);
 
-  encoder.encode(frame1);
-  encoder.encode(frame2);
+  encoder.encode(frame1.clone());
+  encoder.encode(frame2.clone());
 
   // Could be 0, 1, or 2. We can't guarantee this check runs before the UA has
   // processed the encodes.
@@ -176,10 +176,10 @@ async_test(async (t) => {
   let frame1 = await createVideoFrame(640, 480, 0);
   let frame2 = await createVideoFrame(640, 480, 33333);
 
-  encoder.encode(frame1);
+  encoder.encode(frame1.clone());
   encoder.configure(config);
 
-  encoder.encode(frame2);
+  encoder.encode(frame2.clone());
 
   await encoder.flush();
 
@@ -206,13 +206,13 @@ async_test(async (t) => {
   let frame3 = await createVideoFrame(640, 480, 66666);
   let frame4 = await createVideoFrame(640, 480, 100000);
 
-  encoder.encode(frame3);
+  encoder.encode(frame3.clone());
 
   // 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);
+  encoder.encode(frame4.clone());
 
   await encoder.flush();
 
@@ -224,5 +224,38 @@ async_test(async (t) => {
   asyncDone(t);
 }, 'Test successful encode() after re-configure().');
 
+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(); },
+  });
+
+  let timestamp = 33333;
+  let frame = await createVideoFrame(640, 480, timestamp);
+
+  t.step(() => {
+    // No encodes yet.
+    assert_equals(encoder.encodeQueueSize, 0);
+
+    const config = {
+      codec: 'vp8',
+      framerate: 25,
+      width: 640,
+      height: 480
+    };
+    encoder.configure(config);
+
+    encoder.encode(frame);
+
+    assert_not_equals(frame.timestamp, timestamp);
+    assert_throws_dom("InvalidStateError", () => frame.clone());
+
+    encoder.close();
+  });
+
+  asyncDone(t);
+}, 'Test encoder consumes (destroys) frames.');
+
 </script>
 </html>