code review 13779043: go.image/tiff: encoder fix non LZW Predictor and Palett...

go.image/tiff: encoder fix non LZW Predictor and Paletted SamplesPerPixel and non RGB ExtraSamples.

TIFF6.0 Spec:
Page 31: ExtraSamples can only included in RGB mode image.
Page 39, SamplesPerPixel is usually 1 for bilevel, grayscale, and palette-color images.
Page 64, Currently Predictor field is used only with LZW.

Fixes issue 6421.

[chai2010, 2013-09-19 09:16:37]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go.image

[bsiegert, 2013-09-19 13:57:33]

LGTM

https://codereview.appspot.com/13779043/diff/5001/tiff/writer.go
File tiff/writer.go (right):

https://codereview.appspot.com/13779043/diff/5001/tiff/writer.go#newcode300
tiff/writer.go:300: // Currently this field is used only with LZW
Ah, I had not realized this. But the predictor is off by default and only used
when requested explicitly. I find it hard to believe that this is really an
issue. But then again, this is fine.

https://codereview.appspot.com/13779043/diff/5001/tiff/writer.go#newcode360
tiff/writer.go:360: // SamplesPerPixel is usually 1 for bilevel, grayscale, and
palette-color images.
You don't need this comment. But thanks for fixing the bug.

https://codereview.appspot.com/13779043/diff/5001/tiff/writer.go#newcode432
tiff/writer.go:432: ifd = append(ifd, ifdEntry{tPredictor, dtShort,
[]uint32{pr}})
Here again, I am finding it strange that other TIFF implementations treat a
tPredictor with value prNone as anything other than a no-op.

[gbulmer, 2013-09-19 14:15:04]

On 2013/09/19 09:16:37, chai2010 wrote:
> Hello mailto:golang-dev@googlegroups.com,
> 
> I'd like you to review this change to
> https://code.google.com/p/go.image

Thank you very much. That is very impressively fast turnaround.

The patch fixes:
go.image/testdata/roundtrip.predictor.video-001.tiff
Mac Preview, Mac Safari and Finer Preview display it correctly

However, these three still display incorrectly with Mac Preview and Finder
Preview, and Mac Safari, 
in the same way as before (4 quadrants, top two 1/2 x 1/2 scale, bottom
transparent):
go.image/testdata/roundtrip.bw-packbits.tiff
go.image/testdata/roundtrip.video-001-gray-16bit.tiff
go.image/testdata/roundtrip.video-001-gray.tiff

As before, Finder Preview and Mac Preview will not open:
go.image/testdata/roundtrip.video-001-paletted.tiff

The killers are the gray scale images.

[bsiegert, 2013-09-19 14:54:38]

I'll try to investigate this tomorrow.

[chai2010, 2013-09-20 00:44:35]

On 2013/09/19 14:15:04, gbulmer wrote:
> On 2013/09/19 09:16:37, chai2010 wrote:
> > Hello mailto:golang-dev@googlegroups.com,
> > 
> > I'd like you to review this change to
> > https://code.google.com/p/go.image
> 
> Thank you very much. That is very impressively fast turnaround.
> 
> The patch fixes:
> go.image/testdata/roundtrip.predictor.video-001.tiff
> Mac Preview, Mac Safari and Finer Preview display it correctly
> 
> However, these three still display incorrectly with Mac Preview and Finder
> Preview, and Mac Safari, 
> in the same way as before (4 quadrants, top two 1/2 x 1/2 scale, bottom
> transparent):
> go.image/testdata/roundtrip.bw-packbits.tiff
> go.image/testdata/roundtrip.video-001-gray-16bit.tiff
> go.image/testdata/roundtrip.video-001-gray.tiff
> 
> As before, Finder Preview and Mac Preview will not open:
> go.image/testdata/roundtrip.video-001-paletted.tiff
> 
> The killers are the gray scale images.

gbulmer,
I have no Mac OS, but GIMP 2.8.6 can't open these images on Win7/64.
Now i fix the ExtraSamples issue,and GIMP can open them.

