Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(29)

Issue 1733863003: Fix bug in SkGifCodec / Switch SkImageDec tests to use Codec (Closed)

Created:
4 years, 10 months ago by msarett
Modified:
4 years, 9 months ago
Reviewers:
scroggo
CC:
reviews_skia.org
Base URL:
https://skia.googlesource.com/skia.git@master
Target Ref:
refs/heads/master
Project:
skia
Visibility:
Public.

Description

Fix bug in SkGifCodec / Switch SkImageDec tests to use Codec SkImageDecoder is still used throughout tests, tools, gms etc. Deleting it from tests is an easy first step. Bonus is that we add tests of SkCodec. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1733863003 Committed: https://skia.googlesource.com/skia/+/7f7ec206de39fde8dc490e9feb0f65322af1b989

Patch Set 1 #

Total comments: 8

Patch Set 2 : Use SkCodec instead of SkImageDecoder #

Total comments: 4

Patch Set 3 : Remove unnecessary includes #

Patch Set 4 : Gif bug fix! #

Unified diffs Side-by-side diffs Delta from patch set Stats (+144 lines, -252 lines) Patch
M src/codec/SkGifCodec.cpp View 1 2 3 1 chunk +22 lines, -18 lines 0 comments Download
D tests/BadIcoTest.cpp View 1 2 chunks +13 lines, -6 lines 0 comments Download
M tests/CachedDecodingPixelRefTest.cpp View 1 chunk +2 lines, -1 line 0 comments Download
A tests/CodecPriv.h View 1 1 chunk +36 lines, -0 lines 0 comments Download
M tests/CodexTest.cpp View 3 chunks +5 lines, -10 lines 0 comments Download
M tests/FrontBufferedStreamTest.cpp View 2 chunks +5 lines, -7 lines 0 comments Download
D tests/GifTest.cpp View 1 8 chunks +55 lines, -50 lines 0 comments Download
D tests/IndexedPngOverflowTest.cpp View 1 2 2 chunks +2 lines, -6 lines 0 comments Download
D tests/InvalidIndexedPngTest.cpp View 1 2 2 chunks +2 lines, -4 lines 0 comments Download
D tests/KtxTest.cpp View 1 chunk +0 lines, -144 lines 0 comments Download
M tests/PathOpsSkpClipTest.cpp View 3 chunks +1 line, -4 lines 0 comments Download
M tests/SkpSkGrTest.cpp View 2 chunks +1 line, -2 lines 0 comments Download

Messages

Total messages: 23 (11 generated)
msarett
4 years, 10 months ago (2016-02-25 00:03:36 UTC) #3
scroggo
You have to delete KtxTest, but for the others, I think it makes sense to ...
4 years, 10 months ago (2016-02-25 12:43:29 UTC) #4
msarett
> You have to delete KtxTest, but for the others, I think it makes sense ...
4 years, 9 months ago (2016-03-01 00:21:11 UTC) #6
scroggo
lgtm https://codereview.chromium.org/1733863003/diff/40001/tests/GifTest.cpp File tests/GifTest.cpp (right): https://codereview.chromium.org/1733863003/diff/40001/tests/GifTest.cpp#newcode212 tests/GifTest.cpp:212: if (kIndex_8_SkColorType == codec->getInfo().colorType()) { Fine to have ...
4 years, 9 months ago (2016-03-01 14:51:54 UTC) #7
msarett
https://codereview.chromium.org/1733863003/diff/40001/tests/GifTest.cpp File tests/GifTest.cpp (right): https://codereview.chromium.org/1733863003/diff/40001/tests/GifTest.cpp#newcode212 tests/GifTest.cpp:212: if (kIndex_8_SkColorType == codec->getInfo().colorType()) { On 2016/03/01 14:51:54, scroggo ...
4 years, 9 months ago (2016-03-01 16:24:28 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1733863003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1733863003/60001
4 years, 9 months ago (2016-03-01 16:25:00 UTC) #11
commit-bot: I haz the power
Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot/builds/6638)
4 years, 9 months ago (2016-03-01 16:42:02 UTC) #13
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1733863003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1733863003/80001
4 years, 9 months ago (2016-03-01 18:48:26 UTC) #15
msarett
PTAL Glad you suggested that we keep these tests! The Release trybot caught an uninitialized ...
4 years, 9 months ago (2016-03-01 18:50:06 UTC) #16
scroggo
lgtm
4 years, 9 months ago (2016-03-01 18:55:01 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1733863003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1733863003/80001
4 years, 9 months ago (2016-03-01 20:10:06 UTC) #21
commit-bot: I haz the power
4 years, 9 months ago (2016-03-01 20:12:30 UTC) #23
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as
https://skia.googlesource.com/skia/+/7f7ec206de39fde8dc490e9feb0f65322af1b989

Powered by Google App Engine
This is Rietveld 408576698