Referring to the following comment I wrote:

"As far as I can tell, updateMetadata of ImageIOImageWriter
is only used by Tiff, not by PNG or JPEG and should probably just be moved to
ImageIOTIFFImageWriter."

I refer not to the method itself, but to the functionality within the current updateMetadata method of ImageIOImageWriter.  This functionality appears to be TIFF-specific and 
(1) referred to via super in ImageIOTIFFImageWriter, 
(2) completely overridden in ImageIOJPEGImageWriter and 
(3) ignored(?) by ImageIOPNGImageWriter.

Making updateMetadata abstract may be a viable option.

Created attachment 26552 [details]
Replaces earlier attachment, use this instead.

I made a couple mistakes in my initial patch.

Differences in this patch are

ImageWriterParams 
1) now uses constant from TIFFImageDecoder for cm instead of arbitrary hard-coded value 3.

2) replaced unused proposed rowsPerStrip value and new boolean rowsPerStripOverridden with more appropriately named oneRowPerStrip boolean.

3) revised hasResolution method to check both X and Y, which are both set in setResolution and their own individual methods.

4) added javadoc comments to many methods that were not commented; my bad.

ImageIOTIFFImageWriter
1) replaced call to isRowsPerStripOverridden with call to NOT isOneRowPerStrip

2) Consolidated identical boolean checks.

Thanks for the patch, Joshua. Would you mind doing a couple of changes?

You've added a few constants to TIFFImageDecoder which are only used in the package o.a.x.image.writer (which is an API package and should not access any implementation packages). I think it would be better to change these constants into an enum in the ...image.writer package.

The other one concerns the ImageWriterParams.setOneRowPerStrip(boolean). Wouldn't it make more sense to actually use an integer here? With the boolean the only possible values are 1 and <image-height>. With an integer, one could set 32 rows for a strip, for example. Alternatively, this method could be renamed to setSingleStrip(boolean) (or something like that) essentially inverting the meaning of the boolean. My reasoning: setOneRowPerStrip(false) says "not one row per strip", but setSingleStrip(true) would say "one strip with <image-height> rows" and setSingleStrip(false) would set the internal "Integer rowsPerStrip" to 1, allowing to later add a method setRowsPerStrip(int) if anyone needs that.

Please let me know what you think. I'll later take a deeper look again. Thanks.

--
Regarding TIFFImageDecoder:

I can certainly create an enum in writer if you like.
I originally placed the value constants in the TIFFImageDecoder class to persist alongside the name constant they are used with, TiffImageDecoder.TIFF_RESOLUTION_UNIT.  Figured they might be referred-to elsewhere in the codebase, but have not yet looked.

I do see now that TIFFImageDecoder extends ImageDecoderImpl, and agree this might not be the best place.

--
Regarding RowsPerStrip:

With ImageWriterParams not being page-specific, as is the case with multi-paged TIFF files, RowsPerStrip set to image height would need to vary on a per-page basis and might not be able to be set until the image is rendered, as the value might not be known.

A different solution that I am going to submit a patch for will have the following changes to ImageWriterParams:

* VARIABLE rowsPerStrip is an int with standard get/set methods.  Defaults to 1.
* METHOD void setSingleStrip(boolean) method - true sets to -1, false sets to 1.
* METHOD boolean isSingleStrip() - convenience returns true if rowsPerStrip == -1

--
Regarding BitsPerSample

I believe adding the "BitsPerSample" line might have been a mistake on my part, and I'll remove it in the upcoming patch revision to this bug.

Your suggestion is even better, Joshua. Looking forward to your updated patch.

Created attachment 26727 [details]
Patch revision with enum and other changes from comments

Been a little over a month, but the patch has been revised pretty much according to my comments earlier.

Also, I do believe we should have a variable bits per sample value tag set (hardened depending on compression) in a future update to the TIFF family along the lines of this code:

ifd.appendChild(createShortMetadataNode(TIFFImageDecoder.TIFF_BITS_PER_SAMPLE, "BitsPerSample", "1"));

Changed back to NEW since info was provided and I didn't change back to NEW.

Sorry for the late response, Joshua. You forgot to include ResolutionUnit in your patch. Would you please attach that one, too?

Created attachment 27104 [details]
ResolutionUnit DOT java ENUM attached

ENUM ResolutionUnit.java attached; svn -diff doesn't add completely new files.

