Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
813943003:
Fix handling of broken GIFs with weird frame sizes (Closed)
|
Created:
5 years, 11 months ago by Alpha Left Google Modified:
5 years, 11 months ago CC:
blink-reviews Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionFix handling of broken GIFs with weird frame sizes
Code didn't handle well if a GIF frame has dimension greater than the
"screen" dimension. This will break deferred image decoding.
This change reports the size as final only when the first frame is
encountered.
Added a test to verify this behavior. Frame size reported by the decoder
should be constant.
BUG=437651
R=pkasting@chromium.org, senorblanco@chromium.org
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=188423
Patch Set 1 #Patch Set 2 : don't report error if subsequent frame has larger size #Patch Set 3 : nits #
Total comments: 18
Patch Set 4 : comments #
Total comments: 6
Patch Set 5 : comments #Patch Set 6 : comments #
Total comments: 1
Patch Set 7 : commens #
Total comments: 2
Patch Set 8 : comments #
Messages
Total messages: 19 (4 generated)
hclam@chromium.org changed reviewers: + pkasting@chromium.org
https://codereview.chromium.org/813943003/diff/40001/Source/platform/image-de... File Source/platform/image-decoders/gif/GIFImageDecoderTest.cpp (right): https://codereview.chromium.org/813943003/diff/40001/Source/platform/image-de... Source/platform/image-decoders/gif/GIFImageDecoderTest.cpp:507: const size_t fullLength = fullData->size(); Nit: I'd probably omit this temp and just do the call below, but up to you. https://codereview.chromium.org/813943003/diff/40001/Source/platform/image-de... Source/platform/image-decoders/gif/GIFImageDecoderTest.cpp:513: const size_t increment = 1; If you're not going to use a random increment size or something here, I'd eliminate this and just use "++i" in the loop below. https://codereview.chromium.org/813943003/diff/40001/Source/platform/image-de... Source/platform/image-decoders/gif/GIFImageDecoderTest.cpp:520: frameSize = decoder->decodedSize(); It seems like this should just continue, since this assignment makes the rest of the loop vacuous. https://codereview.chromium.org/813943003/diff/40001/Source/platform/image-de... Source/platform/image-decoders/gif/GIFImageDecoderTest.cpp:527: break; Why not just ASSERT_EQ above and omit this conditional? https://codereview.chromium.org/813943003/diff/40001/Source/platform/image-de... File Source/platform/image-decoders/gif/GIFImageReader.cpp (right): https://codereview.chromium.org/813943003/diff/40001/Source/platform/image-de... Source/platform/image-decoders/gif/GIFImageReader.cpp:631: // Work around broken GIF file where the first frame has size greater than the Nit: files I also wouldn't mind wrapping the comments to 80 chars. https://codereview.chromium.org/813943003/diff/40001/Source/platform/image-de... Source/platform/image-decoders/gif/GIFImageReader.cpp:632: // "screen size". What about when subsequent frames have larger size? Should we give a justification for why we don't care about that? (Is that even possible? I don't really have the details of the GIF format paged in, but that's the only reason I can think of for why we used to have the std::max() calls on old lines 672-3, which you've removed now.) https://codereview.chromium.org/813943003/diff/40001/Source/platform/image-de... Source/platform/image-decoders/gif/GIFImageReader.cpp:636: m_screenWidth = std::max(m_screenWidth, xOffset + width); This is a behavior change from the previous code in two ways: (1) We don't unconditionally do this for GIF87a files. I'm not clear on what the consequences of this are. (2) We now respect the x/y offsets, where we didn't before. This seems possibly more correct, at least when e.g. width < m_screenWidth < (width + xOffset), which wouldn't have triggered a resize before but probably should have. But I'd like to understand better why the code reset the offsets to begin with, and/or whether e.g. Firefox still does this. https://codereview.chromium.org/813943003/diff/40001/Source/platform/image-de... Source/platform/image-decoders/gif/GIFImageReader.cpp:640: if (!m_sizeFinal && m_client && !m_client->setSize(m_screenWidth, m_screenHeight)) Is it possible to have m_client null here but have it be non-null later? I hope not, since in that case I don't think we'll ever inform it of the right size. https://codereview.chromium.org/813943003/diff/40001/Source/platform/image-de... File Source/platform/image-decoders/gif/GIFImageReader.h (right): https://codereview.chromium.org/813943003/diff/40001/Source/platform/image-de... Source/platform/image-decoders/gif/GIFImageReader.h:355: bool m_sizeFinal; Nit: Please comment about what this means. I might name this m_sentSizeToClient, that seems possibly more descriptive.
https://codereview.chromium.org/813943003/diff/40001/Source/platform/image-de... File Source/platform/image-decoders/gif/GIFImageDecoderTest.cpp (right): https://codereview.chromium.org/813943003/diff/40001/Source/platform/image-de... Source/platform/image-decoders/gif/GIFImageDecoderTest.cpp:507: const size_t fullLength = fullData->size(); On 2015/01/08 06:56:18, Peter Kasting wrote: > Nit: I'd probably omit this temp and just do the call below, but up to you. Done. https://codereview.chromium.org/813943003/diff/40001/Source/platform/image-de... Source/platform/image-decoders/gif/GIFImageDecoderTest.cpp:513: const size_t increment = 1; On 2015/01/08 06:56:18, Peter Kasting wrote: > If you're not going to use a random increment size or something here, I'd > eliminate this and just use "++i" in the loop below. Done. https://codereview.chromium.org/813943003/diff/40001/Source/platform/image-de... Source/platform/image-decoders/gif/GIFImageDecoderTest.cpp:520: frameSize = decoder->decodedSize(); On 2015/01/08 06:56:18, Peter Kasting wrote: > It seems like this should just continue, since this assignment makes the rest of > the loop vacuous. Done. https://codereview.chromium.org/813943003/diff/40001/Source/platform/image-de... Source/platform/image-decoders/gif/GIFImageDecoderTest.cpp:527: break; On 2015/01/08 06:56:18, Peter Kasting wrote: > Why not just ASSERT_EQ above and omit this conditional? Done. https://codereview.chromium.org/813943003/diff/40001/Source/platform/image-de... File Source/platform/image-decoders/gif/GIFImageReader.cpp (right): https://codereview.chromium.org/813943003/diff/40001/Source/platform/image-de... Source/platform/image-decoders/gif/GIFImageReader.cpp:631: // Work around broken GIF file where the first frame has size greater than the On 2015/01/08 06:56:18, Peter Kasting wrote: > Nit: files > > I also wouldn't mind wrapping the comments to 80 chars. Done. https://codereview.chromium.org/813943003/diff/40001/Source/platform/image-de... Source/platform/image-decoders/gif/GIFImageReader.cpp:632: // "screen size". On 2015/01/08 06:56:18, Peter Kasting wrote: > What about when subsequent frames have larger size? Should we give a > justification for why we don't care about that? (Is that even possible? I > don't really have the details of the GIF format paged in, but that's the only > reason I can think of for why we used to have the std::max() calls on old lines > 672-3, which you've removed now.) I added the std::max() code in 672 and 673 in the last refactoring. Not realizing that it didn't have any real effect. m_screenWidth and m_screenHeight is used to set up the canvas. If subsequent frames have larger sizes they don't change the size of canvas. In fact after notifying the client of the canvas size, m_screenWidth and m_screenHeight are used only to correct frame size for corrupted frames. Once the canvas is set up a subsequent larger frame will be cropped to the canvas. This is done in GIFImageDecoder. That is why we don't care about this case here. https://codereview.chromium.org/813943003/diff/40001/Source/platform/image-de... Source/platform/image-decoders/gif/GIFImageReader.cpp:636: m_screenWidth = std::max(m_screenWidth, xOffset + width); On 2015/01/08 06:56:18, Peter Kasting wrote: > This is a behavior change from the previous code in two ways: > > (1) We don't unconditionally do this for GIF87a files. I'm not clear on what > the consequences of this are. The behavior changes unless the image is GIF87 and it is incorrectly formed such that first frame has size smaller than the canvas size. I've added the condition of GIF87 back. > (2) We now respect the x/y offsets, where we didn't before. This seems possibly > more correct, at least when e.g. width < m_screenWidth < (width + xOffset), > which wouldn't have triggered a resize before but probably should have. But I'd > like to understand better why the code reset the offsets to begin with, and/or > whether e.g. Firefox still does this. FireFox still resets the offsets to 0 and width/height = m_screenWidth/m_screenHeight. My guess for why it used to reset to 0 is for simplicity. But I don't think it was correct. I verified this code change with a corrupted GIF file, which FireFox could not display. But with this change Chrome can decode correctly. https://codereview.chromium.org/813943003/diff/40001/Source/platform/image-de... Source/platform/image-decoders/gif/GIFImageReader.cpp:640: if (!m_sizeFinal && m_client && !m_client->setSize(m_screenWidth, m_screenHeight)) On 2015/01/08 06:56:18, Peter Kasting wrote: > Is it possible to have m_client null here but have it be non-null later? I hope > not, since in that case I don't think we'll ever inform it of the right size. m_client is always non null. We shouldn't need to check for m_client but that can be a later cleanup. https://codereview.chromium.org/813943003/diff/40001/Source/platform/image-de... File Source/platform/image-decoders/gif/GIFImageReader.h (right): https://codereview.chromium.org/813943003/diff/40001/Source/platform/image-de... Source/platform/image-decoders/gif/GIFImageReader.h:355: bool m_sizeFinal; On 2015/01/08 06:56:18, Peter Kasting wrote: > Nit: Please comment about what this means. > > I might name this m_sentSizeToClient, that seems possibly more descriptive. Done.
So why do we only enlarge the image on the first frame, but not if subsequent frames are larger? It seems a bit inconsistent. I'd think we'd want to do it on all frames or none. https://codereview.chromium.org/813943003/diff/60001/Source/platform/image-de... File Source/platform/image-decoders/gif/GIFImageReader.cpp (right): https://codereview.chromium.org/813943003/diff/60001/Source/platform/image-de... Source/platform/image-decoders/gif/GIFImageReader.cpp:440: // Note that size is final only when the first frame is encountered. Nit: "Note that we don't inform the client of the size yet, as it might change after we read the first frame's image header." https://codereview.chromium.org/813943003/diff/60001/Source/platform/image-de... Source/platform/image-decoders/gif/GIFImageReader.cpp:631: // Work around broken GIF file where the first frame has size greater than the "screen size". Nit: "Work around broken GIF files where the first frame doesn't fit within the screen size." https://codereview.chromium.org/813943003/diff/60001/Source/platform/image-de... Source/platform/image-decoders/gif/GIFImageReader.cpp:633: && (m_screenHeight < yOffset + height || m_screenWidth < xOffset + width || m_version == 87)) { After looking at this, I think we should just remove everything but the currentFrameIsFirstFrame() check. The rest of the conditional has no functional effect on how the code works (which wasn't true the way the code was previously written).
> So why do we only enlarge the image on the first frame, but not if subsequent frames are larger? It seems a bit inconsistent. I'd think we'd want to do it on all frames or none. That is because the decoding infrastructure assumes the image size is constant. This is true for all correctly formed images. This requirement is needed to enable deferred image decoding as well. We can delay getting the frame size for GIF to handle some corruptions. If later frames have larger sizes then they will be cropped. If we change the canvas size then rendering will crash. https://codereview.chromium.org/813943003/diff/60001/Source/platform/image-de... File Source/platform/image-decoders/gif/GIFImageReader.cpp (right): https://codereview.chromium.org/813943003/diff/60001/Source/platform/image-de... Source/platform/image-decoders/gif/GIFImageReader.cpp:440: // Note that size is final only when the first frame is encountered. On 2015/01/12 22:40:45, Peter Kasting wrote: > Nit: "Note that we don't inform the client of the size yet, as it might change > after we read the first frame's image header." Done. https://codereview.chromium.org/813943003/diff/60001/Source/platform/image-de... Source/platform/image-decoders/gif/GIFImageReader.cpp:631: // Work around broken GIF file where the first frame has size greater than the "screen size". On 2015/01/12 22:40:45, Peter Kasting wrote: > Nit: "Work around broken GIF files where the first frame doesn't fit within the > screen size." Done. https://codereview.chromium.org/813943003/diff/60001/Source/platform/image-de... Source/platform/image-decoders/gif/GIFImageReader.cpp:633: && (m_screenHeight < yOffset + height || m_screenWidth < xOffset + width || m_version == 87)) { On 2015/01/12 22:40:45, Peter Kasting wrote: > After looking at this, I think we should just remove everything but the > currentFrameIsFirstFrame() check. The rest of the conditional has no functional > effect on how the code works (which wasn't true the way the code was previously > written). Sure that seems good to me as well.
On 2015/01/12 22:48:22, Alpha wrote: > > So why do we only enlarge the image on the first frame, but not if subsequent > frames are larger? It seems a bit inconsistent. I'd think we'd want to do it > on all frames or none. > > That is because the decoding infrastructure assumes the image size is constant. > This is true for all correctly formed images. This requirement is needed to > enable deferred image decoding as well. > > We can delay getting the frame size for GIF to handle some corruptions. If later > frames have larger sizes then they will be cropped. If we change the canvas size > then rendering will crash. So in other words, it's basically a technical reason why we can't enlarge on later frames, whereas for the first frame, where we can do either, we think it's more correct to enlarge the frame. (Any particular reason why?) We should probably modify the comments here to note this, then? Something like "Some GIF files have frames that don't fit in the specified overall image size. For the first frame, we can simply enlarge the image size to allow the frame to be visible. We can't do this on subsequent frames because the rest of the decoding infrastructure assumes the image size won't change as we continue decoding, so any subsequent frames that are even larger will be cropped. Luckily, handling just the first frame is sufficient to deal with most cases, e.g. ones where the image size is erroneously set to zero, since usually the first frame completely fills the image." I wonder if it would make sense to allow the decoding infrastructure to handle the image size changing later on.
On 2015/01/12 22:56:00, Peter Kasting wrote: > On 2015/01/12 22:48:22, Alpha wrote: > > > So why do we only enlarge the image on the first frame, but not if > subsequent > > frames are larger? It seems a bit inconsistent. I'd think we'd want to do it > > on all frames or none. > > > > That is because the decoding infrastructure assumes the image size is > constant. > > This is true for all correctly formed images. This requirement is needed to > > enable deferred image decoding as well. > > > > We can delay getting the frame size for GIF to handle some corruptions. If > later > > frames have larger sizes then they will be cropped. If we change the canvas > size > > then rendering will crash. > > So in other words, it's basically a technical reason why we can't enlarge on > later frames, whereas for the first frame, where we can do either, we think it's > more correct to enlarge the frame. (Any particular reason why?) > That is correct. I think I should be clear this change doesn't change the outcome in the case when first frame size > "screen size" (excluding the change of adding the offset to the dimension). This change aims at fixing corrupted GIFs that crashes under impl side painting by making sure GIFImageDecoder sets the canvas size once only. Either of the behavior is correct as you said. > We should probably modify the comments here to note this, then? Something like > "Some GIF files have frames that don't fit in the specified overall image size. > For the first frame, we can simply enlarge the image size to allow the frame to > be visible. We can't do this on subsequent frames because the rest of the > decoding infrastructure assumes the image size won't change as we continue > decoding, so any subsequent frames that are even larger will be cropped. > Luckily, handling just the first frame is sufficient to deal with most cases, > e.g. ones where the image size is erroneously set to zero, since usually the > first frame completely fills the image." I've added the comments to the code. > I wonder if it would make sense to allow the decoding infrastructure to handle > the image size changing later on. If there is a real use case then we should do it. Not sure about the added complexity for a rare corner case.
LGTM https://codereview.chromium.org/813943003/diff/100001/Source/platform/image-d... File Source/platform/image-decoders/gif/GIFImageReader.cpp (right): https://codereview.chromium.org/813943003/diff/100001/Source/platform/image-d... Source/platform/image-decoders/gif/GIFImageReader.cpp:440: // Note that we don't inform the client of the size yet, as it might change after we read the first frame's image header. Nit: I still think all the comments in this change ought to be wrapped at 80 columns for readability.
The CQ bit was checked by hclam@chromium.org
On 2015/01/13 01:47:33, Peter Kasting wrote: > LGTM > > https://codereview.chromium.org/813943003/diff/100001/Source/platform/image-d... > File Source/platform/image-decoders/gif/GIFImageReader.cpp (right): > > https://codereview.chromium.org/813943003/diff/100001/Source/platform/image-d... > Source/platform/image-decoders/gif/GIFImageReader.cpp:440: // Note that we don't > inform the client of the size yet, as it might change after we read the first > frame's image header. > Nit: I still think all the comments in this change ought to be wrapped at 80 > columns for readability. Fixed.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/813943003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: blink_presubmit on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/2...)
hclam@chromium.org changed reviewers: + senorblanco@chromium.org
+senorblanco for owner approval.
LGTM https://codereview.chromium.org/813943003/diff/120001/Source/platform/image-d... File Source/platform/image-decoders/gif/GIFImageDecoderTest.cpp (right): https://codereview.chromium.org/813943003/diff/120001/Source/platform/image-d... Source/platform/image-decoders/gif/GIFImageDecoderTest.cpp:505: RefPtr<SharedBuffer> fullData = readFile("/Source/platform/image-decoders/testing/first-frame-has-greater-size-than-screen-size.gif"); Not new to this CL, but we should probably shorten these paths or make them relative to the file somehoow. At a minimum, we should put the common prefix in a single string at the top of the file, IMHO.
https://codereview.chromium.org/813943003/diff/120001/Source/platform/image-d... File Source/platform/image-decoders/gif/GIFImageDecoderTest.cpp (right): https://codereview.chromium.org/813943003/diff/120001/Source/platform/image-d... Source/platform/image-decoders/gif/GIFImageDecoderTest.cpp:505: RefPtr<SharedBuffer> fullData = readFile("/Source/platform/image-decoders/testing/first-frame-has-greater-size-than-screen-size.gif"); On 2015/01/14 22:17:02, Stephen White wrote: > Not new to this CL, but we should probably shorten these paths or make them > relative to the file somehoow. At a minimum, we should put the common prefix in > a single string at the top of the file, IMHO. Done.
Message was sent while issue was closed.
Committed patchset #8 (id:140001) manually as 188423 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