[gbulmer, 2013-09-20 16:41:12]

On 2013/09/20 00:44:35, chai2010 wrote:
> On 2013/09/19 14:15:04, gbulmer wrote:
> > On 2013/09/19 09:16:37, chai2010 wrote:
> > > Hello mailto:golang-dev@googlegroups.com,
> > > 
> > > I'd like you to review this change to
> > > https://code.google.com/p/go.image
> > 
> > Thank you very much. That is very impressively fast turnaround.
> > 
> > The patch fixes:
> > go.image/testdata/roundtrip.predictor.video-001.tiff
> > Mac Preview, Mac Safari and Finer Preview display it correctly
> > 
> > However, these three still display incorrectly with Mac Preview and Finder
> > Preview, and Mac Safari, 
> > in the same way as before (4 quadrants, top two 1/2 x 1/2 scale, bottom
> > transparent):
> > go.image/testdata/roundtrip.bw-packbits.tiff
> > go.image/testdata/roundtrip.video-001-gray-16bit.tiff
> > go.image/testdata/roundtrip.video-001-gray.tiff
> > 
> > As before, Finder Preview and Mac Preview will not open:
> > go.image/testdata/roundtrip.video-001-paletted.tiff
> > 
> > The killers are the gray scale images.
> 
> gbulmer,
> I have no Mac OS, but GIMP 2.8.6 can't open these images on Win7/64.
> Now i fix the ExtraSamples issue,and GIMP can open them.

Chai2010, than you very much for pursuing this.

AFAICT, that hasn't changed the contents of the output files 
(roundtrip.) bw-packbits.tiff
(roundtrip.) video-001-gray-16bit.tiff
(roundtrip.) video-001-gray.tiff.hex

The originals open, and look fine, but these three still show the strange four
quadrants, top two 1/2 x 1/2 resolution.

I have looked at Mac Preview's 'Inspector'. There is a difference between the
three original testdata file and and decode/encode versions. The inspector says
the decode/encode versions "Has Alpha 1", however the inspector doesn't have
that for original testdata. The inspector information is otherwise identical for
the original files and the 'roundtrip' versions.

I has changed the contents of the palletted file:
(roundtrip.) video-001-paletted.tiff

Unfortunately, the palletted file isn't read by Mac Preview. 
Mac Preview writes into the console log:
20/09/2013 17:28:52	Preview[2149]	Couldn't initialize PVIVImage
I'm not sure if that helps.

I'll try to gets some more information.

[chai2010, 2013-09-21 01:11:46]

On 2013/09/20 16:41:12, gbulmer wrote:
> On 2013/09/20 00:44:35, chai2010 wrote:
> > On 2013/09/19 14:15:04, gbulmer wrote:
> > > On 2013/09/19 09:16:37, chai2010 wrote:
> > > > Hello mailto:golang-dev@googlegroups.com,
> > > > 
> > > > I'd like you to review this change to
> > > > https://code.google.com/p/go.image
> > > 
> > > Thank you very much. That is very impressively fast turnaround.
> > > 
> > > The patch fixes:
> > > go.image/testdata/roundtrip.predictor.video-001.tiff
> > > Mac Preview, Mac Safari and Finer Preview display it correctly
> > > 
> > > However, these three still display incorrectly with Mac Preview and Finder
> > > Preview, and Mac Safari, 
> > > in the same way as before (4 quadrants, top two 1/2 x 1/2 scale, bottom
> > > transparent):
> > > go.image/testdata/roundtrip.bw-packbits.tiff
> > > go.image/testdata/roundtrip.video-001-gray-16bit.tiff
> > > go.image/testdata/roundtrip.video-001-gray.tiff
> > > 
> > > As before, Finder Preview and Mac Preview will not open:
> > > go.image/testdata/roundtrip.video-001-paletted.tiff
> > > 
> > > The killers are the gray scale images.
> > 
> > gbulmer,
> > I have no Mac OS, but GIMP 2.8.6 can't open these images on Win7/64.
> > Now i fix the ExtraSamples issue,and GIMP can open them.
> 
> Chai2010, than you very much for pursuing this.
> 
> AFAICT, that hasn't changed the contents of the output files 
> (roundtrip.) bw-packbits.tiff
> (roundtrip.) video-001-gray-16bit.tiff
> (roundtrip.) video-001-gray.tiff.hex
> 
> The originals open, and look fine, but these three still show the strange four
> quadrants, top two 1/2 x 1/2 resolution.
> 
> I have looked at Mac Preview's 'Inspector'. There is a difference between the
> three original testdata file and and decode/encode versions. The inspector
says
> the decode/encode versions "Has Alpha 1", however the inspector doesn't have
> that for original testdata. The inspector information is otherwise identical
for
> the original files and the 'roundtrip' versions.
> 
> I has changed the contents of the palletted file:
> (roundtrip.) video-001-paletted.tiff
> 
> Unfortunately, the palletted file isn't read by Mac Preview. 
> Mac Preview writes into the console log:
> 20/09/2013 17:28:52	Preview[2149]	Couldn't initialize PVIVImage
> I'm not sure if that helps.
> 
> I'll try to gets some more information.

