CDP: Make colors in Page.screencast API accurate

When capturing frames using the Page.screencast CDP API, the colors are
a little off.
It's hard to see with the naked eye, but using a color picker, you can
see a difference between colors in those frames and colors in similar
images taken with, e.g., the snip tool on windows.

The reason for this is that the DevTools Video Consumer used by CDP
does not specify any pixel format or color space when creating its
underlying FrameSinkVideoConsumer, and therefore the default values are
used: PIXEL_FORMAT_I420, COLOR_SPACE_HD_REC709

With this fix, we make it possible for the Page.screencast API to
specify the ARGB pixel format and REC709 color space, and this seems to
produce images with accurate colors.

Bug: 1141314
Change-Id: If06f5419c1feaebb3134a5d422769d8123f5518b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2593354
Commit-Queue: Patrick Brosset <patrick.brosset@microsoft.com>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Cr-Commit-Position: refs/heads/master@{#838137}

content/browser/devtools/devtools_video_consumer.cc

@@ -28,6 +28,11 @@ constexpr base::TimeDelta kDefaultMinPeriod = base::TimeDelta();
 // Allow variable aspect ratio.
 const bool kDefaultUseFixedAspectRatio = false;
 
+constexpr media::VideoPixelFormat kDefaultPixelFormat =
+    media::PIXEL_FORMAT_I420;
+
+constexpr gfx::ColorSpace kDefaultColorSpace = gfx::ColorSpace::CreateREC709();
+
 // Creates a ClientFrameSinkVideoCapturer via HostFrameSinkManager.
 std::unique_ptr<viz::ClientFrameSinkVideoCapturer> CreateCapturer() {
   return GetHostFrameSinkManager()->CreateVideoCapturer();
@@ -45,7 +50,9 @@ DevToolsVideoConsumer::DevToolsVideoConsumer(OnFrameCapturedCallback callback)
     : callback_(std::move(callback)),
       min_capture_period_(kDefaultMinCapturePeriod),
       min_frame_size_(kDefaultMinFrameSize),
-      max_frame_size_(kDefaultMaxFrameSize) {}
+      max_frame_size_(kDefaultMaxFrameSize),
+      pixel_format_(kDefaultPixelFormat),
+      color_space_(kDefaultColorSpace) {}
 
 DevToolsVideoConsumer::~DevToolsVideoConsumer() = default;
 
@@ -102,6 +109,15 @@ void DevToolsVideoConsumer::SetMinAndMaxFrameSize(gfx::Size min_frame_size,
   }
 }
 
+void DevToolsVideoConsumer::SetFormat(media::VideoPixelFormat format,
+                                      gfx::ColorSpace color_space) {
+  pixel_format_ = format;
+  color_space_ = color_space;
+  if (capturer_) {
+    capturer_->SetFormat(pixel_format_, color_space_);
+  }
+}
+
 void DevToolsVideoConsumer::InnerStartCapture(
     std::unique_ptr<viz::ClientFrameSinkVideoCapturer> capturer) {
   capturer_ = std::move(capturer);
@@ -111,6 +127,7 @@ void DevToolsVideoConsumer::InnerStartCapture(
   capturer_->SetMinSizeChangePeriod(kDefaultMinPeriod);
   capturer_->SetResolutionConstraints(min_frame_size_, max_frame_size_,
                                       kDefaultUseFixedAspectRatio);
+  capturer_->SetFormat(pixel_format_, color_space_);
   if (frame_sink_id_.is_valid())
     capturer_->ChangeTarget(frame_sink_id_);
 

content/browser/devtools/devtools_video_consumer.h

@@ -42,12 +42,11 @@ class CONTENT_EXPORT DevToolsVideoConsumer
 
   // These functions cache the values passed to them and if we're currently
   // capturing, they call the corresponding |capturer_| functions.
-  // TODO(samans): Add a SetFormat function here so that ARGB pixel format can
-  // be used.
   void SetFrameSinkId(const viz::FrameSinkId& frame_sink_id);
   void SetMinCapturePeriod(base::TimeDelta min_capture_period);
   void SetMinAndMaxFrameSize(gfx::Size min_frame_size,
                              gfx::Size max_frame_size);
+  void SetFormat(media::VideoPixelFormat format, gfx::ColorSpace color_space);
 
  private:
   friend class DevToolsVideoConsumerTest;
@@ -86,6 +85,8 @@ class CONTENT_EXPORT DevToolsVideoConsumer
   gfx::Size min_frame_size_;
   gfx::Size max_frame_size_;
   viz::FrameSinkId frame_sink_id_;
+  media::VideoPixelFormat pixel_format_;
+  gfx::ColorSpace color_space_;
 
   // If |capturer_| is alive, then we are currently capturing.
   std::unique_ptr<viz::ClientFrameSinkVideoCapturer> capturer_;

content/browser/devtools/protocol/page_handler.cc

@@ -212,6 +212,8 @@ PageHandler::PageHandler(EmulationHandler* emulation_handler,
     video_consumer_ = std::make_unique<DevToolsVideoConsumer>(
         base::BindRepeating(&PageHandler::OnFrameFromVideoConsumer,
                             weak_factory_.GetWeakPtr()));
+    video_consumer_->SetFormat(media::PIXEL_FORMAT_ARGB,
+                               gfx::ColorSpace::CreateREC709());
   }
   DCHECK(emulation_handler_);
 }

media/renderers/paint_canvas_video_renderer.cc

@@ -386,6 +386,18 @@ void ConvertVideoFrameToRGBPixelsTask(const VideoFrame* video_frame,
   uint8_t* pixels = static_cast<uint8_t*>(rgb_pixels) +
                     row_bytes * chunk_start * rows_per_chunk;
 
+  if (format == PIXEL_FORMAT_ARGB) {
+    DCHECK_LE(width, static_cast<int>(row_bytes));
+    const uint8_t* data = plane_meta[VideoFrame::kARGBPlane].data;
+    for (size_t i = 0; i < rows; i++) {
+      memcpy(pixels, data, width * 4);
+      pixels += row_bytes;
+      data += plane_meta[VideoFrame::kARGBPlane].stride;
+    }
+    done->Run();
+    return;
+  }
+
   // TODO(crbug.com/828599): This should default to BT.709 color space.
   SkYUVColorSpace color_space = kRec601_SkYUVColorSpace;
   video_frame->ColorSpace().ToSkYUVColorSpace(&color_space);
@@ -917,6 +929,7 @@ void PaintCanvasVideoRenderer::Paint(
   if (!video_frame.get() || video_frame->natural_size().IsEmpty() ||
       !(media::IsYuvPlanar(video_frame->format()) ||
         video_frame->format() == media::PIXEL_FORMAT_Y16 ||
+        video_frame->format() == media::PIXEL_FORMAT_ARGB ||
         video_frame->HasTextures())) {
     cc::PaintFlags black_with_alpha_flags;
     black_with_alpha_flags.setAlpha(flags.getAlpha());