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

Fix embedding TIFF bilevel + alpha when width not whole number of bytes. Closes #154 #165

Closed carygravel closed 3 years ago

carygravel commented 3 years ago

Without GT, the bug is bigger. I'll look at that in a separate PR.

PhilterPaper commented 3 years ago

I don't think alpha is handled at all without GT, so you might be off on a wild goose chase trying to fix it without GT (you're perfectly welcome to add alpha handling to non-GT, if you wish). I'll look at your changes later today.

I hope to reach a fairly stable state for Builder soon, so I can push out a new release by the end of the month. Any further updates or fixes to TIFF handling should only be minor ones over the next couple weeks, and major work should hold off until July. Thanks!

PhilterPaper commented 3 years ago

Before I forget again to ask you, in the check_version() call in t/tiff.t you have diag($output);, which prints a bunch of version information to output. It would be great to see this information only if there was an error (the t-test fails). Can we detect that one or more subtests have failed, and only then kick off a call to diag() at the end? Do routines such as is() return a pass/fail value?

PhilterPaper commented 3 years ago

I'm a bit concerned about the following in t/tiff.t:

# for reasons I don't understand, gs swaps the last two pixels here, so let's # ignore them

Did you determine that the output PDF is in fact correct (including the last two pixels), but GhostScript consistently screws up something while converting it to a testable/comparable format? How confident are you that the revised Builder code is correct and the fault in fact lies with GS? Was this verified with more than one test setup?

I'm still looking at the code changes.

carygravel commented 3 years ago

Before I forget again to ask you, in the check_version() call in t/tiff.t you have diag($output);, which prints a bunch of version information to output. It would be great to see this information only if there was an error (the t-test fails). Can we detect that one or more subtests have failed, and only then kick off a call to diag() at the end? Do routines such as is() return a pass/fail value?

is() does return a pass/fail. I've just pushed a commit which does exactly that. Not sure I prefer it, though.

carygravel commented 3 years ago

Did you determine that the output PDF is in fact correct (including the last two pixels), but GhostScript consistently screws up something while converting it to a testable/comparable format? How confident are you that the revised Builder code is correct and the fault in fact lies with GS? Was this verified with more than one test setup?

I tested a load of different-sized test images with pattern:gray50. gs consistently swapped two pixels in the last byte of the first row. The PDFs looked OK to me.

I agree the test is not perfect, but better than not having it, and the code is better than before.

PhilterPaper commented 3 years ago

I was thinking in terms of just setting a global flag on any subtest failure, and after the last subtest (currently number 19), call diag() to output the various versions if the flag is set. That way you just see it once, instead of for each failure, and not at all if all subtests passed. Does "is()" cover all the tests where versions are of interest? I think there's one test (colormap) which has an unconditional "pass". We probably don't need version dump for "like", "isa_ok", and "ok" in the first 8 or so testgs.

carygravel commented 3 years ago

This works, but is still more boilerplate.

PhilterPaper commented 3 years ago

OK, your changes appear to work well. t/tiff.t is clean (with new test 19) and only shows the version info once (if at least one test failed). It looks nice that way. My TIFF test suite, including 2.tif and 3.tif bilevel+alpha from #154, appears to run well. Thanks much!

If you still want to do something with non-GT, I think you'll have a lot of work ahead to implement alpha. I have no problem with restricting alpha handling to GT.