Fix 30bpp rendering on Linux/X11

This CL adds support for some additional X11 color types: RGB565, ARGB4444,
RGBA1010102. Also avoid unnecessarily rendering into an RGBA8888 buffer and
converting to the window format; just render into the correct format directly.
Also simplify/refactor some code.

BUG=1016383
R=rjkroege

Change-Id: I08051526b81d364a236c37654fda1be6bad1bc20
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1899324Reviewed-by: Robert Kroeger <rjkroege@chromium.org>
Commit-Queue: Thomas Anderson <thomasanderson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#712812}

components/viz/service/display_embedder/software_output_device_x11.cc

@@ -39,38 +39,25 @@ SoftwareOutputDeviceX11::~SoftwareOutputDeviceX11() {
 void SoftwareOutputDeviceX11::Resize(const gfx::Size& pixel_size,
                                      float scale_factor) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
-  // See comment in X11SoftwareBitmapPresenter::Resize
-  if (x11_software_bitmap_presenter_.Resize(pixel_size)) {
-    viewport_pixel_size_ = pixel_size;
-    surface_ = nullptr;
-  } else {
-    SoftwareOutputDevice::Resize(pixel_size, scale_factor);
-  }
+  viewport_pixel_size_ = pixel_size;
+  x11_software_bitmap_presenter_.Resize(pixel_size);
 }
 
 SkCanvas* SoftwareOutputDeviceX11::BeginPaint(const gfx::Rect& damage_rect) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   damage_rect_ = damage_rect;
