Skip to content

GitLab

T
tangled
Commit 4e786c18 authored by Corentin Wallez's avatar Corentin Wallez Committed by Commit Bot
Browse files
Options

Revert "Reuse pixmap across VFPool/MailboxVFConverter." 

This reverts commit f204c589.

Reason for revert: crbug.com/1097755

Bug: 1097755

Original change's description:
> Reuse pixmap across VFPool/MailboxVFConverter.
> 
> In the newly introduced VideoDecoder, we create a native buffer for
> video decoding on demand in the PlatformVideoFramePool. This buffer
> eventually makes it to the MailboxVideoFrameConverter in order to
> produce a SharedImage for the client.
> 
> This CL ensures that the NativePixmap created in the
> PlatformVideoFramePool can be re-used in the MailboxVideoFrameConverter.
> Before this CL, we would call CreateNativePixmapFromHandle()
> unnecessarily when creating the SharedImage for a buffer. That's because
> we didn't propagate the GpuMemoryBufferId correctly from the
> PlatformVideoFramePool to the MailboxVideoFrameConverter and the check
> in [1] would fail. As a result, we would end up creating an additional
> DRM framebuffer which is unnecessary because the allocation in the
> PlatformVideoFramePool would have already created one. Thus, the
> video.MemCheck.h264_hw test would fail because of having created too
> many objects.
> 
> To propagate the GpuMemoryBufferId correctly, we create a
> GpuMemoryBuffer VideoFrame in the PlatformVideoFramePool (instead of a
> dma-buf VideoFrame). That way, when MailboxVideoFrameConverter calls
> CreateGpuMemoryBufferHandle(), it gets a GpuMemoryBufferHandle with the
> same ID that PlatformVideoFramePool gets. Additionally, we make
> MailboxVideoFrameConverter use kPlatformVideoFramePoolClientId since
> that identifies the namespace of the GpuMemoryBufferIds in question.
> 
> Note that for SCANOUT_VDA_WRITE buffers (which we use for video
> decoding), we end up creating a ClientNativePixmapOpaque when creating a
> GpuMemoryBufferImplNativePixmap. This is problematic because a
> ClientNativePixmapOpaque doesn't let us query the stride and we need to
> know it in order to create a GpuMemoryBuffer VideoFrame. Therefore,
> GpuMemoryBufferImplNativePixmap::stride() is changed so that it's not
> necessary to have a gfx::ClientNativePixmap to query the stride.
> Instead, we query the underlying NativePixmapHandle directly.
> 
> For that same reason, we must introduce a workaround in the
> VideoFrameValidator: when we get a GMB VideoFrame, we cannot map it
> directly because ClientNativePixmapOpaque doesn't allow us to.
> Therefore, we extract the dma-buf FDs and use the regular dma-buf
> mapping path.
> 
> [1] https://cs.chromium.org/chromium/src/gpu/ipc/service/gpu_memory_buffer_factory_native_pixmap.cc?l=152&rcl=d96e9f16ae852ec9dbd15bf17df3d440402413bb
> 
> Bug: 1091450, 1093644
> Test: video.MemCheck.h264_hw_switch, video.DecodeAccelVD.h264 on eve
> Change-Id: I3d60efa362b66bd2f01139296d4ed756c8777ef8
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2233534
> Commit-Queue: Andres Calderon Jaramillo <andrescj@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
> Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> Reviewed-by: Chih-Yu Huang <akahuang@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#780504}

