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

Issue 2618633004: Add support for Animated PNG (Closed)

Created:
3 years, 11 months ago by scroggo_chromium
Modified:
3 years, 9 months ago
CC:
chromium-reviews, shans, rjwright, blink-reviews-animation_chromium.org, darktears, blink-reviews, kinuko+watch, Eric Willigers
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add support for Animated PNG Update the browser accept header to state that APNG is supported. Split the decoding of PNG images into two stages: parsing and decoding. During parsing, chunks are handled one of three ways: - acTL and fcTL chunks, which specify properties of an APNG are read and the properties are stored. If they contain an error before IDAT, the image is treated as a static PNG, as recommended by the spec [1]. If they contain an error after IDAT, this is a failure, since some frames may have been decoded. CRCs are calculated and compared to their expected values. Mismatches are considered errors. - fdAT and IDAT chunks have their locations stored for decoding later. Any ordering violations result in either a static image or failed image, depending on whether IDAT has been seen yet. - Other chunks before IDAT are passed to libpng for processing. Each frame is decoded as if it were a complete PNG file. fdATs are converted to IDATs (and their CRCs are ignored**) and the IHDR is modified for subset frames. The rowAvailable callback positions subset frames properly within the full image buffer. For a static PNG or the first frame decoded (assuming its size matches IHDR) the png struct used during parsing is reused. Otherwise, a new png struct is created for the duration of the decode. Follow the APNG spec as closely as possible and use the APNG test page [2] as a guide for intended behavior. All of the valid APNG on the test page work as expected. For the invalid APNG: - Errors that occur before IDAT show the default image, as intended - Errors afterwards are typically treated as failures (see Future work, below) - The final three images draw incorrectly. They have incorrectly sized IDATs/ fdATs. These could be respected by checking the warning that libpng sends (in the case of extra data) or keeping track of how many rows have been seen (in the case of too little data), although we currently ignore this for static PNGs anyway. (crbug.com/698808) The first frame can be decoded progressively. Other frames are not reported until the following fcTL chunk has been reached during parse. Add a reference test, modified from WebKit's fast/images/animated-png.html with the following changes: - use window.internals.advanceImageAnimation instead of waiting for a timeout, for more reliable testing - disable two of the images, which look the same to my eyes, but are not identical (I suspect due to blending differences as compared to how the reference tests were created). Add gtests. Update progressive tests to reuse a SharedBuffer, rather than recreating it, to reduce test run-time. Fix a bug in ImageFrameGenerator where it called setMemoryAllocator before setting the data, and add related tests. Stop calling setFailed inside ImageDecoder::initFrameBuffer, return false instead. Clients are now expected to call setFailed (update the WEBP and GIF clients). This is safer, because setFailed may delete an object that called initFrameBuffer. In WEBPImageDecoder: rename frameIsLoadedAtIndex to frameIsReceivedAtIndex, for consistency with PNGImageDecoder. Always call ImageFrame::setStatus last (e.g. after calling onInitFrameBuffer, correctAlphaWhenFrameBufferSawNoAlpha). Future work: - Revert to showing the default image for failures past IDAT. This is tricky, since the client may be holding on to previous frames, and we need to make sure they switch to using the IDAT frame, even if it is not part of the animation. For now, we mark the decoder as having failed. (crbug.com/699675) ** We cannot allow libpng to check the CRC since we modified the chunk. We could check the CRC directly as a separate step. [1] https://wiki.mozilla.org/APNG_Specification [2] https://philip.html5.org/tests/apng/tests.html BUG=1171 BUG=437662 Initial patch is a re-upload of issue 2386453003 at patchset 39 (http://crrev.com/2386453003#ps1260001) by joostouwerling@google.com, which also used https://codereview.chromium.org/1567053002/ by maxstepin@gmail.com as a reference. Review-Url: https://codereview.chromium.org/2618633004 Cr-Commit-Position: refs/heads/master@{#456840} Committed: https://chromium.googlesource.com/chromium/src/+/7d2b8c45afc9c0230410011293cc2e1dbb8943a7