Is packaged for org.apache.xmlgraphics.image.writer and should be combined with the prior patch, Patch 26727

Thanks for supplying the missing file, Joshua. I'm not done with the patch. I'm currently writing some unit tests to verify that the new code works as expected. I found that you set the default resolution unit to centimeters although ImageWriterParams currently uses inches. Later this is converted internally to units per centimeters. Your new code now mixed up the two, although the end result in the image file was correct. I'm in the process to correct that. Please don't post a new patch. I'll fix the problems. I don't want to restart fixing code style problems (like the tab characters). At this point I have to interrupt the processing of the patch but I'll continue shortly. Just to let you know I'm on it as time allows.

In the Fop code I was working from, without my patch, tiffs created would generate with Centimeter set in the resulting image metadata by default.  You and I discussed it mildly on the list almost a year ago ("Various TIFF Rendering Questions" - July 2010)

My alterations were an attempt to maintain this default state, while still adding the Inch and none-specified functionality.

I haven't checked TIFF, yet, but with PNG, I stumbled over a bug from 2004 that has still not been fixed. As you may have seen in the ImageIO JPEG writer, there is a similar (but different) bug there. More info here:
http://www.tracemodeler.com/articles/aging-bugs-and-setting-dpi-with-java-image-io/

So, it could very well be that it works fine with TIFF but not with other formats. Still looking for the best way to deal with this.....

Hi Joshua,

I'm speaking as a prospective user under the FOP framework. I have a requirement for a project that has some specific needs for TIFF images (header and content). Using the current FOP and XMLGraphics release, the images are not conforming to these requirements. One of the repairs (single strip images) would require decompressing and recompressing which I would rather avoid by having the image created that way in the first place. I can see a path to be able to reprocess the TIFF to do that but your patch update would seem to make that unnecessary. The other requirement I am faced with is for ResolutionUnit to be 'inches' - I have successfully configured for X/YResolution=200 but it still presents that as 787402/10000 dots/centimeter.

I understand I could use the options in your patch by calling the appropriate methods. Since the 'options' I need to set would be the same for every image generated, is there a way for them to be set through initialization either in the fop.xconf or XSL file?

Hi Dave,

From what understand, Jeremias is incorporating my patch as time allows.  

There were no alterations made to allow new settings to propagate upwards from xconf.fop.

You will have to call getWriterParams().setResolutionUnit(2) from your renderer to enforce inches.

I missed the comment about single-strip.

You will have to call getWriterParams().setSingleStrip(true) from your renderer
to set that appropriately.

Again, I didn't adjust XSL or xconf.fop, and I'll hold off on alterations to this Bug to allow this until Jeremias gives the go-ahead.  My submitted code did not conform to the coding standards.

The part about the single-strip refers to the fact that the images we are getting now have a strip per row (strip count = image height) due to the default rowsperstrip = 1 behaviour.

Similar to requirements for resolutionunit being in inches, I was asking whether setting rowsperstrip = imageheight (which would be a constant in my application) could also be done through either the fop.xconf or the XSL itself.

However, since the 'set it in configuration' option is not there for resolutionunit, requiring a call to set it, then I can also live with having to make the call to set rowsperstrip.

Thanks.

P.S. Can I assume that Joshua's changes are, or will soon be, incorporated in the snapshot builds?

Jeremias is working on the incorporation; he is doing additional work for it as well, so I can't comment to that status (see his prior post in this bug).  You may be better off grabbing the code and creating a custom IOWriter to use than waiting.

The patch in this bug does not add any additional fop.xconf/XSL functionality to pre-set any values added in this bug.

If you set setSingleStrip(true), it will use the incoming image height automatically for rowsperstrip; you will not need to set height specifically.

As Jeremias has stated recently in an e-mail to the FOP mailing list, "I'm on hiatus from working on FOP due to various experiences and the direction the project has taken policy-wise in the last few months," I am uncertain if that also pertains to the XMLGraphics project and this patch remains up in the air.

(In reply to comment #11)
> Thanks for supplying the missing file, Joshua. I'm not done with the patch.

jeremias, any chance you will be able to commit this (with your changes)?

Committed 1384277 based upon the contributions of Joshua Marquart and Jeremias Maerki with minor tweaks.

Note that the unit testing of Tiff metadata was disabled as it required JAI ImageIO support and this is not currently a dependency of the XGC build process.