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

Issue 2386453003: WIP: Implement APNG (Closed)

Created:
4 years, 2 months ago by joostouwerling
Modified:
3 years, 8 months ago
CC:
chromium-reviews, shans, rjwright, blink-reviews-animation_chromium.org, darktears, blink-reviews, Eric Willigers
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Implement Animated PNG. Implement an internal query system in PNGImageDecoder to parse the size and frame data, by passing a PNGImageReader::PNGParseQuery to PNGImageDecoder::parse(). This utilizes the PNGImageReader to answer the query. For a PNGSizeQuery, PNGImageReader processes the stream until the first IDAT frame chunk. At this point, the size should have been decoded from the IHDR chunk. With a PNGFrameCountQuery, the frame data is parsed by the PNGImageReader. This will parse the complete data stream it has so far and reports frames when they're fully seen, so PNGImageDecoder won't report information on partial frames. This prevents decoding half of the frame on top of the other half of the previous frame, in case of blending and/or disposal. The expection for this is the first frame, which is reported as soon as the starting point of the frame is received, so it can be decoded progressively. When the reader does not encounter an acTL chunk before the first IDAT chunk, the image is considered to be non-animated, and frame count parsing is stopped, even if frames may appear after the IDAT chunk(s). The reason for this is a) that it is required by the specification, so most APNG files should have it, and b) that it does not slow down decoding for non-animated PNG's, which is the something like 99.9% of the use cases for now. The following behavioral decisions are implemented: - An fcTL with the wrong chunk length results in a failed decoder, since the decoder can't make any sensible predicitions about missing data. - An acTL with the wrong chunk length is ignored, so the resulting image will be non animated. - The sequence numbers in fcTL and fdAT chunks are ignored. - The frame count in the acTL chunk is ignored. Instead, the actual number of complete frames that are in the stream is reported. - A frame with a frame rect that falls outside of the original image's rect is clipped, so it fits within the image. The frame data outside of the clipped rectangle is ignored. Reimplement frame decoding for non-animated PNGs. When the decoder reads a non-animated PNG, it will reuse the existing libpng pointers and continue processing the data stream from the first IDAT chunk, where it ended after a PNGFrameCountQuery. This is similar to the old approach and is still used to retain performance. Progressive decoding is supported. Frame decoding for animated images is implemented. Single frames are decoded by mocking them as complete PNG images. This is necessary since libpng does not support animated PNG. This works by recreating libpng pointers, and reprocess the PNG header data, but with the frame's width and height. Then, the frame data is processed, by converting fdAT chunks to IDAT chunks. With this approach, the existing infrastructure in PNGImageDecoder to write rows into an ImageFrame does not need many changes. Progressive decoding is supported for non-animated PNGs and for the first frame of animated PNGs. Failures during decoding or parsing are handled differently, based on the image and state of the decoder: 1) When a non-animated PNG, or the first frame of an animated PNG, can't be decoded or parsed, set the decoder to the failed state, because there are no frames we can show to the client. Also set the state to failed if a parse error occurs before any frames were received. 2) When a decoding failure occurs for non-first frames, we still want to show earlier frames. This means, if frame n has a failure during decoding, the frame count is adjusted to n - 1. 3) When a parsing failure occurs, the frame count is adjusted to the number of successfully parsed frames, since we can still show those. BUG=1171 BUG=437662 R=scroggo@google.com,cblume@google.com

Patch Set 1 #

Total comments: 31

Patch Set 2 : Tests for size and repetition count. Decoder stores repetition count for APNG #

Total comments: 3

Patch Set 3 : Implement frame meta data decoding, include tests #

Total comments: 69

Patch Set 4 : Fixes in accordance with feedback on patch set 3 #

Patch Set 5 : Extra tests for invalid images; new image for animation tests #

Total comments: 7

Patch Set 6 : Basic frame decoding with tests, no alpha blending and disposal yet #

Total comments: 75

Patch Set 7 : Processed feedback patch 6 #

Total comments: 9

Patch Set 8 : Progressive decoding for animated images #

Total comments: 64

Patch Set 9 : Fixed feedback on patch 8 #

