Skip to content

Commit

Permalink
[ImageCapture] Do not reject grabFrame() on muted tracks.
Browse files Browse the repository at this point in the history 
This makes grabFrame() align better with the spec.
To avoid regressions, grabFrame() will reject after a timeout if
no frame is ever delivered to grabFrame().

Bug: 1462012
Change-Id: Id831e8cc50ea52a3a32d91638ec733ece03cef6d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4936177
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Guido Urdaneta <guidou@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1209844}
  • Loading branch information
Guido Urdaneta authored and Chromium LUCI CQ committed Oct 14, 2023
1 parent 5fd9ab9 commit 1288c07
Show file tree
Hide file tree
Showing 5 changed files with 84 additions and 18 deletions.
12 changes: 8 additions & 4 deletions third_party/blink/renderer/modules/imagecapture/image_capture.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

#include "base/containers/contains.h"
#include "base/functional/callback_helpers.h"
#include "base/time/time.h"
#include "base/trace_event/trace_event.h"
#include "base/types/strong_alias.h"
#include "mojo/public/cpp/bindings/pending_remote.h"
Expand Down Expand Up @@ -503,7 +504,7 @@ bool TrackIsInactive(const MediaStreamTrack& track) {
// Spec instructs to return an exception if the Track's readyState() is not
// "live". Also reject if the track is disabled or muted.
// TODO(https://crbug.com/1462012): Do not consider muted tracks inactive.
return track.readyState() != "live" || !track.enabled() || track.muted();
return track.readyState() != "live" || !track.enabled();
}

BackgroundBlurMode ParseBackgroundBlur(bool blink_mode) {
Expand Down Expand Up @@ -1569,7 +1570,8 @@ ScriptPromise ImageCapture::grabFrame(ScriptState* script_state) {
frame_grabber_->GrabFrame(stream_track_->Component(),
std::move(resolver_callback_adapter),
ExecutionContext::From(script_state)
->GetTaskRunner(TaskType::kDOMManipulation));
->GetTaskRunner(TaskType::kDOMManipulation),
grab_frame_timeout_);

return promise;
}
Expand Down Expand Up @@ -1870,7 +1872,8 @@ void ImageCapture::GetMediaTrackSettings(MediaTrackSettings* settings) const {
ImageCapture::ImageCapture(ExecutionContext* context,
MediaStreamTrack* track,
bool pan_tilt_zoom_allowed,
base::OnceClosure initialized_callback)
base::OnceClosure initialized_callback,
base::TimeDelta grab_frame_timeout)
: ExecutionContextLifecycleObserver(context),
stream_track_(track),
service_(context),
Expand All @@ -1881,7 +1884,8 @@ ImageCapture::ImageCapture(ExecutionContext* context,
permission_observer_receiver_(this, context),
capabilities_(MediaTrackCapabilities::Create()),
settings_(MediaTrackSettings::Create()),
photo_settings_(PhotoSettings::Create()) {
photo_settings_(PhotoSettings::Create()),
grab_frame_timeout_(grab_frame_timeout) {
TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("video_and_image_capture"),
"ImageCapture::CreateImageCapture");
DCHECK(stream_track_);
Expand Down
6 changes: 5 additions & 1 deletion third_party/blink/renderer/modules/imagecapture/image_capture.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

#include <memory>

#include "base/time/time.h"
#include "media/capture/mojom/image_capture.mojom-blink.h"
#include "third_party/abseil-cpp/absl/types/optional.h"
#include "third_party/blink/public/mojom/permissions/permission.mojom-blink.h"
Expand Down Expand Up @@ -49,7 +50,8 @@ class MODULES_EXPORT ImageCapture final
ImageCapture(ExecutionContext*,
MediaStreamTrack*,
bool pan_tilt_zoom_allowed,
base::OnceClosure initialized_callback);
base::OnceClosure initialized_callback,
base::TimeDelta grab_frame_timeout = base::Seconds(2));
~ImageCapture() override;

// ExecutionContextLifecycleObserver
Expand Down Expand Up @@ -191,6 +193,8 @@ class MODULES_EXPORT ImageCapture final
Member<PhotoCapabilities> photo_capabilities_;

HeapHashSet<Member<ScriptPromiseResolver>> service_requests_;

const base::TimeDelta grab_frame_timeout_;
};

} // namespace blink
Expand Down
43 changes: 38 additions & 5 deletions third_party/blink/renderer/modules/imagecapture/image_capture_frame_grabber.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,11 @@

#include "third_party/blink/renderer/modules/imagecapture/image_capture_frame_grabber.h"

