Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2490473005:
Pull up equivalent image decoding tests (Closed)
|
Created:
4 years, 1 month ago by joostouwerling Modified:
4 years, 1 month ago CC:
chromium-reviews, blink-reviews, jzern, skal, urvang Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPull up equivalent image decoding tests
Besides a small change for GIFImageDecoderTest::testProgressiveDecode (see
below), this is just a refactor. No behavior change, no new tests.
Pull up equivalent image decoding tests from WEBPImageDecoderTest and
GIFImageDecoderTest to ImageDecoderTestHelpers. These are the following tests:
- testRandomFrameDecode
- testRandomDecodeAfterClearFrameBufferCache
- testDecodeAfterReallocatingData
- testByteByByteSizeAvailable
- testProgressiveDecoding
For testProgressiveDecoding, a small change has been made. The tests for
repetition count before and after the progressive while loop are removed.
Therefore, an extra test is added in verifyRepetitionCount which tests the
repetition count of the same image.
Committed: https://crrev.com/5465208debdeb2e6f5e898c9d08768d0a833db02
Cr-Commit-Position: refs/heads/master@{#431568}
Patch Set 1 #Patch Set 2 : Add repetition count test for radient.gif #
Total comments: 12
Patch Set 3 : Fixed feedback on patch 1 and ran WebKit formnatting #
Total comments: 2
Patch Set 4 : Separate API for file and dir + file #
Total comments: 6
Patch Set 5 : Removed unnecessary header #
Messages
Total messages: 25 (13 generated)
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Pull up equivalent image decoding tests Besides a small change for GIFImageDecoderTest::testProgressiveDecode (see below), this is just a refactor. No behavior change, no new tests. Pull up equivalent image decoding tests from WEBPImageDecoderTest and GIFImageDecoderTest to ImageDecoderTestHelpers. These are the following tests: - testRandomFrameDecode - testRandomDecodeAfterClearFrameBufferCache - testDecodeAfterReallocatingData - testByteByByteSizeAvailable - testProgressiveDecoding For testProgressiveDecoding, a small change has been made. The tests for repetition count before and after the progressive while loop are removed. Therefore, an extra test is added in verifyRepetitionCount which tests the repetition count of the same image. ========== to ========== Pull up equivalent image decoding tests Besides a small change for GIFImageDecoderTest::testProgressiveDecode (see below), this is just a refactor. No behavior change, no new tests. Pull up equivalent image decoding tests from WEBPImageDecoderTest and GIFImageDecoderTest to ImageDecoderTestHelpers. These are the following tests: - testRandomFrameDecode - testRandomDecodeAfterClearFrameBufferCache - testDecodeAfterReallocatingData - testByteByByteSizeAvailable - testProgressiveDecoding For testProgressiveDecoding, a small change has been made. The tests for repetition count before and after the progressive while loop are removed. Therefore, an extra test is added in verifyRepetitionCount which tests the repetition count of the same image. ==========
joostouwerling@google.com changed reviewers: + scroggo@chromium.org, urvang@chromium.org
https://codereview.chromium.org/2490473005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoderTest.cpp (right): https://codereview.chromium.org/2490473005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoderTest.cpp:54: void testRepetitionCount(const char *dir, const char* file, An extra note about the repetition count test for GIF. The new test is different since it does not incrementally provide the data to the decoder, in between the tests for the repetition count. I don't think this should influence the reported repetition count in the parsing process, but maybe I'm missing something specific for GIF images. If it is easier to land this CL without sharing the progressiveDecode test, I can revert it for GIF, and change the GIF test in a different CL.
https://codereview.chromium.org/2490473005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp (right): https://codereview.chromium.org/2490473005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp:20: std::string dirStr(dir); Elsewhere in the file, we use String. For consistency, please continue to use String. https://codereview.chromium.org/2490473005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp:270: if (decoder->failed()) If you change EXPECT_FALSE to ASSERT_FALSE, this check is no longer needed. https://codereview.chromium.org/2490473005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp:283: ImageFrame* frame; Please move this to where it's used. No need for it outside of the for loops, where it's sometimes unitialized and sometimes pointing to something that no longer exists. https://codereview.chromium.org/2490473005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp:316: if (truncatedHashes[i] != progressiveHashes[i]) { I think this came up in your CL for APNG - this can just be ASSERT_EQ(truncatedHashes[i], progressiveHashes[i]); Then you don't need the variable match. https://codereview.chromium.org/2490473005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.h (right): https://codereview.chromium.org/2490473005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.h:17: const char* concatDirAndFile(const char *dir, const char *file); nit, here and elsewhere - * goes next to the type: const char* dir On another note, this method is only used by GIFImageDecoderTest, so maybe put it there instead? https://codereview.chromium.org/2490473005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoderTest.cpp (right): https://codereview.chromium.org/2490473005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoderTest.cpp:54: void testRepetitionCount(const char *dir, const char* file, On 2016/11/09 14:25:08, joostouwerling wrote: > An extra note about the repetition count test for GIF. The new test is different > since it does not incrementally provide the data to the decoder, in between the > tests for the repetition count. I don't think this should influence the reported > repetition count in the parsing process, but maybe I'm missing something > specific for GIF images. > > If it is easier to land this CL without sharing the progressiveDecode test, I > can revert it for GIF, and change the GIF test in a different CL. No, I think this is fine.
lgtm LGTM after any comments from scroggo@
https://codereview.chromium.org/2490473005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp (right): https://codereview.chromium.org/2490473005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp:20: std::string dirStr(dir); On 2016/11/09 14:38:37, scroggo_chromium wrote: > Elsewhere in the file, we use String. For consistency, please continue to use > String. Done. https://codereview.chromium.org/2490473005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp:270: if (decoder->failed()) On 2016/11/09 14:38:37, scroggo_chromium wrote: > If you change EXPECT_FALSE to ASSERT_FALSE, this check is no longer needed. Done. https://codereview.chromium.org/2490473005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp:283: ImageFrame* frame; On 2016/11/09 14:38:37, scroggo_chromium wrote: > Please move this to where it's used. No need for it outside of the for loops, > where it's sometimes unitialized and sometimes pointing to something that no > longer exists. Done. https://codereview.chromium.org/2490473005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp:316: if (truncatedHashes[i] != progressiveHashes[i]) { On 2016/11/09 14:38:37, scroggo_chromium wrote: > I think this came up in your CL for APNG - this can just be > > ASSERT_EQ(truncatedHashes[i], progressiveHashes[i]); > > Then you don't need the variable match. Done. https://codereview.chromium.org/2490473005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.h (right): https://codereview.chromium.org/2490473005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.h:17: const char* concatDirAndFile(const char *dir, const char *file); On 2016/11/09 14:38:37, scroggo_chromium wrote: > nit, here and elsewhere - * goes next to the type: > > const char* dir > > On another note, this method is only used by GIFImageDecoderTest, so maybe put > it there instead? Done.
https://codereview.chromium.org/2490473005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoderTest.cpp (right): https://codereview.chromium.org/2490473005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoderTest.cpp:52: return reinterpret_cast<const char*>(filePath.characters8()); I missed this in patch set 2, where I think we have a similar issue, even though we're using a different class: characters8() returns a pointer to memory internal to the StringBuilder filePath. When this method returns, filePath's destructor will get called. You're getting "lucky" when you run the test, and the memory hasn't been cleaned up, so it still looks like what you expect. (I say "lucky" because it'd be even luckier for it to crash, so you didn't accidentally check this in!) There's already a readFile method that concats a directory with a file. I suggest you make testProgressiveDecoding (and others) take a SharedBuffer parameter (possibly PassRefPtr<SharedBuffer> - I don't remember the current proper Blink way to pass a ref-counted object; I think PassRefPtr may have gone away...). To make the call sites simpler, you can make the SharedBuffer version private to ImageDecoderTestHelpers.cpp, and provide two public versions - one takes two parameters (dir, file), and the other takes one. *Or* you can have one version where the dir can be set to nullptr (possibly even as a default parameter, although then it would come last, which might be awkward since they'd be in reverse logical order). (Another version is the null dir version gets called by a public version that takes one param for the file name.)
Patchset #5 (id:100001) has been deleted
Patchset #4 (id:80001) has been deleted
https://codereview.chromium.org/2490473005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoderTest.cpp (right): https://codereview.chromium.org/2490473005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoderTest.cpp:52: return reinterpret_cast<const char*>(filePath.characters8()); On 2016/11/10 17:34:39, scroggo_chromium wrote: > I missed this in patch set 2, where I think we have a similar issue, even though > we're using a different class: > > characters8() returns a pointer to memory internal to the StringBuilder > filePath. When this method returns, filePath's destructor will get called. > You're getting "lucky" when you run the test, and the memory hasn't been cleaned > up, so it still looks like what you expect. (I say "lucky" because it'd be even > luckier for it to crash, so you didn't accidentally check this in!) > > There's already a readFile method that concats a directory with a file. I > suggest you make testProgressiveDecoding (and others) take a SharedBuffer > parameter (possibly PassRefPtr<SharedBuffer> - I don't remember the current > proper Blink way to pass a ref-counted object; I think PassRefPtr may have gone > away...). To make the call sites simpler, you can make the SharedBuffer version > private to ImageDecoderTestHelpers.cpp, and provide two public versions - one > takes two parameters (dir, file), and the other takes one. *Or* you can have one > version where the dir can be set to nullptr (possibly even as a default > parameter, although then it would come last, which might be awkward since they'd > be in reverse logical order). (Another version is the null dir version gets > called by a public version that takes one param for the file name.) Done. From what I've seen in e.g. [1] it is fine to pass a raw pointer to a SharedBuffer. This is also in line with the "modern" C++ approach that a naked pointer implies that you shouldn't take ownership of it. [1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image...
The CQ bit was checked by joostouwerling@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
A couple of nits, mostly around parameters where we always use the defaults. It doesn't seem terrible to have them, but if we never supply another value (maybe you intend to in APNG tests?) it makes me question their necessity, and they add a bit of clutter. LGTM https://codereview.chromium.org/2490473005/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.h (right): https://codereview.chromium.org/2490473005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.h:41: size_t skippingStep = 5); I may have overlooked something, but it looks like we always use the default? Why not remove the parameter and make this a constant inside the method? (Same for the other three with skippingStep) https://codereview.chromium.org/2490473005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.h:72: size_t increment = 1); It also looks like we also use the default for both of these? Why not remove the parameter? https://codereview.chromium.org/2490473005/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoderTest.cpp (right): https://codereview.chromium.org/2490473005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoderTest.cpp:40: #include "wtf/text/StringBuilder.h" I think this is no longer needed?
https://codereview.chromium.org/2490473005/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.h (right): https://codereview.chromium.org/2490473005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.h:41: size_t skippingStep = 5); On 2016/11/11 13:18:42, scroggo_chromium wrote: > I may have overlooked something, but it looks like we always use the default? > Why not remove the parameter and make this a constant inside the method? (Same > for the other three with skippingStep) As you guessed correctly, I have a different value for APNG tests :) https://codereview.chromium.org/2490473005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.h:72: size_t increment = 1); On 2016/11/11 13:18:42, scroggo_chromium wrote: > It also looks like we also use the default for both of these? Why not remove the > parameter? See above. https://codereview.chromium.org/2490473005/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoderTest.cpp (right): https://codereview.chromium.org/2490473005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoderTest.cpp:40: #include "wtf/text/StringBuilder.h" On 2016/11/11 13:18:42, scroggo_chromium wrote: > I think this is no longer needed? Done.
The CQ bit was checked by joostouwerling@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from urvang@chromium.org, scroggo@chromium.org Link to the patchset: https://codereview.chromium.org/2490473005/#ps140001 (title: "Removed unnecessary header")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Pull up equivalent image decoding tests Besides a small change for GIFImageDecoderTest::testProgressiveDecode (see below), this is just a refactor. No behavior change, no new tests. Pull up equivalent image decoding tests from WEBPImageDecoderTest and GIFImageDecoderTest to ImageDecoderTestHelpers. These are the following tests: - testRandomFrameDecode - testRandomDecodeAfterClearFrameBufferCache - testDecodeAfterReallocatingData - testByteByByteSizeAvailable - testProgressiveDecoding For testProgressiveDecoding, a small change has been made. The tests for repetition count before and after the progressive while loop are removed. Therefore, an extra test is added in verifyRepetitionCount which tests the repetition count of the same image. ========== to ========== Pull up equivalent image decoding tests Besides a small change for GIFImageDecoderTest::testProgressiveDecode (see below), this is just a refactor. No behavior change, no new tests. Pull up equivalent image decoding tests from WEBPImageDecoderTest and GIFImageDecoderTest to ImageDecoderTestHelpers. These are the following tests: - testRandomFrameDecode - testRandomDecodeAfterClearFrameBufferCache - testDecodeAfterReallocatingData - testByteByByteSizeAvailable - testProgressiveDecoding For testProgressiveDecoding, a small change has been made. The tests for repetition count before and after the progressive while loop are removed. Therefore, an extra test is added in verifyRepetitionCount which tests the repetition count of the same image. ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Pull up equivalent image decoding tests Besides a small change for GIFImageDecoderTest::testProgressiveDecode (see below), this is just a refactor. No behavior change, no new tests. Pull up equivalent image decoding tests from WEBPImageDecoderTest and GIFImageDecoderTest to ImageDecoderTestHelpers. These are the following tests: - testRandomFrameDecode - testRandomDecodeAfterClearFrameBufferCache - testDecodeAfterReallocatingData - testByteByByteSizeAvailable - testProgressiveDecoding For testProgressiveDecoding, a small change has been made. The tests for repetition count before and after the progressive while loop are removed. Therefore, an extra test is added in verifyRepetitionCount which tests the repetition count of the same image. ========== to ========== Pull up equivalent image decoding tests Besides a small change for GIFImageDecoderTest::testProgressiveDecode (see below), this is just a refactor. No behavior change, no new tests. Pull up equivalent image decoding tests from WEBPImageDecoderTest and GIFImageDecoderTest to ImageDecoderTestHelpers. These are the following tests: - testRandomFrameDecode - testRandomDecodeAfterClearFrameBufferCache - testDecodeAfterReallocatingData - testByteByByteSizeAvailable - testProgressiveDecoding For testProgressiveDecoding, a small change has been made. The tests for repetition count before and after the progressive while loop are removed. Therefore, an extra test is added in verifyRepetitionCount which tests the repetition count of the same image. Committed: https://crrev.com/5465208debdeb2e6f5e898c9d08768d0a833db02 Cr-Commit-Position: refs/heads/master@{#431568} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/5465208debdeb2e6f5e898c9d08768d0a833db02 Cr-Commit-Position: refs/heads/master@{#431568} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
