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

Issue 23646005: Improve GIF decoding performance (Closed)

Created:
7 years, 3 months ago by Alpha Left Google
Modified:
7 years, 2 months ago
CC:
blink-reviews, eae+blinkwatch, dglazkov+blink, jeez
Visibility:
Public.

Description

Improve GIF decoding performance This change improves LZW decoding performance for GIF decoding. BACKGROUND LZW decoding uses a dictionary to store LZW codes. The data structure stores the last character for each code and the index of last code. This allows back tracing to construct the entire code. Current implementation builds the code in reverse order in a temporary buffer and then copies it into the row buffer. ALGORITHM This change eliminates the temporary buffer and saves a memory copy. If the code length is known in advance then reconstruction can write directly to the row buffer. The index output buffer is advanced by the code length, and then reconstruct as usual. The code length is saved such that it can be used next time. Testing is done locally with a corpus of 70k images. There's no change in output and roughly result in a 20% performance gain. BUG=233076 R=pkasting@chromium.org Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=158842

Patch Set 1 #

Total comments: 48

Patch Set 2 : fixed comments #

Total comments: 13

Patch Set 3 : fixed bug #

Patch Set 4 : bad code initial dictionary #

Total comments: 10

Patch Set 5 : test with bad code #

Total comments: 2

Patch Set 6 : comments #

Patch Set 7 : comments #

Patch Set 8 : test data moved to Source/core/platform/image-decoders/testing #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+128 lines, -96 lines) Patch
M Source/core/platform/image-decoders/gif/GIFImageDecoder.h View 1 2 3 4 5 6 7 2 chunks +3 lines, -1 line 0 comments Download
M Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp View 1 2 3 4 5 6 7 3 chunks +3 lines, -5 lines 0 comments Download
M Source/core/platform/image-decoders/gif/GIFImageDecoderTest.cpp View 1 2 3 4 5 6 7 1 chunk +27 lines, -0 lines 2 comments Download
M Source/core/platform/image-decoders/gif/GIFImageReader.h View 1 2 3 4 5 6 7 3 chunks +10 lines, -13 lines 0 comments Download
M Source/core/platform/image-decoders/gif/GIFImageReader.cpp View 1 2 3 4 5 6 7 10 chunks +85 lines, -77 lines 0 comments Download
A + Source/core/platform/image-decoders/testing/bad-code.gif View 1 2 3 4 5 6 7 Binary file 0 comments Download
A Source/core/platform/image-decoders/testing/bad-initial-code.gif View 1 2 3 4 5 6 7 Binary file 0 comments Download

Messages