#include "base/synchronization/lock.h"
#include "base/task/bind_post_task.h"
#include "base/task/single_thread_task_runner.h"
#include "base/thread_annotations.h"
#include "base/time/time.h"
#include "cc/paint/skia_paint_canvas.h"
#include "media/base/video_frame.h"
#include "media/base/video_types.h"
Expand All @@ -17,6 +20,7 @@
#include "third_party/blink/renderer/platform/graphics/video_frame_image_util.h"
#include "third_party/blink/renderer/platform/mediastream/media_stream_component.h"
#include "third_party/blink/renderer/platform/mediastream/media_stream_source.h"
#include "third_party/blink/renderer/platform/scheduler/public/post_cancellable_task.h"
#include "third_party/blink/renderer/platform/scheduler/public/post_cross_thread_task.h"
#include "third_party/blink/renderer/platform/wtf/cross_thread_copier_base.h"
#include "third_party/blink/renderer/platform/wtf/cross_thread_functional.h"
Expand Down Expand Up @@ -70,6 +74,8 @@ class ImageCaptureFrameGrabber::SingleShotFrameHandler
SingleShotFrameHandler(const SingleShotFrameHandler&) = delete;
SingleShotFrameHandler& operator=(const SingleShotFrameHandler&) = delete;

~SingleShotFrameHandler();

// Receives a |frame| and converts its pixels into a SkImage via an internal
// PaintSurface and SkPixmap. Alpha channel, if any, is copied.
void OnVideoFrameOnIOThread(
Expand All @@ -84,14 +90,24 @@ class ImageCaptureFrameGrabber::SingleShotFrameHandler
void ConvertAndDeliverFrame(SkImageDeliverCB callback,
scoped_refptr<media::VideoFrame> frame);

base::Lock lock_;
// Null once the initial frame has been queued for delivery.
SkImageDeliverCB deliver_cb_;
SkImageDeliverCB deliver_cb_ GUARDED_BY(lock_);
};

ImageCaptureFrameGrabber::SingleShotFrameHandler::~SingleShotFrameHandler() {
base::AutoLock locker(lock_);
if (deliver_cb_) {
// Reject the promise if no frame was received.
std::move(deliver_cb_).Run(nullptr);
}
}

