media/gpu/v4l2: validate DMAbuf buffers against current format

Queuing DMAbuf buffers require some safety checks to be performed,
especially regarding the number of FDs submitted and their use of
offset. We used to have an ad-hoc verification function in
V4L2SliceVideoDecoder, but now that V4L2Queue is aware of the current
format we can move that check there, where it ought to be.

Bug: 1003223
Test: video_decode_accelerator_tests passing on Kevin.
Change-Id: Ic5d960c626c4146059311fb1633c25a3e207793f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1839320
Commit-Queue: Alexandre Courbot <acourbot@chromium.org>
Auto-Submit: Alexandre Courbot <acourbot@chromium.org>
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#703205}

media/gpu/v4l2/v4l2_device.cc

@@ -269,6 +269,9 @@ class V4L2BufferRefBase {
   void* GetPlaneMapping(const size_t plane);
 
   scoped_refptr<VideoFrame> GetVideoFrame();
+  // Checks that the number of passed FDs is adequate for the current format
+  // and buffer configuration. Only useful for DMABUF buffers.
+  bool CheckNumFDsForFormat(const size_t num_fds) const;
 
   // Data from the buffer, that users can query and/or write.
   struct v4l2_buffer v4l2_buffer_;
@@ -350,6 +353,40 @@ scoped_refptr<VideoFrame> V4L2BufferRefBase::GetVideoFrame() {
   return queue_->buffers_[BufferId()]->GetVideoFrame();
 }
 
+bool V4L2BufferRefBase::CheckNumFDsForFormat(const size_t num_fds) const {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  if (!queue_)
+    return false;
+
+  // We have not used SetFormat(), assume this is ok.
+  // Hopefully we standardize SetFormat() in the future.
+  if (!queue_->current_format_)
+    return true;
+
+  const size_t required_fds = queue_->current_format_->fmt.pix_mp.num_planes;
+  // Sanity check.
+  DCHECK_EQ(v4l2_buffer_.length, required_fds);
+  if (num_fds < required_fds) {
+    VLOGF(1) << "Insufficient number of FDs given for the current format. "
+             << num_fds << " provided, " << required_fds << " required.";
+    return false;
+  }
+
+  const auto& planes = v4l2_buffer_.m.planes;
+  for (size_t i = v4l2_buffer_.length - 1; i >= num_fds; --i) {
+    // Assume that an fd is a duplicate of a previous plane's fd if offset != 0.
+    // Otherwise, if offset == 0, return error as it is likely pointing to
+    // a new plane.
+    if (planes[i].data_offset == 0) {
+      VLOGF(1) << "Additional dmabuf fds point to a new buffer.";
+      return false;
+    }
+  }
+
+  return true;
+}
+
 V4L2WritableBufferRef::V4L2WritableBufferRef() {
   // Invalid buffers can be created from any thread.
   DETACH_FROM_SEQUENCE(sequence_checker_);
@@ -473,12 +510,10 @@ bool V4L2WritableBufferRef::QueueDMABuf(
     return false;
   }
 
-  size_t num_planes = self.PlanesCount();
-  // TODO(hiroh): Strengthen this check with v4l2 pixel format.
-  if (fds.size() < num_planes) {
-    VLOGF(1) << "The given number of fds is less than required one";
+  if (!self.buffer_data_->CheckNumFDsForFormat(fds.size()))
     return false;
-  }
+
+  size_t num_planes = self.PlanesCount();
   for (size_t i = 0; i < num_planes; i++)
     self.buffer_data_->v4l2_buffer_.m.planes[i].m.fd = fds[i].get();
 
@@ -498,12 +533,10 @@ bool V4L2WritableBufferRef::QueueDMABuf(
     return false;
   }
 
-  size_t num_planes = self.PlanesCount();
-  // TODO(hiroh): Strengthen this check with v4l2 pixel format.
-  if (planes.size() < num_planes) {
-    VLOGF(1) << "The given number of fds is less than required one";
+  if (!self.buffer_data_->CheckNumFDsForFormat(planes.size()))
     return false;
-  }
+
+  size_t num_planes = self.PlanesCount();
   for (size_t i = 0; i < num_planes; i++)
     self.buffer_data_->v4l2_buffer_.m.planes[i].m.fd = planes[i].fd.get();
 

media/gpu/v4l2/v4l2_slice_video_decoder.cc

@@ -49,31 +49,6 @@ constexpr uint32_t kSupportedInputFourccs[] = {
     V4L2_PIX_FMT_VP9_FRAME,
 };
 
-// Checks an underlying video frame buffer of |frame| is valid for VIDIOC_DQBUF
-// that requires |target_num_fds| fds.
-bool IsValidFrameForQueueDMABuf(const VideoFrame* frame,
-                                size_t target_num_fds) {
-  DCHECK(frame);
-  if (frame->DmabufFds().size() < target_num_fds) {
-    VLOGF(1) << "The count of dmabuf fds (" << frame->DmabufFds().size()
-             << ") are not enough, needs " << target_num_fds << " fds.";
-    return false;
-  }
-
-  const auto& planes = frame->layout().planes();
-  for (size_t i = frame->DmabufFds().size() - 1; i >= target_num_fds; --i) {
-    // Assume that an fd is a duplicate of a previous plane's fd if offset != 0.
-    // Otherwise, if offset == 0, return error as surface_it may be pointing to
-    // a new plane.
-    if (planes[i].offset == 0) {
-      VLOGF(1) << "Additional dmabuf fds point to a new buffer.";
-      return false;
-    }
-  }
-
-  return true;
-}
-
 }  // namespace
 
 V4L2SliceVideoDecoder::DecodeRequest::DecodeRequest(
@@ -560,7 +535,6 @@ base::Optional<VideoFrameLayout> V4L2SliceVideoDecoder::SetupOutputFormat(
         continue;
       }
 
-      num_output_planes_ = format->fmt.pix_mp.num_planes;
       return frame_layout;
     }
   }
@@ -962,11 +936,6 @@ void V4L2SliceVideoDecoder::DecodeSurface(
     return;
   }
 
-  if (!IsValidFrameForQueueDMABuf(dec_surface->video_frame().get(),
-                                  num_output_planes_)) {
-    SetState(State::kError);
-    return;
-  }
   if (!std::move(dec_surface->output_buffer())
            .QueueDMABuf(dec_surface->video_frame()->DmabufFds())) {
     SetState(State::kError);

media/gpu/v4l2/v4l2_slice_video_decoder.h

@@ -265,9 +265,6 @@ class MEDIA_GPU_EXPORT V4L2SliceVideoDecoder : public VideoDecoder,
   // Queue of pending output request.
   base::queue<OutputRequest> output_request_queue_;
 
-  // The number of planes, which is the number of DMA-buf fds we enqueue into
-  // the V4L2 device.
-  size_t num_output_planes_ = 0;
   // True if the decoder needs bitstream conversion before decoding.
   bool needs_bitstream_conversion_ = false;
   // Next bitstream ID.