Patch Set 1 : Reupload of 2386453003 #

Patch Set 2 : Fix bug caught by fuzzer #

Total comments: 2

Patch Set 3 : Update accept header and its test #

Patch Set 4 : Small cleanups #

Total comments: 3

Patch Set 5 : Reject bad data. Cleanups #

Total comments: 174

Patch Set 6 : Respond to comments. Fix a bug. #

Total comments: 2

Patch Set 7 : Update frame accept header #

Patch Set 8 : Call initFrameBuffer before decoding #

Patch Set 9 : Fix GIF alpha issue #

Patch Set 10 : Inline onInitFrameBuffer in PNG #

Patch Set 11 : Respond to comments #

Patch Set 12 : Rebase #

Total comments: 92

Patch Set 13 : Respond to comments #

Total comments: 24

Patch Set 14 : Respond to comments #

Patch Set 15 : Small cleanups #

Patch Set 16 : Respond to comments #

Total comments: 37

Patch Set 17 : Respond to comments #

Patch Set 18 : Rebase #

Patch Set 19 : Fix heap-buffer-overflow in test (caught by ASAN) #

Patch Set 20 : Use a custom allocator in GIFImageDecoderTest #

Total comments: 1

Patch Set 21 : Fix LayoutTest due to accept header change #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2053 lines, -235 lines) Patch
M content/browser/loader/mime_sniffing_resource_handler.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +2 lines, -2 lines 0 comments Download
M content/browser/loader/mime_sniffing_resource_handler_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +4 lines, -4 lines 0 comments Download
M third_party/WebKit/LayoutTests/canvas/philip/tests/2d.drawImage.animated.poster.html View 1 chunk +18 lines, -2 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/misc/xhtml-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +1 line, -1 line 0 comments Download
A third_party/WebKit/LayoutTests/images/animated-png.html View 1 2 3 4 1 chunk +63 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/images/animated-png-expected.html View 1 2 3 4 1 chunk +16 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/images/resources/apng00.png View 1 2 3 4 Binary file 0 comments Download
A third_party/WebKit/LayoutTests/images/resources/apng00-ref.png View 1 2 3 4 Binary file 0 comments Download
A third_party/WebKit/LayoutTests/images/resources/apng01.png View 1 2 3 4 Binary file 0 comments Download
A third_party/WebKit/LayoutTests/images/resources/apng01-ref.png View 1 2 3 4 Binary file 0 comments Download
A third_party/WebKit/LayoutTests/images/resources/apng02.png View 1 2 3 4 Binary file 0 comments Download
A third_party/WebKit/LayoutTests/images/resources/apng02-ref.png View 1 2 3 4 Binary file 0 comments Download
A third_party/WebKit/LayoutTests/images/resources/apng04.png View 1 2 3 4 Binary file 0 comments Download
A third_party/WebKit/LayoutTests/images/resources/apng04-ref.png View 1 2 3 4 Binary file 0 comments Download
A third_party/WebKit/LayoutTests/images/resources/apng08.png View 1 2 3 4 Binary file 0 comments Download
A third_party/WebKit/LayoutTests/images/resources/apng08-ref.png View 1 2 3 4 Binary file 0 comments Download
A third_party/WebKit/LayoutTests/images/resources/apng10.png View 1 2 3 4 Binary file 0 comments Download
A third_party/WebKit/LayoutTests/images/resources/apng10-ref.png View 1 2 3 4 Binary file 0 comments Download
A third_party/WebKit/LayoutTests/images/resources/apng11.png View 1 2 3 4 Binary file 0 comments Download
A third_party/WebKit/LayoutTests/images/resources/apng11-ref.png View 1 2 3 4 Binary file 0 comments Download
A third_party/WebKit/LayoutTests/images/resources/apng12.png View 1 2 3 4 Binary file 0 comments Download
A third_party/WebKit/LayoutTests/images/resources/apng12-ref.png View 1 2 3 4 Binary file 0 comments Download
A third_party/WebKit/LayoutTests/images/resources/apng14.png View 1 2 3 4 Binary file 0 comments Download
A third_party/WebKit/LayoutTests/images/resources/apng14-ref.png View 1 2 3 4 Binary file 0 comments Download
A third_party/WebKit/LayoutTests/images/resources/apng18.png View 1 2 3 4 Binary file 0 comments Download
A third_party/WebKit/LayoutTests/images/resources/apng18-ref.png View 1 2 3 4 Binary file 0 comments Download
A third_party/WebKit/LayoutTests/images/resources/apng24.png View 1 2 3 4 Binary file 0 comments Download
A third_party/WebKit/LayoutTests/images/resources/apng24-ref.png View 1 2 3 4 Binary file 0 comments Download
A third_party/WebKit/LayoutTests/images/resources/apng26.png View 1 2 3 4 Binary file 0 comments Download
A third_party/WebKit/LayoutTests/images/resources/apng26-ref.png View 1 2 3 4 Binary file 0 comments Download
A third_party/WebKit/LayoutTests/images/resources/empty-frame.png View 1 2 3 4 Binary file 0 comments Download
A third_party/WebKit/LayoutTests/images/resources/png-animated-idat-not-part-of-animation.png View Binary file 0 comments Download
A third_party/WebKit/LayoutTests/images/resources/png-animated-idat-part-of-animation.png View Binary file 0 comments Download
A third_party/WebKit/LayoutTests/images/resources/png-animated-three-independent-frames.png View Binary file 0 comments Download
M third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +39 lines, -20 lines 0 comments Download
M third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp View 1 2 3 4 5 1 chunk +18 lines, -9 lines 0 comments Download
M third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +11 lines, -7 lines 0 comments Download
M third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +4 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.h View 1 2 3 4 5 1 chunk +4 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp View 1 2 3 4 5 6 7 8 9 10 11 6 chunks +19 lines, -15 lines 0 comments Download
M third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoderTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +30 lines, -0 lines 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 2 chunks +22 lines, -11 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 11 chunks +212 lines, -91 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 7 chunks +897 lines, -29 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 3 chunks +95 lines, -12 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 3 chunks +589 lines, -24 lines 0 comments Download
M third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +6 lines, -3 lines 0 comments Download

