From 1793e7bb46c1f9d430c1a699a1c3d3168159b659 Mon Sep 17 00:00:00 2001 From: Leon Scroggins Date: Tue, 5 Dec 2017 15:38:14 +0000 Subject: Revert "Hide SkEncodedInfo" This reverts commit c6f7a4ffa9522159efc42f7c948bba5e66bb8844. Reason for revert: Causing differences in Gold, stemming from the fact that this changes the recommended SkImageInfo for 16 bits-per-component PNG from N32 to F16. - an F16 bitmap already png-encodes to a 16 bits-per-component PNG, but it does not encode a linear colorspace (possibly a bug?). when we decode this PNG using getInfo(), it fails because it has an F16 color type and non-linear colorspace. (In the encode-srgb-png gm, this results in blank results for F16.) We could correct this on the encoder side, but it seems possible that a 16 bits-per-component PNG could be encoded with a different color space. In that case, we'd want SkCodec to recommend F16/SRGBLinear, but I think we'd want the SkCodec to store the encoded SkColorSpace so that we can Xform between the two. Currently SkCodec only stores one color space, so that will require a refactor. - When decoding 16-bits-per-component PNGs, we are now decoding them to F16. This shows differences in Gold. The srgb/gpu results now look more like F16. I think this is fine. Original change's description: > Hide SkEncodedInfo > > Bug: skia:7353 > Bug: skia:6839 > > This contains information that is not necessary for clients to know. The > Color enum tells the number of components in the input, but this is only > interesting internally (to the SkSwizzler). > > Similarly, the Alpha enum differs from SkAlphaType in that it has > kBinary instead of kPremul. This is useful information only internally > for determining whether the SkColorSpaceXform needs to premultiply. > > The bitsPerComponent is potentially useful for a client; Android (in > SkAndroidCodec) uses it to determine the SkColorType. Rather than > exposing bitsPerComponent, use it to make the same decision that Android > would have made - 16 bits per component means to set the info to F16. Add > a test that computeOutputColorType behaves as expected. > > Switch conversionSupported to use an SkColorType, which is enough info. > > Replace the SkEncodedInfo::Alpha field on SkCodec::FrameInfo with an > SkAlphaType. > > SkCodec still needs an SkEncodedInfo, so move its header (which is > already not SK_API) to include/private. > > Change-Id: Ie2cf11339bf999ebfd4390c0f448f7edd6feabda > Reviewed-on: https://skia-review.googlesource.com/79260 > Reviewed-by: Mike Reed > Reviewed-by: Mike Klein > Commit-Queue: Leon Scroggins TBR=mtklein@chromium.org,scroggo@google.com,reed@google.com Change-Id: I0c5dd1461e1b70d1e55349a8e7ee6b029c3f556e No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: skia:7353, skia:6839 Reviewed-on: https://skia-review.googlesource.com/80660 Reviewed-by: Leon Scroggins Commit-Queue: Leon Scroggins --- tests/CodecAnimTest.cpp | 36 +++++++++++++++++++++--------------- 1 file changed, 21 insertions(+), 15 deletions(-) (limited to 'tests/CodecAnimTest.cpp') diff --git a/tests/CodecAnimTest.cpp b/tests/CodecAnimTest.cpp index b274ee538a..e354ea6cbf 100644 --- a/tests/CodecAnimTest.cpp +++ b/tests/CodecAnimTest.cpp @@ -67,8 +67,9 @@ static bool restore_previous(const SkCodec::FrameInfo& info) { } DEF_TEST(Codec_frames, r) { - #define kOpaque kOpaque_SkAlphaType - #define kUnpremul kUnpremul_SkAlphaType + #define kOpaque SkEncodedInfo::kOpaque_Alpha + #define kUnpremul SkEncodedInfo::kUnpremul_Alpha + #define kBinary SkEncodedInfo::kBinary_Alpha #define kKeep SkCodecAnimation::DisposalMethod::kKeep #define kRestoreBG SkCodecAnimation::DisposalMethod::kRestoreBGColor #define kRestorePrev SkCodecAnimation::DisposalMethod::kRestorePrevious @@ -78,8 +79,8 @@ DEF_TEST(Codec_frames, r) { // One less than fFramecount, since the first frame is always // independent. std::vector fRequiredFrames; - // Same, since the first frame should match getInfo - std::vector fAlphas; + // Same, since the first frame should match getEncodedInfo + std::vector fAlphas; // The size of this one should match fFrameCount for animated, empty // otherwise. std::vector fDurations; @@ -88,15 +89,15 @@ DEF_TEST(Codec_frames, r) { } gRecs[] = { { "required.gif", 7, { 0, 1, 2, 3, 4, 5 }, - { kOpaque, kUnpremul, kUnpremul, kUnpremul, kUnpremul, kUnpremul }, + { kOpaque, kBinary, kBinary, kBinary, kBinary, kBinary }, { 100, 100, 100, 100, 100, 100, 100 }, 0, { kKeep, kRestoreBG, kKeep, kKeep, kKeep, kRestoreBG, kKeep } }, { "alphabetAnim.gif", 13, { SkCodec::kNone, 0, 0, 0, 0, 5, 6, SkCodec::kNone, SkCodec::kNone, 9, 10, 11 }, - { kUnpremul, kUnpremul, kUnpremul, kUnpremul, kUnpremul, kUnpremul, - kUnpremul, kUnpremul, kUnpremul, kUnpremul, kUnpremul, kUnpremul }, + { kBinary, kBinary, kBinary, kBinary, kBinary, kBinary, + kBinary, kBinary, kBinary, kBinary, kBinary, kBinary }, { 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100, 100 }, 0, { kKeep, kRestorePrev, kRestorePrev, kRestorePrev, kRestorePrev, @@ -115,8 +116,8 @@ DEF_TEST(Codec_frames, r) { { "randPixelsAnim.gif", 13, // required frames { 0, 1, 2, 3, 4, 3, 6, 7, 7, 7, 9, 9 }, - { kUnpremul, kUnpremul, kUnpremul, kUnpremul, kUnpremul, kUnpremul, - kUnpremul, kUnpremul, kUnpremul, kUnpremul, kUnpremul, kUnpremul }, + { kBinary, kBinary, kBinary, kBinary, kBinary, kBinary, + kBinary, kBinary, kBinary, kBinary, kBinary, kBinary }, // durations { 0, 1000, 170, 40, 220, 7770, 90, 90, 90, 90, 90, 90, 90 }, // repetition count @@ -160,6 +161,7 @@ DEF_TEST(Codec_frames, r) { }; #undef kOpaque #undef kUnpremul + #undef kBinary #undef kKeep #undef kRestorePrev #undef kRestoreBG @@ -268,20 +270,22 @@ DEF_TEST(Codec_frames, r) { rec.fName, i, rec.fDurations[i], frameInfo.fDuration); } - auto to_string = [](SkAlphaType alpha) { + auto to_string = [](SkEncodedInfo::Alpha alpha) { switch (alpha) { - case kUnpremul_SkAlphaType: + case SkEncodedInfo::kUnpremul_Alpha: return "unpremul"; - case kOpaque_SkAlphaType: + case SkEncodedInfo::kOpaque_Alpha: return "opaque"; + case SkEncodedInfo::kBinary_Alpha: + return "binary"; default: SkASSERT(false); return "unknown"; } }; - auto expectedAlpha = 0 == i ? codec->getInfo().alphaType() : rec.fAlphas[i-1]; - auto alpha = frameInfo.fAlphaType; + auto expectedAlpha = 0 == i ? codec->getEncodedInfo().alpha() : rec.fAlphas[i-1]; + auto alpha = frameInfo.fAlpha; if (expectedAlpha != alpha) { ERRORF(r, "%s's frame %i has wrong alpha type! expected: %s\tactual: %s", rec.fName, i, to_string(expectedAlpha), to_string(alpha)); @@ -315,7 +319,9 @@ DEF_TEST(Codec_frames, r) { auto decode = [&](SkBitmap* bm, int index, int cachedIndex) { auto decodeInfo = info; if (index > 0) { - decodeInfo = info.makeAlphaType(frameInfos[index].fAlphaType); + auto alphaType = frameInfos[index].fAlpha == SkEncodedInfo::kOpaque_Alpha + ? kOpaque_SkAlphaType : kPremul_SkAlphaType; + decodeInfo = info.makeAlphaType(alphaType); } bm->allocPixels(decodeInfo); if (cachedIndex != SkCodec::kNone) { -- cgit v1.2.3