Issue 102130044: code review 102130044: image/png: interlacing support for png.
Descriptionimage/png: interlacing support for png.
Fixes issue 6293.
Image "testdata/benchRGB-interlace.png" was generated by opening "testdata/benchRGB.png" in the editor Gimp and saving it with interlacing enabled.
Benchmark:
BenchmarkDecodeRGB 500 7014194 ns/op 37.37 MB/s
ok pkg/image/png 4.657s
BenchmarkDecodeInterlacing 100 10623241 ns/op 24.68 MB/s
ok pkg/image/png 1.339s
Patch Set 1 #Patch Set 2 : diff -r a078b2056ebc https://code.google.com/p/go/ #
Total comments: 5
Patch Set 3 : diff -r c8e9255aed3f https://code.google.com/p/go/ #Patch Set 4 : diff -r c8e9255aed3f https://code.google.com/p/go/ #
Total comments: 20
Patch Set 5 : diff -r 30307cc8bef2 https://code.google.com/p/go/ #
Total comments: 28
Patch Set 6 : diff -r ee11f19bc514 https://code.google.com/p/go/ #Patch Set 7 : diff -r ee11f19bc514 https://code.google.com/p/go/ #Patch Set 8 : diff -r 877f66c3a650 https://code.google.com/p/go/ #
MessagesTotal messages: 16
Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go/
Sign in to reply to this message.
https://codereview.appspot.com/102130044/diff/20001/src/pkg/image/png/reader.go File src/pkg/image/png/reader.go (right): https://codereview.appspot.com/102130044/diff/20001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:359: // read a single image pass, sized according to the pass number. From http://golang.org/doc/effective_go.html#commentary: "The first sentence should be a one-sentence summary that starts with the name being declared." // readImagePass reads a single image pass... https://codereview.appspot.com/102130044/diff/20001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:595: // Image that can be modified "A MutableImage interface is implemented by images that can be modified." or something like that to my point above. https://codereview.appspot.com/102130044/diff/20001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:603: // merge single pass into a full sized image // mergePassInto merges... https://codereview.appspot.com/102130044/diff/20001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:614: // create a blank, full-sized image of the approriate color model you get the point :) https://codereview.appspot.com/102130044/diff/20001/src/pkg/image/png/reader_... File src/pkg/image/png/reader_test.go (right): https://codereview.appspot.com/102130044/diff/20001/src/pkg/image/png/reader_... src/pkg/image/png/reader_test.go:358: } I would add a Benchmark to see if there are any significant performance costs associated with interlace support.
Sign in to reply to this message.
On 2014/06/06 15:04:06, Andrew Bonventre wrote: > https://codereview.appspot.com/102130044/diff/20001/src/pkg/image/png/reader.go > File src/pkg/image/png/reader.go (right): > > https://codereview.appspot.com/102130044/diff/20001/src/pkg/image/png/reader.... > src/pkg/image/png/reader.go:359: // read a single image pass, sized according to > the pass number. > From http://golang.org/doc/effective_go.html#commentary: > > "The first sentence should be a one-sentence summary that starts with the name > being declared." > > // readImagePass reads a single image pass... Done. > > https://codereview.appspot.com/102130044/diff/20001/src/pkg/image/png/reader.... > src/pkg/image/png/reader.go:595: // Image that can be modified > "A MutableImage interface is implemented by images that can be modified." > > or something like that to my point above. > Done. > https://codereview.appspot.com/102130044/diff/20001/src/pkg/image/png/reader.... > src/pkg/image/png/reader.go:603: // merge single pass into a full sized image > // mergePassInto merges... > Done. > https://codereview.appspot.com/102130044/diff/20001/src/pkg/image/png/reader.... > src/pkg/image/png/reader.go:614: // create a blank, full-sized image of the > approriate color model > you get the point :) Done. > > https://codereview.appspot.com/102130044/diff/20001/src/pkg/image/png/reader_... > File src/pkg/image/png/reader_test.go (right): > > https://codereview.appspot.com/102130044/diff/20001/src/pkg/image/png/reader_... > src/pkg/image/png/reader_test.go:358: } > I would add a Benchmark to see if there are any significant performance costs > associated with interlace support. Done. There is, as expected, about a ~3-4 times slowdown for interlacing.
Sign in to reply to this message.
https://codereview.appspot.com/102130044/diff/60001/src/pkg/image/png/reader.go File src/pkg/image/png/reader.go (right): https://codereview.appspot.com/102130044/diff/60001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:71: // definition of Adam7 interlacing, with 7 passes of reduced images. // interlacing defines the Adam7 interlacing order, with 7 passes etc. https://codereview.appspot.com/102130044/diff/60001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:596: type MutableImage interface { Why does this type have to be exported? In other words, why the "M" and not "m"? https://codereview.appspot.com/102130044/diff/60001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:598: ColorModel() color.Model Why are any methods other than Set necessary? https://codereview.appspot.com/102130044/diff/60001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:604: func (d *decoder) mergePassInto(imagePass image.Image, passNumber int, image MutableImage) { The usual argument order is destinations, then sources. For example, the io.Copy function arguments are (w, r) and not (r, w). Similarly for the built-in copy function, and plain assignment "dst = src". https://codereview.appspot.com/102130044/diff/60001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:609: image.Set(x*pass.xFactor+pass.xOffset, y*pass.yFactor+pass.yOffset, imagePass.At(x, y)) Calling image.Set and imagePass.At means two interface method calls and converting bytes to and from a color.Color, per pixel (per pass?). For a 1000x1000 image, that's a million times a bunch of (avoidable) work. You should be able to work with the Pix []byte slices instead of going through Set/At. That should be a zillion times faster. https://codereview.appspot.com/102130044/diff/60001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:615: func (d *decoder) getBlankImage() MutableImage { This two-line method is only called in one place. I'd just inline the code there. https://codereview.appspot.com/102130044/diff/60001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:617: mutableImage, _ := img.(MutableImage) return img.(MutableImage) https://codereview.appspot.com/102130044/diff/60001/src/pkg/image/png/reader_... File src/pkg/image/png/reader_test.go (right): https://codereview.appspot.com/102130044/diff/60001/src/pkg/image/png/reader_... src/pkg/image/png/reader_test.go:33: "basn3p04-31i", The change description should say how the "basn3p04-31i.png" file was generated. https://codereview.appspot.com/102130044/diff/60001/src/pkg/image/png/reader_... src/pkg/image/png/reader_test.go:246: pdone := !pb.Scan() Please run "hg sync". https://codereview.appspot.com/102130044/diff/60001/src/pkg/image/png/reader_... src/pkg/image/png/reader_test.go:361: benchmarkDecode(b, "testdata/benchRGB-interlace.png", 4) Please also add some benchmark numbers to the change description.
Sign in to reply to this message.
https://codereview.appspot.com/102130044/diff/60001/src/pkg/image/png/reader.go File src/pkg/image/png/reader.go (right): https://codereview.appspot.com/102130044/diff/60001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:71: // definition of Adam7 interlacing, with 7 passes of reduced images. On 2014/06/10 03:34:46, nigeltao wrote: > // interlacing defines the Adam7 interlacing order, with 7 passes etc. Done. https://codereview.appspot.com/102130044/diff/60001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:596: type MutableImage interface { On 2014/06/10 03:34:46, nigeltao wrote: > Why does this type have to be exported? In other words, why the "M" and not "m"? Mistake, didn't need to be. But removed anyway because we're not using Pix instead of needing Set. https://codereview.appspot.com/102130044/diff/60001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:598: ColorModel() color.Model On 2014/06/10 03:34:46, nigeltao wrote: > Why are any methods other than Set necessary? Originally, we needed the other methods to convert mutableImage to Image in the decode method, after merging was done. But the mutableImage is removed now. https://codereview.appspot.com/102130044/diff/60001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:604: func (d *decoder) mergePassInto(imagePass image.Image, passNumber int, image MutableImage) { On 2014/06/10 03:34:46, nigeltao wrote: > The usual argument order is destinations, then sources. For example, the io.Copy > function arguments are (w, r) and not (r, w). Similarly for the built-in copy > function, and plain assignment "dst = src". Done. https://codereview.appspot.com/102130044/diff/60001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:609: image.Set(x*pass.xFactor+pass.xOffset, y*pass.yFactor+pass.yOffset, imagePass.At(x, y)) On 2014/06/10 03:34:46, nigeltao wrote: > Calling image.Set and imagePass.At means two interface method calls and > converting bytes to and from a color.Color, per pixel (per pass?). For a > 1000x1000 image, that's a million times a bunch of (avoidable) work. > > You should be able to work with the Pix []byte slices instead of going through > Set/At. That should be a zillion times faster. Done. I'm still fairly new to golang, is there a better way to do this than adding another type switch block? I believe the only better way would require generics, right? https://codereview.appspot.com/102130044/diff/60001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:615: func (d *decoder) getBlankImage() MutableImage { On 2014/06/10 03:34:46, nigeltao wrote: > This two-line method is only called in one place. I'd just inline the code > there. Done. https://codereview.appspot.com/102130044/diff/60001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:617: mutableImage, _ := img.(MutableImage) On 2014/06/10 03:34:46, nigeltao wrote: > return img.(MutableImage) Removed. https://codereview.appspot.com/102130044/diff/60001/src/pkg/image/png/reader_... File src/pkg/image/png/reader_test.go (right): https://codereview.appspot.com/102130044/diff/60001/src/pkg/image/png/reader_... src/pkg/image/png/reader_test.go:33: "basn3p04-31i", On 2014/06/10 03:34:46, nigeltao wrote: > The change description should say how the "basn3p04-31i.png" file was generated. Done. https://codereview.appspot.com/102130044/diff/60001/src/pkg/image/png/reader_... src/pkg/image/png/reader_test.go:246: pdone := !pb.Scan() On 2014/06/10 03:34:46, nigeltao wrote: > Please run "hg sync". Running "hg sync" doesn't pull in this fix. Is it in a branch or something? https://codereview.appspot.com/102130044/diff/60001/src/pkg/image/png/reader_... src/pkg/image/png/reader_test.go:361: benchmarkDecode(b, "testdata/benchRGB-interlace.png", 4) On 2014/06/10 03:34:46, nigeltao wrote: > Please also add some benchmark numbers to the change description. Done. This new implementation is much faster.
Sign in to reply to this message.
Please run "hg sync" and then "hg upload 102130044". Also, please sign the Contributor License Agreement, if you haven't already done so. http://golang.org/doc/contribute.html#copyright https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.go File src/pkg/image/png/reader.go (right): https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:321: var image image.Image I'd rather not shadow the package name. var img image.Image https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:322: var lastRow []uint8 No need for this variable (see below). https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:327: mergeInto, _, _ := d.readImagePass(nil, 0, true) You don't need a separate mergeInto variable. img, _, _ = etc https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:331: err = e You break here, but the error handling below is a little tricky wrt io.EOF. You should at least comment why what you're doing here is OK. https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:339: return nil, FormatError("invalid interlace type") I wouldn't bother with this case. You already check for validity in parseIHDR. https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:348: n, err = r.Read(lastRow[:1]) (see above) just use d.tmp[:1] instead of lastRow[:1]. https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:362: var cr []uint8 Delete this line and the next. https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:376: width := d.width width, height := d.width, d.height https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:379: pass := interlacing[passNumber] I'd rename pass to p and passNumber to pass. https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:430: cr = make([]uint8, rowSize) Use := instead of a plain =. https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:597: func (d *decoder) mergePassInto(mergeInto image.Image, imagePass image.Image, passNumber int) { I'd rename mergeInto and imagePass to be dst and src. https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:599: pass := interlacing[passNumber] I'd rename pass to p and passNumber to pass. https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:607: switch src := imagePass.(type) { It seems easier if you switch on mergeInto.(type) instead of imagePass.(type). https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:663: srcIndex := 0 I'd write s, bounds := 0, src.Bounds() for y := bounds.Y.Min; y < bounds.Y.Max; y++ { dBase := (y*p.yFactor + p.yOffset - rect.Min.Y) * stride + (p.xOffset - rect.Min.X) * bytesPerPixel for x := bounds.X.Min; x < bounds.X.Max; x++ { d := dBase + x * p.xFactor * bytesPerPixel copy(dstPix[d:], srcPix[s:s+bytesPerPixel]) s += bytesPerPixel } }
Sign in to reply to this message.
Will look into the corporate contributor license agreement and get to you on that. https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.go File src/pkg/image/png/reader.go (right): https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:321: var image image.Image On 2014/06/23 03:52:47, nigeltao wrote: > I'd rather not shadow the package name. > var img image.Image Done. https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:322: var lastRow []uint8 On 2014/06/23 03:52:47, nigeltao wrote: > No need for this variable (see below). Done. https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:327: mergeInto, _, _ := d.readImagePass(nil, 0, true) On 2014/06/23 03:52:47, nigeltao wrote: > You don't need a separate mergeInto variable. > > img, _, _ = etc Done. https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:331: err = e On 2014/06/23 03:52:47, nigeltao wrote: > You break here, but the error handling below is a little tricky wrt io.EOF. You > should at least comment why what you're doing here is OK. Actually, I think it's better just to return the error. https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:339: return nil, FormatError("invalid interlace type") On 2014/06/23 03:52:47, nigeltao wrote: > I wouldn't bother with this case. You already check for validity in parseIHDR. Done. https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:348: n, err = r.Read(lastRow[:1]) On 2014/06/23 03:52:47, nigeltao wrote: > (see above) just use d.tmp[:1] instead of lastRow[:1]. Done. https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:362: var cr []uint8 On 2014/06/23 03:52:47, nigeltao wrote: > Delete this line and the next. Done. https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:376: width := d.width On 2014/06/23 03:52:47, nigeltao wrote: > width, height := d.width, d.height Done. https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:379: pass := interlacing[passNumber] On 2014/06/23 03:52:47, nigeltao wrote: > I'd rename pass to p and passNumber to pass. Done. https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:430: cr = make([]uint8, rowSize) On 2014/06/23 03:52:46, nigeltao wrote: > Use := instead of a plain =. Done. https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:597: func (d *decoder) mergePassInto(mergeInto image.Image, imagePass image.Image, passNumber int) { On 2014/06/23 03:52:47, nigeltao wrote: > I'd rename mergeInto and imagePass to be dst and src. Done. https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:599: pass := interlacing[passNumber] On 2014/06/23 03:52:47, nigeltao wrote: > I'd rename pass to p and passNumber to pass. Done. https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:607: switch src := imagePass.(type) { On 2014/06/23 03:52:47, nigeltao wrote: > It seems easier if you switch on mergeInto.(type) instead of imagePass.(type). Done. https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... src/pkg/image/png/reader.go:663: srcIndex := 0 On 2014/06/23 03:52:47, nigeltao wrote: > I'd write > s, bounds := 0, src.Bounds() > for y := bounds.Y.Min; y < bounds.Y.Max; y++ { > dBase := (y*p.yFactor + p.yOffset - rect.Min.Y) * stride + > (p.xOffset - rect.Min.X) * bytesPerPixel > for x := bounds.X.Min; x < bounds.X.Max; x++ { > d := dBase + x * p.xFactor * bytesPerPixel > copy(dstPix[d:], srcPix[s:s+bytesPerPixel]) > s += bytesPerPixel > } > } Done.
Sign in to reply to this message.
The code LGTM but I can't submit until the contributor license agreement is done.
Sign in to reply to this message.
The contributor license agreement was signed and mailed out late last week (the 26th), not sure when it should arrive and be processed. On Wed, Jun 25, 2014 at 6:51 AM, <nigeltao@golang.org> wrote: > The code LGTM but I can't submit until the contributor license agreement > is done. > > https://codereview.appspot.com/102130044/ >
Sign in to reply to this message.
Maybe mention in description that you are fixing https://code.google.com/p/go/issues/detail?id=6293 W dniu wtorek, 24 czerwca 2014 07:49:03 UTC+2 użytkownik dus...@gmail.com napisał: > > Will look into the corporate contributor license agreement and get to > you on that. > > > > https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.go > File src/pkg/image/png/reader.go (right): > > https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... > > src/pkg/image/png/reader.go:321 > <https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader....>: > var image image.Image > On 2014/06/23 03:52:47, nigeltao wrote: > > I'd rather not shadow the package name. > > var img image.Image > > Done. > > https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... > > src/pkg/image/png/reader.go:322 > <https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader....>: > var lastRow []uint8 > On 2014/06/23 03:52:47, nigeltao wrote: > > No need for this variable (see below). > > Done. > > https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... > > src/pkg/image/png/reader.go:327 > <https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader....>: > mergeInto, _, _ := d.readImagePass(nil, > 0, true) > On 2014/06/23 03:52:47, nigeltao wrote: > > You don't need a separate mergeInto variable. > > > img, _, _ = etc > > Done. > > https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... > > src/pkg/image/png/reader.go:331 > <https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader....>: > err = e > On 2014/06/23 03:52:47, nigeltao wrote: > > You break here, but the error handling below is a little tricky wrt > io.EOF. You > > should at least comment why what you're doing here is OK. > > Actually, I think it's better just to return the error. > > https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... > > src/pkg/image/png/reader.go:339 > <https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader....>: > return nil, FormatError("invalid > interlace type") > On 2014/06/23 03:52:47, nigeltao wrote: > > I wouldn't bother with this case. You already check for validity in > parseIHDR. > > Done. > > https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... > > src/pkg/image/png/reader.go:348 > <https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader....>: > n, err = r.Read(lastRow[:1]) > On 2014/06/23 03:52:47, nigeltao wrote: > > (see above) just use d.tmp[:1] instead of lastRow[:1]. > > Done. > > https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... > > src/pkg/image/png/reader.go:362 > <https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader....>: > var cr []uint8 > On 2014/06/23 03:52:47, nigeltao wrote: > > Delete this line and the next. > > Done. > > https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... > > src/pkg/image/png/reader.go:376 > <https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader....>: > width := d.width > On 2014/06/23 03:52:47, nigeltao wrote: > > width, height := d.width, d.height > > Done. > > https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... > > src/pkg/image/png/reader.go:379 > <https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader....>: > pass := interlacing[passNumber] > On 2014/06/23 03:52:47, nigeltao wrote: > > I'd rename pass to p and passNumber to pass. > > Done. > > https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... > > src/pkg/image/png/reader.go:430 > <https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader....>: > cr = make([]uint8, rowSize) > On 2014/06/23 03:52:46, nigeltao wrote: > > Use := instead of a plain =. > > Done. > > https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... > > src/pkg/image/png/reader.go:597 > <https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader....>: > func (d *decoder) > mergePassInto(mergeInto image.Image, imagePass image.Image, passNumber > int) { > On 2014/06/23 03:52:47, nigeltao wrote: > > I'd rename mergeInto and imagePass to be dst and src. > > Done. > > https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... > > src/pkg/image/png/reader.go:599 > <https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader....>: > pass := interlacing[passNumber] > On 2014/06/23 03:52:47, nigeltao wrote: > > I'd rename pass to p and passNumber to pass. > > Done. > > https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... > > src/pkg/image/png/reader.go:607 > <https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader....>: > switch src := imagePass.(type) { > On 2014/06/23 03:52:47, nigeltao wrote: > > It seems easier if you switch on mergeInto.(type) instead of > imagePass.(type). > > Done. > > https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader.... > > src/pkg/image/png/reader.go:663 > <https://codereview.appspot.com/102130044/diff/80001/src/pkg/image/png/reader....>: > srcIndex := 0 > On 2014/06/23 03:52:47, nigeltao wrote: > > I'd write > > s, bounds := 0, src.Bounds() > > for y := bounds.Y.Min; y < bounds.Y.Max; y++ { > > dBase := (y*p.yFactor + p.yOffset - rect.Min.Y) * stride + > > (p.xOffset - rect.Min.X) * bytesPerPixel > > for x := bounds.X.Min; x < bounds.X.Max; x++ { > > d := dBase + x * p.xFactor * bytesPerPixel > > copy(dstPix[d:], srcPix[s:s+bytesPerPixel]) > > s += bytesPerPixel > > } > > } > > Done. > > https://codereview.appspot.com/102130044/ >
Sign in to reply to this message.
On Sun, Jul 6, 2014 at 1:51 PM, Dobrosław Żybort <matrixik@gmail.com> wrote: > Maybe mention in description that you are fixing > https://code.google.com/p/go/issues/detail?id=6293 > > Done. CLA mailed to cla-submissions@google.com as well.
Sign in to reply to this message.
In the CL description, Fixes issue https://code.google.com/p/go/issues/detail?id=6293 should be just Fixes issue 6293. See https://code.google.com/p/support/wiki/IssueTracker#Integration_with_version_... for details.
Sign in to reply to this message.
On Mon, Jul 7, 2014 at 8:16 PM, <nigeltao@golang.org> wrote: > In the CL description, > > Fixes issue https://code.google.com/p/go/issues/detail?id=6293 > > should be just > > Fixes issue 6293. > Done
Sign in to reply to this message.
$ hg clpatch 102130044 add : patch did not apply cleanly abort: hgapplydiff failed Can you run "hg sync" and then "hg upload 102130044"? (Sorry for the yak shaving.)
Sign in to reply to this message.
Done On Thu, Jul 10, 2014 at 4:11 AM, <nigeltao@golang.org> wrote: > $ hg clpatch 102130044 > add : patch did not apply cleanly > abort: hgapplydiff failed > > Can you run "hg sync" and then "hg upload 102130044"? > > (Sorry for the yak shaving.) > > https://codereview.appspot.com/102130044/ >
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=c0a68bcf19ae *** image/png: interlacing support for png. Fixes issue 6293. Image "testdata/benchRGB-interlace.png" was generated by opening "testdata/benchRGB.png" in the editor Gimp and saving it with interlacing enabled. Benchmark: BenchmarkDecodeRGB 500 7014194 ns/op 37.37 MB/s ok pkg/image/png 4.657s BenchmarkDecodeInterlacing 100 10623241 ns/op 24.68 MB/s ok pkg/image/png 1.339s LGTM=nigeltao R=nigeltao, andybons, matrixik CC=golang-codereviews https://codereview.appspot.com/102130044 Committer: David Symonds <dsymonds@golang.org>
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