I change the ResolutionUnit value from resPerInch to resNone.
The XResolution/ResolutionUnit tags may relate the Orientation(not set) tag.
I'm not sure if that helps.

[gbulmer, 2013-09-21 20:08:18]

On 2013/09/21 01:11:46, chai2010 wrote:
> On 2013/09/20 16:41:12, gbulmer wrote:
> > On 2013/09/20 00:44:35, chai2010 wrote:
> > > On 2013/09/19 14:15:04, gbulmer wrote:
> > > > On 2013/09/19 09:16:37, chai2010 wrote:
> > > > > Hello mailto:golang-dev@googlegroups.com,
> > > > > 
> > > > > I'd like you to review this change to
> > > > > https://code.google.com/p/go.image
> > > > 
> > > > Thank you very much. That is very impressively fast turnaround.
> > > > 
> > > > The patch fixes:
> > > > go.image/testdata/roundtrip.predictor.video-001.tiff
> > > > Mac Preview, Mac Safari and Finer Preview display it correctly
> > > > 
> > > > However, these three still display incorrectly with Mac Preview and
Finder
> > > > Preview, and Mac Safari, 
> > > > in the same way as before (4 quadrants, top two 1/2 x 1/2 scale, bottom
> > > > transparent):
> > > > go.image/testdata/roundtrip.bw-packbits.tiff
> > > > go.image/testdata/roundtrip.video-001-gray-16bit.tiff
> > > > go.image/testdata/roundtrip.video-001-gray.tiff
> > > > 
> > > > As before, Finder Preview and Mac Preview will not open:
> > > > go.image/testdata/roundtrip.video-001-paletted.tiff
> > > > 
> > > > The killers are the gray scale images.
> > > 
> > > gbulmer,
> > > I have no Mac OS, but GIMP 2.8.6 can't open these images on Win7/64.
> > > Now i fix the ExtraSamples issue,and GIMP can open them.
> > 
> > Chai2010, than you very much for pursuing this.
> > 
> > AFAICT, that hasn't changed the contents of the output files 
> > (roundtrip.) bw-packbits.tiff
> > (roundtrip.) video-001-gray-16bit.tiff
> > (roundtrip.) video-001-gray.tiff.hex
> > 
> > The originals open, and look fine, but these three still show the strange
four
> > quadrants, top two 1/2 x 1/2 resolution.
> > 
> > I have looked at Mac Preview's 'Inspector'. There is a difference between
the
> > three original testdata file and and decode/encode versions. The inspector
> says
> > the decode/encode versions "Has Alpha 1", however the inspector doesn't have
> > that for original testdata. The inspector information is otherwise identical
> for
> > the original files and the 'roundtrip' versions.
> > 
> > I has changed the contents of the palletted file:
> > (roundtrip.) video-001-paletted.tiff
> > 
> > Unfortunately, the palletted file isn't read by Mac Preview. 
> > Mac Preview writes into the console log:
> > 20/09/2013 17:28:52	Preview[2149]	Couldn't initialize PVIVImage
> > I'm not sure if that helps.
> > 
> > I'll try to gets some more information.
> 
> I change the ResolutionUnit value from resPerInch to resNone.
> The XResolution/ResolutionUnit tags may relate the Orientation(not set) tag.
> I'm not sure if that helps.

