|
|
Created:
4 years, 9 months ago by urvang Modified:
4 years, 9 months ago CC:
chromium-reviews, shans, rjwright, blink-reviews-animation_chromium.org, darktears, blink-reviews, kinuko+watch, Eric Willigers Base URL:
https://chromium.googlesource.com/chromium/src.git@test_file Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionGIF decoding: Add a unit test for repetition count
Also add a test to BitmapImageTest to ensure we loop one
extra time when animating.
BUG=592735
Committed: https://crrev.com/a45163dc68252ce3f1958ab58b06a765e652eb70
Cr-Commit-Position: refs/heads/master@{#380800}
Patch Set 1 #Patch Set 2 : Revert code change; test existing behavior instead #
Total comments: 2
Patch Set 3 : Add a test in BitmapImage to make sure we loop one extra time #
Total comments: 2
Patch Set 4 : decode all frames #
Messages
Total messages: 38 (15 generated)
urvang@chromium.org changed reviewers: + noel@chromium.org
On 2016/03/08 00:47:12, urvang wrote: > Note: I had added a note about 'repetitionCount()' method earlier: http://crrev.com/284043003, but that got erroneously removed I think: https://codereview.chromium.org/985583002/diff/120001/Source/platform/graphic... So, I've added the comment back. Looks like that patch didn't remove the comment it just moved it into ImageAnimation.h [1] [1] https://codereview.chromium.org/985583002/patch/120001/130013 Also, Peter would have a better idea about GIF loop counts and their interpretation, and so might be the appropriate reviewer for this patch.
Description was changed from ========== GIF decoding: Fix repetition count. GIF was looping once more than it should. This was because: - BitmapImage.cpp loops an animation _one more than_ repetitionCount() as mentioned in the note here: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... (This is for simplifying the implementation). - This meant that repetitionCount() should return _one less than_ actual repetition count stored in the file. WebP handles this correctly: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... But GIF did not. Note: I had added a note about 'repetitionCount()' method earlier: http://crrev.com/284043003, but that got erroneously removed I think: https://codereview.chromium.org/985583002/diff/120001/Source/platform/graphic... So, I've added the comment back. Also, added a test. BUG=592735 ========== to ========== GIF decoding: Fix repetition count GIF was looping once more than it should. This was because: - BitmapImage.cpp loops an animation _one more than_ repetitionCount() as mentioned in the note here: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... (This is for simplifying the implementation). - This meant that repetitionCount() should return _one less than_ actual repetition count stored in the file. WebP handles this correctly: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... But GIF did not. Note: I had added a note about 'repetitionCount()' method earlier: http://crrev.com/284043003, but that got erroneously removed I think: https://codereview.chromium.org/985583002/diff/120001/Source/platform/graphic... So, I've added the comment back. Also, added a test. BUG=592735 ==========
On 2016/03/08 12:17:55, noel gordon wrote: > On 2016/03/08 00:47:12, urvang wrote: > > > Note: I had added a note about 'repetitionCount()' method earlier: > http://crrev.com/284043003, but that got erroneously removed I think: > https://codereview.chromium.org/985583002/diff/120001/Source/platform/graphic... > So, I've added the comment back. > > Looks like that patch didn't remove the comment it just moved it into > ImageAnimation.h [1] > [1] https://codereview.chromium.org/985583002/patch/120001/130013 > What was moved is the comment at the top of the file. I'm talking about the comment near repetitionCount() method declaration. > Also, Peter would have a better idea about GIF loop counts and their > interpretation, and so might be the appropriate reviewer for this patch. Sure, let me add him as a reviewer.
urvang@chromium.org changed reviewers: + pkasting@chromium.org
@Peter: Noel suggested that you would know better about this.
Replied on bug. Even if we did want to change this, I don't think this is the right way; the right way would be to change the meaning of the repetition count machinery so it didn't have the "automatically add one" behavior. We should also worry about how animated WebP behaves when thinking about all this.
On 2016/03/08 21:29:41, Peter Kasting wrote: > Replied on bug. Even if we did want to change this, I don't think this is the > right way; the right way would be to change the meaning of the repetition count > machinery so it didn't have the "automatically add one" behavior. Yes, changing the behavior in BitmapImage.cpp is ideal. (My understanding was that the current behavior in animation machinery was intentional, to avoid some special casing for single-loop etc as mentioned in this comment: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit.... But there's likely a work-around for that). But yes, let's discuss and get agreement on the bug first. > > We should also worry about how animated WebP behaves when thinking about all > this. Animated WebP behaves same as how GIF would behave *after* this patch, as mentioned in the description.
Sounds like we should probably close this.
Description was changed from ========== GIF decoding: Fix repetition count GIF was looping once more than it should. This was because: - BitmapImage.cpp loops an animation _one more than_ repetitionCount() as mentioned in the note here: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... (This is for simplifying the implementation). - This meant that repetitionCount() should return _one less than_ actual repetition count stored in the file. WebP handles this correctly: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... But GIF did not. Note: I had added a note about 'repetitionCount()' method earlier: http://crrev.com/284043003, but that got erroneously removed I think: https://codereview.chromium.org/985583002/diff/120001/Source/platform/graphic... So, I've added the comment back. Also, added a test. BUG=592735 ========== to ========== GIF decoding: Add a unit test for repetition count BUG= ==========
On 2016/03/10 09:47:57, Peter Kasting wrote: > Sounds like we should probably close this. Changed this to a unit test for existing behavior instead.
Technically this just checks that we read the correct value out of the GIF, not that we actually show the loop the right number of times. But if we don't already have a test for this, LGTM, with bonus points if you find a way to test that we actually show the loop 3 times with a repetition count of 2.
https://codereview.chromium.org/1769343002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoderTest.cpp (right): https://codereview.chromium.org/1769343002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoderTest.cpp:516: Are there 2, or 3, frames in the test GIF image? Seems to me that there were 3 frames when I looked at test image in the Mac OSX image viewer tool. Is that tool wrong?
Description was changed from ========== GIF decoding: Add a unit test for repetition count BUG= ========== to ========== GIF decoding: Add a unit test for repetition count BUG= TODO(urvang): fill in the bug number. ==========
On 2016/03/10 23:54:50, Peter Kasting wrote: > Technically this just checks that we read the correct value out of the GIF, not > that we actually show the loop the right number of times. Yup. But if we don't > already have a test for this, LGTM, Yeah, the existing tests only had files with 'cAnimationLoopInfinite' and 'cAnimationNone'. with bonus points if you find a way to test > that we actually show the loop 3 times with a repetition count of 2. There you go :)
Description was changed from ========== GIF decoding: Add a unit test for repetition count BUG= TODO(urvang): fill in the bug number. ========== to ========== GIF decoding: Add a unit test for repetition count Also add a test to BitmapImageTest to ensure we loop one extra time when animating. BUG=592735 ==========
https://codereview.chromium.org/1769343002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoderTest.cpp (right): https://codereview.chromium.org/1769343002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoderTest.cpp:516: On 2016/03/11 00:39:50, noel gordon wrote: > Are there 2, or 3, frames in the test GIF image? Seems to me that there were 3 > frames when I looked at test image in the Mac OSX image viewer tool. Is that > tool wrong? Yes, there are 3 frames, with repetition count of 2. We are just testing the repetition count here.
On 2016/03/11 01:13:50, urvang wrote: > On 2016/03/11 00:39:50, noel gordon wrote: > > Are there 2, or 3, frames in the test GIF image? Seems to me that there were > 3 > > frames when I looked at test image in the Mac OSX image viewer tool. Is that > > tool wrong? > > Yes, there are 3 frames, with repetition count of 2. OK, I suppose for completeness you should try to decode all 3 frames?
LGTM (again) https://codereview.chromium.org/1769343002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/BitmapImageTest.cpp (right): https://codereview.chromium.org/1769343002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/BitmapImageTest.cpp:187: for (int repeat = 0; repeat < expectedRepetitionCount + 1; ++repeat) { Nit: repeat <= expectedRepetitionCount
Description was changed from ========== GIF decoding: Add a unit test for repetition count Also add a test to BitmapImageTest to ensure we loop one extra time when animating. BUG=592735 ========== to ========== GIF decoding: Add a unit test for repetition count Also add a test to BitmapImageTest to ensure we loop one extra time when animating. BUG=592735 ==========
Description was changed from ========== GIF decoding: Add a unit test for repetition count Also add a test to BitmapImageTest to ensure we loop one extra time when animating. BUG=592735 ========== to ========== GIF decoding: Add a unit test for repetition count Also add a test to BitmapImageTest to ensure we loop one extra time when animating. BUG=592735 ==========
On 2016/03/11 01:23:46, noel gordon wrote: > On 2016/03/11 01:13:50, urvang wrote: > > > On 2016/03/11 00:39:50, noel gordon wrote: > > > Are there 2, or 3, frames in the test GIF image? Seems to me that there > were > > 3 > > > frames when I looked at test image in the Mac OSX image viewer tool. Is > that > > > tool wrong? > > > > Yes, there are 3 frames, with repetition count of 2. > > OK, I suppose for completeness you should try to decode all 3 frames? Ahh, indeed. Done.
https://codereview.chromium.org/1769343002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/BitmapImageTest.cpp (right): https://codereview.chromium.org/1769343002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/BitmapImageTest.cpp:187: for (int repeat = 0; repeat < expectedRepetitionCount + 1; ++repeat) { On 2016/03/11 01:23:46, Peter Kasting wrote: > Nit: repeat <= expectedRepetitionCount Done
The CQ bit was checked by urvang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/1769343002/#ps60001 (title: "decode all frames")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1769343002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1769343002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
urvang@chromium.org changed reviewers: + kbr@chromium.org
@Ken: Need LGTM stamp for third_party/WebKit/Source/platform/graphics/BitmapImageTest.cpp Thanks!
LGTM.
The CQ bit was checked by noel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1769343002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1769343002/60001
Message was sent while issue was closed.
Description was changed from ========== GIF decoding: Add a unit test for repetition count Also add a test to BitmapImageTest to ensure we loop one extra time when animating. BUG=592735 ========== to ========== GIF decoding: Add a unit test for repetition count Also add a test to BitmapImageTest to ensure we loop one extra time when animating. BUG=592735 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== GIF decoding: Add a unit test for repetition count Also add a test to BitmapImageTest to ensure we loop one extra time when animating. BUG=592735 ========== to ========== GIF decoding: Add a unit test for repetition count Also add a test to BitmapImageTest to ensure we loop one extra time when animating. BUG=592735 Committed: https://crrev.com/a45163dc68252ce3f1958ab58b06a765e652eb70 Cr-Commit-Position: refs/heads/master@{#380800} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/a45163dc68252ce3f1958ab58b06a765e652eb70 Cr-Commit-Position: refs/heads/master@{#380800} |