Implement CCITTFaxDecode #346
Conversation
ToGoImage and ImageToRGB now correctly handle 1bit colorspace images
|
|
Codecov Report
@@ Coverage Diff @@
## v3 #346 +/- ##
=========================================
+ Coverage 58.4% 59.83% +1.43%
=========================================
Files 145 149 +4
Lines 25316 26738 +1422
=========================================
+ Hits 14785 15999 +1214
- Misses 10148 10332 +184
- Partials 383 407 +24
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Looks great. I have added a few comments.
It would be nice to add one roundtrip test case which outputs a PDF file with a CCITTFaxDecode encoded image and then loads it, decodes and compares to the original. Can be some very basic image, just defined in code for example.
Reviewed 1 of 12 files at r1, 16 of 21 files at r3, 41 of 44 files at r4, 2 of 4 files at r5.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @nkryuchkov)
pdf/core/encoding.go, line 1636 at r5 (raw file):
// NewCCITTFaxEncoder makes a new CCITTFax encoder. func NewCCITTFaxEncoder() *CCITTFaxEncoder { return &CCITTFaxEncoder{}
Are there any default options that should be set ? or parameters required depending on planned use?
pdf/core/ccittfaxdecode/codes.go, line 6 at r5 (raw file):
*/ package ccittfaxdecode
Place ccittfaxdecode package into pdf/internal/
Should not be necessary to access from outside (other than core.NewCCITTFaxDecoder())
pdf/core/ccittfaxdecode/codes.go, line 17 at r5 (raw file):
masks map[int]byte eol = code{
Can you provide a reference to where this is defined? A URL would be nice if it can be accessed online
pdf/core/ccittfaxdecode/decode.go, line 14 at r5 (raw file):
var ( // ErrEOFBCorrupt is returned when the corrupt EOFB (end-of-block) code is found. ErrEOFBCorrupt = errors.New("EOFB code is corrupted")
Probably no need to export the errors.
pdf/core/ccittfaxdecode/decode.go, line 29 at r5 (raw file):
// trees represent the finite state machine for parsing bit sequences and fetching pixel run lengths whiteTree = &decodingTreeNode{
Put the tree vars in a separate block (not with the errors).
pdf/core/ccittfaxdecode/decode.go, line 241 at r5 (raw file):
case v0: pixelsRow, a0 = decodeVerticalMode(pixels, pixelsRow, isWhite, a0, 0)
Remove unneccessary empty lines
pdf/core/ccittfaxdecode/decode.go, line 345 at r5 (raw file):
case v0: pixelsRow, a0 = decodeVerticalMode(pixels, pixelsRow, isWhite, a0, 0)
remove unnecessary blank lines
pdf/core/ccittfaxdecode/decode_test.go, line 349 at r5 (raw file):
} tests := []testData{
Specify source of test data. From a reference?
pdf/core/ccittfaxdecode/doc.go, line 6 at r5 (raw file):
*/ // Package ccittfaxdecode defines and implements the Group3 and Group4
Maybe ccittfax would be specific enough?
pdf/core/ccittfaxdecode/encoder.go, line 71 at r5 (raw file):
encodedRow, bitPos := encodeRow1D(pixels[i], prevBitPos, eol)
remove extra empty line
pdf/core/ccittfaxdecode/encoder.go, line 85 at r5 (raw file):
// put RTC encodedRTC, _ := encodeRTC(prevBitPos)
remove extra empty line (also in below blocks), for related statements there is no need for an extra line
pdf/core/ccittfaxdecode/encoder.go, line 213 at r5 (raw file):
encoded = e.appendEncodedRow(encoded, encodedRow, prevBitPos)
remove extra empty line
pdf/core/ccittfaxdecode/encoder.go, line 224 at r5 (raw file):
if e.EndOfBlock { // put EOFB encodedEOFB, _ := encodeEOFB(prevBitPos)
remove extra empty line
pdf/core/ccittfaxdecode/encoder_test.go, line 23 at r5 (raw file):
} image.RegisterFormat("png", "png", png.Decode, png.DecodeConfig)
Is it not enough to import "image/png" ? Typically a blank import _ "image/png" should register the handler?
pdf/model/colorspace.go, line 306 at r5 (raw file):
rgbSamples = append(rgbSamples, grayVal, grayVal, grayVal) }
Seems unnecessary
Add default parameters to the NewCCITTFaxEncoder. Default values are taken from the PDF specification. ccittfaxdecode was moved to pdf/internal and renamed to ccittfax. ccittfax package errors were unexported.
There was a problem hiding this comment.
Reviewed 60 of 60 files at r6.
Reviewable status:complete! all files reviewed, all discussions resolved
Relevant to #38 . Added the implementation of the CCITTFaxDecode filter, embedded it within the v3 version. Comes along with the test data