Skip to content

Commit 099df4a

Browse files
kjlubickSkCQ
authored andcommitted
Revert^2 "Remove mipmaps from SkBitmap's implementation"
This reverts commit 6a69295. Reland "Remove mipmaps from SkBitmap's implementation" This reverts commit 6a69295. Reason for revert: Was not the culprit (see http://b/480815083#comment56) Original change's description: > Revert "Remove mipmaps from SkBitmap's implementation" > > This reverts commit 7e79e4a. > > Reason for revert: speculative fix for b/480815083, reverting to > create a canary for checking. Edit: very clear candidate for b/479634079 > > Original change's description: > > Remove mipmaps from SkBitmap's implementation > > > > This pushes them down into SkImage_Raster and makes a Ganesh-specific > > class to hold a bitmap and the mips. > > > > Suggested Review order: > > - SkImage_Raster to see mips put there > > - SkDraw, SkBitmapDevice, SkGlyphRunPainter, src/core/SkOverdrawCanvas to see CPU backend changes to thread mips in to image shader > > - GrBitmap to see new Ganesh-specific class and SkGr.h for the internal API change. > > - The rest of src/gpu/ganesh to see it being used. > > - gm/showmiplevels.cpp (which I broke and then puzzled about what > > it was supposed to look like and then *why* it was supposed to > > look that way and I decided to write it down). > > - All other changes in any order > > > > Change-Id: I93a36df817e5a607305786118b6f443aec166374 > > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1132776 > > Reviewed-by: Greg Daniel <egdaniel@google.com> > > Auto-Submit: Kaylee Lubick <kjlubick@google.com> > > Commit-Queue: Greg Daniel <egdaniel@google.com> > > Change-Id: I663686051f267f323d3775b70ccdbb618825e9dd > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1171276 > Auto-Submit: Noelle Scobie <nscobie@google.com> > Reviewed-by: Jorge Betancourt <jmbetancourt@google.com> > Commit-Queue: Ben Wagner <bungeman@google.com> > Reviewed-by: Ben Wagner <bungeman@google.com> Change-Id: I8bfe4bfda6466405d49a98e9c493264763d89efb Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1182996 Owners-Override: Kaylee Lubick <kjlubick@google.com> Reviewed-by: Jorge Betancourt <jmbetancourt@google.com> Commit-Queue: Kaylee Lubick <kjlubick@google.com>
1 parent 257d042 commit 099df4a

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

46 files changed

+462
-249
lines changed

gm/fp_sample_chaining.cpp

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
#include "src/gpu/ganesh/effects/GrMatrixEffect.h"
1919
#include "src/gpu/ganesh/effects/GrTextureEffect.h"
2020
#include "src/gpu/ganesh/glsl/GrGLSLFragmentShaderBuilder.h"
21+
#include "src/gpu/ganesh/image/GrMippedBitmap.h"
2122
#include "tools/ToolUtils.h"
2223
#include "tools/fonts/FontToolUtils.h"
2324

@@ -189,14 +190,16 @@ DEF_SIMPLE_GPU_GM_CAN_FAIL(fp_sample_chaining, rContext, canvas, errorMsg, 232,
189190
auto nextRow = [&] { x = 10; y += (64 + 10); };
190191

191192
auto draw = [&](std::initializer_list<EffectType> effects) {
192-
// Enable TestPatternEffect to get a fully procedural inner effect. It's not quite as nice
193-
// visually (no text labels in each box), but it avoids the extra GrMatrixEffect.
194-
// Switching it on actually triggers *more* shader compilation failures.
193+
// Enable TestPatternEffect to get a fully procedural inner effect. It's not quite as nice
194+
// visually (no text labels in each box), but it avoids the extra GrMatrixEffect.
195+
// Switching it on actually triggers *more* shader compilation failures.
195196
#if 0
196197
auto fp = std::unique_ptr<GrFragmentProcessor>(new TestPatternEffect());
197198
#else
198-
auto view = std::get<0>(GrMakeCachedBitmapProxyView(
199-
rContext, bmp, /*label=*/"FpSampleChaining", skgpu::Mipmapped::kNo));
199+
auto view = std::get<0>(GrMakeCachedBitmapProxyView(rContext,
200+
GrMippedBitmap(bmp),
201+
/*label=*/"FpSampleChaining",
202+
skgpu::Mipmapped::kNo));
200203
auto fp = GrTextureEffect::Make(std::move(view), bmp.alphaType());
201204
#endif
202205
for (EffectType effectType : effects) {

gm/fpcoordinateoverride.cpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
#include "src/gpu/ganesh/effects/GrSkSLFP.h"
2929
#include "src/gpu/ganesh/effects/GrTextureEffect.h"
3030
#include "src/gpu/ganesh/glsl/GrGLSLFragmentShaderBuilder.h"
31+
#include "src/gpu/ganesh/image/GrMippedBitmap.h"
3132
#include "tools/DecodeUtils.h"
3233
#include "tools/Resources.h"
3334
#include "tools/ToolUtils.h"
@@ -84,8 +85,9 @@ DEF_SIMPLE_GPU_GM_BG(fpcoordinateoverride, rContext, canvas, 512, 512,
8485

8586
SkBitmap bmp;
8687
ToolUtils::GetResourceAsBitmap("images/mandrill_512_q075.jpg", &bmp);
88+
auto bitmap = GrMippedBitmap(bmp);
8789
auto view = std::get<0>(GrMakeCachedBitmapProxyView(
88-
rContext, bmp, /*label=*/"FpCoordinateOverride", skgpu::Mipmapped::kNo));
90+
rContext, bitmap, /*label=*/"FpCoordinateOverride", skgpu::Mipmapped::kNo));
8991
if (!view) {
9092
return;
9193
}

gm/showmiplevels.cpp

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,26 @@
2828

2929
#include <math.h>
3030

31-
class ShowMipLevels3 : public skiagm::GM {
31+
/**
32+
* This GM explicitly makes mip map levels solid colors so we can see which are being used.
33+
*
34+
* Rows 0 and 1 don't use mip maps, so the rocket ship gets smaller (and tiled) from
35+
* left to right. The only difference is the filter mode - Row 0 (nearest) looks blockier
36+
* than Row 1 (linear).
37+
*
38+
* Rows 2 and 3 use the nearest mip map mode, which means we expect
39+
* Everything other than the first two columns (which are closest to mip level 0 - original image)
40+
* will be solid colors (Red, Green, Blue) to show this is happening. The sublte difference in
41+
* filtering can still be observed in Column 1.
42+
*
43+
* Rows 4 and 5 use a linear mipmap mode, which means the we'll see a blend of red and spaceships
44+
* for columns 1 and 2, and non-pure colors for the remaining columns as the breakpoints don't
45+
* land directly on the mip level changes.
46+
*
47+
* If the image is being rendered and all rows show the spaceships from left to right, the
48+
* hand-crafted mips are being lost somewhere in rendering and that is *wrong*.
49+
*/
50+
class ShowMipLevels : public skiagm::GM {
3251
sk_sp<SkImage> fImg;
3352

3453
SkString getName() const override { return SkString("showmiplevels_explicit"); }
@@ -42,6 +61,9 @@ class ShowMipLevels3 : public skiagm::GM {
4261
const SkColor colors[] = { SK_ColorRED, SK_ColorGREEN, SK_ColorBLUE };
4362

4463
SkMipmapBuilder builder(fImg->imageInfo());
64+
// The number of levels is derived from the size of the image, so we intentionally
65+
// pick an image that's big enough to support 3+ levels.
66+
SkASSERT(builder.countLevels() >= (int)std::size(colors));
4567
for (int i = 0; i < builder.countLevels(); ++i) {
4668
auto surf = SkSurfaces::WrapPixels(builder.level(i));
4769
surf->getCanvas()->drawColor(colors[i % std::size(colors)]);
@@ -76,7 +98,5 @@ class ShowMipLevels3 : public skiagm::GM {
7698
}
7799
return r.height() + 10;
78100
}
79-
80-
using INHERITED = skiagm::GM;
81101
};
82-
DEF_GM( return new ShowMipLevels3; )
102+
DEF_GM(return new ShowMipLevels;)

gm/swizzle.cpp

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
#include "src/gpu/ganesh/SkGr.h"
1919
#include "src/gpu/ganesh/SurfaceFillContext.h"
2020
#include "src/gpu/ganesh/effects/GrTextureEffect.h"
21+
#include "src/gpu/ganesh/image/GrMippedBitmap.h"
2122
#include "tools/DecodeUtils.h"
2223
#include "tools/Resources.h"
2324

@@ -29,13 +30,16 @@ DEF_SIMPLE_GPU_GM(swizzle, rContext, canvas, 512, 512) {
2930

3031
SkBitmap bmp;
3132
ToolUtils::GetResourceAsBitmap("images/mandrill_512_q075.jpg", &bmp);
33+
auto bitmap = GrMippedBitmap::Make(bmp.pixmap());
34+
SkASSERT_RELEASE(bitmap);
35+
SkAlphaType alphaType = bitmap->alphaType();
3236
auto view = std::get<0>(GrMakeCachedBitmapProxyView(
33-
rContext, bmp, /*label=*/"Gm_Swizzle", skgpu::Mipmapped::kNo));
37+
rContext, bitmap.value(), /*label=*/"Gm_Swizzle", skgpu::Mipmapped::kNo));
3438
if (!view) {
3539
return;
3640
}
3741
std::unique_ptr<GrFragmentProcessor> imgFP =
38-
GrTextureEffect::Make(std::move(view), bmp.alphaType(), SkMatrix());
42+
GrTextureEffect::Make(std::move(view), alphaType, SkMatrix());
3943
auto fp = GrFragmentProcessor::SwizzleOutput(std::move(imgFP), skgpu::Swizzle("grb1"));
4044

4145
sfc->fillWithFP(std::move(fp));

gm/texelsubset.cpp

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
#include "src/gpu/ganesh/SkGr.h"
2727
#include "src/gpu/ganesh/SurfaceDrawContext.h"
2828
#include "src/gpu/ganesh/effects/GrTextureEffect.h"
29+
#include "src/gpu/ganesh/image/GrMippedBitmap.h"
2930
#include "tools/DecodeUtils.h"
3031
#include "tools/Resources.h"
3132
#include "tools/ganesh/TestOps.h"
@@ -101,8 +102,10 @@ class TexelSubset : public GpuGM {
101102
if (mipmapped == skgpu::Mipmapped::kYes && !rContext->priv().caps()->mipmapSupport()) {
102103
return DrawResult::kSkip;
103104
}
104-
auto view = std::get<0>(GrMakeCachedBitmapProxyView(
105-
rContext, fBitmap, /*label=*/"DrawResult_Draw_BitMap", mipmapped));
105+
auto view = std::get<0>(GrMakeCachedBitmapProxyView(rContext,
106+
GrMippedBitmap(fBitmap),
107+
/*label=*/"DrawResult_Draw_BitMap",
108+
mipmapped));
106109
if (!view) {
107110
*errorMsg = "Failed to create proxy.";
108111
return DrawResult::kFail;
@@ -135,8 +138,11 @@ class TexelSubset : public GpuGM {
135138
SkBitmap subsetBmp;
136139
fBitmap.extractSubset(&subsetBmp, texelSubset);
137140
subsetBmp.setImmutable();
138-
auto subsetView = std::get<0>(GrMakeCachedBitmapProxyView(
139-
rContext, subsetBmp, /*label=*/"DrawResult_Draw_SubsetBitMap", mipmapped));
141+
auto subsetView =
142+
std::get<0>(GrMakeCachedBitmapProxyView(rContext,
143+
GrMippedBitmap(subsetBmp),
144+
/*label=*/"DrawResult_Draw_SubsetBitMap",
145+
mipmapped));
140146

141147
SkRect localRect = SkRect::Make(fBitmap.bounds()).makeOutset(kDrawPad, kDrawPad);
142148

gn/gpu.gni

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -429,6 +429,8 @@ skia_ganesh_private = [
429429
"$_src/gpu/ganesh/gradients/GrGradientShader.h",
430430
"$_src/gpu/ganesh/image/GrImageUtils.cpp",
431431
"$_src/gpu/ganesh/image/GrImageUtils.h",
432+
"$_src/gpu/ganesh/image/GrMippedBitmap.cpp",
433+
"$_src/gpu/ganesh/image/GrMippedBitmap.h",
432434
"$_src/gpu/ganesh/image/GrTextureGenerator.cpp",
433435
"$_src/gpu/ganesh/image/SkImage_Ganesh.cpp",
434436
"$_src/gpu/ganesh/image/SkImage_Ganesh.h",

include/core/SkBitmap.h

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@
2727
class SkColorSpace;
2828
class SkImage;
2929
class SkMatrix;
30-
class SkMipmap;
3130
class SkPaint;
3231
class SkPixelRef;
3332
class SkShader;
@@ -1241,13 +1240,8 @@ class SK_API SkBitmap {
12411240
};
12421241

12431242
private:
1244-
sk_sp<SkPixelRef> fPixelRef;
1245-
SkPixmap fPixmap;
1246-
sk_sp<SkMipmap> fMips;
1247-
1248-
friend class SkImage_Raster;
1249-
friend class SkReadBuffer; // unflatten
1250-
friend class GrProxyProvider; // fMips
1243+
sk_sp<SkPixelRef> fPixelRef;
1244+
SkPixmap fPixmap;
12511245
};
12521246

12531247
///////////////////////////////////////////////////////////////////////////////

src/android/SkAnimatedImage.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -384,6 +384,7 @@ sk_sp<SkImage> SkAnimatedImage::getCurrentFrame() {
384384

385385
SkCanvas canvas(dst);
386386
this->draw(&canvas);
387+
dst.setImmutable();
387388
return SkImage_Raster::MakeFromBitmap(dst, SkCopyPixelsMode::kNever);
388389
}
389390

src/codec/SkWuffsCodec.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
#include "src/codec/SkSampler.h"
3434
#include "src/codec/SkScalingCodec.h"
3535
#include "src/core/SkDraw.h"
36+
#include "src/core/SkMipmap.h"
3637
#include "src/core/SkRasterClip.h"
3738
#include "src/core/SkStreamPriv.h"
3839

@@ -743,7 +744,7 @@ SkCodec::Result SkWuffsCodec::onIncrementalDecodeTwoPass() {
743744
draw.fRC = &rc;
744745

745746
SkMatrix translate = SkMatrix::Translate(dirty_rect.min_incl_x, dirty_rect.min_incl_y);
746-
draw.drawBitmap(src, translate, nullptr, SkSamplingOptions(), paint);
747+
draw.drawBitmap(src, translate, nullptr, SkSamplingOptions(), paint, nullptr);
747748
}
748749

749750
if (result == SkCodec::kSuccess) {

src/core/SkBitmap.cpp

Lines changed: 4 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -42,20 +42,13 @@ static bool reset_return_false(SkBitmap* bm) {
4242

4343
SkBitmap::SkBitmap() {}
4444

45-
SkBitmap::SkBitmap(const SkBitmap& src)
46-
: fPixelRef (src.fPixelRef)
47-
, fPixmap (src.fPixmap)
48-
, fMips (src.fMips)
49-
{
45+
SkBitmap::SkBitmap(const SkBitmap& src) : fPixelRef(src.fPixelRef), fPixmap(src.fPixmap) {
5046
SkDEBUGCODE(src.validate();)
5147
SkDEBUGCODE(this->validate();)
5248
}
5349

5450
SkBitmap::SkBitmap(SkBitmap&& other)
55-
: fPixelRef (std::move(other.fPixelRef))
56-
, fPixmap (std::move(other.fPixmap))
57-
, fMips (std::move(other.fMips))
58-
{
51+
: fPixelRef(std::move(other.fPixelRef)), fPixmap(std::move(other.fPixmap)) {
5952
SkASSERT(!other.fPixelRef);
6053
other.fPixmap.reset();
6154
}
@@ -65,8 +58,7 @@ SkBitmap::~SkBitmap() {}
6558
SkBitmap& SkBitmap::operator=(const SkBitmap& src) {
6659
if (this != &src) {
6760
fPixelRef = src.fPixelRef;
68-
fPixmap = src.fPixmap;
69-
fMips = src.fMips;
61+
fPixmap = src.fPixmap;
7062
}
7163
SkDEBUGCODE(this->validate();)
7264
return *this;
@@ -75,8 +67,7 @@ SkBitmap& SkBitmap::operator=(const SkBitmap& src) {
7567
SkBitmap& SkBitmap::operator=(SkBitmap&& other) {
7668
if (this != &other) {
7769
fPixelRef = std::move(other.fPixelRef);
78-
fPixmap = std::move(other.fPixmap);
79-
fMips = std::move(other.fMips);
70+
fPixmap = std::move(other.fPixmap);
8071
SkASSERT(!other.fPixelRef);
8172
other.fPixmap.reset();
8273
}
@@ -92,7 +83,6 @@ void SkBitmap::swap(SkBitmap& other) {
9283
void SkBitmap::reset() {
9384
fPixelRef = nullptr; // Free pixels.
9485
fPixmap.reset();
95-
fMips.reset();
9686
}
9787

9888
void SkBitmap::getBounds(SkRect* bounds) const {

0 commit comments

Comments
 (0)