Skip to content

Commit

Permalink
missed a place to clamp
Browse files Browse the repository at this point in the history
We have a fast path where we can use a memset for constant colors.
That wasn't being gamut-clamped until now.

Some refactoring to allow append_color_pipeline() to be called
without a this pointer.

Change-Id: I8a10e537d579235e80633a5e480f1e38c7932014
Reviewed-on: https://skia-review.googlesource.com/152583
Commit-Queue: Mike Klein <mtklein@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: Mike Klein <mtklein@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
  • Loading branch information
Mike Klein authored and Skia Commit-Bot committed Sep 7, 2018
1 parent 41b6773 commit c471151
Showing 1 changed file with 22 additions and 21 deletions.
43 changes: 22 additions & 21 deletions src/core/SkRasterPipelineBlitter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ class SkRasterPipelineBlitter final : public SkBlitter {
void blitV (int x, int y, int height, SkAlpha alpha) override;

private:
void append_color_pipeline(SkRasterPipeline*) const;
void append_load_dst (SkRasterPipeline*) const;
void append_store (SkRasterPipeline*) const;

Expand Down Expand Up @@ -87,6 +86,23 @@ class SkRasterPipelineBlitter final : public SkBlitter {
typedef SkBlitter INHERITED;
};

static void append_color_pipeline(SkRasterPipeline* p,
const SkRasterPipeline& colorPipeline,
SkImageInfo dstInfo) {
p->extend(colorPipeline);

// TODO: can we refine this condition further to avoid clamps when we're known in-gamut?
// When opaque we could _probably_ get away without a clamp, but for consistency we keep it.
if (dstInfo.colorType() != kRGBA_F16_SkColorType &&
dstInfo.colorType() != kRGBA_F32_SkColorType &&
dstInfo.alphaType() == kPremul_SkAlphaType)
{
// TODO: this will be common enough that we may want to fuse into ::clamp_premul.
p->append(SkRasterPipeline::clamp_0);
p->append(SkRasterPipeline::clamp_a);
}
}

SkBlitter* SkCreateRasterPipelineBlitter(const SkPixmap& dst,
const SkPaint& paint,
const SkMatrix& ctm,
Expand Down Expand Up @@ -224,7 +240,7 @@ SkBlitter* SkRasterPipelineBlitter::Create(const SkPixmap& dst,
// Run our color pipeline all the way through to produce what we'd memset when we can.
// Not all blits can memset, so we need to keep colorPipeline too.
SkRasterPipeline_<256> p;
p.extend(*colorPipeline);
append_color_pipeline(&p, *colorPipeline, dst.info());
blitter->fDstPtr = SkJumper_MemoryCtx{&blitter->fMemsetColor, 0};
blitter->append_store(&p);
p.run(0,0,1,1);
Expand All @@ -240,21 +256,6 @@ SkBlitter* SkRasterPipelineBlitter::Create(const SkPixmap& dst,
return blitter;
}

void SkRasterPipelineBlitter::append_color_pipeline(SkRasterPipeline* p) const {
p->extend(fColorPipeline);

// TODO: can we refine this condition further to avoid clamps when we're known in-gamut?
// When opaque we could _probably_ get away without a clamp, but for consistency we keep it.
if (fDst.info().colorType() != kRGBA_F16_SkColorType &&
fDst.info().colorType() != kRGBA_F32_SkColorType &&
fDst.info().alphaType() == kPremul_SkAlphaType)
{
// TODO: this will be common enough that we may want to fuse into ::clamp_premul.
p->append(SkRasterPipeline::clamp_0);
p->append(SkRasterPipeline::clamp_a);
}
}

void SkRasterPipelineBlitter::append_load_dst(SkRasterPipeline* p) const {
const void* ctx = &fDstPtr;
switch (fDst.info().colorType()) {
Expand Down Expand Up @@ -340,7 +341,7 @@ void SkRasterPipelineBlitter::blitRect(int x, int y, int w, int h) {

if (!fBlitRect) {
SkRasterPipeline p(fAlloc);
this->append_color_pipeline(&p);
append_color_pipeline(&p, fColorPipeline, fDst.info());
if (fBlend == SkBlendMode::kSrcOver
&& (fDst.info().colorType() == kRGBA_8888_SkColorType ||
fDst.info().colorType() == kBGRA_8888_SkColorType)
Expand Down Expand Up @@ -376,7 +377,7 @@ void SkRasterPipelineBlitter::blitRect(int x, int y, int w, int h) {
void SkRasterPipelineBlitter::blitAntiH(int x, int y, const SkAlpha aa[], const int16_t runs[]) {
if (!fBlitAntiH) {
SkRasterPipeline p(fAlloc);
this->append_color_pipeline(&p);
append_color_pipeline(&p, fColorPipeline, fDst.info());
if (SkBlendMode_ShouldPreScaleCoverage(fBlend, /*rgb_coverage=*/false)) {
p.append(SkRasterPipeline::scale_1_float, &fCurrentCoverage);
this->append_load_dst(&p);
Expand Down Expand Up @@ -460,7 +461,7 @@ void SkRasterPipelineBlitter::blitMask(const SkMask& mask, const SkIRect& clip)
// Lazily build whichever pipeline we need, specialized for each mask format.
if (effectiveMaskFormat == SkMask::kA8_Format && !fBlitMaskA8) {
SkRasterPipeline p(fAlloc);
this->append_color_pipeline(&p);
append_color_pipeline(&p, fColorPipeline, fDst.info());
if (SkBlendMode_ShouldPreScaleCoverage(fBlend, /*rgb_coverage=*/false)) {
p.append(SkRasterPipeline::scale_u8, &fMaskPtr);
this->append_load_dst(&p);
Expand All @@ -475,7 +476,7 @@ void SkRasterPipelineBlitter::blitMask(const SkMask& mask, const SkIRect& clip)
}
if (effectiveMaskFormat == SkMask::kLCD16_Format && !fBlitMaskLCD16) {
SkRasterPipeline p(fAlloc);
this->append_color_pipeline(&p);
append_color_pipeline(&p, fColorPipeline, fDst.info());
if (SkBlendMode_ShouldPreScaleCoverage(fBlend, /*rgb_coverage=*/true)) {
// Somewhat unusually, scale_565 needs dst loaded first.
this->append_load_dst(&p);
Expand Down

0 comments on commit c471151

Please sign in to comment.