Total comments: 6

Patch Set 10 : Resynced with master & to 2 space tabs #

Patch Set 11 : Implemented feedback on patch set 9 #

Total comments: 2

Patch Set 12 : Implement disposal of frames #

Total comments: 26

Patch Set 13 : Process feedback on patch 12 #

Total comments: 12

Patch Set 14 : Decode later frames row by row #

Total comments: 22

Patch Set 15 : Implement alpha blending. #

Patch Set 16 : Fix feedback on patch set 14 #

Total comments: 8

Patch Set 17 : Revisit ImageFrame alpha setting. #

Total comments: 9

Patch Set 18 : Change behavior on failure during decoding or parsing. #

Total comments: 22

Patch Set 19 : Verify the current behavior for in- and overcomplete frames is OK with tests. #

Patch Set 20 : Add test to verify IEND before IDAT invalidates decoder. #

Patch Set 21 : Add test to verify frameIsComplete behavior. #

Total comments: 6

Patch Set 22 : Add test to verify erroneous frame parameters default correctly. #

Patch Set 23 : Apply WebKit formatting. #

Patch Set 24 : Fix feedback on patch 16, 17, 18 and 21. #

Patch Set 25 : Rebase master on top of CL #

Total comments: 26

Patch Set 26 : Fix feedback on patch set 25 #

Patch Set 27 : Create m_reader on construction and remove |m_offset| #

Total comments: 12

Patch Set 28 : Fix feedback patch 27 #

Patch Set 29 : Move PNGParseQuery to PNGImageReader. Make PNGImageReader a class member instead of pointer #

Patch Set 30 : Merge master and apply WebKit formatting #

Patch Set 31 : Make sure first frame is initialized correctly when setMemoryAllocator is called #

Patch Set 32 : Fix wrong alpha blend and disposal for static PNG #

Total comments: 2

Patch Set 33 : Use constants for mapping fcTL blend and disposal parameters. #

Total comments: 1

Patch Set 34 : Disable LayoutTest canvas/tests/2d.drawImage.animated.apng #

Total comments: 4

Patch Set 35 : Change test to prevent access to possible invalid memory. #

Patch Set 36 : Fix invalid frame pointer and not destroying FastSharedBuffer on PNG error. #

Patch Set 37 : Another try to disable 2d.drawImage.animated.poster #

Patch Set 38 : Fix memory leaks by using FastSharedReadBuffer and defining it smartly #

Total comments: 6

Patch Set 39 : Fix feedback on previous patches #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1782 lines, -191 lines) Patch
M third_party/WebKit/LayoutTests/canvas/philip/tests/2d.drawImage.animated.poster.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 1 chunk +18 lines, -2 lines 0 comments Download
A third_party/WebKit/LayoutTests/images/resources/png-animated-idat-not-part-of-animation.png View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 Binary file 0 comments Download
A third_party/WebKit/LayoutTests/images/resources/png-animated-idat-part-of-animation.png View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 Binary file 0 comments Download
A third_party/WebKit/LayoutTests/images/resources/png-animated-three-independent-frames.png View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 Binary file 0 comments Download
M third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 2 chunks +52 lines, -8 lines 0 comments Download
M third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 8 chunks +320 lines, -141 lines 0 comments Download
M third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoderTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 5 chunks +670 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/platform/image-decoders/png/PNGImageReader.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 2 chunks +117 lines, -19 lines 0 comments Download
M third_party/WebKit/Source/platform/image-decoders/png/PNGImageReader.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 2 chunks +605 lines, -16 lines 0 comments Download

Messages

