PhilterPaper / Perl-PDF-Builder

Extended version of the popular PDF::API2 Perl-based PDF library for creating, reading, and modifying PDF documents
https://www.catskilltech.com/FreeSW/product/PDF%2DBuilder/title/PDF%3A%3ABuilder/freeSW_full
Other
6 stars 7 forks source link

Lzw3 #156

Closed carygravel closed 3 years ago

carygravel commented 3 years ago

This should add full compress and decompress support for LZW. I've added tests for all corner cases I've found so far. It seems to work for all the TIFFs in your zip.

PhilterPaper commented 3 years ago

Thank you for the code changes. I ran t/filter-lzwdecode.t and it appears to have passed all 9 tests. I'm assuming that the 4 extra lines of diagnostics will run OK during an install or upgrade (?).

However, when I ran t/tiff.t, test 13 (lzw+horizontal predictor (not converted to flate) with GT) failed at line 246 ($example doesn't match $expected):

   **** Error reading a content stream. The page may be incomplete.
               Output may be incorrect.
   **** Error: File did not complete the page properly and may be damaged.
               Output may be incorrect.
not ok 13 - lzw+horizontal predictor (not converted to flate) with GT
#   Failed test 'lzw+horizontal predictor (not converted to flate) with GT'
#   at t\tiff.t line 246.
#          got: '# ImageMagick pixel enumeration: 20,20,255,gray
# 0,0: (255)  #FFFFFF  gray(255) .... dump each pixel

Ideas? I haven't tried my test suite yet.

carygravel commented 3 years ago

Thank you for the code changes. I ran t/filter-lzwdecode.t and it appears to have passed all 9 tests. I'm assuming that the 4 extra lines of diagnostics will run OK during an install or upgrade (?).

Yup. Unless users ask for verbose output, they won't even see it.

However, when I ran t/tiff.t, test 13 (lzw+horizontal predictor (not converted to flate) with GT) failed at line 246 ($example doesn't match $expected):

It passes on the two different repo I have access to here, plus the Github CI, which is different again. So I can't reproduce the problem.

Can you post the test.tif input (in case your version of IM is creating something different), the test.pdf that P::B creates for the test, plus the out.png that gs converts it to?

Would you mind also running it over your TIFF collection and posting the resulting PDF?

Do you get any error messages running

magick convert -depth 8 -size 2x2 pattern:gray50 -scale 1000% -alpha off -define tiff:predictor=2 -compress lzw test.tif

?

PhilterPaper commented 3 years ago

example-13 and expected-13 appear to be the same as in the 'got' and 'expected' error message, if that's what you're looking for. Anything else?

carygravel commented 3 years ago

Thanks for the files, I'll take a look.

Anything else?

Would you mind also running it over your TIFF collection and posting the resulting PDF?

carygravel commented 3 years ago

Your test.tif is not binary-identical to mine, but the same size, and produces here a good PDF. So the reason for the problem is not trivial. I'll look further and have a think.

PhilterPaper commented 3 years ago

using Graphics::TIFF --

C:\Users\Phil\Desktop\PDF-samples\TIFF>tiff.pl 0
output to file outGT.pdf
file: /Users/Phil/Desktop/PDF-samples/TIFF/alpha/alpha.tif, 500 x 284
Graphics::TIFF library flag: 1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/alpha/pag1.tif, 499.5 x 528
Graphics::TIFF library flag: 1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/alpha/sample_detail_3.tif, 223.5 x 473.25
TIFFReadDirectory: Warning, Unknown field with tag 37724 (0x935c) encountered.
TIFFFetchNormalTag: Warning, Incompatible type for "RichTIFFIPTC"; tag ignored.
Graphics::TIFF library flag: 1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/bilevel/1.tiff, 157 x 196
Graphics::TIFF library flag: 1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/bilevel/CCITT_1.TIF, 432 x 594
Graphics::TIFF library flag: 1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/bilevel/CCITT_2.TIF, 432 x 594
Graphics::TIFF library flag: 1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/bilevel/CCITT_3.TIF, 432 x 594
Graphics::TIFF library flag: 1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/bilevel/G31D.TIF, 308 x 406
Graphics::TIFF library flag: 1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/bilevel/G4.tiff, 303.625 x 430.25
Graphics::TIFF library flag: 1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/bilevel/LZW.tiff, 303.625 x 430.25
Graphics::TIFF library flag: 1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/bilevel/d4D_MD0lxD.tif, 620.25 x 876.75
Graphics::TIFF library flag: 1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/bilevel/page_3.tif, 412 x 572.75
Graphics::TIFF library flag: 1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/color/MARBLES.TIF, 709.5 x 500.5
Graphics::TIFF library flag: 1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/color/WpdThumbnail.tiff, 384 x 384
Graphics::TIFF library flag: 1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/color/caution.tif, 192 x 192
Use of uninitialized value in subtraction (-) at C:/Strawberry/perl/site/lib/PDF/Builder/Basic/PDF/Filter/LZWDecode.pm line 282.
(1104 lines of this message)....
Use of uninitialized value within @data in pack at C:/Strawberry/perl/site/lib/PDF/Builder/Basic/PDF/Filter/LZWDecode.pm line 287.
(48 lines of this message)....
Graphics::TIFF library flag: 1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/color/events.tiff, 404 x 228
Graphics::TIFF library flag: 1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/color/protein.tif, 640 x 512
Graphics::TIFF library flag: 1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/gray/8.tiff, 628 x 784
Graphics::TIFF library flag: 1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/gray/note.tif, 384 x 384
Graphics::TIFF library flag: 1 (1=using GT)

outGT.pdf

withOUT Graphics::TIFF --

C:\Users\Phil\Desktop\PDF-samples\TIFF>tiff.pl 1
output to file outNoGT.pdf
file: /Users/Phil/Desktop/PDF-samples/TIFF/bilevel/1.tiff, 157 x 196
Graphics::TIFF library flag: -1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/bilevel/CCITT_1.TIF, 432 x 594
Graphics::TIFF library flag: -1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/bilevel/CCITT_2.TIF, 432 x 594
Graphics::TIFF library flag: -1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/bilevel/CCITT_3.TIF, 432 x 594
Graphics::TIFF library flag: -1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/bilevel/G31D.TIF, 308 x 406
Graphics::TIFF library flag: -1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/bilevel/LZW.tiff, 303.625 x 430.25
Graphics::TIFF library flag: -1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/bilevel/d4D_MD0lxD.tif, 620.25 x 876.75
Graphics::TIFF library flag: -1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/bilevel/page_3.tif, 412 x 572.75
Graphics::TIFF library flag: -1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/color/WpdThumbnail.tiff, 384 x 384
Graphics::TIFF library flag: -1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/color/caution.tif, 192 x 192
Graphics::TIFF library flag: -1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/gray/note.tif, 384 x 384
Graphics::TIFF library flag: -1 (1=using GT)

outNoGT.pdf

carygravel commented 3 years ago

On your machine, it seems to be trying to use horizontal prediction for caution.tif when it shouldn't:

file: /Users/Phil/Desktop/PDF-samples/TIFF/color/caution.tif, 192 x 192
Use of uninitialized value in subtraction (-) at C:/Strawberry/perl/site/lib/PDF/Builder/Basic/PDF/Filter/LZWDecode.pm line 282.
(1104 lines of this message)....
Use of uninitialized value within @data in pack at C:/Strawberry/perl/site/lib/PDF/Builder/Basic/PDF/Filter/LZWDecode.pm line 287.
(48 lines of this message)....

Please apply this patch and post outGT.pdf again:

diff --git a/lib/PDF/Builder/Basic/PDF/Filter/LZWDecode.pm b/lib/PDF/Builder/Basic/PDF/Filter/LZWDecode.pm
index 3b9a194..6e538de 100644
--- a/lib/PDF/Builder/Basic/PDF/Filter/LZWDecode.pm
+++ b/lib/PDF/Builder/Basic/PDF/Filter/LZWDecode.pm
@@ -88,9 +88,16 @@ sub infilt {

     if ( $self->{DecodeParms} and $self->{DecodeParms}->{Predictor} ) {
         my $predictor = $self->{DecodeParms}->{Predictor}->val;
-        if ( $predictor > 1 ) {
+        print "Got predictor $predictor\n";
+        if ( $predictor == 1 ) {
+            return $result;
+        }
+        elsif ( $predictor == 2 ) {
             return $self->_depredict($result);
         }
+        elsif ( $predictor == 3 ) {
+            croak "Floating point TIFF predictor not yet supported";
+        }
         else {
             croak "Invalid predictor: $predictor";
         }
@@ -111,9 +118,16 @@ sub outfilt {

     if ( $self->{DecodeParms} and $self->{DecodeParms}->{Predictor} ) {
         my $predictor = $self->{DecodeParms}->{Predictor}->val;
-        if ( $predictor > 1 ) {
+        print "Got predictor $predictor\n";
+        if ( $predictor == 1 ) {
+            # noop
+        }
+        elsif ( $predictor == 2 ) {
             $str = $self->_predict($str);
         }
+        elsif ( $predictor == 3 ) {
+            croak "Floating point TIFF predictor not yet supported";
+        }
         else {
             croak "Invalid predictor: $predictor";
         }
PhilterPaper commented 3 years ago

C:\Users\Phil\Desktop\PDF-samples\TIFF>tiff.pl 0 output to file outGT.pdf file: /Users/Phil/Desktop/PDF-samples/TIFF/alpha/alpha.tif, 500 x 284 Graphics::TIFF library flag: 1 (1=using GT) file: /Users/Phil/Desktop/PDF-samples/TIFF/alpha/pag1.tif, 499.5 x 528 Graphics::TIFF library flag: 1 (1=using GT) file: /Users/Phil/Desktop/PDF-samples/TIFF/alpha/sample_detail_3.tif, 223.5 x 473.25 TIFFReadDirectory: Warning, Unknown field with tag 37724 (0x935c) encountered. TIFFFetchNormalTag: Warning, Incompatible type for "RichTIFFIPTC"; tag ignored. Got predictor 66453506 (outfilt) Invalid predictor: 66453506 (outfilt) at C:/Strawberry/perl/site/lib/PDF/Builder/Resource/XObject/Image/TIFF_GT.pm line 669.

No PDF produced. You think 66453506 might actually be a floating point number? ASCII "fE5"

PhilterPaper commented 3 years ago

I pulled out the TIFF (sample_detail_3.tif) that blew up (an RGB image with alpha layer) and reran it. Now it blows up on MARBLES.TIF (RGB):

C:\Users\Phil\Desktop\PDF-samples\TIFF>tiff.pl 0 output to file outGT.pdf file: /Users/Phil/Desktop/PDF-samples/TIFF/alpha/alpha.tif, 500 x 284 Graphics::TIFF library flag: 1 (1=using GT) file: /Users/Phil/Desktop/PDF-samples/TIFF/alpha/pag1.tif, 499.5 x 528 Graphics::TIFF library flag: 1 (1=using GT) file: /Users/Phil/Desktop/PDF-samples/TIFF/bilevel/1.tiff, 157 x 196 Graphics::TIFF library flag: 1 (1=using GT) file: /Users/Phil/Desktop/PDF-samples/TIFF/bilevel/CCITT_1.TIF, 432 x 594 Graphics::TIFF library flag: 1 (1=using GT) file: /Users/Phil/Desktop/PDF-samples/TIFF/bilevel/CCITT_2.TIF, 432 x 594 Graphics::TIFF library flag: 1 (1=using GT) file: /Users/Phil/Desktop/PDF-samples/TIFF/bilevel/CCITT_3.TIF, 432 x 594 Graphics::TIFF library flag: 1 (1=using GT) file: /Users/Phil/Desktop/PDF-samples/TIFF/bilevel/G31D.TIF, 308 x 406 Graphics::TIFF library flag: 1 (1=using GT) file: /Users/Phil/Desktop/PDF-samples/TIFF/bilevel/G4.tiff, 303.625 x 430.25 Graphics::TIFF library flag: 1 (1=using GT) file: /Users/Phil/Desktop/PDF-samples/TIFF/bilevel/LZW.tiff, 303.625 x 430.25 Graphics::TIFF library flag: 1 (1=using GT) file: /Users/Phil/Desktop/PDF-samples/TIFF/bilevel/d4D_MD0lxD.tif, 620.25 x 876.75 Graphics::TIFF library flag: 1 (1=using GT) file: /Users/Phil/Desktop/PDF-samples/TIFF/bilevel/page_3.tif, 412 x 572.75 Graphics::TIFF library flag: 1 (1=using GT) file: /Users/Phil/Desktop/PDF-samples/TIFF/color/MARBLES.TIF, 709.5 x 500.5 Got predictor 67764226 (outfilt) Invalid predictor: 67764226 (outfilt) at C:/Strawberry/perl/site/lib/PDF/Builder/Resource/XObject/Image/TIFF_GT.pm line 669.

Pulled out MARBLES.TIF, and blew up on caution.tif, another RGB:

file: /Users/Phil/Desktop/PDF-samples/TIFF/color/caution.tif, 192 x 192 Got predictor 67436545 (outfilt) Invalid predictor: 67436545 (outfilt) at C:/Strawberry/perl/site/lib/PDF/Builder/Resource/XObject/Image/TIFF_GT.pm line 669.

Still no PDF output. And shouldn't we be seeing the predictor value at each TIFF? It must be using the revised code, as I added (infilt) and (outfilt) to show where the messages were coming from.

carygravel commented 3 years ago

sample_detail_3, MARBLES, events, 8 all use horizontal prediction (2). There seems to be an integer length mismatch in GT. I was just unlucky that it didn't show up in any of the tests on my machines. I'll fix it in GT, get a new version out and let you know so that you can test again.

And shouldn't we be seeing the predictor value at each TIFF?

No. If the TIFFTAG_PREDICTOR isn't defined, then 1=None is assumed.

carygravel commented 3 years ago

I've fixed GT which you can get here, or wait for it to index on CPAN. Please install the new version of GT and post another outGT.pdf

PhilterPaper commented 3 years ago

I'll assume that the PDF::Builder code will still need at least a little work (re your changes given above for $predictor 1,2,3,other), if nothing else, to remove the debug. I'll wait for GT to show up on CPAN (probably by tomorrow morning) and try it out. Have a good rest of the weekend!

Regarding my question about $predictor showing up, OK, I see the "Got the..." which only shows up if a predictor is being used.

PhilterPaper commented 3 years ago

OK, new Graphics::TIFF installed.

C:\Users\Phil\Desktop\PDF-samples\TIFF>tiff.pl 0
output to file outGT.pdf
file: /Users/Phil/Desktop/PDF-samples/TIFF/alpha/alpha.tif, 500 x 284
Graphics::TIFF library flag: 1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/alpha/pag1.tif, 499.5 x 528
Graphics::TIFF library flag: 1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/alpha/sample_detail_3.tif, 223.5 x 473.25
TIFFReadDirectory: Warning, Unknown field with tag 37724 (0x935c) encountered.
TIFFFetchNormalTag: Warning, Incompatible type for "RichTIFFIPTC"; tag ignored.
Got predictor 2 (outfilt)
Graphics::TIFF library flag: 1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/bilevel/1.tiff, 157 x 196
Graphics::TIFF library flag: 1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/bilevel/CCITT_1.TIF, 432 x 594
Graphics::TIFF library flag: 1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/bilevel/CCITT_2.TIF, 432 x 594
Graphics::TIFF library flag: 1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/bilevel/CCITT_3.TIF, 432 x 594
Graphics::TIFF library flag: 1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/bilevel/G31D.TIF, 308 x 406
Graphics::TIFF library flag: 1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/bilevel/G4.tiff, 303.625 x 430.25
Graphics::TIFF library flag: 1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/bilevel/LZW.tiff, 303.625 x 430.25
Graphics::TIFF library flag: 1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/bilevel/d4D_MD0lxD.tif, 620.25 x 876.75
Graphics::TIFF library flag: 1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/bilevel/page_3.tif, 412 x 572.75
Graphics::TIFF library flag: 1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/color/MARBLES.TIF, 709.5 x 500.5
Got predictor 2 (outfilt)
Graphics::TIFF library flag: 1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/color/WpdThumbnail.tiff, 384 x 384
Graphics::TIFF library flag: 1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/color/caution.tif, 192 x 192
Graphics::TIFF library flag: 1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/color/events.tiff, 404 x 228
Got predictor 2 (outfilt)
Graphics::TIFF library flag: 1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/color/protein.tif, 640 x 512
Graphics::TIFF library flag: 1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/gray/8.tiff, 628 x 784
Got predictor 2 (outfilt)
Graphics::TIFF library flag: 1 (1=using GT)
file: /Users/Phil/Desktop/PDF-samples/TIFF/gray/note.tif, 384 x 384
Graphics::TIFF library flag: 1 (1=using GT)

outGT.pdf

Ran OK, except that sample_detail_3 and MARBLES seemed to be much, much slower than before. G4 still fails on Adobe Acrobat Reader, but displays fine on Firefox. Anything else needed? I will go ahead and prereq GT version 10, so I don't forget to do that later.

PhilterPaper commented 3 years ago

Couple of items on the GT build:

A Perl error message during build:

Checking if your kit is complete...
Looks good
Generating a gmake-style Makefile
Writing Makefile for Graphics::TIFF
Use of uninitialized value $to_write in concatenation (.) or string at C:/Strawberry/perl/site/lib/ExtUtils/MakeMaker.pm line 1265.
Writing MYMETA.yml and MYMETA.json
  RATCLIFFE/Graphics-TIFF-10.tar.gz
  C:\Strawberry\perl\bin\perl.exe Makefile.PL -- OK
Running make for R/RA/RATCLIFFE/Graphics-TIFF-10.tar.gz

Is the following different from the t/tiff.t detection of ImageMagick module? Maybe now consider directly invoking ImageMagick (as with t/tiff.t), or changing t/tiff.t to use Image::Magick as optional install? Just a thought.

Running make test for RATCLIFFE/Graphics-TIFF-10.tar.gz
"C:\Strawberry\perl\bin\perl.exe" -MExtUtils::Command::MM -e cp_nonempty -- TIFF.bs blib\arch\auto\Graphics\TIFF\TIFF.bs 644
"C:\Strawberry\perl\bin\perl.exe" "-MExtUtils::Command::MM" "-MTest::Harness" "-e" "undef *Test::Harness::Switches; test_harness(0, 'blib\lib', 'blib\arch')" t/*.t
t/1.t ............ ok
t/2.t ............ ok
t/8_enums.t ...... ok
t/90_MANIFEST.t .. ok
t/91_critic.t .... skipped: Author test.  Set $ENV{TEST_AUTHOR} to a true value to run.
t/92_tiffinfo.t .. skipped: Test requires module 'Image::Magick' but it's not found
t/93_tiff2pdf.t .. skipped: Test requires module 'Image::Magick' but it's not found
All tests successful.
carygravel commented 3 years ago

Ran OK, except that sample_detail_3 and MARBLES seemed to be much, much slower than before. G4 still fails on Adobe Acrobat Reader, but displays fine on Firefox. Anything else needed? I will go ahead and prereq GT version 10, so I don't forget to do that later.

Cool. The speed decrease is because the LZW encoding code is pure Perl - whereas before it was using your XS PNG code.

A Perl error message during build:


Checking if your kit is complete...
Looks good
Generating a gmake-style Makefile
Writing Makefile for Graphics::TIFF
Use of uninitialized value $to_write in concatenation (.) or string at C:/Strawberry/perl/site/lib/ExtUtils/MakeMaker.pm line 1265.

That is deep inside MakeMaker. Not sure what is causing it.

Is the following different from the t/tiff.t detection of ImageMagick module? Maybe now consider directly invoking ImageMagick (as with t/tiff.t), or changing t/tiff.t to use Image::Magick as optional install? Just a thought.

Yes. It requires the Perl API for ImageMagick, called Image::Magick, or perlmagick, which you don't seem to have, and so the tests are skipped.

Running make test for RATCLIFFE/Graphics-TIFF-10.tar.gz "C:\Strawberry\perl\bin\perl.exe" -MExtUtils::Command::MM -e cp_nonempty -- TIFF.bs blib\arch\auto\Graphics\TIFF\TIFF.bs 644 "C:\Strawberry\perl\bin\perl.exe" "-MExtUtils::Command::MM" "-MTest::Harness" "-e" "undef Test::Harness::Switches; test_harness(0, 'blib\lib', 'blib\arch')" t/.t t/1.t ............ ok t/2.t ............ ok t/8_enums.t ...... ok t/90_MANIFEST.t .. ok t/91_critic.t .... skipped: Author test. Set $ENV{TEST_AUTHOR} to a true value to run. t/92_tiffinfo.t .. skipped: Test requires module 'Image::Magick' but it's not found t/93_tiff2pdf.t .. skipped: Test requires module 'Image::Magick' but it's not found All tests successful.

OK. I'll tidy things up and make a last push, assuming you would then be happy to merge.

PhilterPaper commented 3 years ago

Please test if you can #151 and #154 specific cases before pushing an update. It would be nice to fix those too, unless either is clearly a whole different problem (esp. 154) and should be fixed separately.

carygravel commented 3 years ago

I think that this pull request already fixes #151, but feel free to contradict me.

154 is not specific to LZW, so I would prefer to fix that in a separate pull.

carygravel commented 3 years ago

If you are fine merging this, I'll work on #154 next.

carygravel commented 3 years ago

BTW, after this is merged, I DO have plans to improve the performance. Just as Flate uses Compress-Raw-Zlib, which in turn uses the C library, it shouldn't be too hard to write some XS to speed up the LZW encoding and prediction routines.

PhilterPaper commented 3 years ago

I just took a first test pass through testing your new code, and it's looking pretty good. The only real problem is that the CCITT faxes in my test suite are inverted again (without GT)... I want to take a look at whether one of us messed up some code, or if the original problem causing the inversion was somehow fixed and my changes aren't needed anymore. There is a minor problem with my test suite when running with Graphics::TIFF removed, but that's not the fault of the TIFF code -- the test suite might be made a little more robust. The two t-tests look good with all combinations of with and without GT and ImageMagick. When both are installed, the test suite is good except for the CCITT inversion. I will probably go ahead and pull your PR on Friday, but it's getting very late right now.

carygravel commented 3 years ago

I can't reproduce your CCITT inversion problem. I ran through your list of TIFFs, both with and without GT (I just did it again, and removed the library, just to make sure).

You are talking about CCITT_1.TIF, CCITT_2.TIF & CCITT_3.TIF, aren't you?

Note that the GT flag is -1 if turned off in software, but present, and 0 if not present. Was that your intention?

PhilterPaper commented 3 years ago

I haven't yet pulled Lzw3, but copied over your changes into my working (\strawberry\perl\site\lib\PDF\Builder) area. I'll have to go back over it and see if I overlooked something. I see that you somehow changed a lot of code styling to something that doesn't match the rest of the style of PDF::Builder. If you're using some sort of pretty-printer to reformat the files you work on, please don't.

Yes, it is those three, plus G31D, when NOT using Graphics::TIFF. And yes, my intention was to have -1 = decline GT use, 0 = not available, 1 = available and use. Comparison should be == 1 (use GT library) or not (don't use GT), in most cases.

Since the new code is so close to working for me, some time today I'll go ahead and pull in the PR, then revert the styling and see about fixing the inversion problem, if it's still there. Then you can go ahead with whatever further work you're planning. Thanks much for the effort you're putting in!

PhilterPaper commented 3 years ago

OK, all your changes are in the repository now. Thank you again for your efforts. Please pull fresh copies before doing any further work, as I cleaned up the styling to match the rest of PDF::Builder. I will close #151 after checking that all is satisfied there.

Did I have to do anything besides "git pull" your changes? I was unable to push my changes back until I had "git pull"ed a second time and edited 5 of your files to clean up conflicts. Then I could commit again and it seemed to work. You said you had changed something in your repository/fork/local copy to prevent interference with me, but I'm not sure it's working.

You mentioned you might do something with XS code to speed up some of the operations. I presume this will only apply to Graphics::TIFF-supported TIFF, and not "ordinary" TIFF? That is, you would be packaging it as part of GT? It's no big deal if non-GT is a lot slower -- I just want to make sure that there isn't a new prereq required for non-GT, such as a separate XS package.