Skip to content

Commit

Permalink
Revert "Update callsites of PaintImage::GetSkImage"
Browse files Browse the repository at this point in the history
This reverts commit 8c9ffba.

Reason for revert: suspected cause of the test failure as observed in android CQ builds:
depth_capture_tests, webgl_conformance_tests in
* https://ci.chromium.org/p/chromium/builders/try/android-marshmallow-arm64-rel/635915
* https://ci.chromium.org/p/chromium/builders/try/android-marshmallow-arm64-rel/635916
* https://ci.chromium.org/p/chromium/builders/try/android-marshmallow-arm64-rel/635919

and webkit_unit_tests in
* https://ci.chromium.org/p/chromium/builders/try/android-lollipop-arm-rel/96663
* https://ci.chromium.org/p/chromium/builders/try/android-lollipop-arm-rel/96670

Original change's description:
> Update callsites of PaintImage::GetSkImage
> 
> With OOPR Canvas we need to be explicit about how SkImages will be used
> in the renderer process. In OOPR mode, SkImages must be software backed
> because we can no longer use GrContext in the renderer. This change
> updates callsites of PaintImage::GetSkImage to specify what kind of
> SkImage is required with the new GetSwSkImage and GetAcceleratedSkImage
> APIs.
> 
> Follow up CLs will cleanup remaining callsites of GetSkImage until
> we can completely remove the function.
> 
> Bug: 1018894
> Change-Id: I69f7247e36c7cbb5d30f444f1255dd84af968df0
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2350027
> Commit-Queue: Nathan Zabriskie <nazabris@microsoft.com>
> Reviewed-by: Khushal <khushalsagar@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#798894}

TBR=dcheng@chromium.org,khushalsagar@chromium.org,nazabris@microsoft.com