-
-  auto* sk_canvas = x11_software_bitmap_presenter_.GetSkCanvas();
-
-  if (!sk_canvas)
-    sk_canvas = SoftwareOutputDevice::BeginPaint(damage_rect);
-  return sk_canvas;
+  return x11_software_bitmap_presenter_.GetSkCanvas();
 }
 
 void SoftwareOutputDeviceX11::EndPaint() {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   SoftwareOutputDevice::EndPaint();
-  x11_software_bitmap_presenter_.EndPaint(surface_, damage_rect_);
+  x11_software_bitmap_presenter_.EndPaint(damage_rect_);
 }
 
 void SoftwareOutputDeviceX11::OnSwapBuffers(
     SwapBuffersCallback swap_ack_callback) {
-  if (x11_software_bitmap_presenter_.ShmPoolReady())
-    x11_software_bitmap_presenter_.OnSwapBuffers(std::move(swap_ack_callback));
-  else
-    SoftwareOutputDevice::OnSwapBuffers(std::move(swap_ack_callback));
+  x11_software_bitmap_presenter_.OnSwapBuffers(std::move(swap_ack_callback));
 }
 
 int SoftwareOutputDeviceX11::MaxFramesPending() const {

ui/base/x/x11_shm_image_pool_base.cc

@@ -165,11 +165,9 @@ bool XShmImagePoolBase::Resize(const gfx::Size& pixel_size) {
   for (std::size_t i = 0; i < frame_states_.size(); ++i) {
     FrameState& state = frame_states_[i];
     state.image->data = state.shminfo_.shmaddr;
-    SkImageInfo image_info = SkImageInfo::Make(
-        state.image->width, state.image->height,
-        state.image->byte_order == LSBFirst ? kBGRA_8888_SkColorType
-                                            : kRGBA_8888_SkColorType,
-        kPremul_SkAlphaType);
+    SkImageInfo image_info =
+        SkImageInfo::Make(state.image->width, state.image->height,
+                          ColorTypeForVisual(visual_), kPremul_SkAlphaType);
     state.bitmap = SkBitmap();
     if (!state.bitmap.installPixels(image_info, state.image->data,
                                     state.image->bytes_per_line)) {

ui/base/x/x11_software_bitmap_presenter.cc

@@ -12,6 +12,7 @@
 #include <memory>
 #include <utility>
 
+#include "base/bind.h"
 #include "base/macros.h"
 #include "third_party/skia/include/core/SkCanvas.h"
 #include "third_party/skia/include/core/SkImageInfo.h"
@@ -125,7 +126,10 @@ X11SoftwareBitmapPresenter::X11SoftwareBitmapPresenter(
     gfx::AcceleratedWidget widget,
     base::TaskRunner* host_task_runner,
     base::TaskRunner* event_task_runner)
-    : widget_(widget), display_(gfx::GetXDisplay()), gc_(nullptr) {
+    : widget_(widget),
+      display_(gfx::GetXDisplay()),
+      gc_(nullptr),
+      host_task_runner_(host_task_runner) {
   DCHECK_NE(widget_, gfx::kNullAcceleratedWidget);
   gc_ = XCreateGC(display_, widget_, 0, nullptr);
   memset(&attributes_, 0, sizeof(attributes_));
@@ -162,7 +166,9 @@ bool X11SoftwareBitmapPresenter::ShmPoolReady() const {
   return shm_pool_ && shm_pool_->Ready();
 }
 
-bool X11SoftwareBitmapPresenter::Resize(const gfx::Size& pixel_size) {
+void X11SoftwareBitmapPresenter::Resize(const gfx::Size& pixel_size) {
+  if (pixel_size == viewport_pixel_size_)
+    return;
   viewport_pixel_size_ = pixel_size;
   // Fallback to the non-shm codepath when |composite_| is true, which only
   // happens for status icon windows that are typically 16x16px.  It's possible
@@ -170,19 +176,24 @@ bool X11SoftwareBitmapPresenter::Resize(const gfx::Size& pixel_size) {
   // affect windows that are tiny and infrequently updated.
   if (!composite_ && shm_pool_ && shm_pool_->Resize(pixel_size)) {
     needs_swap_ = false;
-    return true;
+    surface_ = nullptr;
+  } else {
+    SkImageInfo info = SkImageInfo::Make(
+        viewport_pixel_size_.width(), viewport_pixel_size_.height(),
+        ColorTypeForVisual(attributes_.visual), kOpaque_SkAlphaType);
+    surface_ = SkSurface::MakeRaster(info);
   }
-  return false;
 }
 
 SkCanvas* X11SoftwareBitmapPresenter::GetSkCanvas() {
   if (ShmPoolReady())
     return shm_pool_->CurrentCanvas();
+  else if (surface_)
+    return surface_->getCanvas();
   return nullptr;
 }
 
-void X11SoftwareBitmapPresenter::EndPaint(sk_sp<SkSurface> sk_surface,
-                                          const gfx::Rect& damage_rect) {
+void X11SoftwareBitmapPresenter::EndPaint(const gfx::Rect& damage_rect) {
   gfx::Rect rect = damage_rect;
   rect.Intersect(gfx::Rect(viewport_pixel_size_));
   if (rect.IsEmpty())
@@ -191,7 +202,6 @@ void X11SoftwareBitmapPresenter::EndPaint(sk_sp<SkSurface> sk_surface,
   SkPixmap skia_pixmap;
 
   if (ShmPoolReady()) {
-    DCHECK(!sk_surface);
     // TODO(thomasanderson): Investigate direct rendering with DRI3 to avoid any
     // unnecessary X11 IPC or buffer copying.
     if (XShmPutImage(display_, widget_, gc_, shm_pool_->CurrentImage(),
@@ -203,8 +213,7 @@ void X11SoftwareBitmapPresenter::EndPaint(sk_sp<SkSurface> sk_surface,
     }
     skia_pixmap = shm_pool_->CurrentBitmap().pixmap();
   } else {
-    DCHECK(sk_surface);
-    sk_surface->peekPixels(&skia_pixmap);
+    surface_->peekPixels(&skia_pixmap);
   }
 
   if (composite_ &&
@@ -214,65 +223,26 @@ void X11SoftwareBitmapPresenter::EndPaint(sk_sp<SkSurface> sk_surface,
     return;
   }
 
-  int bpp = gfx::BitsPerPixelForPixmapDepth(display_, attributes_.depth);
-
-  if (bpp != 32 && bpp != 16 && ui::QueryRenderSupport(display_)) {
-    // gfx::PutARGBImage only supports 16 and 32 bpp, but Xrender can do other
-    // conversions.
-    Pixmap pixmap =
-        XCreatePixmap(display_, widget_, rect.width(), rect.height(), 32);
-    GC gc = XCreateGC(display_, pixmap, 0, nullptr);
-    XImage image;
-    memset(&image, 0, sizeof(image));
-
-    image.width = viewport_pixel_size_.width();
-    image.height = viewport_pixel_size_.height();
-    image.depth = 32;
-    image.bits_per_pixel = 32;
-    image.format = ZPixmap;
-    image.byte_order = LSBFirst;
-    image.bitmap_unit = 8;
-    image.bitmap_bit_order = LSBFirst;
-    image.bytes_per_line = skia_pixmap.rowBytes();
-    image.red_mask = 0xff;
-    image.green_mask = 0xff00;
-    image.blue_mask = 0xff0000;
-    image.data =
-        const_cast<char*>(static_cast<const char*>(skia_pixmap.addr()));
-
-    XPutImage(display_, pixmap, gc, &image, rect.x(),
-              rect.y() /* source x, y */, 0, 0 /* dest x, y */, rect.width(),
-              rect.height());
-    XFreeGC(display_, gc);
-    Picture picture = XRenderCreatePicture(
-        display_, pixmap, ui::GetRenderARGB32Format(display_), 0, nullptr);
-    XRenderPictFormat* pictformat =
-        XRenderFindVisualFormat(display_, attributes_.visual);
-    Picture dest_picture =
-        XRenderCreatePicture(display_, widget_, pictformat, 0, nullptr);
-    XRenderComposite(display_,
-                     PictOpSrc,       // op
-                     picture,         // src
-                     0,               // mask
-                     dest_picture,    // dest
-                     0,               // src_x
-                     0,               // src_y
-                     0,               // mask_x
-                     0,               // mask_y
-                     rect.x(),        // dest_x
-                     rect.y(),        // dest_y
-                     rect.width(),    // width
-                     rect.height());  // height
-    XRenderFreePicture(display_, picture);
-    XRenderFreePicture(display_, dest_picture);
-    XFreePixmap(display_, pixmap);
-  } else {
-    gfx::PutARGBImage(display_, attributes_.visual, attributes_.depth, widget_,
-                      gc_, static_cast<const uint8_t*>(skia_pixmap.addr()),
-                      viewport_pixel_size_.width(),
-                      viewport_pixel_size_.height(), rect.x(), rect.y(),
-                      rect.x(), rect.y(), rect.width(), rect.height());
-  }
+  XImage image = {};
+  image.width = viewport_pixel_size_.width();
+  image.height = viewport_pixel_size_.height();
+  image.format = ZPixmap;
+  image.byte_order = LSBFirst;
+  image.bitmap_unit = 8;
+  image.bitmap_bit_order = LSBFirst;
+  image.depth = attributes_.depth;
+
+  image.bits_per_pixel = attributes_.visual->bits_per_rgb;
+  image.bits_per_pixel = skia_pixmap.info().bytesPerPixel() * 8;
+
+  image.bytes_per_line = skia_pixmap.rowBytes();
+  image.red_mask = attributes_.visual->red_mask;
+  image.green_mask = attributes_.visual->green_mask;
+  image.blue_mask = attributes_.visual->blue_mask;
+
+  image.data = reinterpret_cast<char*>(const_cast<void*>(skia_pixmap.addr()));
+  XPutImage(display_, widget_, gc_, &image, rect.x(), rect.y(), rect.x(),
+            rect.y(), rect.width(), rect.height());
 
   // Ensure the new window content appears immediately. On a TYPE_UI thread we
   // can rely on the message loop to flush for us so XFlush() isn't necessary.
@@ -283,12 +253,17 @@ void X11SoftwareBitmapPresenter::EndPaint(sk_sp<SkSurface> sk_surface,
 
 void X11SoftwareBitmapPresenter::OnSwapBuffers(
     SwapBuffersCallback swap_ack_callback) {
-  DCHECK(ShmPoolReady());
-  if (needs_swap_)
-    shm_pool_->SwapBuffers(std::move(swap_ack_callback));
-  else
-    std::move(swap_ack_callback).Run(viewport_pixel_size_);
-  needs_swap_ = false;
+  if (ShmPoolReady()) {
+    if (needs_swap_)
+      shm_pool_->SwapBuffers(std::move(swap_ack_callback));
+    else
+      std::move(swap_ack_callback).Run(viewport_pixel_size_);
+    needs_swap_ = false;
+  } else {
+    host_task_runner_->PostTask(
+        FROM_HERE,
+        base::BindOnce(std::move(swap_ack_callback), viewport_pixel_size_));
+  }
 }
 
 int X11SoftwareBitmapPresenter::MaxFramesPending() const {

ui/base/x/x11_software_bitmap_presenter.h

@@ -9,6 +9,7 @@
 #include "base/macros.h"
 #include "base/memory/scoped_refptr.h"
 #include "third_party/skia/include/core/SkRefCnt.h"
+#include "third_party/skia/include/core/SkSurface.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/geometry/size.h"
 #include "ui/gfx/native_widget_types.h"
@@ -16,7 +17,6 @@
 #include "ui/gfx/x/x11_types.h"
 
 class SkCanvas;
-class SkSurface;
 
 namespace base {
 class TaskRunner;
@@ -37,10 +37,9 @@ class COMPONENT_EXPORT(UI_BASE_X) X11SoftwareBitmapPresenter {
 
   ~X11SoftwareBitmapPresenter();
 
-  bool ShmPoolReady() const;
-  bool Resize(const gfx::Size& pixel_size);
+  void Resize(const gfx::Size& pixel_size);
   SkCanvas* GetSkCanvas();
-  void EndPaint(sk_sp<SkSurface> sk_surface, const gfx::Rect& damage_rect);
+  void EndPaint(const gfx::Rect& damage_rect);
   void OnSwapBuffers(SwapBuffersCallback swap_ack_callback);
   int MaxFramesPending() const;
 
@@ -57,6 +56,8 @@ class COMPONENT_EXPORT(UI_BASE_X) X11SoftwareBitmapPresenter {
                               GC gc,
                               const void* data);
 
+  bool ShmPoolReady() const;
+
   gfx::AcceleratedWidget widget_;
   XDisplay* display_;
   GC gc_;
@@ -69,6 +70,9 @@ class COMPONENT_EXPORT(UI_BASE_X) X11SoftwareBitmapPresenter {
   scoped_refptr<ui::XShmImagePoolBase> shm_pool_;
   bool needs_swap_ = false;
 
+  base::TaskRunner* host_task_runner_;
+  sk_sp<SkSurface> surface_;
+
   gfx::Size viewport_pixel_size_;
 
   DISALLOW_COPY_AND_ASSIGN(X11SoftwareBitmapPresenter);

ui/base/x/x11_util.cc

@@ -1288,6 +1288,31 @@ bool IsSyncExtensionAvailable() {
   return result;
 }
 
+SkColorType ColorTypeForVisual(void* visual) {
+  struct {
+    SkColorType color_type;
+    unsigned long red_mask;
+    unsigned long green_mask;
+    unsigned long blue_mask;
+  } color_infos[] = {
+      {kRGB_565_SkColorType, 0x1f, 0x7e0, 0xf800},
+      {kARGB_4444_SkColorType, 0xf0, 0xf00, 0xf000},
+      {kRGBA_8888_SkColorType, 0xff, 0xff00, 0xff0000},
+      {kBGRA_8888_SkColorType, 0xff0000, 0xff00, 0xff},
+      {kRGBA_1010102_SkColorType, 0x3ff, 0xffc00, 0x3ff00000},
+  };
+  Visual* vis = reinterpret_cast<Visual*>(visual);
+  for (const auto& color_info : color_infos) {
+    if (vis->red_mask == color_info.red_mask &&
+        vis->green_mask == color_info.green_mask &&
+        vis->blue_mask == color_info.blue_mask) {
+      return color_info.color_type;
+    }
+  }
+  LOG(FATAL) << "Unsupported visual: " << XVisualIDFromVisual(vis);
+  return kUnknown_SkColorType;
+}
+
 XRefcountedMemory::XRefcountedMemory(unsigned char* x11_data, size_t length)
     : x11_data_(length ? x11_data : nullptr), length_(length) {
 }

ui/base/x/x11_util.h

@@ -324,6 +324,11 @@ gfx::ICCProfile GetICCProfileForMonitor(int monitor);
 // Return true if the display supports SYNC extension.
 COMPONENT_EXPORT(UI_BASE_X) bool IsSyncExtensionAvailable();
 
+// Returns the preferred Skia colortype for an X11 visual.  LOG(FATAL)'s if
+// there isn't a suitable colortype.
+COMPONENT_EXPORT(UI_BASE_X)
+SkColorType ColorTypeForVisual(void* visual);
+
 // Manages a piece of X11 allocated memory as a RefCountedMemory segment. This
 // object takes ownership over the passed in memory and will free it with the
 // X11 allocator when done.

ui/ozone/platform/x11/x11_canvas_surface.cc

@@ -26,20 +26,14 @@ sk_sp<SkSurface> X11CanvasSurface::GetSurface() {
 }
 
 void X11CanvasSurface::ResizeCanvas(const gfx::Size& viewport_size) {
-  if (viewport_pixel_size_ == viewport_size)
-    return;
-  viewport_pixel_size_ = viewport_size;
-
-  SkCanvas* sk_canvas = nullptr;
-  // If the software painter was able to resize the shm pool, use the sk_canvas
-  // it can create from the bitmap to create an SkSurface.
-  if (x11_software_bitmap_presenter_.Resize(viewport_size))
-    sk_canvas = x11_software_bitmap_presenter_.GetSkCanvas();
-  CreateSkSurface(sk_canvas);
+  x11_software_bitmap_presenter_.Resize(viewport_size);
+  SkImageInfo info = SkImageInfo::MakeN32(
+      viewport_size.width(), viewport_size.height(), kOpaque_SkAlphaType);
+  surface_ = x11_software_bitmap_presenter_.GetSkCanvas()->makeSurface(info);
 }
 
 void X11CanvasSurface::PresentCanvas(const gfx::Rect& damage) {
-  x11_software_bitmap_presenter_.EndPaint(surface_, damage);
+  x11_software_bitmap_presenter_.EndPaint(damage);
 }
 
 std::unique_ptr<gfx::VSyncProvider> X11CanvasSurface::CreateVSyncProvider() {
@@ -53,28 +47,11 @@ bool X11CanvasSurface::SupportsAsyncBufferSwap() const {
 }
 
 void X11CanvasSurface::OnSwapBuffers(SwapBuffersCallback swap_ack_callback) {
-  if (x11_software_bitmap_presenter_.ShmPoolReady()) {
-    x11_software_bitmap_presenter_.OnSwapBuffers(std::move(swap_ack_callback));
-  } else {
-    task_runner_->PostTask(
-        FROM_HERE,
-        base::BindOnce(std::move(swap_ack_callback), viewport_pixel_size_));
-  }
+  x11_software_bitmap_presenter_.OnSwapBuffers(std::move(swap_ack_callback));
 }
 
 int X11CanvasSurface::MaxFramesPending() const {
   return x11_software_bitmap_presenter_.MaxFramesPending();
 }
 
-void X11CanvasSurface::CreateSkSurface(SkCanvas* sk_canvas) {
-  SkImageInfo info =
-      SkImageInfo::MakeN32(viewport_pixel_size_.width(),
-                           viewport_pixel_size_.height(), kOpaque_SkAlphaType);
-  if (!sk_canvas) {
-    surface_ = SkSurface::MakeRaster(info);
-  } else {
-    surface_ = sk_canvas->makeSurface(info);
-  }
-}
-
 }  // namespace ui

ui/ozone/platform/x11/x11_canvas_surface.h

@@ -38,13 +38,6 @@ class X11CanvasSurface : public SurfaceOzoneCanvas {
   int MaxFramesPending() const override;
 
  private:
-  // Creates SkSurface associated to the |sk_canvas| if not nullptr. Otherwise,
-  // allocates raster SkSurface.
-  void CreateSkSurface(SkCanvas* sk_canvas);
-
-  // Current size of the software output device.
-  gfx::Size viewport_pixel_size_;
-
   // Current surface we paint to.
   sk_sp<SkSurface> surface_;