I found files to change with this command:
git grep 'DO NOT EDIT' | grep -v 'Code generated .* DO NOT'
There are more files that match that grep, but I do not intend on fixing
them.
Change-Id: I4b474f1c29ca3135560d414785b0dbe0d1a4e52c
GitHub-Last-Rev: 65804b0263
GitHub-Pull-Request: golang/go#24334
Reviewed-on: https://go-review.googlesource.com/99955
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
The Netscape looping application extension encodes how many
times the animation should restart, and if it's present
there is no way to signal that a GIF should play only once.
Use LoopCount=-1 to signal when a decoded GIF had no looping
extension, and update the encoder to omit that extension
block when LoopCount=-1.
Fixes#15768
GitHub-Last-Rev: 249744f0e2
GitHub-Pull-Request: golang/go#23761
Change-Id: Ic915268505bf12bdad690b59148983a7d78d693b
Reviewed-on: https://go-review.googlesource.com/93076
Reviewed-by: Andrew Bonventre <andybons@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Andrew Bonventre <andybons@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This reverts commit 08f19bbde1.
Reason for revert:
The changed transformation takes effect on a larger set
of code snippets than expected.
For example, this:
func foo() {
// Comment
bar()
}
becomes:
func foo() {
// Comment
bar()
}
This is an unintended consequence.
Change-Id: Ifca88d6267dab8a8170791f7205124712bf8ace8
Reviewed-on: https://go-review.googlesource.com/81335
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Joe Tsai <joetsai@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
The go repository contains a mix of github.com/golang/go/issues/xxxxx
and golang.org/issues/xxxxx URLs for references to issues in the issue
tracker. We should use one for consistency, and golang.org is preferred
in case the project moves the issue tracker in the future.
This reasoning is taken from a comment Sam Whited left on a CL I
recently opened: https://go-review.googlesource.com/c/go/+/73890.
In that CL I referenced an issue using its github.com URL, because other
tests in the file I was changing contained references to issues using
their github.com URL. Sam Whited left a comment on the CL stating I
should change it to the golang.org URL.
If new code is intended to reference issues via golang.org and not
github.com, existing code should be updated so that precedence exists
for contributors who are looking at the existing code as a guide for the
code they should write.
Change-Id: I3b9053fe38a1c56fc101a8b7fd7b8f310ba29724
Reviewed-on: https://go-review.googlesource.com/75673
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
To improve readability when exported fields are removed,
forbid the printer from emitting an empty line before the first comment
in a const, var, or type block.
Also, when printing the "Has filtered or unexported fields." message,
add an empty line before it to separate the message from the struct
or interfact contents.
Before the change:
<<<
type NamedArg struct {
// Name is the name of the parameter placeholder.
//
// If empty, the ordinal position in the argument list will be
// used.
//
// Name must omit any symbol prefix.
Name string
// Value is the value of the parameter.
// It may be assigned the same value types as the query
// arguments.
Value interface{}
// contains filtered or unexported fields
}
>>>
After the change:
<<<
type NamedArg struct {
// Name is the name of the parameter placeholder.
//
// If empty, the ordinal position in the argument list will be
// used.
//
// Name must omit any symbol prefix.
Name string
// Value is the value of the parameter.
// It may be assigned the same value types as the query
// arguments.
Value interface{}
// contains filtered or unexported fields
}
>>>
Fixes#18264
Change-Id: I9fe17ca39cf92fcdfea55064bd2eaa784ce48c88
Reviewed-on: https://go-review.googlesource.com/71990
Run-TryBot: Joe Tsai <thebrokentoaster@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
Function sqDiff is called multiple times in the hot path (x, y loops) of
drawPaletted from the image/draw package; number of sqDiff calls is
between 4×width×height and 4×width×height×len(palette) for each
drawPaletted call.
Simplify this function by removing arguments comparison and relying
instead on signed to unsigned integer conversion rules and properties of
unsigned integer values operations guaranteed by the spec:
> For unsigned integer values, the operations +, -, *, and << are
> computed modulo 2n, where n is the bit width of the unsigned integer's
> type. Loosely speaking, these unsigned integer operations discard high
> bits upon overflow, and programs may rely on ``wrap around''.
image/gif package benchmark that depends on the code updated shows
throughput improvements:
name old time/op new time/op delta
QuantizedEncode-4 788ms ± 2% 468ms ± 9% -40.58% (p=0.000 n=9+10)
name old speed new speed delta
QuantizedEncode-4 1.56MB/s ± 2% 2.63MB/s ± 8% +68.47% (p=0.000 n=9+10)
Closes#22375.
Change-Id: Ic9a540e39ceb21e7741d308af1cfbe61b4ac347b
Reviewed-on: https://go-review.googlesource.com/72373
Reviewed-by: Nigel Tao <nigeltao@golang.org>
drawPaletted has to discover R,G,B,A color values of each source image
pixel in a given rectangle. Doing that by calling image.Image.At()
method returning color.Color interface is quite taxing allocation-wise
since interface values go through heap. Introduce special cases for some
concrete source types by fetching color values using type-specific
methods.
name old time/op new time/op delta
Paletted-4 7.62ms ± 4% 3.72ms ± 3% -51.20% (p=0.008 n=5+5)
name old alloc/op new alloc/op delta
Paletted-4 480kB ± 0% 0kB ± 0% -99.99% (p=0.000 n=4+5)
name old allocs/op new allocs/op delta
Paletted-4 120k ± 0% 0k ± 0% -100.00% (p=0.008 n=5+5)
Updates #15759.
Change-Id: I0ce1770ff600ac80599541aaad4c2c826855c8fb
Reviewed-on: https://go-review.googlesource.com/72370
Reviewed-by: Nigel Tao <nigeltao@golang.org>
decoder.decode() was defering close of lzw.decoders created for each
frame in a loop, thus increasing heap usage (referenced object + defered
function) until decode() returns. Memory increased proportionally to the
number of frames. Fix this by moving the sImageDescriptor case block
into its own method.
Fixes#22237
Change-Id: I819617ea7e539e13c04bc11112f339645391ddb9
Reviewed-on: https://go-review.googlesource.com/70370
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Previously, the code would only check (w*h), not (w*h*bpp).
Fixes#22304
Change-Id: Iaca26d916fe4b894d460448c416b1e0b9fd68e44
Reviewed-on: https://go-review.googlesource.com/72350
Reviewed-by: Rob Pike <r@golang.org>
golang.org/cl/37258 was committed to fix issue #16146.
This patch seemed intent to allow at most one dangling byte. But, as
implemented, many more bytes may actually slip through. This is because
the LZW layer creates a bufio.Reader which will itself consume data
beyond the end of the LZW stream, and this isn't accounted for anywhere.
This change means to avoid the allocation of the bufio.Reader by making
blockReader implement io.ByteReader. Further, it adds a close() method
which detects extra data in the block sequence. To avoid any
regressions with poorly encoded GIFs which may have worked accidentally,
there are no restrictions on how many extra bytes may exist in the final
full sub-block that contained LZW data. If the end of the LZW stream
happened to align with the end of a sub-block, at most one more
sub-block with a length of 1 byte may exist before the block terminator.
This change aims to be at least as performant as the prior
implementation. But the primary gain is avoiding the allocation of a
bufio.Reader per frame:
name old time/op new time/op delta
Decode-8 276µs ± 0% 275µs ± 2% ~ (p=0.690 n=5+5)
name old speed new speed delta
Decode-8 55.9MB/s ± 0% 56.3MB/s ± 2% ~ (p=0.690 n=5+5)
name old alloc/op new alloc/op delta
Decode-8 49.2kB ± 0% 44.8kB ± 0% -9.10% (p=0.008 n=5+5)
name old allocs/op new allocs/op delta
Decode-8 269 ± 0% 267 ± 0% -0.74% (p=0.008 n=5+5)
Change-Id: Iec4f9b895561ad52266313fbc73ec82c070c3349
Reviewed-on: https://go-review.googlesource.com/68350
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
Reviewed-by: Nigel Tao <nigeltao@golang.org>
The indexed bitmap of a frame is encoded into a GIF by first LZW
compression, and then packaged by a simple block mechanism. Each block
of up-to-256 bytes starts with one byte, which indicates the size of the
block (0x01-0xff). The sequence of blocks is terminated by a 0x00.
While the format supports it, there is no good reason why any particular
image should be anything but a sequence of 255-byte blocks with one last
block less than 255-bytes.
The old blockWriter implementation would not buffer between Write()s,
meaning if the lzw Writer needs to flush more than one chunk of data via
a Write, multiple short blocks might exist in the middle of a stream.
Separate but related, the old implementation also forces lzw.NewWriter
to allocate a bufio.Writer because the blockWriter is not an
io.ByteWriter itself. But, even though it doesn't effectively buffer
data between Writes, it does make extra copies of sub-blocks during the
course of writing them to the GIF's writer.
Now, the blockWriter shall continue to use the encoder's [256]byte buf,
but use it to effectively buffer a series of WriteByte calls from the
lzw Writer. Once a WriteByte fills the buffer, the staged block is
Write()n to the underlying GIF writer. After the lzw Writer is Closed,
the blockWriter should also be closed, which will flush any remaining
block along with the block terminator.
BenchmarkEncode indicates slight improvements:
name old time/op new time/op delta
Encode-8 7.71ms ± 0% 7.38ms ± 0% -4.27% (p=0.008 n=5+5)
name old speed new speed delta
Encode-8 159MB/s ± 0% 167MB/s ± 0% +4.46% (p=0.008 n=5+5)
name old alloc/op new alloc/op delta
Encode-8 84.1kB ± 0% 80.0kB ± 0% -4.94% (p=0.008 n=5+5)
name old allocs/op new allocs/op delta
Encode-8 9.00 ± 0% 7.00 ± 0% -22.22% (p=0.008 n=5+5)
Change-Id: I9eb9367d41d7c3d4d7f0adc9b720fc24fb50006a
Reviewed-on: https://go-review.googlesource.com/68351
Reviewed-by: Nigel Tao <nigeltao@golang.org>
The GIF format allows for an image to contain a global color table which
might be used for some or every frame in an animated GIF. This palette
contains 24-bit opaque RGB values. An individual frame may use the
global palette and enable transparency by picking one number to be
transparent, instead of the color value in the palette.
image/gif decodes a GIF, which contains an []*image.Paletted that holds
each frame. When decoded, if a frame has a transparent color and uses
the global palette, a copy of the global []color.Color is made, and the
transparency color index is replaced with color.RGBA{}.
When encoding a GIF, each frame's palette is encoded to the form it
might exist in a GIF, up to 768 bytes "RGBRGBRGBRGB...". If a frame's
encoded palette is equal to the encoded global color table, the frame
will be encoded with the flag set to use the global color table,
otherwise the frame's palette will be included.
So, if the color in the global color table that matches the transparent
index of one frame wasn't black (and it frequently is not), reencoding a
GIF will likely result in a larger file because each frame's palette
will have to be encoded inline.
This commit takes a frame's transparent color index into account when
comparing an individual image.Paletted's encoded color table to the
global color table.
Fixes#22137
Change-Id: I5460021da6e4d7ce19198d5f94a8ce714815bc08
Reviewed-on: https://go-review.googlesource.com/68313
Reviewed-by: Nigel Tao <nigeltao@golang.org>
Decode decodes entire GIF image and returns the first frame as an
image.Image. There's no need for it to keep every decoded frame in
memory except for the one it returns.
Fixes#22199
Change-Id: I76b4bd31608ebc76a1a3df02e85c20eb80df7877
Reviewed-on: https://go-review.googlesource.com/69890
Reviewed-by: Nigel Tao <nigeltao@golang.org>
Also add some b.ReportAllocs calls to other image codec benchmarks.
Change-Id: I0f055dc76bffb66329c621a5f1ccd239f0cdd30b
Reviewed-on: https://go-review.googlesource.com/68390
Reviewed-by: Jed Denlea <jed@fastly.com>
Reviewed-by: Nigel Tao <nigeltao@golang.org>
If an image has been cropped horizontally, writeImageBlock detects that
its width and Stride differ and acts accordingly.
However, if an image has been cropped vertically, trimming from the
bottom, the whole original image will be written in place. This results
in more data in the LZW stream than necessary, and many decoders
including image/gif's itself will fail to load.
Fixes#20692
Change-Id: Id332877e31bcf3729c89d8a50c1be0464028d82e
Reviewed-on: https://go-review.googlesource.com/45972
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
Run-TryBot: Nigel Tao <nigeltao@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Nigel Tao <nigeltao@golang.org>
Previously, the package did not distinguish between baseline and
extended sequential images. Both are non-progressive images, but the Th
range differs between the two, as per Annex B of
https://www.w3.org/Graphics/JPEG/itu-t81.pdf
Extended sequential images are often emitted by the Guetzli encoder.
Fixes#19913
Change-Id: I3d0f9e16d5d374ee1c65e3a8fb87519de61cff94
Reviewed-on: https://go-review.googlesource.com/41831
Reviewed-by: David Symonds <dsymonds@golang.org>
Mostly unnecessary *testing.T arguments.
Found with github.com/mvdan/unparam.
Change-Id: Ifb955cb88f2ce8784ee4172f4f94d860fa36ae9a
Reviewed-on: https://go-review.googlesource.com/41691
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
This changes the decoder's behaviour when there is stray/extra data
found after an image is decompressed (e.g., data sub-blocks after an LZW
End of Information Code). Instead of raising an error, we silently skip
over such data until we find the end of the image data marked by a Block
Terminator. We skip at most one byte as sample problem GIFs exhibit this
property.
GIFs should not have and do not need such stray data (though the
specification is arguably ambiguous). However GIFs with such properties
have been seen in the wild.
Fixes#16146
Change-Id: Ie7e69052bab5256b4834992304e6ca58e93c1879
Reviewed-on: https://go-review.googlesource.com/37258
Reviewed-by: Nigel Tao <nigeltao@golang.org>
Run-TryBot: Nigel Tao <nigeltao@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This change allows greatly reducing memory allocations with a slightly
performance improvement as well.
Instances of (*png).Encoder can have a optional BufferPool attached to
them. This allows reusing temporary buffers used when encoding a new
image. This buffers include instances to zlib.Writer and bufio.Writer.
Also, buffers for current and previous rows are saved in the encoder
instance and reused as long as their cap() is enough to fit the current
image row.
A new benchmark was added to demonstrate the performance improvement
when setting a BufferPool to an Encoder instance:
$ go test -bench BenchmarkEncodeGray -benchmem
BenchmarkEncodeGray-4 1000 2349584 ns/op 130.75 MB/s 852230 B/op 32 allocs/op
BenchmarkEncodeGrayWithBufferPool-4 1000 2241650 ns/op 137.04 MB/s 900 B/op 3 allocs/op
Change-Id: I4488201ae53cb2ad010c68c1e0118ee12beae14e
Reviewed-on: https://go-review.googlesource.com/34150
Reviewed-by: Nigel Tao <nigeltao@golang.org>
Run-TryBot: Nigel Tao <nigeltao@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
The 0x10101 magic constant is a little more principled than 0x10100, as
the rounding adjustment now spans the complete range [0, 0xffff] instead
of [0, 0xff00].
Consider this round-tripping code:
y, cb, cr := color.RGBToYCbCr(r0, g0, b0)
r1, g1, b1 := color.YCbCrToRGB(y, cb, cr)
Due to rounding errors both ways, we often but not always get a perfect
round trip (where r0 == r1 && g0 == g1 && b0 == b1). This is true both
before and after this commit. In some cases we got luckier, in others we
got unluckier.
For example, before this commit, (180, 135, 164) doesn't round trip
perfectly (it's off by 1) but (180, 135, 165) does. After this commit,
both cases are reversed: the former does and the latter doesn't (again
off by 1). Over all possible (r, g, b) triples, there doesn't seem to be
a big change for better or worse.
There is some history in these CLs:
image/color: tweak the YCbCr to RGBA conversion formula.
https://go-review.googlesource.com/#/c/12220/2/src/image/color/ycbcr.go
image/color: have YCbCr.RGBA work in 16-bit color, per the Color
interface.
https://go-review.googlesource.com/#/c/8073/2/src/image/color/ycbcr.go
Change-Id: Ib25ba7039f49feab2a9d1a4141b86db17db7b3e1
Reviewed-on: https://go-review.googlesource.com/36732
Run-TryBot: Nigel Tao <nigeltao@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
This is a re-roll of a previous commit,
a855da29db, which was rolled back in
14347ee480.
It was rolled back because it broke a unit test in image/gif. The
image/gif code was fixed by 9ef65dbe06
"image/gif: fix frame-inside-image bounds checking".
The original commit message:
image: fix the overlap check in Rectangle.Intersect.
The doc comment for Rectangle.Intersect clearly states, "If the two
rectangles do not overlap then the zero rectangle will be returned."
Prior to this fix, calling Intersect on adjacent but non-overlapping
rectangles would return an empty but non-zero rectangle.
The fix essentially changes
if r.Min.X > r.Max.X || r.Min.Y > r.Max.Y { etc }
to
if r.Min.X >= r.Max.X || r.Min.Y >= r.Max.Y { etc }
(note that the > signs have become >= signs), but changing that line to:
if r.Empty() { etc }
seems clearer (and equivalent).
Change-Id: I2e3af1f1686064a573b2e513b39246fe60c03631
Reviewed-on: https://go-review.googlesource.com/36734
Reviewed-by: Rob Pike <r@golang.org>
Run-TryBot: Nigel Tao <nigeltao@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
The semantics of the Go image.Rectangle type is that the In and
Intersects methods treat empty rectangles specially. There are multiple
valid representations of an empty image.Rectangle. One of them is the
zero image.Rectangle but there are others. They're obviously not all
equal in the == sense, so we shouldn't use != to check GIF's semantics.
This change will allow us to re-roll
a855da29db "image: fix the overlap check
in Rectangle.Intersect" which was rolled back in
14347ee480.
Change-Id: Ie1a0d092510a7bb6170e61adbf334b21361ff9e6
Reviewed-on: https://go-review.googlesource.com/36639
Run-TryBot: Nigel Tao <nigeltao@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
The doc comment for Rectangle.Intersect clearly states, "If the two
rectangles do not overlap then the zero rectangle will be returned."
Prior to this fix, calling Intersect on adjacent but non-overlapping
rectangles would return an empty but non-zero rectangle.
The fix essentially changes
if r.Min.X > r.Max.X || r.Min.Y > r.Max.Y { etc }
to
if r.Min.X >= r.Max.X || r.Min.Y >= r.Max.Y { etc }
(note that the > signs have become >= signs), but changing that line to:
if r.Empty() { etc }
seems clearer (and equivalent).
Change-Id: Ia654e4b9dc805978db3e94d7a9718b6366005360
Reviewed-on: https://go-review.googlesource.com/34853
Reviewed-by: David Crawshaw <crawshaw@golang.org>
The existing implementation falls back to using image.At()
for each pixel when encoding an *image.YCbCr which is
inefficient and causes many memory allocations.
This change makes the jpeg encoder directly read Y, Cb, and Cr
pixel values.
benchmark old ns/op new ns/op delta
BenchmarkEncodeYCbCr-4 43990846 24201148 -44.99%
benchmark old MB/s new MB/s speedup
BenchmarkEncodeYCbCr-4 20.95 38.08 1.82x
Fixes#18487
Change-Id: Iaf2ebc646997e3e1fffa5335f1b0d642e15bd453
Reviewed-on: https://go-review.googlesource.com/34773
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Nigel Tao <nigeltao@golang.org>
This makes grayModel and gray16Model in color.go use the exact same
formula as RGBToYCbCr in ycbcr.go. They were the same formula in theory,
but in practice the color.go versions used a divide by 1000 and the
ycbcr.go versions used a (presumably faster) shift by 16.
This implies the nice property that converting an image.RGBA to an
image.YCbCr and then taking only the Y channel is equivalent to
converting an image.RGBA directly to an image.Gray.
The difference between the two formulae is non-zero, but small:
https://play.golang.org/p/qG7oe-eqHI
Updates #16251
Change-Id: I288ecb957fd6eceb9626410bd1a8084d2e4f8198
Reviewed-on: https://go-review.googlesource.com/31538
Reviewed-by: Rob Pike <r@golang.org>
All the prefixes of the testGIF produce errors today,
but they differ wildly in which errors: some are io.EOF,
others are io.ErrUnexpectedEOF, and others are gif-specific.
Make them all gif-specific to explain context, and make
any complaining about EOF be sure to mention the EOF
is unexpected.
Fixes#11390.
Change-Id: I742c39c88591649276268327ea314e68d1de1845
Reviewed-on: https://go-review.googlesource.com/17493
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Compression of paletted images is more efficient if they are not filtered.
This patch skips filtering for cbP8 images.
The improvements are demonstrated at https://github.com/olt/compressbenchFixes#16196
Change-Id: Ie973aad287cacf9057e394bb01cf0e4448a77618
Reviewed-on: https://go-review.googlesource.com/29872
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Benchmarks are much better for opaque fills and slightly worse on non
opaque fills. I think that on balance, this is still a win.
When the source is uniform(color.RGBA{0x11, 0x22, 0x33, 0xff}):
name old time/op new time/op delta
FillOver-8 966µs ± 1% 32µs ± 1% -96.67% (p=0.000 n=10+10)
FillSrc-8 32.4µs ± 1% 32.2µs ± 1% ~ (p=0.053 n=9+10)
When the source is uniform(color.RGBA{0x11, 0x22, 0x33, 0x44}):
name old time/op new time/op delta
FillOver-8 962µs ± 0% 1018µs ± 0% +5.85% (p=0.000 n=9+10)
FillSrc-8 32.2µs ± 1% 32.1µs ± 0% ~ (p=0.148 n=10+10)
Change-Id: I52ec6d5fcd0fbc6710cef0e973a21ee7827c0dd9
Reviewed-on: https://go-review.googlesource.com/28790
Reviewed-by: David Crawshaw <crawshaw@golang.org>
This is an error according to the spec, but Firefox and Google Chrome
seem OK with this.
Fixes#15059.
Change-Id: I841cf44e96655e91a2481555f38fbd7055a32202
Reviewed-on: https://go-review.googlesource.com/22546
Reviewed-by: Rob Pike <r@golang.org>
cmd and runtime were handled separately, and I'm intentionally skipped
syscall. This is the rest of the standard library.
CL generated mechanically with github.com/mdempsky/unconvert.
Change-Id: I9e0eff886974dedc37adb93f602064b83e469122
Reviewed-on: https://go-review.googlesource.com/22104
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Use one comparison to detect underflow and overflow simultaneously.
Use a shift, bitwise complement and uint8 type conversion to handle
clamping to upper and lower bound without additional branching.
Overall the new code is faster for a mix of
common case, underflow and overflow.
name old time/op new time/op delta
YCbCr-2 1.12ms ± 0% 0.64ms ± 0% -43.01% (p=0.000 n=48+47)
name old time/op new time/op delta
YCbCrToRGB/0-2 5.52ns ± 0% 5.77ns ± 0% +4.48% (p=0.000 n=50+49)
YCbCrToRGB/128-2 6.05ns ± 0% 5.52ns ± 0% -8.69% (p=0.000 n=39+50)
YCbCrToRGB/255-2 5.80ns ± 0% 5.77ns ± 0% -0.58% (p=0.000 n=50+49)
Found in collaboration with Josh Bleecher Snyder and Ralph Corderoy.
Change-Id: Ic5020320f704966f545fdc1ae6bc24ddb5d3d09a
Reviewed-on: https://go-review.googlesource.com/21910
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Fixes#14522.
As I said on that issue:
----
This is a progressive JPEG image. There are two dimensions of
progressivity: spectral selection (variables zs and ze in scan.go,
ranging in [0, 63]) and successive approximation (variables ah and al in
scan.go, ranging in [0, 8), from LSB to MSB, although ah=0 implicitly
means ah=8).
For this particular image, there are three components, and the SOS
markers contain this progression:
zs, ze, ah, al: 0 0 0 0 components: 0, 1, 2
zs, ze, ah, al: 1 63 0 0 components: 1
zs, ze, ah, al: 1 63 0 0 components: 2
zs, ze, ah, al: 1 63 0 2 components: 0
zs, ze, ah, al: 1 10 2 1 components: 0
zs, ze, ah, al: 11 63 2 1 components: 0
zs, ze, ah, al: 1 10 1 0 components: 0
The combination of all of these is complete (i.e. spectra 0 to 63 and
bits 8 exclusive to 0) for components 1 and 2, but it is incomplete for
component 0 (the luma component). In particular, there is no data for
component 0, spectra 11 to 63 and bits 1 exclusive to 0.
The image/jpeg code, as of Go 1.6, waits until both dimensions are
complete before performing the de-quantization, IDCT and copy to an
*image.YCbCr. This is the "if zigEnd != blockSize-1 || al != 0 { ...
continue }" code and associated commentary in scan.go.
Almost all progressive JPEG images end up complete in both dimensions
for all components, but this particular image is incomplete for
component 0, so the Go code never writes anything to the Y values of the
resultant *image.YCbCr, which is why the broken output is so dark (but
still looks recognizable in terms of red and blue hues).
My reading of the ITU T.81 JPEG specification (Annex G) doesn't
explicitly say that this is a valid image, but it also doesn't rule it
out.
In any case, the fix is, for progressive JPEG images, to always
reconstruct the decoded blocks (by performing the de-quantization, IDCT
and copy to an *image.YCbCr), regardless of whether or not they end up
complete. Note that, in Go, the jpeg.Decode function does not return
until the entire image is decoded, so we still only want to reconstruct
each block once, not once per SOS (Start Of Scan) marker.
----
A test image was also added, based on video-001.progressive.jpeg. When
decoding that image, inserting a
println("nComp, zs, ze, ah, al:", nComp, zigStart, zigEnd, ah, al)
into decoder.processSOS in scan.go prints:
nComp, zs, ze, ah, al: 3 0 0 0 1
nComp, zs, ze, ah, al: 1 1 5 0 2
nComp, zs, ze, ah, al: 1 1 63 0 1
nComp, zs, ze, ah, al: 1 1 63 0 1
nComp, zs, ze, ah, al: 1 6 63 0 2
nComp, zs, ze, ah, al: 1 1 63 2 1
nComp, zs, ze, ah, al: 3 0 0 1 0
nComp, zs, ze, ah, al: 1 1 63 1 0
nComp, zs, ze, ah, al: 1 1 63 1 0
nComp, zs, ze, ah, al: 1 1 63 1 0
In other words, video-001.progressive.jpeg contains 10 different scans.
This little program below drops half of them (remembering to keep the
"\xff\xd9" End of Image marker):
----
package main
import (
"bytes"
"io/ioutil"
"log"
)
func main() {
sos := []byte{0xff, 0xda}
eoi := []byte{0xff, 0xd9}
src, err := ioutil.ReadFile("video-001.progressive.jpeg")
if err != nil {
log.Fatal(err)
}
b := bytes.Split(src, sos)
println(len(b)) // Prints 11.
dst := bytes.Join(b[:5], sos)
dst = append(dst, eoi...)
if err := ioutil.WriteFile("video-001.progressive.truncated.jpeg", dst, 0666); err != nil {
log.Fatal(err)
}
}
----
The video-001.progressive.truncated.jpeg was converted to png via
libjpeg and ImageMagick:
djpeg -nosmooth video-001.progressive.truncated.jpeg > tmp.tga
convert tmp.tga video-001.progressive.truncated.png
rm tmp.tga
Change-Id: I72b20cd4fb6746d36d8d4d587f891fb3bc641f84
Reviewed-on: https://go-review.googlesource.com/21062
Reviewed-by: Rob Pike <r@golang.org>
Ignore superfluous trailing IDAT chunks which were not consumed when decoding
the image. This change fixes decoding of valid images in which a zero-length
IDAT chunk appears after the actual image data. It also prevents decoding of
trailing garbage IDAT chunks or maliciously embedded additional images.
Fixes#14936
Change-Id: I8c76cfa9a03496d9576f72bed2db109271f97c5e
Reviewed-on: https://go-review.googlesource.com/21045
Reviewed-by: Nigel Tao <nigeltao@golang.org>