void ImageCaptureFrameGrabber::SingleShotFrameHandler::OnVideoFrameOnIOThread(
scoped_refptr<base::SingleThreadTaskRunner> task_runner,
scoped_refptr<media::VideoFrame> frame,
base::TimeTicks /*current_time*/) {
base::AutoLock locker(lock_);
if (!deliver_cb_)
return;

Expand Down Expand Up @@ -284,17 +300,15 @@ void ImageCaptureFrameGrabber::SingleShotFrameHandler::ConvertAndDeliverFrame(
std::move(callback).Run(surface->makeImageSnapshot());
}

ImageCaptureFrameGrabber::ImageCaptureFrameGrabber()
: frame_grab_in_progress_(false) {}

ImageCaptureFrameGrabber::~ImageCaptureFrameGrabber() {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
}

void ImageCaptureFrameGrabber::GrabFrame(
MediaStreamComponent* component,
std::unique_ptr<ImageCaptureGrabFrameCallbacks> callbacks,
scoped_refptr<base::SingleThreadTaskRunner> task_runner) {
scoped_refptr<base::SingleThreadTaskRunner> task_runner,
base::TimeDelta timeout) {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
DCHECK(!!callbacks);

Expand All @@ -317,6 +331,15 @@ void ImageCaptureFrameGrabber::GrabFrame(
// is being processed, which might be further held up if UI is busy, see
// https://crbug.com/623042.
frame_grab_in_progress_ = true;

// Fail the grabFrame request if no frame is received for some time to prevent
// the promise from hanging indefinitely if no frame is ever produced.
timeout_task_handle_ = PostDelayedCancellableTask(
*task_runner, FROM_HERE,
WTF::BindOnce(&ImageCaptureFrameGrabber::OnTimeout,
weak_factory_.GetWeakPtr()),
timeout);

MediaStreamVideoSink::ConnectToTrack(
WebMediaStreamTrack(component),
ConvertToBaseRepeatingCallback(CrossThreadBindRepeating(
Expand All @@ -334,6 +357,7 @@ void ImageCaptureFrameGrabber::OnSkImage(
sk_sp<SkImage> image) {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);

timeout_task_handle_.Cancel();
MediaStreamVideoSink::DisconnectFromTrack();
frame_grab_in_progress_ = false;
if (image)
Expand All @@ -342,4 +366,13 @@ void ImageCaptureFrameGrabber::OnSkImage(
callbacks.PassCallbacks()->OnError();
}

void ImageCaptureFrameGrabber::OnTimeout() {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);

if (frame_grab_in_progress_) {
MediaStreamVideoSink::DisconnectFromTrack();
frame_grab_in_progress_ = false;
}
}

} // namespace blink
10 changes: 7 additions & 3 deletions third_party/blink/renderer/modules/imagecapture/image_capture_frame_grabber.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "third_party/blink/public/platform/web_callbacks.h"
#include "third_party/blink/public/web/modules/mediastream/media_stream_video_sink.h"
#include "third_party/blink/renderer/bindings/core/v8/callback_promise_adapter.h"
#include "third_party/blink/renderer/platform/scheduler/public/post_cancellable_task.h"
#include "third_party/skia/include/core/SkRefCnt.h"

class SkImage;
Expand Down Expand Up @@ -126,7 +127,7 @@ using ImageCaptureGrabFrameCallbacks =
// OnSkBitmap(). This class is single threaded throughout.
class ImageCaptureFrameGrabber final : public MediaStreamVideoSink {
public:
ImageCaptureFrameGrabber();
ImageCaptureFrameGrabber() = default;

ImageCaptureFrameGrabber(const ImageCaptureFrameGrabber&) = delete;
ImageCaptureFrameGrabber& operator=(const ImageCaptureFrameGrabber&) = delete;
Expand All @@ -135,17 +136,20 @@ class ImageCaptureFrameGrabber final : public MediaStreamVideoSink {

void GrabFrame(MediaStreamComponent* component,
std::unique_ptr<ImageCaptureGrabFrameCallbacks> callbacks,
scoped_refptr<base::SingleThreadTaskRunner> task_runner);
scoped_refptr<base::SingleThreadTaskRunner> task_runner,
base::TimeDelta timeout);

private:
// Internal class to receive, convert and forward one frame.
class SingleShotFrameHandler;

void OnSkImage(ScopedWebCallbacks<ImageCaptureGrabFrameCallbacks> callbacks,
sk_sp<SkImage> image);
void OnTimeout();

// Flag to indicate that there is a frame grabbing in progress.
bool frame_grab_in_progress_;
bool frame_grab_in_progress_ = false;
TaskHandle timeout_task_handle_;

THREAD_CHECKER(thread_checker_);
base::WeakPtrFactory<ImageCaptureFrameGrabber> weak_factory_{this};
Expand Down
31 changes: 26 additions & 5 deletions third_party/blink/renderer/modules/imagecapture/image_capture_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include "third_party/blink/renderer/modules/imagecapture/image_capture.h"

#include "base/time/time.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/blink/public/web/web_heap.h"
Expand Down Expand Up @@ -641,13 +642,16 @@ class ImageCaptureTest : public testing::Test {
/*execution_context=*/nullptr,
track_,
/*pan_tilt_zoom_allowed=*/true,
base::DoNothing())) {
base::DoNothing(),
base::Milliseconds(1))) {
track_->SetComponent(component_);
}

void TearDown() override { WebHeap::CollectAllGarbageForTesting(); }

void SetupTrackMocks(V8TestingScope& scope) {
void SetupTrackMocks(V8TestingScope& scope,
bool produce_frame_on_add_sink = true) {
produce_frame_on_add_sink_ = produce_frame_on_add_sink;
source_ = std::make_unique<MediaStreamVideoCapturerSource>(
scope.GetFrame().GetTaskRunner(TaskType::kInternalMediaRealTime),
&scope.GetFrame(),
Expand All @@ -667,8 +671,10 @@ class ImageCaptureTest : public testing::Test {
MediaStreamVideoSink::IsSecure is_secure,
MediaStreamVideoSink::UsesAlpha uses_alpha) {
platform_track_->AddSink(sink, callback, is_secure, uses_alpha);
callback.Run(media::VideoFrame::CreateBlackFrame(gfx::Size(1, 1)),
/*estimated_capture_time=*/base::TimeTicks());
if (produce_frame_on_add_sink_) {
callback.Run(media::VideoFrame::CreateBlackFrame(gfx::Size(1, 1)),
/*estimated_capture_time=*/base::TimeTicks());
}
}));
}

Expand All @@ -679,6 +685,7 @@ class ImageCaptureTest : public testing::Test {
ScopedTestingPlatformSupport<IOTaskRunnerTestingPlatformSupport> platform_;
std::unique_ptr<MediaStreamVideoCapturerSource> source_;
std::unique_ptr<MediaStreamVideoTrack> platform_track_;
bool produce_frame_on_add_sink_ = true;
};

class ImageCaptureConstraintTest : public ImageCaptureTest {
Expand Down Expand Up @@ -1576,7 +1583,7 @@ TEST_F(ImageCaptureTest, GrabFrameOfLiveTrackIsFulfilled) {
EXPECT_TRUE(tester.IsFulfilled());
}

TEST_F(ImageCaptureTest, GrabFrameOfMutedTrackRejects) {
TEST_F(ImageCaptureTest, GrabFrameOfMutedTrackIsFulfilled) {
V8TestingScope scope;
SetupTrackMocks(scope);
track_->SetReadyState("live");
Expand All @@ -1585,6 +1592,20 @@ TEST_F(ImageCaptureTest, GrabFrameOfMutedTrackRejects) {

ScriptPromise result = image_capture_->grabFrame(scope.GetScriptState());

ScriptPromiseTester tester(scope.GetScriptState(), result);
tester.WaitUntilSettled();
EXPECT_TRUE(tester.IsFulfilled());
}

TEST_F(ImageCaptureTest, GrabFrameOfMutedTrackWithoutFramesIsRejected) {
V8TestingScope scope;
SetupTrackMocks(scope, /*produce_frame_on_add_sink=*/false);
track_->SetReadyState("live");
track_->setEnabled(true);
track_->SetMuted(true);

ScriptPromise result = image_capture_->grabFrame(scope.GetScriptState());

ScriptPromiseTester tester(scope.GetScriptState(), result);
tester.WaitUntilSettled();
EXPECT_TRUE(tester.IsRejected());
Expand Down

0 comments on commit 1288c07

Please sign in to comment.