FAQ
https://codereview.appspot.com/7602045/diff/21001/src/pkg/image/gif/reader_test.go
File src/pkg/image/gif/reader_test.go (right):

https://codereview.appspot.com/7602045/diff/21001/src/pkg/image/gif/reader_test.go#newcode8
src/pkg/image/gif/reader_test.go:8: // a gif with two frames, where the
second frame has been tweaked
// bigGIF is a GIF image with two frames, etc.

https://codereview.appspot.com/7602045/diff/21001/src/pkg/image/gif/reader_test.go#newcode9
src/pkg/image/gif/reader_test.go:9: // to have a width/height that's
higher
Higher than what?

https://codereview.appspot.com/7602045/diff/21001/src/pkg/image/gif/reader_test.go#newcode10
src/pkg/image/gif/reader_test.go:10: var bigGif = []byte{
Go capitalization is s/Gif/GIF/.

https://codereview.appspot.com/7602045/diff/21001/src/pkg/image/gif/reader_test.go#newcode27
src/pkg/image/gif/reader_test.go:27: 0x02, 0x04, 0x8c, 0x8f, 0x19, 0x05,
0x00, // lzw pixels
In this case, the lzw-compressed form of the 16 pixels fit into a single
block (it is less than 256 bytes). What if frame 2 was (0,0 - 4000,4000)
and not very compressible? Does DecodeAll still do something reasonable?

Similarly, this oversized frame is also the last frame in the GIF. What
if there was a frame 3 after this? Does DecodeAll still do something
reasonable?

https://codereview.appspot.com/7602045/diff/21001/src/pkg/image/gif/reader_test.go#newcode36
src/pkg/image/gif/reader_test.go:36: t.Errorf("got error %v", err)
Change Errorf to Fatalf. If DecodeAll fails, then there's no point
continuing, and you'll probably crash otherwise.

https://codereview.appspot.com/7602045/

--

---
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Search Discussions

  • Nigeltao at Mar 20, 2013 at 3:55 am
    https://codereview.appspot.com/7602045/diff/9001/src/pkg/image/gif/reader.go
    File src/pkg/image/gif/reader.go (right):

    https://codereview.appspot.com/7602045/diff/9001/src/pkg/image/gif/reader.go#newcode352
    src/pkg/image/gif/reader.go:352: width = d.width
    On 2013/03/19 09:37:10, jeff.allen wrote:
    giflib does not clamp. For the gif in issue 5050, it tries to allocate
    18446744073698482932 bytes, and malloc returns NULL.
    If neither giflib or WebKit clamp, then I'm not sure if Go should. For
    example, the eog program (which presumably uses giflib) on my Ubuntu
    desktop just shows an error on the GIF from issue 5050; it does not fall
    back on the first frame like WebKit does. The spec's "each image must
    fit" suggests that the GIF in issue 5050 is an invalid GIF, and I don't
    read it as a decoder must try to fix up an invalid image.

    Perhaps the right thing to do here is
    if bounds != bounds.Intersect(d.logicalScreen) {
    return nil, fmt.Errorf("gif: frame bounds is larger than image
    bounds")
    }
    instead of clamping and continuing without error.

    https://codereview.appspot.com/7602045/

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Jeff R. Allen at Mar 20, 2013 at 7:15 am
    I think this is now a philosophical question, but unlike most, the answer
    is clear. Should we be liberal, like a browser, or strict? The answer has
    always been clear for questions like this before. The std lib should do the
    simplest most standard thing. Users can always fork and hack images/* if
    their app needs to be more like a browser.

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Jeff Allen at Mar 20, 2013 at 10:23 am
    https://codereview.appspot.com/7602045/diff/21001/src/pkg/image/gif/reader_test.go
    File src/pkg/image/gif/reader_test.go (right):

    https://codereview.appspot.com/7602045/diff/21001/src/pkg/image/gif/reader_test.go#newcode8
    src/pkg/image/gif/reader_test.go:8: // a gif with two frames, where the
    second frame has been tweaked
    On 2013/03/20 02:57:45, nigeltao wrote:
    // bigGIF is a GIF image with two frames, etc.
    Done.

    https://codereview.appspot.com/7602045/diff/21001/src/pkg/image/gif/reader_test.go#newcode9
    src/pkg/image/gif/reader_test.go:9: // to have a width/height that's
    higher
    On 2013/03/20 02:57:45, nigeltao wrote:
    Higher than what?
    Done.

    https://codereview.appspot.com/7602045/diff/21001/src/pkg/image/gif/reader_test.go#newcode10
    src/pkg/image/gif/reader_test.go:10: var bigGif = []byte{
    On 2013/03/20 02:57:45, nigeltao wrote:
    Go capitalization is s/Gif/GIF/.
    Done.

    https://codereview.appspot.com/7602045/diff/21001/src/pkg/image/gif/reader_test.go#newcode27
    src/pkg/image/gif/reader_test.go:27: 0x02, 0x04, 0x8c, 0x8f, 0x19, 0x05,
    0x00, // lzw pixels
    The new tests (next CL update) now check for bounds > data size, bounds
    < data size. So I think your first question is covered. See if you
    agree.

    Bounds checking happens on a per-frame basis, in a loop that processes
    each section without respect to the previous or future sections, only
    with respect to the header. So I see no interest in testing other
    scenarios, like your second question.

    https://codereview.appspot.com/7602045/diff/21001/src/pkg/image/gif/reader_test.go#newcode36
    src/pkg/image/gif/reader_test.go:36: t.Errorf("got error %v", err)
    On 2013/03/20 02:57:45, nigeltao wrote:
    Change Errorf to Fatalf. If DecodeAll fails, then there's no point
    continuing,
    and you'll probably crash otherwise.
    Done.

    https://codereview.appspot.com/7602045/

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedMar 20, '13 at 2:57a
activeMar 20, '13 at 10:23a
posts4
users3
websitegolang.org

People

Translate

site design / logo © 2022 Grokbase