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

Issue 1022673011: Creating a new wrapper for gif decoder (Closed)

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

Description

Patch Set 1 : Adding gif functionality to SkCodec #

Total comments: 90

Patch Set 2 : First round of gif improvements #

Total comments: 32

Patch Set 3 : Second set of fixes to gif codec #

Total comments: 4

Patch Set 4 : Ready to land after giflib lands #

Patch Set 5 : Merge with Hal's wbmp changes #

Patch Set 6 : Trybot fix - unitialized variable #

Patch Set 7 : giflib for Windows #

Patch Set 8 : Windows - added more defines #

Patch Set 9 : Trying cflags approach #

Patch Set 10 : Removing original define #

Patch Set 11 : Last attempt #

Patch Set 12 : Flags within giflib #

Patch Set 13 : More flags #

Patch Set 14 : Using a dummy file for windows #

Patch Set 15 : Dummy giflib include for windows #

Patch Set 16 : Dummy giflib include for windows 2 #

Patch Set 17 : Disable warnings on windows #

Patch Set 18 : Final changes #

Patch Set 19 : Last time #

Total comments: 3

Patch Set 20 : Warnings mostly disabled #

Total comments: 30

Patch Set 21 : Only include extra file on windows #

Patch Set 22 : Fixes from last round #

Total comments: 6

Patch Set 23 : Added comments and do while loop #

Unified diffs Side-by-side diffs Delta from patch set Stats (+730 lines, -9 lines) Patch
M dm/DM.cpp View 1 2 3 4 1 chunk +11 lines, -4 lines 0 comments Download
M gyp/codec.gyp View 1 2 3 4 7 8 2 chunks +3 lines, -0 lines 0 comments Download
M gyp/giflib.gyp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 2 chunks +16 lines, -3 lines 0 comments Download
M src/codec/SkCodec.cpp View 1 2 3 4 5 4 chunks +19 lines, -2 lines 0 comments Download
A src/codec/SkCodec_libgif.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +74 lines, -0 lines 0 comments Download
A src/codec/SkCodec_libgif.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +517 lines, -0 lines 0 comments Download
A src/codec/SkGifInterlaceIter.h View 1 1 chunk +35 lines, -0 lines 0 comments Download
A src/codec/SkGifInterlaceIter.cpp View 1 1 chunk +48 lines, -0 lines 0 comments Download
A third_party/giflib/unistd.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +7 lines, -0 lines 0 comments Download

Messages