Change-Id: I051d4fdbc346e66d08c72fa366ef61dc0eeaa77e
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1018894
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2360557
Reviewed-by: Haiyang Pan <hypan@google.com>
Commit-Queue: Haiyang Pan <hypan@google.com>
Cr-Commit-Position: refs/heads/master@{#798944}
  • Loading branch information
Haiyang Pan authored and Commit Bot committed Aug 18, 2020
1 parent 7797a60 commit 9cbad04
Show file tree
Hide file tree
Showing 23 changed files with 98 additions and 103 deletions.
10 changes: 0 additions & 10 deletions cc/paint/paint_image.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
#include "cc/paint/paint_record.h"
#include "cc/paint/paint_worklet_input.h"
#include "cc/paint/skia_paint_image_generator.h"
#include "third_party/skia/include/gpu/GrBackendSurface.h"
#include "ui/gfx/skia_util.h"

namespace cc {
Expand Down Expand Up @@ -294,15 +293,6 @@ bool PaintImage::IsTextureBacked() const {
return false;
}

void PaintImage::FlushPendingSkiaOps() {
if (!texture_backing_)
return;

auto image = texture_backing_->GetAcceleratedSkImage();
if (image)
image->getBackendTexture(true);
}

int PaintImage::width() const {
return paint_worklet_input_
? static_cast<int>(paint_worklet_input_->GetSize().width())
Expand Down
3 changes: 0 additions & 3 deletions cc/paint/paint_image.h
Original file line number Diff line number Diff line change
Expand Up @@ -272,9 +272,6 @@ class CC_PAINT_EXPORT PaintImage {
}
bool IsPaintWorklet() const { return !!paint_worklet_input_; }
bool IsTextureBacked() const;
// Skia internally buffers commands and flushes them as necessary but there
// are some cases where we need to force a flush.
void FlushPendingSkiaOps();
int width() const;
int height() const;
SkColorSpace* color_space() const {
Expand Down
2 changes: 1 addition & 1 deletion cc/test/test_options_provider.cc
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ void TestOptionsProvider::PushFonts() {
ImageProvider::ScopedResult TestOptionsProvider::GetRasterContent(
const DrawImage& draw_image) {
DCHECK(!draw_image.paint_image().IsPaintWorklet());
uint32_t image_id = draw_image.paint_image().GetSwSkImage()->uniqueID();
uint32_t image_id = draw_image.paint_image().GetSkImage()->uniqueID();
// Lock and reuse the entry if possible.
const EntryKey entry_key(TransferCacheEntryType::kImage, image_id);
if (LockEntryDirect(entry_key)) {
Expand Down
2 changes: 1 addition & 1 deletion cc/tiles/software_image_decode_cache_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1750,7 +1750,7 @@ TEST(SoftwareImageDecodeCacheTest, BitmapImageColorConverted) {
cache.GetDecodedImageForDraw(draw_image);
EXPECT_TRUE(decoded_draw_image.image());
// Expect that we allocated a new image.
EXPECT_NE(decoded_draw_image.image().get(), paint_image.GetSwSkImage().get());
EXPECT_NE(decoded_draw_image.image().get(), paint_image.GetSkImage().get());
// Expect that the image color space match the target color space.
EXPECT_TRUE(decoded_draw_image.image()->colorSpace());
EXPECT_TRUE(SkColorSpace::Equals(decoded_draw_image.image()->colorSpace(),
Expand Down
2 changes: 1 addition & 1 deletion cc/tiles/software_image_decode_cache_utils.cc
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ SoftwareImageDecodeCacheUtils::GenerateCacheEntryFromCandidate(
SoftwareImageDecodeCacheUtils::CacheKey
SoftwareImageDecodeCacheUtils::CacheKey::FromDrawImage(const DrawImage& image,
SkColorType color_type) {
DCHECK(!image.paint_image().IsTextureBacked());
DCHECK(!image.paint_image().GetSkImage()->isTextureBacked());

const PaintImage::FrameKey frame_key = image.frame_key();
const PaintImage::Id stable_id = image.paint_image().stable_id();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ void SystemClipboard::WriteImageWithTag(Image* image,

PaintImage paint_image = image->PaintImageForCurrentFrame();
SkBitmap bitmap;
if (sk_sp<SkImage> sk_image = paint_image.GetSwSkImage())
if (sk_sp<SkImage> sk_image = paint_image.GetSkImage())
sk_image->asLegacyBitmap(&bitmap);
clipboard_->WriteImage(bitmap);

Expand Down
6 changes: 2 additions & 4 deletions third_party/blink/renderer/core/exported/web_image.cc
Original file line number Diff line number Diff line change
Expand Up @@ -101,10 +101,8 @@ SkBitmap WebImage::DecodeSVG(const WebData& data, const WebSize& desired_size) {
container_size = svg_image->ConcreteObjectSize(FloatSize());
scoped_refptr<Image> svg_container =
SVGImageForContainer::Create(svg_image.get(), container_size, 1, KURL());
if (PaintImage image = svg_container->PaintImageForCurrentFrame()) {
image.GetSwSkImage()->asLegacyBitmap(&bitmap,
SkImage::kRO_LegacyBitmapMode);
}
if (PaintImage image = svg_container->PaintImageForCurrentFrame())
image.GetSkImage()->asLegacyBitmap(&bitmap, SkImage::kRO_LegacyBitmapMode);
return bitmap;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ CanvasAsyncBlobCreator::CanvasAsyncBlobCreator(
image_ = image_->ConvertToColorSpace(
SkColorSpace::MakeSRGB(),
GetColorTypeForConversion(skia_image->colorType()));
skia_image = image_->PaintImageForCurrentFrame().GetSwSkImage();
skia_image = image_->PaintImageForCurrentFrame().GetSkImage();
}

if (skia_image->peekPixels(&src_data_)) {
Expand Down Expand Up @@ -240,7 +240,7 @@ CanvasAsyncBlobCreator::CanvasAsyncBlobCreator(
if (needs_color_space_conversion) {
image_ = UnacceleratedStaticBitmapImage::Create(skia_image);
image_ = image_->ConvertToColorSpace(blob_color_space, target_color_type);
skia_image = image_->PaintImageForCurrentFrame().GetSwSkImage();
skia_image = image_->PaintImageForCurrentFrame().GetSkImage();
} else if (skia_image->colorType() != target_color_type) {
size_t data_length = skia_image->width() * skia_image->height() *
SkColorTypeBytesPerPixel(target_color_type);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,7 @@ TEST_F(CanvasAsyncBlobCreatorTest, ColorManagedConvertToBlob) {
source_bitmap_image->ConvertToColorSpace(expected_color_space,
expected_color_type);
sk_sp<SkImage> ref_image =
ref_bitmap->PaintImageForCurrentFrame().GetSwSkImage();
ref_bitmap->PaintImageForCurrentFrame().GetSkImage();

// Jpeg does not support transparent images.
bool compare_alpha = (blob_mime_type != "image/jpeg");
Expand Down
9 changes: 8 additions & 1 deletion third_party/blink/renderer/core/html/canvas/image_data.cc
Original file line number Diff line number Diff line change
Expand Up @@ -314,11 +314,18 @@ ImageData* ImageData::Create(const IntSize& size,
return MakeGarbageCollected<ImageData>(size, data_array, color_settings);
}

static SkImageInfo GetImageInfo(scoped_refptr<StaticBitmapImage> image) {
sk_sp<SkImage> skia_image = image->PaintImageForCurrentFrame().GetSkImage();
return SkImageInfo::Make(skia_image->width(), skia_image->height(),
skia_image->colorType(), skia_image->alphaType(),
skia_image->refColorSpace());
}

ImageData* ImageData::Create(scoped_refptr<StaticBitmapImage> image,
AlphaDisposition alpha_disposition) {
PaintImage paint_image = image->PaintImageForCurrentFrame();
DCHECK(paint_image);
SkImageInfo image_info = image->PaintImageForCurrentFrame().GetSkImageInfo();
SkImageInfo image_info = GetImageInfo(image);
CanvasColorParams color_params(image_info);
if (image_info.alphaType() != kOpaque_SkAlphaType) {
if (alpha_disposition == kPremultiplyAlpha) {
Expand Down
51 changes: 27 additions & 24 deletions third_party/blink/renderer/core/imagebitmap/image_bitmap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -282,16 +282,22 @@ std::unique_ptr<CanvasResourceProvider> CreateProviderForVideoElement(
scoped_refptr<StaticBitmapImage> FlipImageVertically(
scoped_refptr<StaticBitmapImage> input,
const ImageBitmap::ParsedOptions& parsed_options) {
SkImageInfo info = GetSkImageInfo(input);
if (info.isEmpty())
sk_sp<SkImage> image = input->PaintImageForCurrentFrame().GetSkImage();
if (!image)
return nullptr;

PaintImage paint_image = input->PaintImageForCurrentFrame();

if (ShouldAvoidPremul(parsed_options)) {
// Unpremul code path results in a GPU readback if |input| is texture
// backed since CopyImageData() uses SkImage::readPixels() to extract the
// pixels from SkImage.
SkImageInfo info = GetSkImageInfo(input);
if (info.isEmpty())
return nullptr;

PaintImage paint_image = input->PaintImageForCurrentFrame();
if (paint_image.GetSkImageInfo().isEmpty())
return nullptr;

sk_sp<SkData> image_pixels = TryAllocateSkData(info.computeMinByteSize());
if (!image_pixels)
return nullptr;
Expand Down Expand Up @@ -320,10 +326,10 @@ scoped_refptr<StaticBitmapImage> FlipImageVertically(
// can use both accelerated and software surfaces. If the image is unpremul,
// we have to use software surfaces.
bool use_accelerated =
paint_image.IsTextureBacked() && info.alphaType() == kPremul_SkAlphaType;
image->isTextureBacked() && image->alphaType() == kPremul_SkAlphaType;
auto resource_provider = CreateProvider(
use_accelerated ? input->ContextProviderWrapper() : nullptr, info, input,
true /* fallback_to_software */);
use_accelerated ? input->ContextProviderWrapper() : nullptr,
GetSkImageInfo(input), input, true /* fallback_to_software */);
if (!resource_provider)
return nullptr;

Expand Down Expand Up @@ -389,14 +395,14 @@ scoped_refptr<StaticBitmapImage> GetImageWithAlphaDisposition(
scoped_refptr<StaticBitmapImage> ScaleImage(
scoped_refptr<StaticBitmapImage>&& image,
const ImageBitmap::ParsedOptions& parsed_options) {
auto src_image_info = image->PaintImageForCurrentFrame().GetSkImageInfo();
auto sk_image = image->PaintImageForCurrentFrame().GetSkImage();
auto image_info = GetSkImageInfo(image).makeWH(parsed_options.resize_width,
parsed_options.resize_height);

// Try to avoid GPU read back by drawing accelerated premul image on an
// accelerated surface.
if (!ShouldAvoidPremul(parsed_options) && image->IsTextureBacked() &&
src_image_info.alphaType() == kPremul_SkAlphaType) {
sk_image->alphaType() == kPremul_SkAlphaType) {
auto resource_provider =
CreateProvider(image->ContextProviderWrapper(), image_info, image,
false /* fallback_to_software */);
Expand All @@ -405,7 +411,7 @@ scoped_refptr<StaticBitmapImage> ScaleImage(
paint.setFilterQuality(parsed_options.resize_quality);
resource_provider->Canvas()->drawImageRect(
image->PaintImageForCurrentFrame(),
SkRect::MakeWH(src_image_info.width(), src_image_info.height()),
SkRect::MakeWH(sk_image->width(), sk_image->height()),
SkRect::MakeWH(parsed_options.resize_width,
parsed_options.resize_height),
&paint, SkCanvas::kStrict_SrcRectConstraint);
Expand All @@ -425,7 +431,6 @@ scoped_refptr<StaticBitmapImage> ScaleImage(

SkPixmap resized_pixmap(image_info, image_pixels->data(),
image_info.minRowBytes());
auto sk_image = image->PaintImageForCurrentFrame().GetSwSkImage();
sk_image->scalePixels(resized_pixmap, parsed_options.resize_quality);
// Tag the resized Pixmap with the correct color space.
resized_pixmap.setColorSpace(GetSkImageInfo(image).refColorSpace());
Expand All @@ -445,17 +450,15 @@ scoped_refptr<StaticBitmapImage> ApplyColorSpaceConversion(
sk_sp<SkColorSpace> color_space = options.color_params.GetSkColorSpace();
SkColorType color_type =
image->IsTextureBacked() ? kRGBA_8888_SkColorType : kN32_SkColorType;
SkImageInfo src_image_info =
image->PaintImageForCurrentFrame().GetSkImageInfo();
if (src_image_info.isEmpty())
sk_sp<SkImage> sk_image = image->PaintImageForCurrentFrame().GetSkImage();
if (!sk_image)
return nullptr;

// If we should preserve color precision, don't lose it in color space
// conversion.
if (options.pixel_format == kImageBitmapPixelFormat_Default &&
(src_image_info.colorType() == kRGBA_F16_SkColorType ||
(src_image_info.colorSpace() &&
src_image_info.colorSpace()->gammaIsLinear()) ||
(sk_image->colorType() == kRGBA_F16_SkColorType ||
(sk_image->colorSpace() && sk_image->colorSpace()->gammaIsLinear()) ||
(color_space && color_space->gammaIsLinear()))) {
color_type = kRGBA_F16_SkColorType;
}
Expand Down Expand Up @@ -485,16 +488,16 @@ scoped_refptr<StaticBitmapImage> GetImageWithPixelFormat(
return std::move(image);
// If the the image is not half float backed, default and uint8 image bitmap
// pixel formats result in the same uint8 backed image bitmap.
SkImageInfo image_info = image->PaintImageForCurrentFrame().GetSkImageInfo();
if (image_info.colorType() != kRGBA_F16_SkColorType)
sk_sp<SkImage> skia_image = image->PaintImageForCurrentFrame().GetSkImage();
if (skia_image->colorType() != kRGBA_F16_SkColorType)
return std::move(image);

auto skia_image = image->PaintImageForCurrentFrame().GetSwSkImage();
SkImageInfo target_info = image_info.makeColorType(kN32_SkColorType);
SkPixmap pixmap;
skia_image->peekPixels(&pixmap);
SkImageInfo target_info = pixmap.info().makeColorType(kN32_SkColorType);
SkBitmap target_bitmap;
target_bitmap.allocPixels(target_info);
bool read_successful = skia_image->readPixels(target_bitmap.pixmap(), 0, 0);
DCHECK(read_successful);
pixmap.readPixels(target_bitmap.pixmap(), 0, 0);
return UnacceleratedStaticBitmapImage::Create(
SkImage::MakeFromBitmap(target_bitmap));
}
Expand Down Expand Up @@ -645,7 +648,7 @@ ImageBitmap::ImageBitmap(ImageElementBase* image,
SkBitmap bitmap;
SkImageInfo image_info = GetSkImageInfo(input);
bitmap.allocPixels(image_info, image_info.minRowBytes());
if (!paint_image.GetSwSkImage()->readPixels(bitmap.pixmap(), 0, 0))
if (!paint_image.GetSkImage()->readPixels(bitmap.pixmap(), 0, 0))
return;

paint_image = PaintImageBuilder::WithDefault()
Expand Down
47 changes: 23 additions & 24 deletions third_party/blink/renderer/core/imagebitmap/image_bitmap_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -141,33 +141,33 @@ TEST_F(ImageBitmapTest, ImageResourceConsistency) {

ASSERT_EQ(image_bitmap_no_crop->BitmapImage()
->PaintImageForCurrentFrame()
.GetSwSkImage(),
.GetSkImage(),
image_element->CachedImage()
->GetImage()
->PaintImageForCurrentFrame()
.GetSwSkImage());
.GetSkImage());
ASSERT_NE(image_bitmap_interior_crop->BitmapImage()
->PaintImageForCurrentFrame()
.GetSwSkImage(),
.GetSkImage(),
image_element->CachedImage()
->GetImage()
->PaintImageForCurrentFrame()
.GetSwSkImage());
.GetSkImage());
ASSERT_EQ(image_bitmap_exterior_crop->BitmapImage()
->PaintImageForCurrentFrame()
.GetSwSkImage(),
.GetSkImage(),
image_element->CachedImage()
->GetImage()
->PaintImageForCurrentFrame()
.GetSwSkImage());
.GetSkImage());

scoped_refptr<StaticBitmapImage> empty_image =
image_bitmap_outside_crop->BitmapImage();
ASSERT_NE(empty_image->PaintImageForCurrentFrame().GetSwSkImage(),
ASSERT_NE(empty_image->PaintImageForCurrentFrame().GetSkImage(),
image_element->CachedImage()
->GetImage()
->PaintImageForCurrentFrame()
.GetSwSkImage());
.GetSkImage());
}

// Verifies that ImageBitmaps constructed from HTMLImageElements hold a
Expand All @@ -192,48 +192,48 @@ TEST_F(ImageBitmapTest, ImageBitmapSourceChanged) {
MakeGarbageCollected<ImageBitmap>(image, crop_rect, default_options);
ASSERT_TRUE(image_bitmap);
ASSERT_EQ(
image_bitmap->BitmapImage()->PaintImageForCurrentFrame().GetSwSkImage(),
image_bitmap->BitmapImage()->PaintImageForCurrentFrame().GetSkImage(),
original_image_content->GetImage()
->PaintImageForCurrentFrame()
.GetSwSkImage());
.GetSkImage());

ImageResourceContent* new_image_content = ImageResourceContent::CreateLoaded(
UnacceleratedStaticBitmapImage::Create(image2_).get());
image->SetImageForTest(new_image_content);

{
ASSERT_EQ(
image_bitmap->BitmapImage()->PaintImageForCurrentFrame().GetSwSkImage(),
image_bitmap->BitmapImage()->PaintImageForCurrentFrame().GetSkImage(),
original_image_content->GetImage()
->PaintImageForCurrentFrame()
.GetSwSkImage());
.GetSkImage());
SkImage* image1 = image_bitmap->BitmapImage()
->PaintImageForCurrentFrame()
.GetSwSkImage()
.GetSkImage()
.get();
ASSERT_NE(image1, nullptr);
SkImage* image2 = original_image_content->GetImage()
->PaintImageForCurrentFrame()
.GetSwSkImage()
.GetSkImage()
.get();
ASSERT_NE(image2, nullptr);
ASSERT_EQ(image1, image2);
}

{
ASSERT_NE(
image_bitmap->BitmapImage()->PaintImageForCurrentFrame().GetSwSkImage(),
image_bitmap->BitmapImage()->PaintImageForCurrentFrame().GetSkImage(),
new_image_content->GetImage()
->PaintImageForCurrentFrame()
.GetSwSkImage());
.GetSkImage());
SkImage* image1 = image_bitmap->BitmapImage()
->PaintImageForCurrentFrame()
.GetSwSkImage()
.GetSkImage()
.get();
ASSERT_NE(image1, nullptr);
SkImage* image2 = new_image_content->GetImage()
->PaintImageForCurrentFrame()
.GetSwSkImage()
.GetSkImage()
.get();
ASSERT_NE(image2, nullptr);
ASSERT_NE(image1, image2);
Expand Down Expand Up @@ -321,7 +321,7 @@ TEST_F(ImageBitmapTest, ImageBitmapPixelFormat) {

ASSERT_TRUE(image_bitmap);
sk_sp<SkImage> sk_image_internal =
image_bitmap->BitmapImage()->PaintImageForCurrentFrame().GetSwSkImage();
image_bitmap->BitmapImage()->PaintImageForCurrentFrame().GetSkImage();
ASSERT_EQ(kN32_SkColorType, sk_image_internal->colorType());

// source: uint8, bitmap pixel format: uint8
Expand All @@ -331,7 +331,7 @@ TEST_F(ImageBitmapTest, ImageBitmapPixelFormat) {
ASSERT_TRUE(image_bitmap_8888);
sk_sp<SkImage> sk_image_internal_8888 = image_bitmap_8888->BitmapImage()
->PaintImageForCurrentFrame()
.GetSwSkImage();
.GetSkImage();
ASSERT_EQ(kN32_SkColorType, sk_image_internal_8888->colorType());

// Since there is no conversion from uint8 to default for image bitmap pixel
Expand All @@ -353,9 +353,8 @@ TEST_F(ImageBitmapTest, ImageBitmapPixelFormat) {
auto* image_bitmap_f16 = MakeGarbageCollected<ImageBitmap>(
bitmap_image_f16, bitmap_image_f16->Rect(), options_f16);
ASSERT_TRUE(image_bitmap_f16);
sk_sp<SkImage> sk_image_internal_f16 = image_bitmap_f16->BitmapImage()
->PaintImageForCurrentFrame()
.GetSwSkImage();
sk_sp<SkImage> sk_image_internal_f16 =
image_bitmap_f16->BitmapImage()->PaintImageForCurrentFrame().GetSkImage();
ASSERT_EQ(kRGBA_F16_SkColorType, sk_image_internal_f16->colorType());

// source: f16, bitmap pixel format: uint8
Expand All @@ -366,7 +365,7 @@ TEST_F(ImageBitmapTest, ImageBitmapPixelFormat) {
sk_sp<SkImage> sk_image_internal_f16_8888 =
image_bitmap_f16_8888->BitmapImage()
->PaintImageForCurrentFrame()
.GetSwSkImage();
.GetSkImage();
ASSERT_EQ(kN32_SkColorType, sk_image_internal_f16_8888->colorType());
}

Expand Down
Loading

0 comments on commit 9cbad04

Please sign in to comment.