commit e775ac4770bb1e5dfbfe22c5b56752ed4a2317c5 Author: Hirokazu Honda Date: Tue Jan 17 17:54:02 2023 +0000 Revert "media/gpu/v4l2VEA,IP: Workaround to USERPTR API against read-only buf" This reverts commit 203fdd03a4317f2eac57161ee101515a82df704f. Reason for revert: This workaround is no longer required because of the kernel patch was reverted. Original change's description: > media/gpu/v4l2VEA,IP: Workaround to USERPTR API against read-only buf > > VideoFrame fed in VEA::Encode() is read-only since R107 if the > VideoFrame has a shared memory. V4L2VideoEncodeAccelerator fails > because VIDOC_QBUF with USERPTR buffer fails if the pointers > references a read-only buffer. > The USERPTR API issue is apparently to be fixed by a kernel side. > In the meantime, this CL adds the workaround to the issue; the read > only buffer is copied to a writable temporary buffer every Encode > before VIDIOC_QBUF. > Not that VideoFrame has a shared memory in the case of screen share > because a camera stack produces GpuMemoryBuffer. > > Bug: b:243883312 > Test: video_encode_accelerator_tests on elm, kevin and trogdor > Test: screen share in Google Meet on elm, kevin and trogdor > Change-Id: I922d98eaf52f80d9cd6c3784a8544bffb1232856 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3891421 > Reviewed-by: Nathan Hebert > Commit-Queue: Hirokazu Honda > Cr-Commit-Position: refs/heads/main@{#1046659} Bug: b:243883312, b:261660224 Change-Id: I6fe37276f2ebe8c8730cfd3dd766b04277b32a67 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4091205 Reviewed-by: Nathan Hebert Commit-Queue: Hirokazu Honda Cr-Commit-Position: refs/heads/main@{#1093388} diff --git a/media/gpu/v4l2/v4l2_image_processor_backend.cc b/media/gpu/v4l2/v4l2_image_processor_backend.cc index dc72a4ac060a9..f3049f4546e2e 100644 --- a/media/gpu/v4l2/v4l2_image_processor_backend.cc +++ b/media/gpu/v4l2/v4l2_image_processor_backend.cc @@ -925,35 +925,16 @@ bool V4L2ImageProcessorBackend::EnqueueInputRecord( switch (input_memory_type_) { case V4L2_MEMORY_USERPTR: { - VideoFrame& frame = *job_record->input_frame; const size_t num_planes = V4L2Device::GetNumPlanesOfV4L2PixFmt( input_config_.fourcc.ToV4L2PixFmt()); std::vector user_ptrs(num_planes); - if (frame.storage_type() == VideoFrame::STORAGE_SHMEM) { - // TODO(b/243883312): This copies the video frame to a writable buffer - // since the USERPTR API requires writable permission. Remove this - // workaround once the unreasonable permission is fixed. - const size_t buffer_size = frame.shm_region()->GetSize(); - std::vector writable_buffer(buffer_size); - std::memcpy(writable_buffer.data(), frame.data(0), buffer_size); - for (size_t i = 0; i < num_planes; ++i) { - const std::intptr_t plane_offset = - reinterpret_cast(frame.data(i)) - - reinterpret_cast(frame.data(0)); - user_ptrs[i] = writable_buffer.data() + plane_offset; - } - job_record->input_frame->AddDestructionObserver(base::BindOnce( - [](std::vector) {}, std::move(writable_buffer))); - } else { - for (size_t i = 0; i < num_planes; ++i) - user_ptrs[i] = frame.writable_data(i); - } - for (size_t i = 0; i < num_planes; ++i) { int bytes_used = - VideoFrame::PlaneSize(frame.format(), i, input_config_.size) + VideoFrame::PlaneSize(job_record->input_frame->format(), i, + input_config_.size) .GetArea(); buffer.SetPlaneBytesUsed(i, bytes_used); + user_ptrs[i] = const_cast(job_record->input_frame->data(i)); } if (!std::move(buffer).QueueUserPtr(user_ptrs)) { VPLOGF(1) << "Failed to queue a DMABUF buffer to input queue"; diff --git a/media/gpu/v4l2/v4l2_video_encode_accelerator.cc b/media/gpu/v4l2/v4l2_video_encode_accelerator.cc index 56389ea3eec08..d452ba48a69e2 100644 --- a/media/gpu/v4l2/v4l2_video_encode_accelerator.cc +++ b/media/gpu/v4l2/v4l2_video_encode_accelerator.cc @@ -783,7 +783,7 @@ void V4L2VideoEncodeAccelerator::EncodeTask(scoped_refptr frame, const bool is_expected_storage_type = native_input_mode_ ? frame->storage_type() == VideoFrame::STORAGE_GPU_MEMORY_BUFFER - : frame->storage_type() == VideoFrame::STORAGE_SHMEM; + : frame->IsMappable(); if (!is_expected_storage_type) { VLOGF(1) << "Unexpected storage: " << VideoFrame::StorageTypeToString(frame->storage_type()); @@ -1392,27 +1392,16 @@ bool V4L2VideoEncodeAccelerator::EnqueueInputRecord( NOTIFY_ERROR(kPlatformFailureError); } - // TODO(b/243883312): This copies the video frame to a writable buffer - // since the USERPTR API requires writable permission. Remove this - // workaround once the unreasonable permission is fixed. - const size_t buffer_size = frame->shm_region()->GetSize(); - std::vector writable_buffer(buffer_size); - std::memcpy(writable_buffer.data(), frame->data(0), buffer_size); + // The frame data is readable only and the driver doesn't actually write + // the buffer. But USRPTR buffer needs void*. So const_cast<> is required. std::vector user_ptrs(num_planes); for (size_t i = 0; i < num_planes; ++i) { - const std::intptr_t plane_offset = - reinterpret_cast(frame->data(i)) - - reinterpret_cast(frame->data(0)); - user_ptrs[i] = writable_buffer.data() + plane_offset; + user_ptrs[i] = const_cast(frame->data(i)); } - if (!std::move(input_buf).QueueUserPtr(std::move(user_ptrs))) { - VPLOGF(1) << "Failed to queue a USRPTR buffer to input queue"; - NOTIFY_ERROR(kPlatformFailureError); + VPLOGF(1) << "Failed queue a USRPTR buffer to input queue"; return false; } - frame->AddDestructionObserver( - base::DoNothingWithBoundArgs(std::move(writable_buffer))); break; } case V4L2_MEMORY_DMABUF: { @@ -1421,6 +1410,7 @@ bool V4L2VideoEncodeAccelerator::EnqueueInputRecord( VPLOGF(1) << "Failed queue a DMABUF buffer to input queue"; return false; } + // Keep |gmb_handle| alive as long as |frame| is alive so that fds passed // to the driver are valid during encoding. frame->AddDestructionObserver(base::BindOnce(