Commit ac30370c authored by Gyuyoung Kim's avatar Gyuyoung Kim Committed by Commit Bot

Migrate VideoCaptureService to new Mojo types more

This CL applies new Mojo types to VideoCaptureService interface.

 - Replace MakeStrongBinding with MakeSelfOwnedReceiver.
 - Convert FooRequest to mojo::PendingReceiver.
 - Convert FooPtr to mojo::PendingRemote.

Bug: 955171
Change-Id: I7a3f1663168d2bd954d17e8ce5cfddd56028b5e3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1862920Reviewed-by: default avatarKen Rockot <rockot@google.com>
Reviewed-by: default avatarKinuko Yasuda <kinuko@chromium.org>
Reviewed-by: default avatarLuke Sorenson <lasoren@chromium.org>
Commit-Queue: Gyuyoung Kim <gyuyoung@igalia.com>
Cr-Commit-Position: refs/heads/master@{#706981}
parent 3083c63b
...@@ -15,7 +15,7 @@ ...@@ -15,7 +15,7 @@
#include "content/public/browser/chromeos/delegate_to_browser_gpu_service_accelerator_factory.h" #include "content/public/browser/chromeos/delegate_to_browser_gpu_service_accelerator_factory.h"
#include "content/public/browser/render_frame_host.h" #include "content/public/browser/render_frame_host.h"
#include "content/public/browser/video_capture_service.h" #include "content/public/browser/video_capture_service.h"
#include "mojo/public/cpp/bindings/strong_binding.h" #include "mojo/public/cpp/bindings/self_owned_receiver.h"
#include "services/video_capture/public/mojom/device_factory.mojom.h" #include "services/video_capture/public/mojom/device_factory.mojom.h"
namespace extensions { namespace extensions {
...@@ -97,11 +97,12 @@ void MediaPerceptionAPIDelegateChromeOS::LoadCrOSComponent( ...@@ -97,11 +97,12 @@ void MediaPerceptionAPIDelegateChromeOS::LoadCrOSComponent(
void MediaPerceptionAPIDelegateChromeOS::BindVideoSourceProvider( void MediaPerceptionAPIDelegateChromeOS::BindVideoSourceProvider(
mojo::PendingReceiver<video_capture::mojom::VideoSourceProvider> receiver) { mojo::PendingReceiver<video_capture::mojom::VideoSourceProvider> receiver) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI); DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
video_capture::mojom::AcceleratorFactoryPtr accelerator_factory; mojo::PendingRemote<video_capture::mojom::AcceleratorFactory>
mojo::MakeStrongBinding( accelerator_factory;
mojo::MakeSelfOwnedReceiver(
std::make_unique< std::make_unique<
content::DelegateToBrowserGpuServiceAcceleratorFactory>(), content::DelegateToBrowserGpuServiceAcceleratorFactory>(),
mojo::MakeRequest(&accelerator_factory)); accelerator_factory.InitWithNewPipeAndPassReceiver());
auto& service = content::GetVideoCaptureService(); auto& service = content::GetVideoCaptureService();
service.InjectGpuDependencies(std::move(accelerator_factory)); service.InjectGpuDependencies(std::move(accelerator_factory));
......
...@@ -17,7 +17,8 @@ ...@@ -17,7 +17,8 @@
#include "content/public/browser/video_capture_service.h" #include "content/public/browser/video_capture_service.h"
#include "content/public/common/content_features.h" #include "content/public/common/content_features.h"
#include "mojo/public/cpp/bindings/callback_helpers.h" #include "mojo/public/cpp/bindings/callback_helpers.h"
#include "mojo/public/cpp/bindings/strong_binding.h" #include "mojo/public/cpp/bindings/pending_remote.h"
#include "mojo/public/cpp/bindings/self_owned_receiver.h"
#include "services/video_capture/public/mojom/video_capture_service.mojom.h" #include "services/video_capture/public/mojom/video_capture_service.mojom.h"
#include "services/video_capture/public/uma/video_capture_service_event.h" #include "services/video_capture/public/uma/video_capture_service_event.h"
...@@ -214,12 +215,14 @@ ServiceVideoCaptureProvider::LazyConnectToService() { ...@@ -214,12 +215,14 @@ ServiceVideoCaptureProvider::LazyConnectToService() {
auto ui_task_runner = base::CreateSingleThreadTaskRunner({BrowserThread::UI}); auto ui_task_runner = base::CreateSingleThreadTaskRunner({BrowserThread::UI});
#if defined(OS_CHROMEOS) #if defined(OS_CHROMEOS)
video_capture::mojom::AcceleratorFactoryPtr accelerator_factory; mojo::PendingRemote<video_capture::mojom::AcceleratorFactory>
accelerator_factory;
if (!create_accelerator_factory_cb_) if (!create_accelerator_factory_cb_)
create_accelerator_factory_cb_ = create_accelerator_factory_cb_ =
base::BindRepeating(&CreateAcceleratorFactory); base::BindRepeating(&CreateAcceleratorFactory);
mojo::MakeStrongBinding(create_accelerator_factory_cb_.Run(), mojo::MakeSelfOwnedReceiver(
mojo::MakeRequest(&accelerator_factory)); create_accelerator_factory_cb_.Run(),
accelerator_factory.InitWithNewPipeAndPassReceiver());
GetVideoCaptureService().InjectGpuDependencies( GetVideoCaptureService().InjectGpuDependencies(
std::move(accelerator_factory)); std::move(accelerator_factory));
#endif // defined(OS_CHROMEOS) #endif // defined(OS_CHROMEOS)
......
...@@ -17,6 +17,7 @@ ...@@ -17,6 +17,7 @@
#include "content/public/test/browser_task_environment.h" #include "content/public/test/browser_task_environment.h"
#include "mojo/public/cpp/bindings/binding.h" #include "mojo/public/cpp/bindings/binding.h"
#include "mojo/public/cpp/bindings/binding_set.h" #include "mojo/public/cpp/bindings/binding_set.h"
#include "mojo/public/cpp/bindings/pending_receiver.h"
#include "mojo/public/cpp/bindings/pending_remote.h" #include "mojo/public/cpp/bindings/pending_remote.h"
#include "mojo/public/cpp/bindings/receiver_set.h" #include "mojo/public/cpp/bindings/receiver_set.h"
#include "services/video_capture/public/cpp/mock_push_subscription.h" #include "services/video_capture/public/cpp/mock_push_subscription.h"
...@@ -80,10 +81,12 @@ class ServiceVideoCaptureProviderTest : public testing::Test { ...@@ -80,10 +81,12 @@ class ServiceVideoCaptureProviderTest : public testing::Test {
ON_CALL(mock_video_capture_service_, DoConnectToVideoSourceProvider(_)) ON_CALL(mock_video_capture_service_, DoConnectToVideoSourceProvider(_))
.WillByDefault(Invoke( .WillByDefault(Invoke(
[this](video_capture::mojom::VideoSourceProviderRequest& request) { [this](
mojo::PendingReceiver<video_capture::mojom::VideoSourceProvider>
receiver) {
if (source_provider_binding_.is_bound()) if (source_provider_binding_.is_bound())
source_provider_binding_.Close(); source_provider_binding_.Close();
source_provider_binding_.Bind(std::move(request)); source_provider_binding_.Bind(std::move(receiver));
wait_for_connection_to_service_.Quit(); wait_for_connection_to_service_.Quit();
})); }));
......
...@@ -4,6 +4,8 @@ ...@@ -4,6 +4,8 @@
#include "services/video_capture/public/cpp/mock_video_capture_service.h" #include "services/video_capture/public/cpp/mock_video_capture_service.h"
#include "mojo/public/cpp/bindings/pending_receiver.h"
namespace video_capture { namespace video_capture {
MockVideoCaptureService::MockVideoCaptureService() {} MockVideoCaptureService::MockVideoCaptureService() {}
...@@ -11,19 +13,20 @@ MockVideoCaptureService::MockVideoCaptureService() {} ...@@ -11,19 +13,20 @@ MockVideoCaptureService::MockVideoCaptureService() {}
MockVideoCaptureService::~MockVideoCaptureService() = default; MockVideoCaptureService::~MockVideoCaptureService() = default;
void MockVideoCaptureService::ConnectToDeviceFactory( void MockVideoCaptureService::ConnectToDeviceFactory(
video_capture::mojom::DeviceFactoryRequest request) { mojo::PendingReceiver<video_capture::mojom::DeviceFactory> receiver) {
DoConnectToDeviceFactory(request); DoConnectToDeviceFactory(std::move(receiver));
} }
void MockVideoCaptureService::ConnectToVideoSourceProvider( void MockVideoCaptureService::ConnectToVideoSourceProvider(
video_capture::mojom::VideoSourceProviderRequest request) { mojo::PendingReceiver<video_capture::mojom::VideoSourceProvider> receiver) {
DoConnectToVideoSourceProvider(request); DoConnectToVideoSourceProvider(std::move(receiver));
} }
#if defined(OS_CHROMEOS) #if defined(OS_CHROMEOS)
void MockVideoCaptureService::InjectGpuDependencies( void MockVideoCaptureService::InjectGpuDependencies(
video_capture::mojom::AcceleratorFactoryPtr accelerator_factory) { mojo::PendingRemote<video_capture::mojom::AcceleratorFactory>
DoInjectGpuDependencies(accelerator_factory); accelerator_factory) {
DoInjectGpuDependencies(std::move(accelerator_factory));
} }
#endif // defined(OS_CHROMEOS) #endif // defined(OS_CHROMEOS)
......
...@@ -5,6 +5,7 @@ ...@@ -5,6 +5,7 @@
#ifndef SERVICES_VIDEO_CAPTURE_PUBLIC_CPP_MOCK_VIDEO_CAPTURE_SERVICE_H_ #ifndef SERVICES_VIDEO_CAPTURE_PUBLIC_CPP_MOCK_VIDEO_CAPTURE_SERVICE_H_
#define SERVICES_VIDEO_CAPTURE_PUBLIC_CPP_MOCK_VIDEO_CAPTURE_SERVICE_H_ #define SERVICES_VIDEO_CAPTURE_PUBLIC_CPP_MOCK_VIDEO_CAPTURE_SERVICE_H_
#include "mojo/public/cpp/bindings/pending_receiver.h"
#include "services/video_capture/public/mojom/video_capture_service.mojom.h" #include "services/video_capture/public/mojom/video_capture_service.mojom.h"
#include "testing/gmock/include/gmock/gmock.h" #include "testing/gmock/include/gmock/gmock.h"
...@@ -17,18 +18,22 @@ class MockVideoCaptureService ...@@ -17,18 +18,22 @@ class MockVideoCaptureService
~MockVideoCaptureService() override; ~MockVideoCaptureService() override;
void ConnectToDeviceFactory( void ConnectToDeviceFactory(
video_capture::mojom::DeviceFactoryRequest request) override; mojo::PendingReceiver<video_capture::mojom::DeviceFactory> receiver)
override;
void ConnectToVideoSourceProvider( void ConnectToVideoSourceProvider(
video_capture::mojom::VideoSourceProviderRequest request) override; mojo::PendingReceiver<video_capture::mojom::VideoSourceProvider> receiver)
override;
#if defined(OS_CHROMEOS) #if defined(OS_CHROMEOS)
void InjectGpuDependencies( void InjectGpuDependencies(
video_capture::mojom::AcceleratorFactoryPtr accelerator_factory) override; mojo::PendingRemote<video_capture::mojom::AcceleratorFactory>
accelerator_factory) override;
MOCK_METHOD1( MOCK_METHOD1(
DoInjectGpuDependencies, DoInjectGpuDependencies,
void(video_capture::mojom::AcceleratorFactoryPtr& accelerator_factory)); void(mojo::PendingRemote<video_capture::mojom::AcceleratorFactory>
accelerator_factory));
void ConnectToCameraAppDeviceBridge( void ConnectToCameraAppDeviceBridge(
mojo::PendingReceiver<cros::mojom::CameraAppDeviceBridge>) override {} mojo::PendingReceiver<cros::mojom::CameraAppDeviceBridge>) override {}
...@@ -39,9 +44,11 @@ class MockVideoCaptureService ...@@ -39,9 +44,11 @@ class MockVideoCaptureService
MOCK_METHOD1(SetShutdownDelayInSeconds, void(float seconds)); MOCK_METHOD1(SetShutdownDelayInSeconds, void(float seconds));
MOCK_METHOD1(DoConnectToDeviceFactory, MOCK_METHOD1(DoConnectToDeviceFactory,
void(video_capture::mojom::DeviceFactoryRequest& request)); void(mojo::PendingReceiver<video_capture::mojom::DeviceFactory>
receiver));
MOCK_METHOD1(DoConnectToVideoSourceProvider, MOCK_METHOD1(DoConnectToVideoSourceProvider,
void(video_capture::mojom::VideoSourceProviderRequest& request)); void(mojo::PendingReceiver<
video_capture::mojom::VideoSourceProvider> receiver));
MOCK_METHOD1(SetRetryCount, void(int32_t)); MOCK_METHOD1(SetRetryCount, void(int32_t));
}; };
......
...@@ -31,7 +31,7 @@ interface AcceleratorFactory { ...@@ -31,7 +31,7 @@ interface AcceleratorFactory {
// decoding will be performed without gpu acceleration. // decoding will be performed without gpu acceleration.
interface VideoCaptureService { interface VideoCaptureService {
[EnableIf=is_chromeos] [EnableIf=is_chromeos]
InjectGpuDependencies(AcceleratorFactory accelerator_factory); InjectGpuDependencies(pending_remote<AcceleratorFactory> accelerator_factory);
// Binds a bridge for Chrome OS camera app and device. // Binds a bridge for Chrome OS camera app and device.
[EnableIf=is_chromeos] [EnableIf=is_chromeos]
...@@ -39,10 +39,10 @@ interface VideoCaptureService { ...@@ -39,10 +39,10 @@ interface VideoCaptureService {
pending_receiver<cros.mojom.CameraAppDeviceBridge> receiver); pending_receiver<cros.mojom.CameraAppDeviceBridge> receiver);
// Legacy API. Supports one client per device. // Legacy API. Supports one client per device.
ConnectToDeviceFactory(DeviceFactory& request); ConnectToDeviceFactory(pending_receiver<DeviceFactory> receiver);
// Current API. Supports multiple clients per source. // Current API. Supports multiple clients per source.
ConnectToVideoSourceProvider(VideoSourceProvider& request); ConnectToVideoSourceProvider(pending_receiver<VideoSourceProvider> receiver);
// Sets a retry count that is used by the service for logging UMA events in // Sets a retry count that is used by the service for logging UMA events in
// the context of investigation for https://crbug.com/643068. // the context of investigation for https://crbug.com/643068.
......
...@@ -15,6 +15,9 @@ ...@@ -15,6 +15,9 @@
#include "media/capture/video/video_capture_buffer_pool.h" #include "media/capture/video/video_capture_buffer_pool.h"
#include "media/capture/video/video_capture_buffer_tracker.h" #include "media/capture/video/video_capture_buffer_tracker.h"
#include "media/capture/video/video_capture_system_impl.h" #include "media/capture/video/video_capture_system_impl.h"
#include "mojo/public/cpp/bindings/pending_receiver.h"
#include "mojo/public/cpp/bindings/pending_remote.h"
#include "mojo/public/cpp/bindings/remote.h"
#include "mojo/public/cpp/bindings/self_owned_receiver.h" #include "mojo/public/cpp/bindings/self_owned_receiver.h"
#include "services/video_capture/device_factory_media_to_mojo_adapter.h" #include "services/video_capture/device_factory_media_to_mojo_adapter.h"
#include "services/video_capture/testing_controls_impl.h" #include "services/video_capture/testing_controls_impl.h"
...@@ -55,8 +58,9 @@ class VideoCaptureServiceImpl::GpuDependenciesContext { ...@@ -55,8 +58,9 @@ class VideoCaptureServiceImpl::GpuDependenciesContext {
#if defined(OS_CHROMEOS) #if defined(OS_CHROMEOS)
void InjectGpuDependencies( void InjectGpuDependencies(
mojom::AcceleratorFactoryPtrInfo accelerator_factory_info) { mojo::PendingRemote<mojom::AcceleratorFactory> accelerator_factory_info) {
DCHECK(gpu_io_task_runner_->RunsTasksInCurrentSequence()); DCHECK(gpu_io_task_runner_->RunsTasksInCurrentSequence());
accelerator_factory_.reset();
accelerator_factory_.Bind(std::move(accelerator_factory_info)); accelerator_factory_.Bind(std::move(accelerator_factory_info));
} }
...@@ -80,7 +84,7 @@ class VideoCaptureServiceImpl::GpuDependenciesContext { ...@@ -80,7 +84,7 @@ class VideoCaptureServiceImpl::GpuDependenciesContext {
scoped_refptr<base::SequencedTaskRunner> gpu_io_task_runner_; scoped_refptr<base::SequencedTaskRunner> gpu_io_task_runner_;
#if defined(OS_CHROMEOS) #if defined(OS_CHROMEOS)
mojom::AcceleratorFactoryPtr accelerator_factory_; mojo::Remote<mojom::AcceleratorFactory> accelerator_factory_;
#endif // defined(OS_CHROMEOS) #endif // defined(OS_CHROMEOS)
base::WeakPtrFactory<GpuDependenciesContext> weak_factory_for_gpu_io_thread_{ base::WeakPtrFactory<GpuDependenciesContext> weak_factory_for_gpu_io_thread_{
...@@ -94,7 +98,7 @@ VideoCaptureServiceImpl::VideoCaptureServiceImpl( ...@@ -94,7 +98,7 @@ VideoCaptureServiceImpl::VideoCaptureServiceImpl(
ui_task_runner_(std::move(ui_task_runner)) {} ui_task_runner_(std::move(ui_task_runner)) {}
VideoCaptureServiceImpl::~VideoCaptureServiceImpl() { VideoCaptureServiceImpl::~VideoCaptureServiceImpl() {
factory_bindings_.CloseAllBindings(); factory_receivers_.Clear();
device_factory_.reset(); device_factory_.reset();
#if defined(OS_CHROMEOS) #if defined(OS_CHROMEOS)
...@@ -109,12 +113,12 @@ VideoCaptureServiceImpl::~VideoCaptureServiceImpl() { ...@@ -109,12 +113,12 @@ VideoCaptureServiceImpl::~VideoCaptureServiceImpl() {
#if defined(OS_CHROMEOS) #if defined(OS_CHROMEOS)
void VideoCaptureServiceImpl::InjectGpuDependencies( void VideoCaptureServiceImpl::InjectGpuDependencies(
mojom::AcceleratorFactoryPtr accelerator_factory) { mojo::PendingRemote<mojom::AcceleratorFactory> accelerator_factory) {
LazyInitializeGpuDependenciesContext(); LazyInitializeGpuDependenciesContext();
gpu_dependencies_context_->GetTaskRunner()->PostTask( gpu_dependencies_context_->GetTaskRunner()->PostTask(
FROM_HERE, base::BindOnce(&GpuDependenciesContext::InjectGpuDependencies, FROM_HERE, base::BindOnce(&GpuDependenciesContext::InjectGpuDependencies,
gpu_dependencies_context_->GetWeakPtr(), gpu_dependencies_context_->GetWeakPtr(),
accelerator_factory.PassInterface())); std::move(accelerator_factory)));
} }
void VideoCaptureServiceImpl::ConnectToCameraAppDeviceBridge( void VideoCaptureServiceImpl::ConnectToCameraAppDeviceBridge(
...@@ -125,15 +129,15 @@ void VideoCaptureServiceImpl::ConnectToCameraAppDeviceBridge( ...@@ -125,15 +129,15 @@ void VideoCaptureServiceImpl::ConnectToCameraAppDeviceBridge(
#endif // defined(OS_CHROMEOS) #endif // defined(OS_CHROMEOS)
void VideoCaptureServiceImpl::ConnectToDeviceFactory( void VideoCaptureServiceImpl::ConnectToDeviceFactory(
mojom::DeviceFactoryRequest request) { mojo::PendingReceiver<mojom::DeviceFactory> receiver) {
LazyInitializeDeviceFactory(); LazyInitializeDeviceFactory();
factory_bindings_.AddBinding(device_factory_.get(), std::move(request)); factory_receivers_.Add(device_factory_.get(), std::move(receiver));
} }
void VideoCaptureServiceImpl::ConnectToVideoSourceProvider( void VideoCaptureServiceImpl::ConnectToVideoSourceProvider(
mojom::VideoSourceProviderRequest request) { mojo::PendingReceiver<mojom::VideoSourceProvider> receiver) {
LazyInitializeVideoSourceProvider(); LazyInitializeVideoSourceProvider();
video_source_provider_->AddClient(std::move(request)); video_source_provider_->AddClient(std::move(receiver));
} }
void VideoCaptureServiceImpl::SetRetryCount(int32_t count) { void VideoCaptureServiceImpl::SetRetryCount(int32_t count) {
......
...@@ -9,9 +9,10 @@ ...@@ -9,9 +9,10 @@
#include "base/memory/scoped_refptr.h" #include "base/memory/scoped_refptr.h"
#include "base/threading/thread.h" #include "base/threading/thread.h"
#include "mojo/public/cpp/bindings/binding_set.h"
#include "mojo/public/cpp/bindings/pending_receiver.h" #include "mojo/public/cpp/bindings/pending_receiver.h"
#include "mojo/public/cpp/bindings/pending_remote.h"
#include "mojo/public/cpp/bindings/receiver.h" #include "mojo/public/cpp/bindings/receiver.h"
#include "mojo/public/cpp/bindings/receiver_set.h"
#include "services/video_capture/public/mojom/device_factory.mojom.h" #include "services/video_capture/public/mojom/device_factory.mojom.h"
#include "services/video_capture/public/mojom/video_capture_service.mojom.h" #include "services/video_capture/public/mojom/video_capture_service.mojom.h"
...@@ -34,15 +35,16 @@ class VideoCaptureServiceImpl : public mojom::VideoCaptureService { ...@@ -34,15 +35,16 @@ class VideoCaptureServiceImpl : public mojom::VideoCaptureService {
// mojom::VideoCaptureService implementation. // mojom::VideoCaptureService implementation.
#if defined(OS_CHROMEOS) #if defined(OS_CHROMEOS)
void InjectGpuDependencies( void InjectGpuDependencies(mojo::PendingRemote<mojom::AcceleratorFactory>
mojom::AcceleratorFactoryPtr accelerator_factory) override; accelerator_factory) override;
void ConnectToCameraAppDeviceBridge( void ConnectToCameraAppDeviceBridge(
mojo::PendingReceiver<cros::mojom::CameraAppDeviceBridge> receiver) mojo::PendingReceiver<cros::mojom::CameraAppDeviceBridge> receiver)
override; override;
#endif // defined(OS_CHROMEOS) #endif // defined(OS_CHROMEOS)
void ConnectToDeviceFactory(mojom::DeviceFactoryRequest request) override; void ConnectToDeviceFactory(
mojo::PendingReceiver<mojom::DeviceFactory> receiver) override;
void ConnectToVideoSourceProvider( void ConnectToVideoSourceProvider(
mojom::VideoSourceProviderRequest request) override; mojo::PendingReceiver<mojom::VideoSourceProvider> receiver) override;
void SetRetryCount(int32_t count) override; void SetRetryCount(int32_t count) override;
void BindControlsForTesting( void BindControlsForTesting(
mojo::PendingReceiver<mojom::TestingControls> receiver) override; mojo::PendingReceiver<mojom::TestingControls> receiver) override;
...@@ -56,7 +58,7 @@ class VideoCaptureServiceImpl : public mojom::VideoCaptureService { ...@@ -56,7 +58,7 @@ class VideoCaptureServiceImpl : public mojom::VideoCaptureService {
void OnLastSourceProviderClientDisconnected(); void OnLastSourceProviderClientDisconnected();
mojo::Receiver<mojom::VideoCaptureService> receiver_; mojo::Receiver<mojom::VideoCaptureService> receiver_;
mojo::BindingSet<mojom::DeviceFactory> factory_bindings_; mojo::ReceiverSet<mojom::DeviceFactory> factory_receivers_;
std::unique_ptr<VirtualDeviceEnabledDeviceFactory> device_factory_; std::unique_ptr<VirtualDeviceEnabledDeviceFactory> device_factory_;
std::unique_ptr<VideoSourceProviderImpl> video_source_provider_; std::unique_ptr<VideoSourceProviderImpl> video_source_provider_;
std::unique_ptr<GpuDependenciesContext> gpu_dependencies_context_; std::unique_ptr<GpuDependenciesContext> gpu_dependencies_context_;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment