code review 6326057: image/jpeg: support for reading 4:4:0 subsampling.

image/jpeg: support for reading 4:4:0 subsampling.
Updates issue 2362.

[mpl, 2012-06-26 17:34:39]

Hello golang-dev@googlegroups.com (cc: nigeltao@golang.org),

I'd like you to review this change to
https://go.googlecode.com/hg/

[nigeltao, 2012-06-26 18:48:31]

Out of curiousity, where do 440 images usually come from? Most JPEGs I've seen
are either 444 or 420.

http://codereview.appspot.com/6326057/diff/5001/src/pkg/image/jpeg/reader.go
File src/pkg/image/jpeg/reader.go (right):

http://codereview.appspot.com/6326057/diff/5001/src/pkg/image/jpeg/reader.go#...
src/pkg/image/jpeg/reader.go:157: // For color images, we only support 4:4:4,
4:2:2 or 4:2:0 chroma
This comment needs updating.

http://codereview.appspot.com/6326057/diff/5001/src/pkg/image/jpeg/reader.go#...
src/pkg/image/jpeg/reader.go:206: switch h0 * v0 {
At this point, it's probably better to have
switch {
case h0==1 && v0==1:
  // etc
// etc
}

[mpl, 2012-06-26 19:28:11]

The one I attached to http://code.google.com/p/go/issues/detail?id=2362 came
from a friend's camera (which is not really special afaik). I have quite a few
jpegs in my collection (coming from various friends' cameras) that couldn't be
read by the Go lib, but I have not  investigated if they're all 440.

On 2012/06/26 18:48:31, nigeltao wrote:
> Out of curiousity, where do 440 images usually come from? Most JPEGs I've seen
> are either 444 or 420.
> 
> http://codereview.appspot.com/6326057/diff/5001/src/pkg/image/jpeg/reader.go
> File src/pkg/image/jpeg/reader.go (right):
> 
>
http://codereview.appspot.com/6326057/diff/5001/src/pkg/image/jpeg/reader.go#...
> src/pkg/image/jpeg/reader.go:157: // For color images, we only support 4:4:4,
> 4:2:2 or 4:2:0 chroma
> This comment needs updating.
> 
>
http://codereview.appspot.com/6326057/diff/5001/src/pkg/image/jpeg/reader.go#...
> src/pkg/image/jpeg/reader.go:206: switch h0 * v0 {
> At this point, it's probably better to have
> switch {
> case h0==1 && v0==1:
>   // etc
> // etc
> }

[mpl, 2012-06-26 21:44:27]

Hello golang-dev@googlegroups.com, nigeltao@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[mpl, 2012-06-26 21:47:12]

http://codereview.appspot.com/6326057/diff/5001/src/pkg/image/jpeg/reader.go
File src/pkg/image/jpeg/reader.go (right):

http://codereview.appspot.com/6326057/diff/5001/src/pkg/image/jpeg/reader.go#...
src/pkg/image/jpeg/reader.go:157: // For color images, we only support 4:4:4,
4:2:2 or 4:2:0 chroma
On 2012/06/26 18:48:31, nigeltao wrote:
> This comment needs updating.

Done.

http://codereview.appspot.com/6326057/diff/5001/src/pkg/image/jpeg/reader.go#...
src/pkg/image/jpeg/reader.go:206: switch h0 * v0 {
On 2012/06/26 18:48:31, nigeltao wrote:
> At this point, it's probably better to have
> switch {
> case h0==1 && v0==1:
>   // etc
> // etc
> }

Done.

[nigeltao, 2012-06-27 00:56:15]

The change description should start with "image/jpeg", not "jpeg". Also, add a
line that says "Updates issue 2362." Thus:

----
image/jpeg: support for reading 4:4:0 subsampling.

Updates issue 2362.
----

http://codereview.appspot.com/6326057/diff/8001/src/pkg/image/jpeg/reader.go
File src/pkg/image/jpeg/reader.go (right):

http://codereview.appspot.com/6326057/diff/8001/src/pkg/image/jpeg/reader.go#...
src/pkg/image/jpeg/reader.go:318: mx0 := h0*mx + (j % 2)
Note that (h0==1 && v0==1) implies that j == 0. I'd write it like this:

mx0, my0 := h0*mx, v0*my
if h0 == 1 {
  my0 += j
} else {
  mx0 += j%2
  my0 += j/2
}

http://codereview.appspot.com/6326057/diff/8001/src/pkg/image/ycbcr_test.go
File src/pkg/image/ycbcr_test.go (right):

http://codereview.appspot.com/6326057/diff/8001/src/pkg/image/ycbcr_test.go#n...
src/pkg/image/ycbcr_test.go:37: YCbCrSubsampleRatio440,
I'd make this last, the same order as the enum.

[mpl, 2012-06-27 09:01:49]

Hello nigeltao@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[mpl, 2012-06-27 09:08:18]

http://codereview.appspot.com/6326057/diff/8001/src/pkg/image/jpeg/reader.go
File src/pkg/image/jpeg/reader.go (right):

http://codereview.appspot.com/6326057/diff/8001/src/pkg/image/jpeg/reader.go#...
src/pkg/image/jpeg/reader.go:318: mx0 := h0*mx + (j % 2)
On 2012/06/27 00:56:15, nigeltao wrote:
> Note that (h0==1 && v0==1) implies that j == 0. I'd write it like this:
> 
> mx0, my0 := h0*mx, v0*my
> if h0 == 1 {
>   my0 += j
> } else {
>   mx0 += j%2
>   my0 += j/2
> }

Done.

http://codereview.appspot.com/6326057/diff/8001/src/pkg/image/ycbcr_test.go
File src/pkg/image/ycbcr_test.go (right):

http://codereview.appspot.com/6326057/diff/8001/src/pkg/image/ycbcr_test.go#n...
src/pkg/image/ycbcr_test.go:37: YCbCrSubsampleRatio440,
On 2012/06/27 00:56:15, nigeltao wrote:
> I'd make this last, the same order as the enum.

Done. in afterthought, I'd rather have it in second position everywhere (like it
is in jpeg/reader.go), so that they are in a consistent lexicographic ordering.
as well as in the (h, v) order. are you ok with that?

[nigeltao, 2012-06-27 21:33:01]

LGTM.

http://codereview.appspot.com/6326057/diff/8001/src/pkg/image/ycbcr_test.go
File src/pkg/image/ycbcr_test.go (right):

http://codereview.appspot.com/6326057/diff/8001/src/pkg/image/ycbcr_test.go#n...
src/pkg/image/ycbcr_test.go:37: YCbCrSubsampleRatio440,
On 2012/06/27 09:08:18, mpl wrote:
> On 2012/06/27 00:56:15, nigeltao wrote:
> > I'd make this last, the same order as the enum.
> 
> Done. in afterthought, I'd rather have it in second position everywhere (like
it
> is in jpeg/reader.go), so that they are in a consistent lexicographic
ordering.
> as well as in the (h, v) order. are you ok with that?

I can see the appeal, but that would change the integer values of other
constants, such as YCbCrSubsampleRatio420. I'd rather keep those values
unchanged.

[nigeltao, 2012-06-27 21:37:20]

*** Submitted as http://code.google.com/p/go/source/detail?r=d489d533a652 ***

image/jpeg: support for reading 4:4:0 subsampling.
Updates issue 2362.

R=nigeltao
CC=golang-dev
http://codereview.appspot.com/6326057

Committer: Nigel Tao <nigeltao@golang.org>