Total messages: 64 (36 generated)
msarett
Enabling Gif Decoding for SkCodec Gif TODOs Not Addressed by this CL: ***There is a ...
5 years, 9 months ago (2015-03-25 15:30:46 UTC) #3
scroggo
On 2015/03/25 15:30:46, msarett wrote: > Enabling Gif Decoding for SkCodec > > Gif TODOs ...
5 years, 9 months ago (2015-03-25 19:44:50 UTC) #4
scroggo
https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgif.cpp File src/codec/SkCodec_libgif.cpp (right): https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgif.cpp#newcode470 src/codec/SkCodec_libgif.cpp:470: return kSuccess; This function is over 300 lines. Can ...
5 years, 9 months ago (2015-03-25 19:56:58 UTC) #5
msarett
First round of gif improvements Note that CL 1038863003 will include the gif library on ...
5 years, 9 months ago (2015-03-26 19:15:58 UTC) #7
scroggo
https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgif.cpp File src/codec/SkCodec_libgif.cpp (right): https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgif.cpp#newcode44 src/codec/SkCodec_libgif.cpp:44: int32_t SkGifCodec::close_gif(GifFileType* gif) { On 2015/03/26 19:15:57, msarett wrote: ...
5 years, 9 months ago (2015-03-26 20:03:54 UTC) #8
msarett
Second set of improvements to gif codec https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgif.cpp File src/codec/SkCodec_libgif.cpp (right): https://codereview.chromium.org/1022673011/diff/20001/src/codec/SkCodec_libgif.cpp#newcode120 src/codec/SkCodec_libgif.cpp:120: // that ...
5 years, 9 months ago (2015-03-26 22:26:37 UTC) #9
scroggo
LGTM. If you go ahead and land it with it reporting kIndex8, please follow up ...
5 years, 9 months ago (2015-03-27 13:09:36 UTC) #10
msarett
Just waiting to get the giflib dependencies sorted out. Will implement kIndex8 swizzles after this ...
5 years, 9 months ago (2015-03-27 14:23:20 UTC) #11
scroggo
lgtm https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec_libgif.cpp File src/codec/SkCodec_libgif.cpp (right): https://codereview.chromium.org/1022673011/diff/60001/src/codec/SkCodec_libgif.cpp#newcode314 src/codec/SkCodec_libgif.cpp:314: fillBackground = (kYes_ZeroInitialized != zeroInit); On 2015/03/27 14:23:19, ...
5 years, 9 months ago (2015-03-27 14:36:11 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1022673011/110001
5 years, 9 months ago (2015-03-27 19:28:54 UTC) #15
commit-bot: I haz the power
Try jobs failed on following builders: Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x86_64-Debug-Trybot/builds/190)
5 years, 9 months ago (2015-03-27 19:30:04 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1022673011/130001
5 years, 9 months ago (2015-03-27 19:33:28 UTC) #20
commit-bot: I haz the power
Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_64-Debug-Trybot/builds/189)
5 years, 9 months ago (2015-03-27 19:57:31 UTC) #22
msarett
We are failing to include giflib on Windows bots. "Cannot open include file: unistd.h from ...
5 years, 9 months ago (2015-03-27 20:15:20 UTC) #24
msarett
Waiting on the trybots to verify a potential fix. Will revisit on Monday.
5 years, 9 months ago (2015-03-27 20:47:31 UTC) #25
msarett
I still can't get giflib to compile on Windows. I feel like I have started ...
5 years, 8 months ago (2015-03-30 22:25:10 UTC) #34
scroggo
On 2015/03/30 22:25:10, msarett wrote: > I still can't get giflib to compile on Windows. ...
5 years, 8 months ago (2015-03-31 13:13:28 UTC) #35
msarett
I think I've finally solved the windows problem. I haven't been able to get rid ...
5 years, 8 months ago (2015-03-31 17:15:26 UTC) #52
djsollen
https://codereview.chromium.org/1022673011/diff/860001/gyp/giflib.gyp File gyp/giflib.gyp (right): https://codereview.chromium.org/1022673011/diff/860001/gyp/giflib.gyp#newcode20 gyp/giflib.gyp:20: '../third_party/giflib', only add this include directory on windows.
5 years, 8 months ago (2015-03-31 17:33:02 UTC) #53
msarett
Only include dummy file on windows https://codereview.chromium.org/1022673011/diff/860001/gyp/giflib.gyp File gyp/giflib.gyp (right): https://codereview.chromium.org/1022673011/diff/860001/gyp/giflib.gyp#newcode20 gyp/giflib.gyp:20: '../third_party/giflib', On 2015/03/31 ...
5 years, 8 months ago (2015-03-31 17:41:20 UTC) #54
djsollen
lgtm
5 years, 8 months ago (2015-03-31 18:02:57 UTC) #55
scroggo
https://codereview.chromium.org/1022673011/diff/840001/gyp/giflib.gyp File gyp/giflib.gyp (right): https://codereview.chromium.org/1022673011/diff/840001/gyp/giflib.gyp#newcode44 gyp/giflib.gyp:44: '/W3', I guess this did not work? https://codereview.chromium.org/1022673011/diff/860001/src/codec/SkCodec.cpp File ...
5 years, 8 months ago (2015-03-31 18:39:57 UTC) #56
msarett
For me, the remaining outstanding question is what to do about the size check in ...
5 years, 8 months ago (2015-03-31 20:11:41 UTC) #57
scroggo
https://codereview.chromium.org/1022673011/diff/840001/gyp/giflib.gyp File gyp/giflib.gyp (right): https://codereview.chromium.org/1022673011/diff/840001/gyp/giflib.gyp#newcode44 gyp/giflib.gyp:44: '/W3', On 2015/03/31 20:11:40, msarett wrote: > On 2015/03/31 ...
5 years, 8 months ago (2015-03-31 20:39:12 UTC) #58
msarett
Added comments about transIndex and a do-while loop https://codereview.chromium.org/1022673011/diff/710009/src/codec/SkCodec_libgif.cpp File src/codec/SkCodec_libgif.cpp (right): https://codereview.chromium.org/1022673011/diff/710009/src/codec/SkCodec_libgif.cpp#newcode129 src/codec/SkCodec_libgif.cpp:129: return ...
5 years, 8 months ago (2015-03-31 21:10:56 UTC) #59
scroggo
LGTM at patch set 23
5 years, 8 months ago (2015-04-01 13:42:04 UTC) #60
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1022673011/910001
5 years, 8 months ago (2015-04-01 13:48:20 UTC) #63
commit-bot: I haz the power
5 years, 8 months ago (2015-04-01 13:58:55 UTC) #64
Message was sent while issue was closed.
Committed patchset #23 (id:910001) as
https://skia.googlesource.com/skia/+/8c8f22a3bba18edaad41d6ac8451a77aa093a958

Powered by Google App Engine
This is Rietveld 408576698