TBR=dcheng@chromium.org,akahuang@chromium.org,dcastagna@chromium.org,hiroh@chromium.org,andrescj@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 1091450, 1093644
Change-Id: Iace25eb12b50b030327568ae75ebef0f7691934d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2257418Reviewed-by: default avatarCorentin Wallez <cwallez@chromium.org>
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#780775}
parent a2c7377d
Hide whitespace changes
Inline Side-by-side
Showing with 12 additions and 59 deletions
gpu/ipc/common/gpu_memory_buffer_impl_native_pixmap.cc
View file @ 4e786c18
......@@ -106,20 +106,8 @@ void GpuMemoryBufferImplNativePixmap::Unmap() {
}
int GpuMemoryBufferImplNativePixmap::stride(size_t plane) const {
// The caller is responsible for ensuring that |plane| is within bounds.
CHECK_LT(plane, handle_.planes.size());
// |handle_|.planes[plane].stride is a uint32_t. For usages for which we
// create a ClientNativePixmapDmaBuf,
// ClientNativePixmapDmaBuf::ImportFromDmabuf() ensures that the stride fits
// on an int, so this checked_cast shouldn't fail. For usages for which we
// create a ClientNativePixmapOpaque, we don't validate the stride, but the
// expectation is that either a) the stride() method won't be called, or b)
// the stride() method is called on the GPU process and
// |handle_|.planes[plane].stride is also set on the GPU process so there's no
// need to validate it. Refer to http://crbug.com/1093644#c1 for a more
// detailed discussion.
return base::checked_cast<int>(handle_.planes[plane].stride);
DCHECK_LT(plane, gfx::NumberOfPlanesForLinearBufferFormat(format_));
return pixmap_->GetStride(plane);
}
gfx::GpuMemoryBufferType GpuMemoryBufferImplNativePixmap::GetType() const {
......
media/gpu/chromeos/mailbox_video_frame_converter.cc
View file @ 4e786c18
......@@ -14,7 +14,6 @@
#include "base/trace_event/trace_event.h"
#include "gpu/command_buffer/common/shared_image_usage.h"
#include "gpu/command_buffer/service/scheduler.h"
#include "gpu/ipc/common/gpu_client_ids.h"
#include "gpu/ipc/service/gpu_channel.h"
#include "gpu/ipc/service/shared_image_stub.h"
#include "media/base/format_utils.h"
......@@ -155,7 +154,7 @@ void MailboxVideoFrameConverter::ConvertFrame(scoped_refptr<VideoFrame> frame) {
DCHECK(parent_task_runner_->RunsTasksInCurrentSequence());
DVLOGF(4);
if (!frame || frame->storage_type() != VideoFrame::STORAGE_GPU_MEMORY_BUFFER)
if (!frame || !frame->HasDmaBufs())
return OnError(FROM_HERE, "Invalid frame.");
VideoFrame* origin_frame = unwrap_frame_cb_.Run(*frame);
......@@ -337,7 +336,7 @@ bool MailboxVideoFrameConverter::GenerateSharedImageOnGPUThread(
const uint32_t shared_image_usage =
gpu::SHARED_IMAGE_USAGE_DISPLAY | gpu::SHARED_IMAGE_USAGE_SCANOUT;
const bool success = shared_image_stub->CreateSharedImage(
mailbox, gpu::kPlatformVideoFramePoolClientId,
mailbox, shared_image_stub->channel()->client_id(),
std::move(gpu_memory_buffer_handle), *buffer_format,
gpu::kNullSurfaceHandle, destination_visible_rect.size(),
video_frame->ColorSpace(), shared_image_usage);
......
media/gpu/chromeos/platform_video_frame_pool.cc
View file @ 4e786c18
......@@ -25,9 +25,9 @@ scoped_refptr<VideoFrame> DefaultCreateFrame(
const gfx::Rect& visible_rect,
const gfx::Size& natural_size,
base::TimeDelta timestamp) {
return CreateGpuMemoryBufferVideoFrame(
gpu_memory_buffer_factory, format, coded_size, visible_rect, natural_size,
timestamp, gfx::BufferUsage::SCANOUT_VDA_WRITE);
return CreatePlatformVideoFrame(gpu_memory_buffer_factory, format, coded_size,
visible_rect, natural_size, timestamp,
gfx::BufferUsage::SCANOUT_VDA_WRITE);
}
} // namespace
......
media/gpu/test/video_frame_validator.cc
View file @ 4e786c18
......@@ -19,7 +19,6 @@
#include "media/gpu/video_frame_mapper_factory.h"
#include "media/media_buildflags.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "ui/gfx/gpu_memory_buffer.h"
namespace media {
namespace test {
......@@ -109,39 +108,14 @@ void VideoFrameValidator::ProcessVideoFrameTask(
scoped_refptr<const VideoFrame> frame = video_frame;
// If this is a DMABuf-backed memory frame we need to map it before accessing.
#if BUILDFLAG(USE_CHROMEOS_MEDIA_ACCELERATION)
if (frame->storage_type() == VideoFrame::STORAGE_GPU_MEMORY_BUFFER) {
// TODO(andrescj): This is a workaround. ClientNativePixmapFactoryDmabuf
// creates ClientNativePixmapOpaque for SCANOUT_VDA_WRITE buffers which does
// not allow us to map GpuMemoryBuffers easily for testing. Therefore, we
// extract the dma-buf FDs. Alternatively, we could consider creating our
// own ClientNativePixmapFactory for testing.
gfx::GpuMemoryBuffer* gmb = video_frame->GetGpuMemoryBuffer();
gfx::GpuMemoryBufferHandle gmb_handle = gmb->CloneHandle();
ASSERT_EQ(gmb_handle.type, gfx::GpuMemoryBufferType::NATIVE_PIXMAP);
std::vector<ColorPlaneLayout> planes;
std::vector<base::ScopedFD> dmabuf_fds;
for (auto& plane : gmb_handle.native_pixmap_handle.planes) {
planes.emplace_back(plane.stride, plane.offset, plane.size);
dmabuf_fds.emplace_back(plane.fd.release());
}
auto layout = VideoFrameLayout::CreateWithPlanes(
frame->format(), video_frame->coded_size(), std::move(planes),
VideoFrameLayout::kBufferAddressAlignment,
gmb_handle.native_pixmap_handle.modifier);
ASSERT_TRUE(layout);
frame = VideoFrame::WrapExternalDmabufs(
*layout, frame->visible_rect(), frame->natural_size(),
std::move(dmabuf_fds), frame->timestamp());
ASSERT_TRUE(frame);
}
if (frame->storage_type() == VideoFrame::STORAGE_DMABUFS) {
if (frame->storage_type() == VideoFrame::STORAGE_DMABUFS ||
frame->storage_type() == VideoFrame::STORAGE_GPU_MEMORY_BUFFER) {
// Create VideoFrameMapper if not yet created. The decoder's output pixel
// format is not known yet when creating the VideoFrameValidator. We can
// only create the VideoFrameMapper upon receiving the first video frame.
if (!video_frame_mapper_) {
video_frame_mapper_ = VideoFrameMapperFactory::CreateMapper(
frame->format(), frame->storage_type());
video_frame->format(), video_frame->storage_type());
ASSERT_TRUE(video_frame_mapper_) << "Failed to create VideoFrameMapper";
}
......
ui/gfx/linux/client_native_pixmap_dmabuf.cc
View file @ 4e786c18
......@@ -213,8 +213,6 @@ ClientNativePixmapDmaBuf::ImportFromDmabuf(gfx::NativePixmapHandle handle,
for (size_t i = 0; i < handle.planes.size(); ++i) {
// Verify that the plane buffer has appropriate size.
const size_t plane_stride =
base::strict_cast<size_t>(handle.planes[i].stride);
size_t min_stride = 0;
size_t subsample_factor = SubsamplingFactorForBufferFormat(format, i);
base::CheckedNumeric<size_t> plane_height =
......@@ -222,20 +220,14 @@ ClientNativePixmapDmaBuf::ImportFromDmabuf(gfx::NativePixmapHandle handle,
subsample_factor;
if (!gfx::RowSizeForBufferFormatChecked(size.width(), format, i,
&min_stride) ||
plane_stride < min_stride) {
handle.planes[i].stride < min_stride) {
return nullptr;
}
base::CheckedNumeric<size_t> min_size =
base::CheckedNumeric<size_t>(plane_stride) * plane_height;
base::CheckedNumeric<size_t>(handle.planes[i].stride) * plane_height;
if (!min_size.IsValid() || handle.planes[i].size < min_size.ValueOrDie())
return nullptr;
// The stride must be a valid integer in order to be consistent with the
// GpuMemoryBuffer::stride() API. Also, refer to http://crbug.com/1093644#c1
// for some comments on this check and others in this method.
if (!base::IsValueInRangeForNumericType<int>(plane_stride))
return nullptr;
const size_t map_size = base::checked_cast<size_t>(handle.planes[i].size);
plane_info[i].offset = handle.planes[i].offset;
plane_info[i].size = map_size;
......
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