Total messages: 151 (80 generated)
joostouwerling
4 years, 2 months ago (2016-09-30 21:19:04 UTC) #1
cblume
https://codereview.chromium.org/2386453003/diff/1/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (right): https://codereview.chromium.org/2386453003/diff/1/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode80 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:80: // Similar reasoning to GIFImageDecodeer. If decoding fails, we ...
4 years, 2 months ago (2016-10-01 12:01:14 UTC) #3
joostouwerling
https://codereview.chromium.org/2386453003/diff/1/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (right): https://codereview.chromium.org/2386453003/diff/1/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode80 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:80: // Similar reasoning to GIFImageDecodeer. If decoding fails, we ...
4 years, 2 months ago (2016-10-01 17:51:39 UTC) #4
joostouwerling
https://codereview.chromium.org/2386453003/diff/20001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoderTest.cpp File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoderTest.cpp (right): https://codereview.chromium.org/2386453003/diff/20001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoderTest.cpp#newcode65 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoderTest.cpp:65: EXPECT_EQ(false, data.get()->isEmpty()); Line 65 is a little weird, changed ...
4 years, 2 months ago (2016-10-01 19:59:05 UTC) #5
cblume
https://codereview.chromium.org/2386453003/diff/1/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (right): https://codereview.chromium.org/2386453003/diff/1/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode88 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:88: return decoder->frameIsCompleteAtIndex(0); On 2016/10/01 17:51:38, joostouwerling wrote: > Hmm... ...
4 years, 2 months ago (2016-10-02 19:55:44 UTC) #6
joostouwerling
https://codereview.chromium.org/2386453003/diff/1/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (right): https://codereview.chromium.org/2386453003/diff/1/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode88 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:88: return decoder->frameIsCompleteAtIndex(0); On 2016/10/02 19:55:43, cblume wrote: > On ...
4 years, 2 months ago (2016-10-03 14:39:10 UTC) #7
scroggo_chromium
> Initial start with APNGs. Includes tests for frame count, decodeFrameCount > implementation and a ...
4 years, 2 months ago (2016-10-04 14:50:17 UTC) #9
joostouwerling
https://codereview.chromium.org/2386453003/diff/1/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (right): https://codereview.chromium.org/2386453003/diff/1/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode230 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:230: // dealing with a non animated PNG. On 2016/10/04 ...
4 years, 2 months ago (2016-10-04 17:48:32 UTC) #10
joostouwerling
This is an implementation of parsing the frame count. Some remarks: A frame is recognized ...
4 years, 2 months ago (2016-10-11 16:31:07 UTC) #12
scroggo_chromium
> An open question is what we want to do with the IDAT when it ...
4 years, 2 months ago (2016-10-11 20:13:11 UTC) #13
joostouwerling
All comments that I did not address in this response can be considered silently acknowledged. ...
4 years, 2 months ago (2016-10-12 20:49:47 UTC) #14
scroggo_chromium
On 2016/10/12 20:49:47, joostouwerling wrote: > > > An open question is what we want ...
4 years, 2 months ago (2016-10-13 13:49:33 UTC) #15
joostouwerling
https://codereview.chromium.org/2386453003/diff/40001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (right): https://codereview.chromium.org/2386453003/diff/40001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode78 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:78: return m_frameCount; On 2016/10/12 20:49:46, joostouwerling wrote: > On ...
4 years, 2 months ago (2016-10-13 18:50:22 UTC) #16
scroggo_chromium
On 2016/10/13 18:50:22, joostouwerling wrote: > https://codereview.chromium.org/2386453003/diff/40001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp > File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp > (right): > > https://codereview.chromium.org/2386453003/diff/40001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode78 ...
4 years, 2 months ago (2016-10-13 20:03:46 UTC) #17
joostouwerling
PTAL I've processed @scroggo's feedback, and improved the test suite by: a) adding two tests ...
4 years, 2 months ago (2016-10-14 19:44:40 UTC) #21
cblume
https://codereview.chromium.org/2386453003/diff/100001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (right): https://codereview.chromium.org/2386453003/diff/100001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode45 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:45: nit: extra line? https://codereview.chromium.org/2386453003/diff/100001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode110 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:110: m_repetitionCount = (repetitionCount == ...
4 years, 2 months ago (2016-10-15 00:30:50 UTC) #22
scroggo_chromium
https://codereview.chromium.org/2386453003/diff/100001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.h File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.h (right): https://codereview.chromium.org/2386453003/diff/100001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.h#newcode58 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.h:58: void setMetaDataDecoded() { m_metaDataDecoded = true; }; On 2016/10/15 ...
4 years, 2 months ago (2016-10-17 13:24:01 UTC) #23
scroggo_chromium
https://codereview.chromium.org/2386453003/diff/100001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoderTest.cpp File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoderTest.cpp (right): https://codereview.chromium.org/2386453003/diff/100001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoderTest.cpp#newcode194 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoderTest.cpp:194: // discovered. The fourth frame should *not* be registered ...
4 years, 2 months ago (2016-10-17 15:27:06 UTC) #24
cblume
https://codereview.chromium.org/2386453003/diff/100001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.h File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.h (right): https://codereview.chromium.org/2386453003/diff/100001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.h#newcode58 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.h:58: void setMetaDataDecoded() { m_metaDataDecoded = true; }; On 2016/10/17 ...
4 years, 2 months ago (2016-10-17 17:08:17 UTC) #25
joostouwerling
PTAL. This patch implements frame decoding and tests. Does not yet support alpha blending and ...
4 years, 1 month ago (2016-10-24 22:26:49 UTC) #32
scroggo_chromium
https://codereview.chromium.org/2386453003/diff/140001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (right): https://codereview.chromium.org/2386453003/diff/140001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode68 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:68: , m_decodedFrameCount(false) This variable is never read. https://codereview.chromium.org/2386453003/diff/140001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode117 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:117: ...
4 years, 1 month ago (2016-10-25 14:59:07 UTC) #33
joostouwerling
I processed @scroggo's feedback in a new patch. Some comments are still open for debate. ...
4 years, 1 month ago (2016-10-26 15:44:17 UTC) #34
scroggo_chromium
https://codereview.chromium.org/2386453003/diff/140001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (right): https://codereview.chromium.org/2386453003/diff/140001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode324 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:324: int y = rowIndex + rect.y(); On 2016/10/26 15:44:16, ...
4 years, 1 month ago (2016-10-26 18:25:59 UTC) #35
joostouwerling
Implemented progressive decoding for the first frame of animated images, and included tests. Solved more ...
4 years, 1 month ago (2016-10-27 20:29:55 UTC) #37
scroggo_chromium
https://codereview.chromium.org/2386453003/diff/140001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageReader.cpp File third_party/WebKit/Source/platform/image-decoders/png/PNGImageReader.cpp (right): https://codereview.chromium.org/2386453003/diff/140001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageReader.cpp#newcode218 third_party/WebKit/Source/platform/image-decoders/png/PNGImageReader.cpp:218: png_destroy_read_struct(m_png ? &m_png : 0, m_info ? &m_info : ...
4 years, 1 month ago (2016-10-28 14:20:34 UTC) #39
cblume
https://codereview.chromium.org/2386453003/diff/200001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageReader.cpp File third_party/WebKit/Source/platform/image-decoders/png/PNGImageReader.cpp (right): https://codereview.chromium.org/2386453003/diff/200001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageReader.cpp#newcode186 third_party/WebKit/Source/platform/image-decoders/png/PNGImageReader.cpp:186: // Progressive decoding is only done if: On 2016/10/28 ...
4 years, 1 month ago (2016-10-28 17:29:17 UTC) #40
joostouwerling
https://codereview.chromium.org/2386453003/diff/140001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageReader.cpp File third_party/WebKit/Source/platform/image-decoders/png/PNGImageReader.cpp (right): https://codereview.chromium.org/2386453003/diff/140001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageReader.cpp#newcode218 third_party/WebKit/Source/platform/image-decoders/png/PNGImageReader.cpp:218: png_destroy_read_struct(m_png ? &m_png : 0, m_info ? &m_info : ...
4 years, 1 month ago (2016-10-28 18:41:26 UTC) #41
scroggo_chromium
https://codereview.chromium.org/2386453003/diff/160001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoderTest.cpp File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoderTest.cpp (right): https://codereview.chromium.org/2386453003/diff/160001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoderTest.cpp#newcode498 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoderTest.cpp:498: // Change the width and height of the frame ...
4 years, 1 month ago (2016-10-31 13:35:12 UTC) #43
joostouwerling
https://codereview.chromium.org/2386453003/diff/140001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoderTest.cpp File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoderTest.cpp (right): https://codereview.chromium.org/2386453003/diff/140001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoderTest.cpp#newcode411 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoderTest.cpp:411: // ... and then decode frames from |reallocatedData|. On ...
4 years, 1 month ago (2016-10-31 18:40:19 UTC) #47
scroggo_chromium
Changes so far look to be on the right track. https://codereview.chromium.org/2386453003/diff/200001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageReader.cpp File third_party/WebKit/Source/platform/image-decoders/png/PNGImageReader.cpp (right): https://codereview.chromium.org/2386453003/diff/200001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageReader.cpp#newcode181 ...
4 years, 1 month ago (2016-10-31 19:34:06 UTC) #48
joostouwerling
Implemented frame disposal. For alpha blending, I plan on pulling some functions from WEBPImageDecoder up ...
4 years, 1 month ago (2016-11-02 18:21:53 UTC) #53
scroggo_chromium
https://codereview.chromium.org/2386453003/diff/400001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (right): https://codereview.chromium.org/2386453003/diff/400001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode71 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:71: if (!m_reader || !m_reader->parseCompleted()) It's somewhat noticeable that this ...
4 years, 1 month ago (2016-11-07 13:02:17 UTC) #54
joostouwerling
https://codereview.chromium.org/2386453003/diff/400001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (right): https://codereview.chromium.org/2386453003/diff/400001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode71 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:71: if (!m_reader || !m_reader->parseCompleted()) On 2016/11/07 13:02:16, scroggo_chromium wrote: ...
4 years, 1 month ago (2016-11-08 21:58:17 UTC) #58
scroggo_chromium
https://codereview.chromium.org/2386453003/diff/400001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (right): https://codereview.chromium.org/2386453003/diff/400001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode202 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:202: // should be changed to stick with m_repetitionCount to ...
4 years, 1 month ago (2016-11-09 13:42:51 UTC) #60
joostouwerling
I revisited the decision to write the pixels to the frame buffer in the complete() ...
4 years, 1 month ago (2016-11-11 20:22:19 UTC) #62
scroggo_chromium
https://codereview.chromium.org/2386453003/diff/540001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (left): https://codereview.chromium.org/2386453003/diff/540001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#oldcode313 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:313: if (m_reader->decodingSizeOnly()) { Why is this no longer necessary? ...
4 years, 1 month ago (2016-11-11 21:31:06 UTC) #63
joostouwerling
Three new patches: Patch set 15 implements alpha blending. Patch set 16 fixes feedback from ...
4 years, 1 month ago (2016-11-21 20:27:45 UTC) #65
joostouwerling
I implemented different behavior for failures during decoding or parsing, and added tests to verify ...
4 years, 1 month ago (2016-11-22 23:05:03 UTC) #72
joostouwerling
I've added tests to verify the behavior for in- and overcomplete data. There is no ...
4 years ago (2016-11-23 21:09:43 UTC) #76
scroggo_chromium
https://codereview.chromium.org/2386453003/diff/140001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (right): https://codereview.chromium.org/2386453003/diff/140001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode400 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:400: return ImageDecoder::frameIsCompleteAtIndex(index); Revisiting this because your test (VerifyFrameCompleteBehavior) checks ...
4 years ago (2016-11-29 16:30:52 UTC) #77
scroggo_chromium
https://codereview.chromium.org/2386453003/diff/620001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (right): https://codereview.chromium.org/2386453003/diff/620001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode661 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:661: // Now, if we're at a DisposeNotSpecified or DisposeKeep ...
4 years ago (2016-12-02 15:55:36 UTC) #78
joostouwerling
https://codereview.chromium.org/2386453003/diff/600001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (right): https://codereview.chromium.org/2386453003/diff/600001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode407 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:407: !m_colorSpaceSet) { On 2016/11/29 16:30:51, scroggo_chromium wrote: > It's ...
4 years ago (2016-12-02 16:08:42 UTC) #80
joostouwerling
On 2016/12/02 15:55:36, scroggo_chromium wrote: > https://codereview.chromium.org/2386453003/diff/620001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp > File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp > (right): > > https://codereview.chromium.org/2386453003/diff/620001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode661 ...
4 years ago (2016-12-02 16:09:21 UTC) #81
scroggo_chromium
Changes 1gtm. I'll want to take another look over it once you rebase on top ...
4 years ago (2016-12-02 21:28:49 UTC) #82
joostouwerling
I will rebase it later today. https://codereview.chromium.org/2386453003/diff/660001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (right): https://codereview.chromium.org/2386453003/diff/660001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode79 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:79: if (m_isParsing) On ...
4 years ago (2016-12-05 15:32:46 UTC) #83
joostouwerling
PTAL at patch set 25. This is a rebase of the current master branch which ...
4 years ago (2016-12-06 20:14:27 UTC) #87
scroggo_chromium
https://codereview.chromium.org/2386453003/diff/920001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (right): https://codereview.chromium.org/2386453003/diff/920001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode49 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:49: sk_sp<SkColorSpace> colorSpace, I'm guessing colorSpace changed since you touched ...
4 years ago (2016-12-07 13:44:44 UTC) #88
joostouwerling
https://codereview.chromium.org/2386453003/diff/920001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (right): https://codereview.chromium.org/2386453003/diff/920001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode49 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:49: sk_sp<SkColorSpace> colorSpace, On 2016/12/07 13:44:44, scroggo_chromium wrote: > I'm ...
4 years ago (2016-12-07 16:49:10 UTC) #89
joostouwerling
I've moved the initialization of PNGImageDecoder::m_reader to the constructor in patch set 27. Since we ...
4 years ago (2016-12-07 16:59:36 UTC) #90
scroggo_chromium
https://codereview.chromium.org/2386453003/diff/960001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (right): https://codereview.chromium.org/2386453003/diff/960001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode61 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:61: m_reader = wrapUnique(new PNGImageReader(this, offset)); Now that m_reader has ...
4 years ago (2016-12-08 13:59:09 UTC) #91
joostouwerling
https://codereview.chromium.org/2386453003/diff/960001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (right): https://codereview.chromium.org/2386453003/diff/960001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode61 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:61: m_reader = wrapUnique(new PNGImageReader(this, offset)); On 2016/12/08 13:59:09, scroggo_chromium ...
4 years ago (2016-12-08 16:28:39 UTC) #92
scroggo_chromium
https://codereview.chromium.org/2386453003/diff/960001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (right): https://codereview.chromium.org/2386453003/diff/960001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode61 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:61: m_reader = wrapUnique(new PNGImageReader(this, offset)); On 2016/12/08 16:28:39, joostouwerling ...
4 years ago (2016-12-08 16:33:25 UTC) #93
joostouwerling
https://codereview.chromium.org/2386453003/diff/960001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (right): https://codereview.chromium.org/2386453003/diff/960001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode61 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:61: m_reader = wrapUnique(new PNGImageReader(this, offset)); On 2016/12/08 16:33:25, scroggo_chromium ...
4 years ago (2016-12-08 21:33:20 UTC) #95
scroggo_chromium
Some nits on the commit message: > Implement an internal query system in PNGImageDecoder to ...
4 years ago (2016-12-08 21:45:13 UTC) #96
cblume
https://codereview.chromium.org/2386453003/diff/1080001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (right): https://codereview.chromium.org/2386453003/diff/1080001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode165 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:165: case 0: Nit: I like that there is a ...
4 years ago (2016-12-11 22:08:26 UTC) #117
joostouwerling
https://codereview.chromium.org/2386453003/diff/1080001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (right): https://codereview.chromium.org/2386453003/diff/1080001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode165 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:165: case 0: On 2016/12/11 22:08:26, cblume wrote: > Nit: ...
4 years ago (2016-12-12 14:48:50 UTC) #122
scroggo_chromium
lgtm https://codereview.chromium.org/2386453003/diff/1100001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageReader.h File third_party/WebKit/Source/platform/image-decoders/png/PNGImageReader.h (right): https://codereview.chromium.org/2386453003/diff/1100001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageReader.h#newcode43 third_party/WebKit/Source/platform/image-decoders/png/PNGImageReader.h:43: const uint8_t kAPNGDisposeKeep = 0; nit: Maybe leave ...
4 years ago (2016-12-12 14:52:44 UTC) #123
joostouwerling
Disabled the LayoutTest 2d.drawImage.animated.apng. Stated the why's as a comment in the test.
4 years ago (2016-12-12 15:57:11 UTC) #124
scroggo_chromium
https://codereview.chromium.org/2386453003/diff/1120001/third_party/WebKit/LayoutTests/canvas/philip/tests/2d.drawImage.animated.poster.html File third_party/WebKit/LayoutTests/canvas/philip/tests/2d.drawImage.animated.poster.html (right): https://codereview.chromium.org/2386453003/diff/1120001/third_party/WebKit/LayoutTests/canvas/philip/tests/2d.drawImage.animated.poster.html#newcode15 third_party/WebKit/LayoutTests/canvas/philip/tests/2d.drawImage.animated.poster.html:15: // This test verifies that the poster frame of ...
4 years ago (2016-12-12 16:06:39 UTC) #125
joostouwerling
https://codereview.chromium.org/2386453003/diff/1120001/third_party/WebKit/LayoutTests/canvas/philip/tests/2d.drawImage.animated.poster.html File third_party/WebKit/LayoutTests/canvas/philip/tests/2d.drawImage.animated.poster.html (right): https://codereview.chromium.org/2386453003/diff/1120001/third_party/WebKit/LayoutTests/canvas/philip/tests/2d.drawImage.animated.poster.html#newcode15 third_party/WebKit/LayoutTests/canvas/philip/tests/2d.drawImage.animated.poster.html:15: // This test verifies that the poster frame of ...
4 years ago (2016-12-12 17:53:21 UTC) #126
cblume
non-owner LGTM
4 years ago (2016-12-12 18:46:54 UTC) #132
joostouwerling
I've changed PNGImageReader so it passes all tests - more specifically, it was leaking memory ...
4 years ago (2016-12-13 20:13:08 UTC) #140
cblume
On 2016/12/13 20:13:08, joostouwerling wrote: > I've changed PNGImageReader so it passes all tests - ...
4 years ago (2016-12-13 21:09:55 UTC) #143
joostouwerling
On 2016/12/13 21:09:55, cblume wrote: > On 2016/12/13 20:13:08, joostouwerling wrote: > > I've changed ...
4 years ago (2016-12-13 21:42:08 UTC) #144
joostouwerling
On 2016/12/13 21:42:08, joostouwerling wrote: > On 2016/12/13 21:09:55, cblume wrote: > > On 2016/12/13 ...
4 years ago (2016-12-13 22:23:40 UTC) #145
scroggo_chromium
https://codereview.chromium.org/2386453003/diff/1120001/third_party/WebKit/LayoutTests/canvas/philip/tests/2d.drawImage.animated.poster.html File third_party/WebKit/LayoutTests/canvas/philip/tests/2d.drawImage.animated.poster.html (right): https://codereview.chromium.org/2386453003/diff/1120001/third_party/WebKit/LayoutTests/canvas/philip/tests/2d.drawImage.animated.poster.html#newcode15 third_party/WebKit/LayoutTests/canvas/philip/tests/2d.drawImage.animated.poster.html:15: // This test verifies that the poster frame of ...
4 years ago (2016-12-14 14:18:06 UTC) #146
joostouwerling
https://codereview.chromium.org/2386453003/diff/1120001/third_party/WebKit/LayoutTests/canvas/philip/tests/2d.drawImage.animated.poster.html File third_party/WebKit/LayoutTests/canvas/philip/tests/2d.drawImage.animated.poster.html (right): https://codereview.chromium.org/2386453003/diff/1120001/third_party/WebKit/LayoutTests/canvas/philip/tests/2d.drawImage.animated.poster.html#newcode15 third_party/WebKit/LayoutTests/canvas/philip/tests/2d.drawImage.animated.poster.html:15: // This test verifies that the poster frame of ...
4 years ago (2016-12-16 19:59:32 UTC) #147
MaxStepin
Calling this this method for every frame feels a bit unsafe. void createInterlaceBuffer(int size) { ...
3 years, 12 months ago (2016-12-27 06:51:37 UTC) #148
cblume
On 2016/12/27 06:51:37, MaxStepin wrote: > Calling this this method for every frame feels a ...
3 years, 11 months ago (2017-01-04 01:08:05 UTC) #149
scroggo_chromium
On 2017/01/04 01:08:05, cblume wrote: > On 2016/12/27 06:51:37, MaxStepin wrote: > > Calling this ...
3 years, 11 months ago (2017-01-06 15:08:40 UTC) #150
scroggo_chromium
3 years, 9 months ago (2017-03-15 15:19:23 UTC) #151
On 2017/01/06 15:08:40, scroggo_chromium wrote:
> On 2017/01/04 01:08:05, cblume wrote:
> > On 2016/12/27 06:51:37, MaxStepin wrote:
> > > Calling this this method for every frame feels a bit unsafe.
> > > 
> > >   void createInterlaceBuffer(int size) {
> > >     m_interlaceBuffer = wrapArrayUnique(new png_byte[size]);
> > >   }
> > > 
> > > You'll have to be extra careful to avoid a memory leak.
> > > 
> > > I think allocating this buffer just once and re-using it for all frames
> would
> > be
> > > safer, faster, and it should work just fine.
> > 
> > Hello Max. :) Thank you for your input.
> > 
> > It looks like this CL doesn't change the behavior of createInterlaceBuffer.
> > 
> > If we want to do some work there, we should probably do it in a separate bug
/
> > CL.
> > 
> > 
> > 
> > But at a cursory glance, I think I agree.
> > 
> > createInterlaceBuffer seems to only be called from one location,
> > PNGImageDecoder::rowAvailable
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image...
> > 
> > And PNGImageDecoder::rowAvailable is only called from pngRowAvailable
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image...
> > 
> > pngRowAvailable is then passed into libpng's png_set_progressive_read_fn
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image...
> > 
> > pngRowAvailable is called every time a new row is ready.
> > But we only call createInterlaceBuffer if the frame status is empty. On the
> > first row callback, this is true.
> > After we allocate, we set the frame status to partial.
> > So even though this is called for every row, it only allocates once per
frame
> > like you mentioned.
> > 
> > 
> > After being allocated once per frame, it is only ever read in future row
> > callbacks.
> > When we hit the next frame, the frame status is once again empty. So we
> allocate
> > again.
> > After we allocate, we assign to the member unique_ptr, which ends up freeing
> the
> > old frame.
> > 
> > 
> > This probably wasn't much of a concern before APNG, since there was only
ever
> > one frame.
> > It wouldn't end up ever deallocating until the reader is done.
> > But now that we have multiple frames, it will deallocate and reallocate each
> > frame.
> 
> As cblume@ mentions, we're not leaking memory because m_interlaceBuffer is a
> smart pointer.
> 
> But you are right, we could reuse the buffer from frame to frame, which would
> avoid the cost of reallocating. There are a few different ways we could handle
> this, which have their own tradeoffs:
> 
> - This method is simple, and if the client never asks for another frame, we
> freed the buffer early. (I just realized that if the decoder fails we do *not*
> free the buffer. In the existing implementation (i.e. no APNG support), a
failed
> decode deletes the PNGImageReader, which also frees the buffer, so this is a
> change.)
> - On the other extreme, we could create the buffer only once, and keep it
until
> the client is done with the PNGImageDecoder. This would mean we pay the cost
of
> allocating only once, but we would be holding onto memory that we do not (at
> least currently) need.
> - Another option would be to create it in PNGImageDecoder::decode(), prior to
> decoding any dependencies, and clear it after the loop (unless the frame was
> partial). We would still clear it unnecessarily sometimes, but we would only
> hang on to it when we were confident we still need it.

filed crbug.com/701779

Powered by Google App Engine
This is Rietveld 408576698