SkWuffsCodec: Use drawing to do color correction and scaling
Bug: skia:8235 (Wuffs)
Bug: skia:8751 (Color correction)
Bug: skia:8762 (565)
This was brought up in https://skia-review.googlesource.com/c/skia/+/171645/1/src/codec/SkWuffsCodec.cpp#499
Remove the SkSwizzler and code that supports it (e.g.
SkWuffsSpySampler). In its place, use an SkDraw and draw from Wuffs'
working buffer into the output pixel memory. This trivially allows
support for 565 and f16, while removing the blend function that was
8888 specific. The draw operation also takes care of color correction
and allows arbitrary downscaling.
Add the intermediate subclass SkScalingCodec, to share methods between
SkWebpCodec and SkWuffsCodec.
Allowing arbitrary scaling from an SkCodec subclass is a bit of a break
from an original goal of SkCodec, which was to only do what the
underlying format supports naturally. Otherwise a client could just use
Skia to do effectively the same thing in a layer above SkCodec. (e.g.
jpegs can be downscaled by throwing away information during decode,
which libjpeg(-turbo) will do as requested. But gifs don't scale down
naturally in the same way, so SkGifCodec did not support downscaling
except when used with SkAndroidCodec.) But SkCodec has already deviated
from this goals in two ways. With SkAndroidCodec, sampling is done
inside SkGifCodec (and others) in order to save on memory allocation (we
can sample on a single line without allocating memory for the full size
image at its original size). (Note that this memory is not saved anyway
in SkWuffsCodec, because wuffs has its own image buffer.) The second
deviation is color correction, which could also be done in a layer
outside SkCodec. Since SkCodec is already doing this extra work,
simplify it.
Change-Id: If9a1698e0c353f60250aef5b6229855e6538a42d
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/194186
Commit-Queue: Leon Scroggins <scroggo@google.com>
Reviewed-by: Mike Reed <reed@google.com>
Reviewed-by: Mike Klein <mtklein@google.com>
Reviewed-by: Nigel Tao <nigeltao@google.com>
diff --git a/src/codec/SkWuffsCodec.cpp b/src/codec/SkWuffsCodec.cpp
index 2a8c984..ea5cb80 100644
--- a/src/codec/SkWuffsCodec.cpp
+++ b/src/codec/SkWuffsCodec.cpp
@@ -8,9 +8,14 @@
#include "SkWuffsCodec.h"
#include "../private/SkMalloc.h"
+#include "SkBitmap.h"
+#include "SkDraw.h"
#include "SkFrameHolder.h"
+#include "SkMatrix.h"
+#include "SkPaint.h"
+#include "SkRasterClip.h"
#include "SkSampler.h"
-#include "SkSwizzler.h"
+#include "SkScalingCodec.h"
#include "SkUtils.h"
// Wuffs ships as a "single file C library" or "header file library" as per
@@ -116,48 +121,7 @@
typedef SkFrameHolder INHERITED;
};
-// SkWuffsSpySampler is a placeholder SkSampler implementation. The Skia API
-// expects to manipulate the codec's sampler (i.e. call setSampleX and
-// setSampleY) in between the startIncrementalDecode (SID) and
-// incrementalDecode (ID) calls. But creating the SkSwizzler (the real sampler)
-// requires knowing the destination buffer's dimensions, i.e. the animation
-// frame's width and height. That width and height are decoded in ID, not SID.
-//
-// To break that circle, the SkWuffsSpySampler always exists, so its methods
-// can be called between SID and ID. It doesn't actually do any sampling, it
-// merely records the arguments given to setSampleX (explicitly) and setSampleY
-// (implicitly, via the superclass' implementation). Inside ID, those recorded
-// arguments are forwarded on to the SkSwizzler (the real sampler) when that
-// SkSwizzler is created, after the frame width and height are known.
-//
-// Roughly speaking, the SkWuffsSpySampler is an eager proxy for the lazily
-// constructed real sampler. But that laziness is out of necessity.
-//
-// The "Spy" name is because it records its arguments. See
-// https://martinfowler.com/articles/mocksArentStubs.html#TheDifferenceBetweenMocksAndStubs
-class SkWuffsSpySampler final : public SkSampler {
-public:
- SkWuffsSpySampler(int imageWidth)
- : INHERITED(), fFillWidth(0), fImageWidth(imageWidth), fSampleX(1) {}
-
- void reset();
- int sampleX() const;
-
- int fFillWidth;
-
-private:
- // SkSampler overrides.
- int fillWidth() const override;
- int onSetSampleX(int sampleX) override;
-
- const int fImageWidth;
-
- int fSampleX;
-
- typedef SkSampler INHERITED;
-};
-
-class SkWuffsCodec final : public SkCodec {
+class SkWuffsCodec final : public SkScalingCodec {
public:
SkWuffsCodec(SkEncodedInfo&& encodedInfo,
std::unique_ptr<SkStream> stream,
@@ -184,8 +148,6 @@
int onGetFrameCount() override;
bool onGetFrameInfo(int, FrameInfo*) const override;
int onGetRepetitionCount() override;
- SkSampler* getSampler(bool createIfNecessary) override;
- bool conversionSupported(const SkImageInfo& dst, bool, bool) override;
void readFrames();
Result seekFrame(int frameIndex);
@@ -195,7 +157,6 @@
const char* decodeFrame();
void updateNumFullyReceivedFrames();
- SkWuffsSpySampler fSpySampler;
SkWuffsFrameHolder fFrameHolder;
std::unique_ptr<SkStream> fStream;
std::unique_ptr<wuffs_gif__decoder, decltype(&sk_free)> fDecoder;
@@ -211,9 +172,7 @@
// Incremental decoding state.
uint8_t* fIncrDecDst;
size_t fIncrDecRowBytes;
-
- std::unique_ptr<SkSwizzler> fSwizzler;
- int fScaledHeight;
+ bool fFirstCallToIncrementalDecode;
uint64_t fNumFullyReceivedFrames;
std::vector<SkWuffsFrame> fFrames;
@@ -233,7 +192,7 @@
uint8_t fBuffer[SK_WUFFS_CODEC_BUFFER_SIZE];
- typedef SkCodec INHERITED;
+ typedef SkScalingCodec INHERITED;
};
// -------------------------------- SkWuffsFrame implementation
@@ -280,27 +239,6 @@
return fCodec->frame(i);
};
-// -------------------------------- SkWuffsSpySampler implementation
-
-void SkWuffsSpySampler::reset() {
- fFillWidth = 0;
- fSampleX = 1;
- this->setSampleY(1);
-}
-
-int SkWuffsSpySampler::sampleX() const {
- return fSampleX;
-}
-
-int SkWuffsSpySampler::fillWidth() const {
- return fFillWidth;
-}
-
-int SkWuffsSpySampler::onSetSampleX(int sampleX) {
- fSampleX = sampleX;
- return get_scaled_dimension(fImageWidth, sampleX);
-}
-
// -------------------------------- SkWuffsCodec implementation
SkWuffsCodec::SkWuffsCodec(SkEncodedInfo&& encodedInfo,
@@ -318,7 +256,6 @@
// manage the stream ourselves, as the default SkCodec behavior
// is too trigger-happy on rewinding the stream.
nullptr),
- fSpySampler(imgcfg.pixcfg.width()),
fFrameHolder(),
fStream(std::move(stream)),
fDecoder(std::move(dec)),
@@ -331,8 +268,7 @@
fIOBuffer(wuffs_base__null_io_buffer()),
fIncrDecDst(nullptr),
fIncrDecRowBytes(0),
- fSwizzler(nullptr),
- fScaledHeight(0),
+ fFirstCallToIncrementalDecode(false),
fNumFullyReceivedFrames(0),
fFramesComplete(false),
fDecoderIsSuspended(false) {
@@ -381,15 +317,14 @@
if (options.fSubset) {
return SkCodec::kUnimplemented;
}
+ if (options.fFrameIndex > 0 && SkColorTypeIsAlwaysOpaque(dstInfo.colorType())) {
+ return SkCodec::kInvalidConversion;
+ }
SkCodec::Result result = this->seekFrame(options.fFrameIndex);
if (result != SkCodec::kSuccess) {
return result;
}
- fSpySampler.reset();
- fSwizzler = nullptr;
- fScaledHeight = 0;
-
const char* status = this->decodeFrameConfig();
if (status == wuffs_base__suspension__short_read) {
return SkCodec::kIncompleteInput;
@@ -415,27 +350,12 @@
fIncrDecDst = static_cast<uint8_t*>(dst);
fIncrDecRowBytes = rowBytes;
+ fFirstCallToIncrementalDecode = true;
return SkCodec::kSuccess;
}
-static bool independent_frame(SkCodec* codec, int frameIndex) {
- if (frameIndex == 0) {
- return true;
- }
-
- SkCodec::FrameInfo frameInfo;
- SkAssertResult(codec->getFrameInfo(frameIndex, &frameInfo));
- return frameInfo.fRequiredFrame == SkCodec::kNoFrame;
-}
-
-static void blend(uint32_t* dst, const uint32_t* src, int width) {
- while (width --> 0) {
- if (*src != 0) {
- *dst = *src;
- }
- src++;
- dst++;
- }
+static SkAlphaType to_alpha_type(bool opaque) {
+ return opaque ? kOpaque_SkAlphaType : kPremul_SkAlphaType;
}
SkCodec::Result SkWuffsCodec::onIncrementalDecode(int* rowsDecoded) {
@@ -445,7 +365,17 @@
SkCodec::Result result = SkCodec::kSuccess;
const char* status = this->decodeFrame();
- const bool independent = independent_frame(this, options().fFrameIndex);
+ bool independent;
+ SkAlphaType alphaType;
+ const int index = options().fFrameIndex;
+ if (index == 0) {
+ independent = true;
+ alphaType = to_alpha_type(getEncodedInfo().opaque());
+ } else {
+ const SkWuffsFrame* f = this->frame(index);
+ independent = f->getRequiredFrame() == SkCodec::kNoFrame;
+ alphaType = to_alpha_type(f->reportedAlpha() == SkEncodedInfo::kOpaque_Alpha);
+ }
if (status != nullptr) {
if (status == wuffs_base__suspension__short_read) {
result = SkCodec::kIncompleteInput;
@@ -469,110 +399,81 @@
size_t src_bytes_per_pixel = src_bits_per_pixel / 8;
wuffs_base__rect_ie_u32 frame_rect = fFrameConfig.bounds();
- if (!fSwizzler) {
- auto bounds = SkIRect::MakeLTRB(frame_rect.min_incl_x, frame_rect.min_incl_y,
- frame_rect.max_excl_x, frame_rect.max_excl_y);
- fSwizzler =
- SkSwizzler::Make(this->getEncodedInfo(), nullptr, dstInfo(), this->options(), &bounds);
- fSwizzler->setSampleX(fSpySampler.sampleX());
- fSwizzler->setSampleY(fSpySampler.sampleY());
- fScaledHeight = get_scaled_dimension(dstInfo().height(), fSpySampler.sampleY());
-
+ if (fFirstCallToIncrementalDecode) {
if (frame_rect.width() > (SIZE_MAX / src_bytes_per_pixel)) {
return SkCodec::kInternalError;
}
+ auto bounds = SkIRect::MakeLTRB(frame_rect.min_incl_x, frame_rect.min_incl_y,
+ frame_rect.max_excl_x, frame_rect.max_excl_y);
+
// If the frame rect does not fill the output, ensure that those pixels are not
// left uninitialized.
if (independent && (bounds != this->bounds() || result != kSuccess)) {
- auto fillInfo = dstInfo().makeWH(fSwizzler->fillWidth(), fScaledHeight);
- SkSampler::Fill(fillInfo, fIncrDecDst, fIncrDecRowBytes, options().fZeroInitialized);
+ SkSampler::Fill(dstInfo(), fIncrDecDst, fIncrDecRowBytes,
+ options().fZeroInitialized);
}
- }
- if (fScaledHeight == 0) {
- return SkCodec::kInternalError;
+ fFirstCallToIncrementalDecode = false;
+ } else {
+ // Existing clients intend to only show frames beyond the first if they
+ // are complete (based on FrameInfo::fFullyReceived), since it might
+ // look jarring to draw a partial frame over an existing frame. If they
+ // changed their behavior and expected to continue decoding a partial
+ // frame after the first one, we'll need to update our blending code.
+ // Otherwise, if the frame were interlaced and not independent, the
+ // second pass may have an overlapping dirty_rect with the first,
+ // resulting in blending with the first pass.
+ SkASSERT(index == 0);
}
- // The semantics of *rowsDecoded is: say you have a 10 pixel high image
- // (both the frame and the image). If you only decoded the first 3 rows,
- // set this to 3, and then SkCodec (or the caller of incrementalDecode)
- // would zero-initialize the remaining 7 (unless the memory was already
- // zero-initialized).
- //
- // Now let's say that the image is still 10 pixels high, but the frame is
- // from row 5 to 9. If you only decoded 3 rows, but you initialized the
- // first 5, you could return 8, and the caller would zero-initialize the
- // final 2. For GIF (where a frame can be smaller than the image and can be
- // interlaced), we just zero-initialize all 10 rows ahead of time and
- // return the height of the image, so the caller knows it doesn't need to
- // do anything.
- //
- // Similarly, if the output is scaled, we zero-initialized all
- // |fScaledHeight| rows (the scaled image height), so we inform the caller
- // that it doesn't need to do anything.
if (rowsDecoded) {
- *rowsDecoded = fScaledHeight;
+ *rowsDecoded = dstInfo().height();
}
// If the frame's dirty rect is empty, no need to swizzle.
wuffs_base__rect_ie_u32 dirty_rect = fDecoder->frame_dirty_rect();
if (!dirty_rect.is_empty()) {
- std::unique_ptr<uint8_t[]> tmpBuffer;
- if (!independent) {
- tmpBuffer.reset(new uint8_t[dstInfo().minRowBytes()]);
- }
wuffs_base__table_u8 pixels = fPixelBuffer.plane(0);
- const int sampleY = fSwizzler->sampleY();
- for (uint32_t y = dirty_rect.min_incl_y; y < dirty_rect.max_excl_y; y++) {
- int dstY = y;
- if (sampleY != 1) {
- if (!fSwizzler->rowNeeded(y)) {
- continue;
- }
- dstY /= sampleY;
- if (dstY >= fScaledHeight) {
- break;
- }
- }
- // We don't adjust d by (frame_rect.min_incl_x * dst_bpp) as we
- // have already accounted for that in swizzleRect, above.
- uint8_t* d = fIncrDecDst + (dstY * fIncrDecRowBytes);
+ // The Wuffs model is that the dst buffer is the image, not the frame.
+ // The expectation is that you allocate the buffer once, but re-use it
+ // for the N frames, regardless of each frame's top-left co-ordinate.
+ //
+ // To get from the start (in the X-direction) of the image to the start
+ // of the dirty_rect, we adjust s by (dirty_rect.min_incl_x * src_bytes_per_pixel).
+ uint8_t* s = pixels.ptr + (dirty_rect.min_incl_y * pixels.stride)
+ + (dirty_rect.min_incl_x * src_bytes_per_pixel);
- // The Wuffs model is that the dst buffer is the image, not the frame.
- // The expectation is that you allocate the buffer once, but re-use it
- // for the N frames, regardless of each frame's top-left co-ordinate.
- //
- // To get from the start (in the X-direction) of the image to the start
- // of the frame, we adjust s by (frame_rect.min_incl_x *
- // src_bytes_per_pixel).
- //
- // We adjust (in the X-direction) by the frame rect, not the dirty
- // rect, because the swizzler (which operates on rows) was
- // configured with the frame rect's X range.
- uint8_t* s =
- pixels.ptr + (y * pixels.stride) + (frame_rect.min_incl_x * src_bytes_per_pixel);
- if (independent) {
- fSwizzler->swizzle(d, s);
- } else {
- SkASSERT(tmpBuffer.get());
- fSwizzler->swizzle(tmpBuffer.get(), s);
- d = SkTAddOffset<uint8_t>(d, fSwizzler->swizzleOffsetBytes());
- const auto* swizzled = SkTAddOffset<uint32_t>(tmpBuffer.get(),
- fSwizzler->swizzleOffsetBytes());
- blend(reinterpret_cast<uint32_t*>(d), swizzled, fSwizzler->swizzleWidth());
- }
+ // Currently, this is only used for GIF, which will never have an ICC profile. When it is
+ // used for other formats that might have one, we will need to transform from profiles that
+ // do not have corresponding SkColorSpaces.
+ SkASSERT(!getEncodedInfo().profile());
+
+ auto srcInfo = getInfo().makeWH(dirty_rect.width(), dirty_rect.height())
+ .makeAlphaType(alphaType);
+ SkBitmap src;
+ src.installPixels(srcInfo, s, pixels.stride);
+ SkPaint paint;
+ if (independent) {
+ paint.setBlendMode(SkBlendMode::kSrc);
}
+
+ SkDraw draw;
+ draw.fDst.reset(dstInfo(), fIncrDecDst, fIncrDecRowBytes);
+ SkMatrix matrix = SkMatrix::MakeRectToRect(SkRect::Make(this->dimensions()),
+ SkRect::Make(this->dstInfo().dimensions()),
+ SkMatrix::kFill_ScaleToFit);
+ draw.fMatrix = &matrix;
+ SkRasterClip rc(SkIRect::MakeSize(this->dstInfo().dimensions()));
+ draw.fRC = &rc;
+
+ SkMatrix translate = SkMatrix::MakeTrans(dirty_rect.min_incl_x, dirty_rect.min_incl_y);
+ draw.drawBitmap(src, translate, nullptr, paint);
}
if (result == SkCodec::kSuccess) {
- fSpySampler.reset();
fIncrDecDst = nullptr;
fIncrDecRowBytes = 0;
- fSwizzler = nullptr;
- } else {
- // Make fSpySampler return whatever fSwizzler would have for fillWidth.
- fSpySampler.fFillWidth = fSwizzler->fillWidth();
}
return result;
}
@@ -609,32 +510,6 @@
return n < INT_MAX ? n : INT_MAX;
}
-SkSampler* SkWuffsCodec::getSampler(bool createIfNecessary) {
- // fIncrDst being non-nullptr means that we are between an
- // onStartIncrementalDecode call and the matching final (successful)
- // onIncrementalDecode call.
- if (createIfNecessary || fIncrDecDst) {
- return &fSpySampler;
- }
- return nullptr;
-}
-
-bool SkWuffsCodec::conversionSupported(const SkImageInfo& dst, bool srcIsOpaque, bool needsColorXform) {
- if (!this->INHERITED::conversionSupported(dst, srcIsOpaque, needsColorXform)) {
- return false;
- }
-
- switch (dst.colorType()) {
- case kRGBA_8888_SkColorType:
- case kBGRA_8888_SkColorType:
- return true;
- default:
- // FIXME: Add skcms to support F16
- // FIXME: Add support for 565 on the first frame
- return false;
- }
-}
-
void SkWuffsCodec::readFrames() {
size_t n = fFrames.size();
int i = n ? n - 1 : 0;