Commit 058241f9 authored by Michael Spang's avatar Michael Spang Committed by Commit Bot

ozone: scenic: Enable vulkan presentation to scenic via magma surface

Using the VK_LAYER_GOOGLE_image_pipe_swapchain layer we can create a
swapchain for an image pipe and assign it as the texture of a scenic
window.

This does not work from the viz process yet as we would need to plumb the
image pipe handle over from the browser using FIDL or mojo. Rendering
needs to be done from the UI thread via VulkanBrowserCompositorOutputSurface
or a test app such as ozone_demo.

This also will not work until vulkan synchronization is fixed, see
https://crbug.com/877684

Bug: 861853
Test: content_shell --ozone-platform=scenic --enable-vulkan --disable-gpu

Change-Id: I79ac045c6d6deb20f49023c8e8e9492631700703
Reviewed-on: https://chromium-review.googlesource.com/1188974
Commit-Queue: Michael Spang <spang@chromium.org>
Reviewed-by: default avatarWez <wez@chromium.org>
Reviewed-by: default avatarVictor Miura <vmiura@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589008}
parent dfc6b1dc
......@@ -188,13 +188,14 @@ def BuildManifest(root_dir, out_dir, app_name, app_filename,
expanded_files = expanded_files.union(dist_libs)
# Format and write out the manifest contents.
gen_dir = os.path.join(out_dir, "gen")
app_found = False
for current_file in expanded_files:
if _IsBinary(current_file):
current_file = _GetStrippedPath(current_file)
in_package_path = MakePackagePath(os.path.join(out_dir, current_file),
[root_dir, out_dir])
[gen_dir, root_dir, out_dir])
if in_package_path == app_filename:
in_package_path = 'bin/app'
app_found = True
......
......@@ -61,6 +61,11 @@ if (enable_vulkan) {
"//base",
"//ui/gfx",
]
data_deps = []
if (is_fuchsia) {
data_deps += [ "//third_party/fuchsia-sdk:vulkan_layers" ]
}
}
# TODO(cblume): These tests should run on each platform -- crbug.com/858614
......
......@@ -46,7 +46,8 @@ VulkanInstance::~VulkanInstance() {
}
bool VulkanInstance::Initialize(
const std::vector<const char*>& required_extensions) {
const std::vector<const char*>& required_extensions,
const std::vector<const char*>& required_layers) {
DCHECK(!vk_instance_);
VulkanFunctionPointers* vulkan_function_pointers =
......@@ -124,6 +125,9 @@ bool VulkanInstance::Initialize(
enabled_layer_names.push_back(layer_property.layerName);
}
#endif
enabled_layer_names.insert(std::end(enabled_layer_names),
std::begin(required_layers),
std::end(required_layers));
VkInstanceCreateInfo instance_create_info = {};
instance_create_info.sType = VK_STRUCTURE_TYPE_INSTANCE_CREATE_INFO;
......
......@@ -21,7 +21,13 @@ class VULKAN_EXPORT VulkanInstance {
~VulkanInstance();
bool Initialize(const std::vector<const char*>& required_extensions);
// Creates the vulkan instance.
//
// The extensions in |required_extensions| and the layers in |required_layers|
// will be enabled in the created instance. See the "Extended Functionality"
// section of vulkan specification for more information.
bool Initialize(const std::vector<const char*>& required_extensions,
const std::vector<const char*>& required_layers);
void Destroy();
......
......@@ -34,7 +34,7 @@ bool VulkanImplementationX11::InitializeVulkanInstance() {
if (!vulkan_function_pointers->vulkan_loader_library_)
return false;
if (!vulkan_instance_.Initialize(required_extensions)) {
if (!vulkan_instance_.Initialize(required_extensions, {})) {
vulkan_instance_.Destroy();
return false;
}
......
......@@ -428,3 +428,19 @@ fuchsia_sdk_pkg("zx") {
"vmo.cpp",
]
}
copy("vulkan_layers") {
sources = [
"sdk/pkg/vulkan_layers/data/vulkan/explicit_layer.d/VkLayer_core_validation.json",
"sdk/pkg/vulkan_layers/data/vulkan/explicit_layer.d/VkLayer_image_pipe_swapchain.json",
"sdk/pkg/vulkan_layers/data/vulkan/explicit_layer.d/VkLayer_object_tracker.json",
"sdk/pkg/vulkan_layers/data/vulkan/explicit_layer.d/VkLayer_parameter_validation.json",
"sdk/pkg/vulkan_layers/data/vulkan/explicit_layer.d/VkLayer_standard_validation.json",
"sdk/pkg/vulkan_layers/data/vulkan/explicit_layer.d/VkLayer_threading.json",
"sdk/pkg/vulkan_layers/data/vulkan/explicit_layer.d/VkLayer_unique_objects.json",
]
outputs = [
"${root_gen_dir}/data/vulkan/explicit_layer.d/{{source_file_part}}",
]
}
......@@ -31,7 +31,7 @@ bool VulkanImplementationGbm::InitializeVulkanInstance() {
"VK_KHR_external_fence_capabilities",
"VK_KHR_get_physical_device_properties2",
};
if (!vulkan_instance_.Initialize(required_extensions)) {
if (!vulkan_instance_.Initialize(required_extensions, {})) {
vulkan_instance_.Destroy();
return false;
}
......
......@@ -4,6 +4,10 @@
assert(is_fuchsia)
import("//gpu/vulkan/features.gni")
visibility = [ "//ui/ozone/*" ]
source_set("scenic") {
sources = [
"client_native_pixmap_factory_scenic.cc",
......@@ -44,4 +48,13 @@ source_set("scenic") {
"//ui/ozone/common",
"//ui/platform_window",
]
if (enable_vulkan) {
sources += [
"vulkan_implementation_scenic.cc",
"vulkan_implementation_scenic.h",
"vulkan_magma.h",
]
defines += [ "VK_USE_PLATFORM_MAGMA_KHR" ]
}
}
......@@ -103,6 +103,21 @@ ScenicSession::ResourceId ScenicSession::CreateImage(
return image_id;
}
ScenicSession::ResourceId ScenicSession::CreateImagePipe(
fidl::InterfaceRequest<fuchsia::images::ImagePipe> request) {
fuchsia::ui::gfx::ImagePipeArgs image_pipe;
image_pipe.image_pipe_request = std::move(request);
fuchsia::ui::gfx::ResourceArgs resource;
resource.set_image_pipe(std::move(image_pipe));
ResourceId image_pipe_id = AllocateResourceId();
EnqueueGfxCommand(
NewCreateResourceCommand(image_pipe_id, std::move(resource)));
return image_pipe_id;
}
ScenicSession::ResourceId ScenicSession::ImportResource(
fuchsia::ui::gfx::ImportSpec spec,
zx::eventpair import_token) {
......
......@@ -59,6 +59,8 @@ class ScenicSession : public fuchsia::ui::scenic::SessionListener {
ResourceId CreateImage(ResourceId memory_id,
ResourceId memory_offset,
fuchsia::images::ImageInfo info);
ResourceId CreateImagePipe(
fidl::InterfaceRequest<fuchsia::images::ImagePipe> request);
ResourceId ImportResource(fuchsia::ui::gfx::ImportSpec spec,
zx::eventpair import_token);
ResourceId CreateEntityNode();
......
......@@ -15,6 +15,10 @@
#include "ui/ozone/platform/scenic/scenic_window_canvas.h"
#include "ui/ozone/platform/scenic/scenic_window_manager.h"
#if BUILDFLAG(ENABLE_VULKAN)
#include "ui/ozone/platform/scenic/vulkan_implementation_scenic.h"
#endif
namespace ui {
namespace {
......@@ -100,4 +104,11 @@ scoped_refptr<gfx::NativePixmap> ScenicSurfaceFactory::CreateNativePixmap(
return new ScenicPixmap(widget, size, format);
}
#if BUILDFLAG(ENABLE_VULKAN)
std::unique_ptr<gpu::VulkanImplementation>
ScenicSurfaceFactory::CreateVulkanImplementation() {
return std::make_unique<ui::VulkanImplementationScenic>(window_manager_);
}
#endif
} // namespace ui
......@@ -9,6 +9,7 @@
#include <vector>
#include "base/macros.h"
#include "gpu/vulkan/buildflags.h"
#include "ui/ozone/public/gl_ozone.h"
#include "ui/ozone/public/surface_factory_ozone.h"
......@@ -33,6 +34,10 @@ class ScenicSurfaceFactory : public SurfaceFactoryOzone {
gfx::Size size,
gfx::BufferFormat format,
gfx::BufferUsage usage) override;
#if BUILDFLAG(ENABLE_VULKAN)
std::unique_ptr<gpu::VulkanImplementation> CreateVulkanImplementation()
override;
#endif
private:
ScenicWindowManager* const window_manager_;
......
......@@ -225,6 +225,7 @@ void ScenicWindow::UpdateSize() {
scenic_session_.SetNodeShape(shape_id_, rect_id);
scenic_session_.SetNodeTranslation(shape_id_, translation);
scenic_session_.ReleaseResource(rect_id);
scenic_session_.Present();
delegate_->OnBoundsChanged(size_rect);
}
......
......@@ -43,8 +43,7 @@ class OZONE_EXPORT ScenicWindow : public PlatformWindow,
ScenicSession::ResourceId node_id() const { return node_id_; }
float device_pixel_ratio() const { return device_pixel_ratio_; }
// Overrides texture of the window. This is used by ScenicWindowCanvas.
// TODO(spang): Deprecate software rendering on fuchsia.
// Sets texture of the window to a scenic resource.
void SetTexture(ScenicSession::ResourceId texture);
// PlatformWindow implementation.
......
// Copyright 2018 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include "ui/ozone/platform/scenic/vulkan_implementation_scenic.h"
#include <lib/zx/channel.h>
#include "base/files/file_path.h"
#include "base/fuchsia/fuchsia_logging.h"
#include "base/native_library.h"
#include "gpu/vulkan/vulkan_function_pointers.h"
#include "gpu/vulkan/vulkan_instance.h"
#include "gpu/vulkan/vulkan_surface.h"
#include "ui/gfx/gpu_fence.h"
#include "ui/ozone/platform/scenic/scenic_window.h"
#include "ui/ozone/platform/scenic/scenic_window_manager.h"
#include "ui/ozone/platform/scenic/vulkan_magma.h"
namespace ui {
VulkanImplementationScenic::VulkanImplementationScenic(
ScenicWindowManager* scenic_window_manager)
: scenic_window_manager_(scenic_window_manager) {}
VulkanImplementationScenic::~VulkanImplementationScenic() = default;
bool VulkanImplementationScenic::InitializeVulkanInstance() {
base::NativeLibraryLoadError error;
base::NativeLibrary handle =
base::LoadNativeLibrary(base::FilePath("libvulkan.so"), &error);
if (!handle) {
LOG(ERROR) << "Failed to load vulkan: " << error.ToString();
return false;
}
gpu::VulkanFunctionPointers* vulkan_function_pointers =
gpu::GetVulkanFunctionPointers();
vulkan_function_pointers->vulkan_loader_library_ = handle;
std::vector<const char*> required_extensions = {
VK_KHR_SURFACE_EXTENSION_NAME, VK_KHR_MAGMA_SURFACE_EXTENSION_NAME,
};
std::vector<const char*> required_layers = {
"VK_LAYER_GOOGLE_image_pipe_swapchain",
};
if (!vulkan_instance_.Initialize(required_extensions, required_layers)) {
vulkan_instance_.Destroy();
return false;
}
vkCreateMagmaSurfaceKHR_ = vkGetInstanceProcAddr(
vulkan_instance_.vk_instance(), "vkCreateMagmaSurfaceKHR");
if (!vkCreateMagmaSurfaceKHR_) {
vulkan_instance_.Destroy();
return false;
}
vkGetPhysicalDeviceMagmaPresentationSupportKHR_ =
vkGetInstanceProcAddr(vulkan_instance_.vk_instance(),
"vkGetPhysicalDeviceMagmaPresentationSupportKHR");
if (!vkGetPhysicalDeviceMagmaPresentationSupportKHR_) {
vulkan_instance_.Destroy();
return false;
}
return true;
}
VkInstance VulkanImplementationScenic::GetVulkanInstance() {
return vulkan_instance_.vk_instance();
}
std::unique_ptr<gpu::VulkanSurface>
VulkanImplementationScenic::CreateViewSurface(gfx::AcceleratedWidget window) {
ScenicWindow* scenic_window = scenic_window_manager_->GetWindow(window);
if (!scenic_window)
return nullptr;
ScenicSession* scenic_session = scenic_window->scenic_session();
fuchsia::images::ImagePipePtr image_pipe;
ScenicSession::ResourceId image_pipe_id =
scenic_session->CreateImagePipe(image_pipe.NewRequest());
scenic_window->SetTexture(image_pipe_id);
scenic_session->ReleaseResource(image_pipe_id);
scenic_session->Present();
VkSurfaceKHR surface;
VkMagmaSurfaceCreateInfoKHR surface_create_info = {};
surface_create_info.sType = VK_STRUCTURE_TYPE_MAGMA_SURFACE_CREATE_INFO_KHR;
surface_create_info.imagePipeHandle =
image_pipe.Unbind().TakeChannel().release();
VkResult result =
reinterpret_cast<PFN_vkCreateMagmaSurfaceKHR>(vkCreateMagmaSurfaceKHR_)(
GetVulkanInstance(), &surface_create_info, nullptr, &surface);
if (result != VK_SUCCESS) {
// This shouldn't fail, and we don't know whether imagePipeHandle was closed
// if it does.
LOG(FATAL) << "vkCreateMagmaSurfaceKHR failed: " << result;
}
return std::make_unique<gpu::VulkanSurface>(GetVulkanInstance(), surface);
}
bool VulkanImplementationScenic::GetPhysicalDevicePresentationSupport(
VkPhysicalDevice physical_device,
const std::vector<VkQueueFamilyProperties>& queue_family_properties,
uint32_t queue_family_index) {
// TODO(spang): vkGetPhysicalDeviceMagmaPresentationSupportKHR returns false
// here. Use it once it is fixed.
NOTIMPLEMENTED();
return true;
}
std::vector<const char*>
VulkanImplementationScenic::GetRequiredDeviceExtensions() {
return {VK_KHR_SWAPCHAIN_EXTENSION_NAME};
}
VkFence VulkanImplementationScenic::CreateVkFenceForGpuFence(
VkDevice vk_device) {
NOTIMPLEMENTED();
return VK_NULL_HANDLE;
}
std::unique_ptr<gfx::GpuFence>
VulkanImplementationScenic::ExportVkFenceToGpuFence(VkDevice vk_device,
VkFence vk_fence) {
NOTIMPLEMENTED();
return nullptr;
}
} // namespace ui
// Copyright 2018 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#ifndef UI_OZONE_PLATFORM_SCENIC_VULKAN_IMPLEMENTATION_SCENIC_H_
#define UI_OZONE_PLATFORM_SCENIC_VULKAN_IMPLEMENTATION_SCENIC_H_
#include <memory>
#include "gpu/vulkan/vulkan_implementation.h"
#include "gpu/vulkan/vulkan_instance.h"
namespace ui {
class ScenicWindowManager;
class VulkanImplementationScenic : public gpu::VulkanImplementation {
public:
VulkanImplementationScenic(ScenicWindowManager* scenic_window_manager);
~VulkanImplementationScenic() override;
// VulkanImplementation:
bool InitializeVulkanInstance() override;
VkInstance GetVulkanInstance() override;
std::unique_ptr<gpu::VulkanSurface> CreateViewSurface(
gfx::AcceleratedWidget window) override;
bool GetPhysicalDevicePresentationSupport(
VkPhysicalDevice device,
const std::vector<VkQueueFamilyProperties>& queue_family_properties,
uint32_t queue_family_index) override;
std::vector<const char*> GetRequiredDeviceExtensions() override;
VkFence CreateVkFenceForGpuFence(VkDevice vk_device) override;
std::unique_ptr<gfx::GpuFence> ExportVkFenceToGpuFence(
VkDevice vk_device,
VkFence vk_fence) override;
private:
ScenicWindowManager* const scenic_window_manager_;
gpu::VulkanInstance vulkan_instance_;
PFN_vkVoidFunction vkCreateMagmaSurfaceKHR_ = nullptr;
PFN_vkVoidFunction vkGetPhysicalDeviceMagmaPresentationSupportKHR_ = nullptr;
DISALLOW_COPY_AND_ASSIGN(VulkanImplementationScenic);
};
} // namespace ui
#endif // UI_OZONE_PLATFORM_SCENIC_VULKAN_IMPLEMENTATION_SCENIC_H_
// Copyright 2018 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#ifndef UI_OZONE_PLATFORM_SCENIC_VULKAN_MAGMA_H_
#define UI_OZONE_PLATFORM_SCENIC_VULKAN_MAGMA_H_
// These definitions are from
// https://fuchsia.googlesource.com/third_party/vulkan_loader_and_validation_layers/+/master/include/vulkan/vulkan.h
// TODO(spang): Remove these once the definitions go upstream.
#include <vulkan/vulkan.h>
#if !defined(VK_KHR_external_memory_fuchsia)
#define VK_KHR_external_memory_fuchsia 1
#define VK_KHR_EXTERNAL_MEMORY_FUCHSIA_SPEC_VERSION 1
#define VK_KHR_EXTERNAL_MEMORY_FUCHSIA_EXTENSION_NAME \
"VK_KHR_external_memory_fuchsia"
typedef struct VkImportMemoryFuchsiaHandleInfoKHR {
VkStructureType sType;
const void* pNext;
VkExternalMemoryHandleTypeFlagBitsKHR handleType;
uint32_t handle;
} VkImportMemoryFuchsiaHandleInfoKHR;
typedef struct VkMemoryFuchsiaHandlePropertiesKHR {
VkStructureType sType;
void* pNext;
uint32_t memoryTypeBits;
} VkMemoryFuchsiaHandlePropertiesKHR;
typedef struct VkMemoryGetFuchsiaHandleInfoKHR {
VkStructureType sType;
const void* pNext;
VkDeviceMemory memory;
VkExternalMemoryHandleTypeFlagBitsKHR handleType;
} VkMemoryGetFuchsiaHandleInfoKHR;
typedef VkResult(VKAPI_PTR* PFN_vkGetMemoryFuchsiaHandleKHR)(
VkDevice device,
const VkMemoryGetFuchsiaHandleInfoKHR* pGetFuchsiaHandleInfo,
uint32_t* pFuchsiaHandle);
typedef VkResult(VKAPI_PTR* PFN_vkGetMemoryFuchsiaHandlePropertiesKHR)(
VkDevice device,
VkExternalMemoryHandleTypeFlagBitsKHR handleType,
uint32_t fuchsiaHandle,
VkMemoryFuchsiaHandlePropertiesKHR* pMemoryFuchsiaHandleProperties);
#ifndef VK_NO_PROTOTYPES
VKAPI_ATTR VkResult VKAPI_CALL vkGetMemoryFuchsiaHandleKHR(
VkDevice device,
const VkMemoryGetFuchsiaHandleInfoKHR* pGetFuchsiaHandleInfo,
uint32_t* pFuchsiaHandle);
VKAPI_ATTR VkResult VKAPI_CALL vkGetMemoryFuchsiaHandlePropertiesKHR(
VkDevice device,
VkExternalMemoryHandleTypeFlagBitsKHR handleType,
uint32_t fuchsiaHandle,
VkMemoryFuchsiaHandlePropertiesKHR* pMemoryFuchsiaHandleProperties);
#endif
#endif // !defined(VK_KHR_external_memory_fuchsia)
#if !defined(VK_KHR_external_semaphore_fuchsia)
#define VK_KHR_external_semaphore_fuchsia 1
#define VK_KHR_EXTERNAL_SEMAPHORE_FUCHSIA_SPEC_VERSION 1
#define VK_KHR_EXTERNAL_SEMAPHORE_FUCHSIA_EXTENSION_NAME \
"VK_KHR_external_semaphore_fuchsia"
typedef struct VkImportSemaphoreFuchsiaHandleInfoKHR {
VkStructureType sType;
const void* pNext;
VkSemaphore semaphore;
VkSemaphoreImportFlagsKHR flags;
VkExternalSemaphoreHandleTypeFlagBitsKHR handleType;
uint32_t handle;
} VkImportSemaphoreFuchsiaHandleInfoKHR;
typedef struct VkSemaphoreGetFuchsiaHandleInfoKHR {
VkStructureType sType;
const void* pNext;
VkSemaphore semaphore;
VkExternalSemaphoreHandleTypeFlagBitsKHR handleType;
} VkSemaphoreGetFuchsiaHandleInfoKHR;
typedef VkResult(VKAPI_PTR* PFN_vkImportSemaphoreFuchsiaHandleKHR)(
VkDevice device,
const VkImportSemaphoreFuchsiaHandleInfoKHR*
pImportSemaphoreFuchsiaHandleInfo);
typedef VkResult(VKAPI_PTR* PFN_vkGetSemaphoreFuchsiaHandleKHR)(
VkDevice device,
const VkSemaphoreGetFuchsiaHandleInfoKHR* pGetFuchsiaHandleInfo,
uint32_t* pFuchsiaHandle);
#ifndef VK_NO_PROTOTYPES
VKAPI_ATTR VkResult VKAPI_CALL
vkImportSemaphoreFuchsiaHandleKHR(VkDevice device,
const VkImportSemaphoreFuchsiaHandleInfoKHR*
pImportSemaphoreFuchsiaHandleInfo);
VKAPI_ATTR VkResult VKAPI_CALL vkGetSemaphoreFuchsiaHandleKHR(
VkDevice device,
const VkSemaphoreGetFuchsiaHandleInfoKHR* pGetFuchsiaHandleInfo,
uint32_t* pFuchsiaHandle);
#endif
#endif // !defined(VK_KHR_external_semaphore_fuchsia)
#if defined(VK_USE_PLATFORM_MAGMA_KHR) && !defined(VK_KHR_magma_surface)
#define VK_KHR_magma_surface 1
#define VK_KHR_MAGMA_SURFACE_SPEC_VERSION 1
#define VK_KHR_MAGMA_SURFACE_EXTENSION_NAME "VK_KHR_magma_surface"
typedef VkFlags VkMagmaSurfaceCreateFlagsKHR;
typedef struct VkMagmaSurfaceCreateInfoKHR {
VkStructureType sType;
const void* pNext;
uint32_t imagePipeHandle;
uint32_t width;
uint32_t height;
} VkMagmaSurfaceCreateInfoKHR;
typedef VkResult(VKAPI_PTR* PFN_vkCreateMagmaSurfaceKHR)(
VkInstance instance,
const VkMagmaSurfaceCreateInfoKHR* pCreateInfo,
const VkAllocationCallbacks* pAllocator,
VkSurfaceKHR* pSurface);
typedef VkBool32(VKAPI_PTR* PFN_vkGetPhysicalDeviceMagmaPresentationSupportKHR)(
VkPhysicalDevice physicalDevice,
uint32_t queueFamilyIndex);
#ifndef VK_NO_PROTOTYPES
VKAPI_ATTR VkResult VKAPI_CALL
vkCreateMagmaSurfaceKHR(VkInstance instance,
const VkMagmaSurfaceCreateInfoKHR* pCreateInfo,
const VkAllocationCallbacks* pAllocator,
VkSurfaceKHR* pSurface);
VKAPI_ATTR VkBool32 VKAPI_CALL
vkGetPhysicalDeviceMagmaPresentationSupportKHR(VkPhysicalDevice physicalDevice,
uint32_t queueFamilyIndex);
#endif
#endif // defined(VK_USE_PLATFORM_MAGMA_KHR) && !defined(VK_KHR_magma_surface)
#if !defined(VK_GOOGLE_image_usage_scanout)
#define VK_GOOGLE_image_usage_scanout 1
#define VK_GOOGLE_IMAGE_USAGE_SCANOUT_SPEC_VERSION 1
#define VK_GOOGLE_IMAGE_USAGE_SCANOUT_EXTENSION_NAME \
"VK_GOOGLE_image_usage_scanout"
#define VK_STRUCTURE_TYPE_MAGMA_SURFACE_CREATE_INFO_KHR \
(static_cast<VkStructureType>(1001002000))
#endif // !defined(VK_GOOGLE_image_usage_scanout)
#endif // UI_OZONE_PLATFORM_MAGMA_VULKAN_MAGMA_H_
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