I have worked through the TIFF spec and the code, and discovered that you have
fixed go.image/tiff

Patch Set 4 *did* fix all the defects. 

Specifically:
  extrasamples := uint32(0)
...
  extrasamples = 1
...
  extrasamples = 1
...
  if extrasamples > 0 {
                 ifd = append(ifd, ifdEntry{tExtraSamples, dtShort,
[]uint32{extrasamples}})

  }

Fixed the problems with gray-scale images.


I clearly made a mistake applying the patches.

Patch Set 5 also works, but the extra change ( tResolutionUnit resNone) is not
necessary

I apologise again.

Would you prefer to roll back to Patch Set 4, the smaller set of changes?

GB-(

[gbulmer, 2013-09-21 20:41:14]

The defect in
bw-packbits.tiff
video-001-gray-16bit.tiff
video-001-gray.tiff
video-001-paletted.tiff
which produce 1/2 x 1/2 resolution, repeated images (or non-display for
video-001-paletted.tiff) is fixed on Mac OS X 10.6 by the changes to
extrasamples.
Further, the other files are output (by my test function) correctly.
I've checked using Mac's Finder, Preview and Safari 5.1.9 on OS X 10.6.8

https://codereview.appspot.com/13779043/diff/11001/tiff/writer.go
File tiff/writer.go (right):

https://codereview.appspot.com/13779043/diff/11001/tiff/writer.go#newcode439
tiff/writer.go:439: if extrasamples > 0 {
This (only adding the ExtraSamples directory entry if extrasamples > 0),
combined with the appropriate values for extrasamples, fixes the defective
display by Finder Preview, Preview and Safari on Mac OS X

[chai2010, 2013-09-21 23:58:38]

Rollback to PatchSet 4.
PTAL

https://codereview.appspot.com/13779043/diff/5001/tiff/writer.go
File tiff/writer.go (right):

https://codereview.appspot.com/13779043/diff/5001/tiff/writer.go#newcode300
tiff/writer.go:300: // Currently this field is used only with LZW
On 2013/09/19 13:57:33, bsiegert wrote:
> Ah, I had not realized this. But the predictor is off by default and only used
> when requested explicitly. I find it hard to believe that this is really an
> issue. But then again, this is fine.

I think some tiff lib check tags too strict.

https://codereview.appspot.com/13779043/diff/5001/tiff/writer.go#newcode360
tiff/writer.go:360: // SamplesPerPixel is usually 1 for bilevel, grayscale, and
palette-color images.
On 2013/09/19 13:57:33, bsiegert wrote:
> You don't need this comment. But thanks for fixing the bug.

Done.

[nigeltao, 2013-09-22 01:10:15]

LGTM.

I'll make the trivial fixes and submit.

https://codereview.appspot.com/13779043/diff/20001/tiff/writer.go
File tiff/writer.go (right):

https://codereview.appspot.com/13779043/diff/20001/tiff/writer.go#newcode300
tiff/writer.go:300: // Currently this field is used only with LZW
Add a full stop to complete the sentence.

But I would write it as

if opt != nil {
  compression = opt.Compression.specValue()
  if compression == cLZW {
    // The predictor field is only used with LZW.
    // See page 64 of the spec.
    predictor = opt.Predictor
  }
}

https://codereview.appspot.com/13779043/diff/20001/tiff/writer.go#newcode351
tiff/writer.go:351: extrasamples := uint32(0) // Unspecified data.
While you're here, extrasamples should be extraSamples with an "S".

https://codereview.appspot.com/13779043/diff/20001/tiff/writer.go#newcode388
tiff/writer.go:388: extrasamples = 1
Add a comment
// Associated alpha.
Similarly below.

[nigeltao, 2013-09-22 01:24:42]

*** Submitted as
https://code.google.com/p/go/source/detail?r=14beb99f9689&repo=image ***

go.image/tiff: encoder fix non LZW Predictor and Paletted SamplesPerPixel and
non RGB ExtraSamples.

TIFF6.0 Spec:
Page 31: ExtraSamples can only included in RGB mode image.
Page 39, SamplesPerPixel is usually 1 for bilevel, grayscale, and palette-color
images.
Page 64, Currently Predictor field is used only with LZW.

Fixes issue 6421.

R=bsiegert, nigeltao, gbulmer
CC=golang-dev
https://codereview.appspot.com/13779043

Committer: Nigel Tao <nigeltao@golang.org>

[gbulmer, 2013-09-22 16:58:41]

I'd like to add a small observation.

I believe the solution causes tiff.Options.Predictor to have no effect.
So this may have an impact on existing users of the go.image/tiff package

Analysis: 
------------
The only compression that can be requested via tiff.Options.Compression
is either Deflate (which uses zlib compression) or Uncompressed, 
so  tiff/Encode compression can never take the value cLZW

tiff.Encode predictor is initialised to false, and can only become true by
     predictor = opt.Predictor && compression == cLZW)
so it is always false

Hence AFAICT the value of tiff.Options.Predictor can't trigger any path
sensitive to predictor == true through tiff.Encode or any functions it calls.

tiff.Encode pr has a value of prNone, and so there is never a Predictor value
added to the ifd,

I believe the files produced without a predictor are valid.

However, I assume prior to this change, some users may have used tiff.Encode
with a Predictor tag, and had had the 'predictor' algorithm applied.

Is Predictor required?
------------------------------
In the TIFF6 spec, in "Section 14: Differencing Predictor", page 64, under the
definition of "Predictor Tag = 317 (13D.H)" it says:
"Currently this field is used only with LZW (Com- pression=5) ..."
So I think there is no requirement in the TIFF 6 specification to implement the
Predictor functionality for anything other LZW compression, and go.image/tiff
does not support LZW compression in tiff.Encode

However, I believe it is valid to have a Predictor value for Deflate
compression, because in the "Adobe Photoshop® TIFF Technical Notes March 22,
2002",
http://partners.adobe.com/public/developer/en/tiff/TIFFphotoshop.pdf
which introduced "Deflate" as a compression algorithm, it says in
"Introduction": 
"Note that the Predictor defined in (TIFF6.pdf Section 14) works well for both
Deflate and LZW compression methods."

So is there anything which needs to change?
-------------------------------------------------------------
Currently the comment on tiff.Options.Predictor is likely to be misleading:
"	// Predictor determines whether a differencing predictor is used;
	// if true, instead of each pixel's color, the color difference to the
	// preceding one is saved.  This improves the compression for certain
	// types of images and compressors. For example, it works well for
	// photos with Deflate compression.
"

This might confuse someone if they try to use Predictor == true.

Also, files which are currently being generated using tiff.Encode with opt =
&Options{Predictor: true} may be effected.

Though the value of tiff.Options.Predictor is now ignored, it may cause an issue
in the future if user code passes tiff.Options.Predictor == true to Encode, and
there are future changes to Encode to use the value of Predictor. 


So it may be worth doing something about tiff.Options.Predictor

It appears that a simple fix might be to change the comment on type
Options.Predictor to say that it is not used, and maybe add that its value
should be set to false.
This would avoid changing the API to tiff.Encode, and reduce misunderstanding
from now onwards.

Of course, an alternative would be to re-activate Predictor functionality.
However that is more work.

GB

On 22 Sep 2013, at 02:10, nigeltao@golang.org wrote:

> LGTM.
> 
> I'll make the trivial fixes and submit.
> 
> 
> https://codereview.appspot.com/13779043/diff/20001/tiff/writer.go
> File tiff/writer.go (right):
> 
> https://codereview.appspot.com/13779043/diff/20001/tiff/writer.go#newcode300
> tiff/writer.go:300: // Currently this field is used only with LZW
> Add a full stop to complete the sentence.
> 
> But I would write it as
> 
> if opt != nil {
>  compression = opt.Compression.specValue()
>  if compression == cLZW {
>    // The predictor field is only used with LZW.
>    // See page 64 of the spec.
>    predictor = opt.Predictor
>  }
> }
> 
> https://codereview.appspot.com/13779043/diff/20001/tiff/writer.go#newcode351
> tiff/writer.go:351: extrasamples := uint32(0) // Unspecified data.
> While you're here, extrasamples should be extraSamples with an "S".
> 
> https://codereview.appspot.com/13779043/diff/20001/tiff/writer.go#newcode388
> tiff/writer.go:388: extrasamples = 1
> Add a comment
> // Associated alpha.
> Similarly below.
> 
> https://codereview.appspot.com/13779043/

[nigeltao, 2013-09-22 23:37:26]

On Mon, Sep 23, 2013 at 2:58 AM, G Bulmer <gbulmer@gmail.com> wrote:
> Of course, an alternative would be to re-activate Predictor functionality.
However that is more work.

As Chaishushan said in https://codereview.appspot.com/13779043
"encoder fix non LZW Predictor", page 64 of
http://partners.adobe.com/public/developer/en/tiff/TIFF6.pdf says that
Predictor "is used only with LZW (Compression=5) encoding".

Benny, you wrote https://codereview.appspot.com/6567077 "support
writing files with differencing predictor" and
https://codereview.appspot.com/6866051 "support for writing compressed
images. Only Deflate compression is supported for now". Any thoughts?

[bsiegert, 2013-09-23 08:43:10]

On Mon, Sep 23, 2013 at 1:37 AM, Nigel Tao <nigeltao@golang.org> wrote:
> As Chaishushan said in https://codereview.appspot.com/13779043
> "encoder fix non LZW Predictor", page 64 of
> http://partners.adobe.com/public/developer/en/tiff/TIFF6.pdf says that
> Predictor "is used only with LZW (Compression=5) encoding".
>
> Benny, you wrote https://codereview.appspot.com/6567077 "support
> writing files with differencing predictor" and
> https://codereview.appspot.com/6866051 "support for writing compressed
> images. Only Deflate compression is supported for now". Any thoughts?

Yes. If I interpret the spec right, the section on Deflate compression
was added later, without updating the main text. So I think that it
should be possible to use a differencing predictor (at least) with
Deflate compression as well. I will send a CL.

Shushan: If you want a difficult TIFF problem to tackle, you could try
fixing the decoding of LZW-compressed TIFF images.

--Benny.

[chai2010, 2013-09-24 12:48:10]

On 2013/09/23 08:43:10, bsiegert wrote:
> On Mon, Sep 23, 2013 at 1:37 AM, Nigel Tao <mailto:nigeltao@golang.org> wrote:
> > As Chaishushan said in https://codereview.appspot.com/13779043
> > "encoder fix non LZW Predictor", page 64 of
> > http://partners.adobe.com/public/developer/en/tiff/TIFF6.pdf says that
> > Predictor "is used only with LZW (Compression=5) encoding".
> >
> > Benny, you wrote https://codereview.appspot.com/6567077 "support
> > writing files with differencing predictor" and
> > https://codereview.appspot.com/6866051 "support for writing compressed
> > images. Only Deflate compression is supported for now". Any thoughts?
> 
> Yes. If I interpret the spec right, the section on Deflate compression
> was added later, without updating the main text. So I think that it
> should be possible to use a differencing predictor (at least) with
> Deflate compression as well. I will send a CL.
> 
> Shushan: If you want a difficult TIFF problem to tackle, you could try
> fixing the decoding of LZW-compressed TIFF images.
> 
> --Benny.

Benny,
I don't understanding TIFF LZW clearly.
I need more time to read the spec.

ChaiShushan