Messages

Total messages: 74 (33 generated)
scroggo_chromium
This CL revives crrev.com/2386453003. It fixes a bug in that CL, updates the accept header, ...
3 years, 11 months ago (2017-01-06 17:13:10 UTC) #3
MaxStepin
On 2017/01/06 17:13:10, scroggo_chromium wrote: > This was an interesting bug caught by the fuzzer. ...
3 years, 11 months ago (2017-01-06 19:42:34 UTC) #4
Nate Chapin
https://codereview.chromium.org/2618633004/diff/60001/content/browser/loader/mime_sniffing_resource_handler.cc File content/browser/loader/mime_sniffing_resource_handler.cc (right): https://codereview.chromium.org/2618633004/diff/60001/content/browser/loader/mime_sniffing_resource_handler.cc#newcode51 content/browser/loader/mime_sniffing_resource_handler.cc:51: const char kImageAcceptHeader[] = "image/webp,image/apng,image/*,*/*;q=0.8"; On 2017/01/06 17:13:10, scroggo_chromium ...
3 years, 11 months ago (2017-01-06 19:51:30 UTC) #5
scroggo_chromium
https://codereview.chromium.org/2618633004/diff/20001/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/2618633004/diff/20001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode543 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:543: initFrameBuffer(m_currentFrame); >> This was an interesting bug caught by ...
3 years, 11 months ago (2017-01-12 22:34:52 UTC) #6
MaxStepin
On 2017/01/12 22:34:52, scroggo_chromium wrote: > Good point - we could also consider this an ...
3 years, 11 months ago (2017-01-13 08:43:44 UTC) #7
Noel Gordon
Good points, if Firefox is stricter then we should be too, to weed out broken ...
3 years, 11 months ago (2017-01-13 10:21:46 UTC) #8
MaxStepin
Need to change append() to push_back() in PNGImageReader.cpp
3 years, 10 months ago (2017-02-09 21:00:25 UTC) #9
scroggo_chromium
PTAL The latest patch set treats errors as errors, rather than attempting to correct them ...
3 years, 10 months ago (2017-02-17 23:00:57 UTC) #11
Noel Gordon
https://codereview.chromium.org/2618633004/diff/80001/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/2618633004/diff/80001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode42 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:42: #include <memory> Remove these two #include, PNGImageDecoder.h provides them ...
3 years, 10 months ago (2017-02-21 13:53:57 UTC) #12
scroggo_chromium
PTAL https://codereview.chromium.org/2618633004/diff/80001/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/2618633004/diff/80001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode42 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:42: #include <memory> On 2017/02/21 13:53:54, noel gordon wrote: ...
3 years, 10 months ago (2017-02-23 22:09:56 UTC) #14
Nate Chapin
https://codereview.chromium.org/2618633004/diff/100001/content/browser/loader/mime_sniffing_resource_handler.cc File content/browser/loader/mime_sniffing_resource_handler.cc (right): https://codereview.chromium.org/2618633004/diff/100001/content/browser/loader/mime_sniffing_resource_handler.cc#newcode48 content/browser/loader/mime_sniffing_resource_handler.cc:48: "text/html,application/xhtml+xml,application/xml;q=0.9,image/webp," I believe igrigorik recommended adding image/apng to this ...
3 years, 10 months ago (2017-02-23 22:12:52 UTC) #16
scroggo_chromium
https://codereview.chromium.org/2618633004/diff/80001/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/2618633004/diff/80001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode324 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:324: if (!initFrameBuffer(m_currentFrame)) On 2017/02/23 22:09:54, scroggo_chromium wrote: > On ...
3 years, 10 months ago (2017-02-24 14:57:25 UTC) #17
Nate Chapin
Accept header looks good, thanks! (I don't have OWNERS on those files though)
3 years, 10 months ago (2017-02-24 17:39:07 UTC) #18
Noel Gordon
https://codereview.chromium.org/2618633004/diff/80001/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/2618633004/diff/80001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode99 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:99: On 2017/02/23 22:09:53, scroggo_chromium wrote: > On 2017/02/21 13:53:55, ...
3 years, 9 months ago (2017-02-27 07:45:31 UTC) #19
scroggo_chromium
https://codereview.chromium.org/2618633004/diff/80001/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/2618633004/diff/80001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode270 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:270: On 2017/02/27 07:45:31, noel gordon wrote: > On 2017/02/23 ...
3 years, 9 months ago (2017-02-27 20:31:05 UTC) #20
Noel Gordon
https://codereview.chromium.org/2618633004/diff/80001/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/2618633004/diff/80001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode270 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:270: On 2017/02/27 20:31:05, scroggo_chromium wrote: > On 2017/02/27 07:45:31, ...
3 years, 9 months ago (2017-03-01 09:11:34 UTC) #21
Noel Gordon
https://codereview.chromium.org/2618633004/diff/80001/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/2618633004/diff/80001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode321 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:321: if (m_frameBufferCache[0].originalFrameRect().size() == IntSize(0, 0)) On 2017/02/27 20:31:05, scroggo_chromium ...
3 years, 9 months ago (2017-03-01 09:19:35 UTC) #22
Noel Gordon
https://codereview.chromium.org/2618633004/diff/80001/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/2618633004/diff/80001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode324 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:324: if (!initFrameBuffer(m_currentFrame)) On 2017/02/27 20:31:05, scroggo_chromium wrote: > On ...
3 years, 9 months ago (2017-03-01 09:26:50 UTC) #23
Noel Gordon
https://codereview.chromium.org/2618633004/diff/80001/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/2618633004/diff/80001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode326 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:326: On 2017/02/27 20:31:05, scroggo_chromium wrote: > On 2017/02/27 07:45:31, ...
3 years, 9 months ago (2017-03-01 11:29:46 UTC) #24
scroggo_chromium
PTAL. Rebased to ToT https://codereview.chromium.org/2618633004/diff/80001/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/2618633004/diff/80001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode270 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:270: On 2017/03/01 09:11:34, noel gordon ...
3 years, 9 months ago (2017-03-03 21:56:40 UTC) #25
Noel Gordon
https://codereview.chromium.org/2618633004/diff/80001/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/2618633004/diff/80001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode321 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:321: if (m_frameBufferCache[0].originalFrameRect().size() == IntSize(0, 0)) On 2017/03/03 21:56:40, scroggo_chromium ...
3 years, 9 months ago (2017-03-06 03:06:46 UTC) #30
Noel Gordon
https://codereview.chromium.org/2618633004/diff/220001/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/2618633004/diff/220001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode143 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:143: inline float pngFixedToFloat(png_fixed_point x) { Merge funk: remove this ...
3 years, 9 months ago (2017-03-06 05:21:48 UTC) #31
scroggo_chromium
https://codereview.chromium.org/2618633004/diff/80001/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/2618633004/diff/80001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode326 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:326: On 2017/03/06 03:06:45, noel gordon wrote: > > noel ...
3 years, 9 months ago (2017-03-07 20:25:38 UTC) #32
Noel Gordon
https://codereview.chromium.org/2618633004/diff/80001/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/2618633004/diff/80001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode326 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:326: On 2017/03/06 03:06:45, noel gordon wrote: > On 2017/03/03 ...
3 years, 9 months ago (2017-03-08 15:36:02 UTC) #33
Noel Gordon
Looking good, minor nits and comment fixes ... https://codereview.chromium.org/2618633004/diff/240001/third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTest.cpp File third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTest.cpp (right): https://codereview.chromium.org/2618633004/diff/240001/third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTest.cpp#newcode87 third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTest.cpp:87: const ...
3 years, 9 months ago (2017-03-08 16:26:15 UTC) #34
scroggo_chromium
https://codereview.chromium.org/2618633004/diff/220001/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/2618633004/diff/220001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode333 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:333: int y = rowIndex + frameRect.y(); On 2017/03/08 15:36:01, ...
3 years, 9 months ago (2017-03-08 20:53:23 UTC) #36
Noel Gordon
https://codereview.chromium.org/2618633004/diff/220001/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/2618633004/diff/220001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode333 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:333: int y = rowIndex + frameRect.y(); On 2017/03/08 20:53:21, ...
3 years, 9 months ago (2017-03-13 12:16:17 UTC) #37
scroggo_chromium
https://codereview.chromium.org/2618633004/diff/240001/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/2618633004/diff/240001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp#newcode486 third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:486: if (!index) On 2017/03/13 12:16:17, noel gordon wrote: > ...
3 years, 9 months ago (2017-03-13 13:50:08 UTC) #38
Noel Gordon
Code is looking solid, mostly nits ... https://codereview.chromium.org/2618633004/diff/300001/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/2618633004/diff/300001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageReader.cpp#newcode135 third_party/WebKit/Source/platform/image-decoders/png/PNGImageReader.cpp:135: const bool ...
3 years, 9 months ago (2017-03-13 16:00:45 UTC) #39
scroggo_chromium
https://codereview.chromium.org/2618633004/diff/300001/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/2618633004/diff/300001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageReader.cpp#newcode135 third_party/WebKit/Source/platform/image-decoders/png/PNGImageReader.cpp:135: const bool matchesIHDR = On 2017/03/13 16:00:45, noel gordon ...
3 years, 9 months ago (2017-03-13 16:37:10 UTC) #40
Noel Gordon
https://codereview.chromium.org/2618633004/diff/300001/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/2618633004/diff/300001/third_party/WebKit/Source/platform/image-decoders/png/PNGImageReader.h#newcode86 third_party/WebKit/Source/platform/image-decoders/png/PNGImageReader.h:86: bool frameIsFullyReceivedAtIndex(size_t index) const { On 2017/03/13 16:37:10, scroggo_chromium ...
3 years, 9 months ago (2017-03-13 16:56:06 UTC) #41
Noel Gordon
LGTM Nice work. With the changes we've made, bugs filed for follow-up work and tests, ...
3 years, 9 months ago (2017-03-13 17:03:29 UTC) #42
scroggo_chromium
On 2017/03/13 17:03:29, noel gordon wrote: > LGTM > > Nice work. With the changes ...
3 years, 9 months ago (2017-03-13 17:49:50 UTC) #44
scroggo_chromium
kinuko@, will you please look at the files in content/browser/loader/ ? I still need OWNERS ...
3 years, 9 months ago (2017-03-13 17:51:01 UTC) #46
kinuko
On 2017/03/13 17:51:01, scroggo_chromium wrote: > kinuko@, will you please look at the files in ...
3 years, 9 months ago (2017-03-14 13:35:24 UTC) #60
scroggo_chromium
https://codereview.chromium.org/2618633004/diff/380001/content/browser/loader/mime_sniffing_resource_handler.cc File content/browser/loader/mime_sniffing_resource_handler.cc (right): https://codereview.chromium.org/2618633004/diff/380001/content/browser/loader/mime_sniffing_resource_handler.cc#newcode50 content/browser/loader/mime_sniffing_resource_handler.cc:50: "image/apng,*/*;q=0.8"; japhet@ or kinuko@, could this change be causing ...
3 years, 9 months ago (2017-03-14 18:18:53 UTC) #65
scroggo
On 2017/03/14 18:18:53, scroggo_chromium wrote: > https://codereview.chromium.org/2618633004/diff/380001/content/browser/loader/mime_sniffing_resource_handler.cc > File content/browser/loader/mime_sniffing_resource_handler.cc (right): > > https://codereview.chromium.org/2618633004/diff/380001/content/browser/loader/mime_sniffing_resource_handler.cc#newcode50 > ...
3 years, 9 months ago (2017-03-14 19:32:49 UTC) #66
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2618633004/400001
3 years, 9 months ago (2017-03-14 19:36:37 UTC) #69
MaxStepin
On 2017/03/13 17:49:50, scroggo_chromium wrote: > On 2017/03/13 17:03:29, noel gordon wrote: > > LGTM ...
3 years, 9 months ago (2017-03-14 21:35:15 UTC) #70
commit-bot: I haz the power
Committed patchset #21 (id:400001) as https://chromium.googlesource.com/chromium/src/+/7d2b8c45afc9c0230410011293cc2e1dbb8943a7
3 years, 9 months ago (2017-03-14 21:37:39 UTC) #73
scroggo_chromium
3 years, 9 months ago (2017-03-15 15:18:25 UTC) #74
Message was sent while issue was closed.
On 2017/03/14 21:35:15, MaxStepin wrote:
> On 2017/03/13 17:49:50, scroggo_chromium wrote:
> > On 2017/03/13 17:03:29, noel gordon wrote:
> > > LGTM
> > > 
> > > Nice work.  With the changes we've made, bugs filed for follow-up work and
> > > tests, etc, you might want to look over the change description to check
it's
> > > up-to-date.  Perhaps add a line attributing the contributions of Joost and
> Max
> > > (since their work helped develop this patch, IIUC).
> > 
> > Done
> 
> BTW, what about createInterlaceBuffer() issue we discussed in
> http://crrev.com/2386453003
> Maybe you can file it as a separate bug for follow-up work? Otherwise we might
> forget about it.

Filed crbug.com/701779

Powered by Google App Engine
This is Rietveld 408576698