Total messages: 24 (0 generated)
Alpha Left Google
7 years, 3 months ago (2013-08-29 07:49:28 UTC) #1
Peter Kasting
This looks better than the number of comments would indicate -- most are either about ...
7 years, 3 months ago (2013-08-29 23:46:25 UTC) #2
Alpha Left Google
Getting rid of the rowBuffer is possible but a bit tricky. We'll have to write ...
7 years, 3 months ago (2013-08-30 23:25:36 UTC) #3
Peter Kasting
On 2013/08/30 23:25:36, Alpha wrote: > Getting rid of the rowBuffer is possible but a ...
7 years, 3 months ago (2013-08-30 23:30:39 UTC) #4
Peter Kasting
https://codereview.chromium.org/23646005/diff/7001/Source/core/platform/image-decoders/gif/GIFImageReader.cpp File Source/core/platform/image-decoders/gif/GIFImageReader.cpp (right): https://codereview.chromium.org/23646005/diff/7001/Source/core/platform/image-decoders/gif/GIFImageReader.cpp#newcode210 Source/core/platform/image-decoders/gif/GIFImageReader.cpp:210: for (const unsigned char *ch = block; bytesInBlock-- > ...
7 years, 3 months ago (2013-08-31 00:08:49 UTC) #5
Peter Kasting
https://codereview.chromium.org/23646005/diff/7001/Source/core/platform/image-decoders/gif/GIFImageReader.cpp File Source/core/platform/image-decoders/gif/GIFImageReader.cpp (right): https://codereview.chromium.org/23646005/diff/7001/Source/core/platform/image-decoders/gif/GIFImageReader.cpp#newcode239 Source/core/platform/image-decoders/gif/GIFImageReader.cpp:239: if (oldcode == -1) { On 2013/08/31 00:08:49, Peter ...
7 years, 3 months ago (2013-08-31 02:07:19 UTC) #6
Alpha Left Google
I've updated the code to fix the bug for intentional bad initial code. Good catch! ...
7 years, 3 months ago (2013-09-01 01:40:21 UTC) #7
Alpha Left Google
So at the end I think the new behavior is better. The corrupted image will ...
7 years, 3 months ago (2013-09-01 01:42:12 UTC) #8
Peter Kasting
https://codereview.chromium.org/23646005/diff/33001/Source/core/platform/image-decoders/gif/GIFImageDecoderTest.cpp File Source/core/platform/image-decoders/gif/GIFImageDecoderTest.cpp (right): https://codereview.chromium.org/23646005/diff/33001/Source/core/platform/image-decoders/gif/GIFImageDecoderTest.cpp#newcode458 Source/core/platform/image-decoders/gif/GIFImageDecoderTest.cpp:458: RefPtr<SharedBuffer> testData = readFile("/Source/web/tests/data/bad-initial-code.gif"); Nit: Might want a comment ...
7 years, 3 months ago (2013-09-03 18:49:45 UTC) #9
Alpha Left Google
https://codereview.chromium.org/23646005/diff/33001/Source/core/platform/image-decoders/gif/GIFImageReader.cpp File Source/core/platform/image-decoders/gif/GIFImageReader.cpp (right): https://codereview.chromium.org/23646005/diff/33001/Source/core/platform/image-decoders/gif/GIFImageReader.cpp#newcode246 Source/core/platform/image-decoders/gif/GIFImageReader.cpp:246: } else if (code >= avail && oldcode != ...
7 years, 3 months ago (2013-09-04 23:47:28 UTC) #10
Peter Kasting
LGTM https://codereview.chromium.org/23646005/diff/33001/Source/core/platform/image-decoders/gif/GIFImageReader.cpp File Source/core/platform/image-decoders/gif/GIFImageReader.cpp (right): https://codereview.chromium.org/23646005/diff/33001/Source/core/platform/image-decoders/gif/GIFImageReader.cpp#newcode252 Source/core/platform/image-decoders/gif/GIFImageReader.cpp:252: *--rowIter = firstchar; On 2013/09/04 23:47:28, Alpha wrote: ...
7 years, 3 months ago (2013-09-05 06:13:01 UTC) #11
Alpha Left Google
https://codereview.chromium.org/23646005/diff/38001/Source/core/platform/image-decoders/gif/GIFImageDecoderTest.cpp File Source/core/platform/image-decoders/gif/GIFImageDecoderTest.cpp (right): https://codereview.chromium.org/23646005/diff/38001/Source/core/platform/image-decoders/gif/GIFImageDecoderTest.cpp#newcode455 Source/core/platform/image-decoders/gif/GIFImageDecoderTest.cpp:455: // The first LZW code in the image is ...
7 years, 3 months ago (2013-09-05 17:02:27 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hclam@chromium.org/23646005/50001
7 years, 3 months ago (2013-09-05 17:04:30 UTC) #13
commit-bot: I haz the power
Can't process patch for file Source/web/tests/data/bad-code.gif. Binary file support is temporarilly disabled due to a ...
7 years, 3 months ago (2013-09-05 17:04:36 UTC) #14
Alpha Left Google
+jamesr: OWNERS approval for the added data files in Source/web
7 years, 3 months ago (2013-09-05 22:53:03 UTC) #15
jamesr
Could you put these data files somewhere else? It doesn't make sense to add files ...
7 years, 3 months ago (2013-09-05 23:27:18 UTC) #16
Alpha Left Google
On 2013/09/05 23:27:18, jamesr wrote: > Could you put these data files somewhere else? It ...
7 years, 3 months ago (2013-09-11 03:36:37 UTC) #17
jamesr
If they aren't for LayoutTests then why would you put them in LayoutTests/? Your test ...
7 years, 3 months ago (2013-09-11 21:14:17 UTC) #18
Alpha Left Google
Picking this up after vacation. It appears that other test files for images are also ...
7 years, 2 months ago (2013-10-03 01:08:06 UTC) #19
jamesr
Source/core/platform/image-decoders/testing/ ? maybe /test/ ? I don't think it makes a huge difference so long ...
7 years, 2 months ago (2013-10-03 01:09:28 UTC) #20
Alpha Left Google
Okay. I'll create that dir and gradually move files there.
7 years, 2 months ago (2013-10-03 01:10:22 UTC) #21
Alpha Left Google
-jamesr since this is only in Source/core/platform/image-decoders. pkasting: please review again as I created Source/core/platform/image-decoders/testing ...
7 years, 2 months ago (2013-10-03 01:26:58 UTC) #22
Peter Kasting
LGTM https://codereview.chromium.org/23646005/diff/64001/Source/core/platform/image-decoders/gif/GIFImageDecoderTest.cpp File Source/core/platform/image-decoders/gif/GIFImageDecoderTest.cpp (right): https://codereview.chromium.org/23646005/diff/64001/Source/core/platform/image-decoders/gif/GIFImageDecoderTest.cpp#newcode456 Source/core/platform/image-decoders/gif/GIFImageDecoderTest.cpp:456: // dictionary. Decoding should fail, but not infinitely ...
7 years, 2 months ago (2013-10-03 17:52:27 UTC) #23
Alpha Left Google
7 years, 2 months ago (2013-10-03 22:49:28 UTC) #24
Message was sent while issue was closed.
Committed patchset #8 manually as r158842 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698